Messages in this thread | | | Date | Thu, 7 Jul 2022 14:39:35 +0800 | Subject | Re: [PATCH v2] LoongArch: Clean useless vcsr in loongarch_fpu. | From | Qi Hu <> |
| |
On 2022/7/7 14:24, WANG Xuerui wrote: > > On 2022/7/7 14:09, Qi Hu wrote: >> >> On 2022/7/7 12:22, WANG Xuerui wrote: >>> On 2022/7/7 12:04, Xi Ruoyao wrote: >>>> On Thu, 2022-07-07 at 11:05 +0800, WANG Xuerui wrote: >>>> >>>>> To be frank, at this point I think you're trying to hide something. >>>>> (This is not your fault, blame someone else of course because they >>>>> told >>>>> you the fact.) In the old-world kernel the VCSR a.k.a. FCSR16 is >>>>> certainly being saved/restored, and there's apparently no harm in >>>>> doing >>>>> so. And if the contents are indeed "undefined", why are the code >>>>> there >>>>> in the first place? Certainly the bits *are* meaningful, only that >>>>> for >>>>> some reason you aren't revealing the semantics and pretending that >>>>> they >>>>> are "undefined" and probably "do nothing externally observable" if >>>>> accessed in the first place. >>>> On a 3A5000LL, I did an experiment via a kernel module, which enables >>>> LSX/LASX and tries to write and read fcsr16. I tried each bit (1, >>>> 2, 4, >>>> 8, ..., 1 << 31) one by one. The result: no matter which bit I wrote >>>> into fcsr16, I always read out 0. >>>> >>>> And I've objdump'ed a kernel shipped in an early Loongnix release. It >>>> seems the only reference to fcsr16 is a "movgr2fcsr $r16, $r0" >>>> instruction. >>> >>> Hmm this is weird. I can't understand why the vcsr code was there in >>> the first place then... I'd like to check a few Loongnix/Kylin/UOS >>> kernels but currently I don't have the time. >>> >>> If this is the case, indeed all vcsr-related code should be removed. >>> Although I'm still not sure how to best word the commit message. >>> >> Thanks for the Ruoyao's experiment. >> >> Removing the vcsr is the first step to trying to support LBT and >> LSX/LASX in Kernel. In my opinion, the vcsr relevant code may be used >> for debugging and forgot to remove. >> > Thinking about it harder, it actually makes sense. Given that access > to the LSX/LASX manuals is currently restricted, outsiders can never > know whether the code in question is really needed, so one has to err > on the conservative side. Thanks for the clarification, and my > apologies for being harsh in the previous reply. > > I think the commit message could be reworded like: > > "The VCSR (also known as FCSR16) is not present on retail steppings of > 3A5000. FCSR16 through FCSR31 are reserved for non-floating-point > LSX/LASX operations, but on 3A5000 all writes to them are no-op and > all reads return zero. FP LSX/LASX operations reuse the FCSR0 as their > CSR. > > Remove VCSR handling that is probably leftover debugging code for an > earlier not-for-retail stepping." > > (And remove the trailing period in your patch title, while at it; the > Linux kernel doesn't use a trailing period for majority of its commits.) > Thanks for reminding me about this. It is my first time committing the patch to Linux Kernel, and the commit message will be modified at V4 patch.
-
Qi
| |