diff mbox series

[-fixes,v2,4/4] riscv: Save/restore envcfg CSR during CPU suspend

Message ID 20240213033744.4069020-5-samuel.holland@sifive.com
State New
Headers
Series riscv: cbo.zero fixes |

Commit Message

Samuel Holland Feb. 13, 2024, 3:37 a.m. UTC 
  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(+)

Comments

Andrew Jones Feb. 13, 2024, 2:49 p.m. UTC | #1 
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.

Thanks,
drew
Stefan O'Rear Feb. 13, 2024, 5:53 p.m. UTC | #2 
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.

-s

> Thanks,
> drew
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Samuel Holland Feb. 18, 2024, 2:09 p.m. UTC | #3 
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
diff mbox series

Patch

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