Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a8780a34-2926-ab6a-ca1d-419dceee5ad1@loongson.cn>
Date:   Thu, 7 Jul 2022 14:39:35 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2] LoongArch: Clean useless vcsr in loongarch_fpu.
Content-Language: en-US
To:     WANG Xuerui <kernel@xen0n.name>, Xi Ruoyao <xry111@xry111.site>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Huacai Chen <chenhuacai@kernel.org>
Cc:     loongarch@lists.linux.dev, LKML <linux-kernel@vger.kernel.org>
References: <20220704153612.314112-1-huqi@loongson.cn>
 <4273e104-8392-6a06-5d18-a1933978d8c3@xen0n.name>
 <22a1ba993e298ce12a374decefebeca484240883.camel@xry111.site>
 <16c9ccaa5e5a2ffd39272cff6f66e487c659b571.camel@xry111.site>
 <CAAhV-H5+qd1ZrOqE8fgRmWshXy57AfEFpyKSK8ZstZZEQ53owQ@mail.gmail.com>
 <ac46f5cb4c8d1154cfc3e862fb5211e869839c9a.camel@xry111.site>
 <c824b9ca-c9c4-1912-7845-99a0989277a4@loongson.cn>
 <9d064771-9402-4e84-96f8-4713cddf42f2@www.fastmail.com>
 <730cb4c4-a6a3-783e-3e4c-7c2bdc35c088@loongson.cn>
 <0583a335-72f7-55cf-3cd9-4dbd8109a440@xen0n.name>
 <bd889cd7b72138a12b1339a33156ff46530c20b0.camel@xry111.site>
 <e67e03ae-e2d4-79e6-7f6a-5558ea9de52b@xen0n.name>
 <41a2a420-adfa-6f8d-392d-0c15892b6945@loongson.cn>
 <c5ed4251-04e3-c8a3-c19a-ff4fee469371@xen0n.name>
From:   Qi Hu <huqi@loongson.cn>
In-Reply-To: <c5ed4251-04e3-c8a3-c19a-ff4fee469371@xen0n.name>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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