From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailbox.box.xen0n.name (mail.xen0n.name [115.28.160.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D25537C for ; Thu, 7 Jul 2022 06:24:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=xen0n.name; s=mail; t=1657175090; bh=8KkOQg6Np0sKzvI4n9mwASctGzwr8vj+dzI40q2/t8o=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=j/Dq+BQMmoqtXk9l5OFH8rgHvcHVw4JU8RYJ4yq9hJJF682Opyf0d13ag70wtpAT6 rkMTF9/06gOztj1ICLRD2Y774WEcuYEx5ozZmvUirAtnZMQf1vdhtNhOMLatQt4jbY RzcQCAPzOlmQPZacrVZdO71+1w55qWbIdoUPQy4E= Received: from [100.100.57.190] (unknown [220.248.53.61]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mailbox.box.xen0n.name (Postfix) with ESMTPSA id C680D60114; Thu, 7 Jul 2022 14:24:49 +0800 (CST) Message-ID: Date: Thu, 7 Jul 2022 14:24:49 +0800 Precedence: bulk X-Mailing-List: loongarch@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:104.0) Gecko/20100101 Thunderbird/104.0a1 Subject: Re: [PATCH v2] LoongArch: Clean useless vcsr in loongarch_fpu. Content-Language: en-US To: Qi Hu , WANG Xuerui , Xi Ruoyao , Jiaxun Yang , Huacai Chen Cc: loongarch@lists.linux.dev, LKML References: <20220704153612.314112-1-huqi@loongson.cn> <4273e104-8392-6a06-5d18-a1933978d8c3@xen0n.name> <22a1ba993e298ce12a374decefebeca484240883.camel@xry111.site> <16c9ccaa5e5a2ffd39272cff6f66e487c659b571.camel@xry111.site> <9d064771-9402-4e84-96f8-4713cddf42f2@www.fastmail.com> <730cb4c4-a6a3-783e-3e4c-7c2bdc35c088@loongson.cn> <0583a335-72f7-55cf-3cd9-4dbd8109a440@xen0n.name> <41a2a420-adfa-6f8d-392d-0c15892b6945@loongson.cn> From: WANG Xuerui In-Reply-To: <41a2a420-adfa-6f8d-392d-0c15892b6945@loongson.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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.)