Re: [PATCH v2] LoongArch: Clean useless vcsr in loongarch_fpu.

[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