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

[WANG Xuerui <kernel@xen0n.name>]

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.)