Received-SPF: pass (google.com: domain of linux-kernel+bounces-70404-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d947f326-e527-45aa-ae7f-bb5a18efa399@sifive.com>
Date: Sun, 18 Feb 2024 08:09:05 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH -fixes v2 4/4] riscv: Save/restore envcfg CSR during CPU
 suspend
Content-Language: en-US
To: Stefan O'Rear <sorear@fastmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, linux-riscv@lists.infradead.org,
 linux-kernel@vger.kernel.org, stable@vger.kernel.org,
 Andrew Jones <ajones@ventanamicro.com>
References: <20240213033744.4069020-1-samuel.holland@sifive.com>
 <20240213033744.4069020-5-samuel.holland@sifive.com>
 <20240213-86af3b49821630b5bdd76c0a@orel>
 <937f0593-0bc8-4df3-aa0e-9059acfd7636@app.fastmail.com>
From: Samuel Holland <samuel.holland@sifive.com>
In-Reply-To: <937f0593-0bc8-4df3-aa0e-9059acfd7636@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2024-02-13 11:53 AM, Stefan O'Rear wrote:
> On Tue, Feb 13, 2024, at 9:49 AM, Andrew Jones wrote:
>> On Mon, Feb 12, 2024 at 07:37:35PM -0800, Samuel Holland wrote:
>>> The value of the [ms]envcfg CSR is lost when entering a nonretentive
>>> idle state, so the CSR must be rewritten when resuming the CPU.
>>>
>>> Cc: <stable@vger.kernel.org> # v6.7+
>>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>> ---
>>>
>>> Changes in v2:
>>>  - Check for privileged ISA v1.12 instead of the specific CSR
>>>  - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s
>>>
>>>  arch/riscv/include/asm/suspend.h | 1 +
>>>  arch/riscv/kernel/suspend.c      | 4 ++++
>>>  2 files changed, 5 insertions(+)
>>>
>>> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
>>> index 02f87867389a..491296a335d0 100644
>>> --- a/arch/riscv/include/asm/suspend.h
>>> +++ b/arch/riscv/include/asm/suspend.h
>>> @@ -14,6 +14,7 @@ struct suspend_context {
>>>  	struct pt_regs regs;
>>>  	/* Saved and restored by high-level functions */
>>>  	unsigned long scratch;
>>> +	unsigned long envcfg;
>>>  	unsigned long tvec;
>>>  	unsigned long ie;
>>>  #ifdef CONFIG_MMU
>>> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
>>> index 239509367e42..be03615486ed 100644
>>> --- a/arch/riscv/kernel/suspend.c
>>> +++ b/arch/riscv/kernel/suspend.c
>>> @@ -15,6 +15,8 @@
>>>  void suspend_save_csrs(struct suspend_context *context)
>>>  {
>>>  	context->scratch = csr_read(CSR_SCRATCH);
>>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
>>> +		context->envcfg = csr_read(CSR_ENVCFG);
>>>  	context->tvec = csr_read(CSR_TVEC);
>>>  	context->ie = csr_read(CSR_IE);
>>>  
>>> @@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
>>>  void suspend_restore_csrs(struct suspend_context *context)
>>>  {
>>>  	csr_write(CSR_SCRATCH, context->scratch);
>>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
>>> +		csr_write(CSR_ENVCFG, context->envcfg);
>>>  	csr_write(CSR_TVEC, context->tvec);
>>>  	csr_write(CSR_IE, context->ie);
>>>  
>>> -- 
>>> 2.43.0
>>>
>>
>> We're still exposing Zicboz to userspace in hwprobe when only
>> RISCV_ISA_EXT_ZICBOZ is present, which will be the case for anything that
>> either doesn't implement 1.12, but does implement Zicboz, or maybe does
>> implement 1.12, but hasn't started putting Ss1p12 in its ISA string yet
>> (e.g. QEMU). We should either stop exposing Zicboz to userspace in those
>> cases (since it won't work) or rethink how we want to determine whether
>> or not we have envcfg CSRs.
> 
> opensbi treats the existence of menvcfg as sufficient evidence to prove that
> the hart supports 1.12.  I wouldn't object to having Zicboz imply Ss1p12/Sm1p12.

Zicboz implies menvcfg, yes, but I don't think menvcfg is sufficient to imply
S[ms]1p12. It's entirely possible for hardware to implement menvcfg but none of
the other S[ms]1p12 features. Or it may attempt to implement S[ms]1p12, but have
some bug that prevents it from being compliant, and therefore prevents declaring
support in the devicetree/ISA string.

What I think might work best is to have a cpufeature flag for "has the envcfg
CSR" that doesn't map to anything in the devicetree/ISA string. Then Zicboz can
imply envcfg, and Ss1p12 can imply envcfg, but we avoid any possible false
implications going the other direction.

Regards,
Samuel