[PATCH -fixes v3 0/2] riscv: cbo.zero fixes

[PATCH -fixes v3 0/2] riscv: cbo.zero fixes

[Samuel Holland]

This series fixes a couple of issues related to using the cbo.zero
instruction in userspace. The first patch fixes a bug where the wrong
enable bit gets set if the kernel is running in M-mode. The second
patch fixes a bug where the enable bit gets reset to its default value
after a nonretentive idle state. I have hardware which reproduces this:

Before this series (or without ss1p12 in the devicetree):
  $ tools/testing/selftests/riscv/hwprobe/cbo
  TAP version 13
  1..3
  ok 1 Zicboz block size
  # Zicboz block size: 64
  Illegal instruction

After applying this series:
  $ tools/testing/selftests/riscv/hwprobe/cbo
  TAP version 13
  1..3
  ok 1 Zicboz block size
  # Zicboz block size: 64
  ok 2 cbo.zero
  ok 3 cbo.zero check
  # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0

Changes in v3:
 - Drop patches added in v2
 - Check for Zicboz instead of the privileged ISA version

Changes in v2:
 - Add patches to allow parsing the privileged ISA version from the DT
 - Check for privileged ISA v1.12 instead of the specific CSR
 - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s

Samuel Holland (2):
  riscv: Fix enabling cbo.zero when running in M-mode
  riscv: Save/restore envcfg CSR during CPU suspend

 arch/riscv/include/asm/csr.h     | 2 ++
 arch/riscv/include/asm/suspend.h | 1 +
 arch/riscv/kernel/cpufeature.c   | 2 +-
 arch/riscv/kernel/suspend.c      | 4 ++++
 4 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH -fixes v3 1/2] riscv: Fix enabling cbo.zero when running in M-mode

[Samuel Holland]

When the kernel is running in M-mode, the CBZE bit must be set in the
menvcfg CSR, not in senvcfg.

Cc: <stable@vger.kernel.org>
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/include/asm/csr.h   | 2 ++
 arch/riscv/kernel/cpufeature.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 510014051f5d..2468c55933cd 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -424,6 +424,7 @@
 # define CSR_STATUS	CSR_MSTATUS
 # define CSR_IE		CSR_MIE
 # define CSR_TVEC	CSR_MTVEC
+# define CSR_ENVCFG	CSR_MENVCFG
 # define CSR_SCRATCH	CSR_MSCRATCH
 # define CSR_EPC	CSR_MEPC
 # define CSR_CAUSE	CSR_MCAUSE
@@ -448,6 +449,7 @@
 # define CSR_STATUS	CSR_SSTATUS
 # define CSR_IE		CSR_SIE
 # define CSR_TVEC	CSR_STVEC
+# define CSR_ENVCFG	CSR_SENVCFG
 # define CSR_SCRATCH	CSR_SSCRATCH
 # define CSR_EPC	CSR_SEPC
 # define CSR_CAUSE	CSR_SCAUSE
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 89920f84d0a3..c5b13f7dd482 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -950,7 +950,7 @@ arch_initcall(check_unaligned_access_all_cpus);
 void riscv_user_isa_enable(void)
 {
 	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
-		csr_set(CSR_SENVCFG, ENVCFG_CBZE);
+		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
 }
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH -fixes v3 2/2] riscv: Save/restore envcfg CSR during CPU suspend

[Samuel Holland]

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.

The [ms]envcfg CSR was added in version 1.12 of the privileged ISA, and
is used by extensions other than Zicboz. However, the kernel currenly
has no way to determine the privileged ISA version. Since Zicboz is the
only in-kernel user of this CSR so far, use it as a proxy for
determining if the CSR is implemented.

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 v3:
 - Check for Zicboz instead of the privileged ISA version

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..28166006688e 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_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
+		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_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
+		csr_write(CSR_ENVCFG, context->envcfg);
 	csr_write(CSR_TVEC, context->tvec);
 	csr_write(CSR_IE, context->ie);
 
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v3 1/2] riscv: Fix enabling cbo.zero when running in M-mode

[Andrew Jones]

On Wed, Feb 14, 2024 at 01:01:56AM -0800, Samuel Holland wrote:
> When the kernel is running in M-mode, the CBZE bit must be set in the
> menvcfg CSR, not in senvcfg.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> (no changes since v1)
> 
>  arch/riscv/include/asm/csr.h   | 2 ++
>  arch/riscv/kernel/cpufeature.c | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 510014051f5d..2468c55933cd 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -424,6 +424,7 @@
>  # define CSR_STATUS	CSR_MSTATUS
>  # define CSR_IE		CSR_MIE
>  # define CSR_TVEC	CSR_MTVEC
> +# define CSR_ENVCFG	CSR_MENVCFG
>  # define CSR_SCRATCH	CSR_MSCRATCH
>  # define CSR_EPC	CSR_MEPC
>  # define CSR_CAUSE	CSR_MCAUSE
> @@ -448,6 +449,7 @@
>  # define CSR_STATUS	CSR_SSTATUS
>  # define CSR_IE		CSR_SIE
>  # define CSR_TVEC	CSR_STVEC
> +# define CSR_ENVCFG	CSR_SENVCFG
>  # define CSR_SCRATCH	CSR_SSCRATCH
>  # define CSR_EPC	CSR_SEPC
>  # define CSR_CAUSE	CSR_SCAUSE
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 89920f84d0a3..c5b13f7dd482 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -950,7 +950,7 @@ arch_initcall(check_unaligned_access_all_cpus);
>  void riscv_user_isa_enable(void)
>  {
>  	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
> -		csr_set(CSR_SENVCFG, ENVCFG_CBZE);
> +		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>  }
>  
>  #ifdef CONFIG_RISCV_ALTERNATIVE
> -- 
> 2.43.0
>

After our back and forth on how we determine the existence of the *envcfg
CSRs, I wonder if we shouldn't put a comment above this
riscv_user_isa_enable() function capturing the [current] decision.

Something like

 /*
  * While the [ms]envcfg CSRs weren't defined until priv spec 1.12,
  * they're assumed to be present when an extension is present which
  * specifies [ms]envcfg bit(s). Hence, we don't do any additional
  * priv spec version checks or CSR probes here.
  */

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v3 2/2] riscv: Save/restore envcfg CSR during CPU suspend

[Andrew Jones]

On Wed, Feb 14, 2024 at 01:01:57AM -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.
> 
> The [ms]envcfg CSR was added in version 1.12 of the privileged ISA, and
> is used by extensions other than Zicboz. However, the kernel currenly
> has no way to determine the privileged ISA version. Since Zicboz is the
> only in-kernel user of this CSR so far, use it as a proxy for
> determining if the CSR is implemented.
> 
> 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 v3:
>  - Check for Zicboz instead of the privileged ISA version
> 
> 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..28166006688e 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_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
> +		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_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
> +		csr_write(CSR_ENVCFG, context->envcfg);
>  	csr_write(CSR_TVEC, context->tvec);
>  	csr_write(CSR_IE, context->ie);
>  
> -- 
> 2.43.0
>


Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v3 1/2] riscv: Fix enabling cbo.zero when running in M-mode

[Alexandre Ghiti]

Hi Samuel,

On 14/02/2024 10:28, Andrew Jones wrote:
> On Wed, Feb 14, 2024 at 01:01:56AM -0800, Samuel Holland wrote:
>> When the kernel is running in M-mode, the CBZE bit must be set in the
>> menvcfg CSR, not in senvcfg.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>> (no changes since v1)
>>
>>   arch/riscv/include/asm/csr.h   | 2 ++
>>   arch/riscv/kernel/cpufeature.c | 2 +-
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>> index 510014051f5d..2468c55933cd 100644
>> --- a/arch/riscv/include/asm/csr.h
>> +++ b/arch/riscv/include/asm/csr.h
>> @@ -424,6 +424,7 @@
>>   # define CSR_STATUS	CSR_MSTATUS
>>   # define CSR_IE		CSR_MIE
>>   # define CSR_TVEC	CSR_MTVEC
>> +# define CSR_ENVCFG	CSR_MENVCFG
>>   # define CSR_SCRATCH	CSR_MSCRATCH
>>   # define CSR_EPC	CSR_MEPC
>>   # define CSR_CAUSE	CSR_MCAUSE
>> @@ -448,6 +449,7 @@
>>   # define CSR_STATUS	CSR_SSTATUS
>>   # define CSR_IE		CSR_SIE
>>   # define CSR_TVEC	CSR_STVEC
>> +# define CSR_ENVCFG	CSR_SENVCFG
>>   # define CSR_SCRATCH	CSR_SSCRATCH
>>   # define CSR_EPC	CSR_SEPC
>>   # define CSR_CAUSE	CSR_SCAUSE
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 89920f84d0a3..c5b13f7dd482 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -950,7 +950,7 @@ arch_initcall(check_unaligned_access_all_cpus);
>>   void riscv_user_isa_enable(void)
>>   {
>>   	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
>> -		csr_set(CSR_SENVCFG, ENVCFG_CBZE);
>> +		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>>   }
>>   
>>   #ifdef CONFIG_RISCV_ALTERNATIVE
>> -- 
>> 2.43.0
>>
> After our back and forth on how we determine the existence of the *envcfg
> CSRs, I wonder if we shouldn't put a comment above this
> riscv_user_isa_enable() function capturing the [current] decision.
>
> Something like
>
>   /*
>    * While the [ms]envcfg CSRs weren't defined until priv spec 1.12,
>    * they're assumed to be present when an extension is present which
>    * specifies [ms]envcfg bit(s). Hence, we don't do any additional
>    * priv spec version checks or CSR probes here.
>    */


I was about to read the whole discussion in v2 to understand the 
v3...thank you Drew :) I think it really makes sense to add this 
comment, do you intend to do so Samuel?

Thanks,

Alex


>
> Thanks,
> drew
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v3 1/2] riscv: Fix enabling cbo.zero when running in M-mode

[Samuel Holland]

Hi Alex,

On 2024-02-27 1:48 PM, Alexandre Ghiti wrote:
> Hi Samuel,
> 
> On 14/02/2024 10:28, Andrew Jones wrote:
>> On Wed, Feb 14, 2024 at 01:01:56AM -0800, Samuel Holland wrote:
>>> When the kernel is running in M-mode, the CBZE bit must be set in the
>>> menvcfg CSR, not in senvcfg.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>   arch/riscv/include/asm/csr.h   | 2 ++
>>>   arch/riscv/kernel/cpufeature.c | 2 +-
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>>> index 510014051f5d..2468c55933cd 100644
>>> --- a/arch/riscv/include/asm/csr.h
>>> +++ b/arch/riscv/include/asm/csr.h
>>> @@ -424,6 +424,7 @@
>>>   # define CSR_STATUS    CSR_MSTATUS
>>>   # define CSR_IE        CSR_MIE
>>>   # define CSR_TVEC    CSR_MTVEC
>>> +# define CSR_ENVCFG    CSR_MENVCFG
>>>   # define CSR_SCRATCH    CSR_MSCRATCH
>>>   # define CSR_EPC    CSR_MEPC
>>>   # define CSR_CAUSE    CSR_MCAUSE
>>> @@ -448,6 +449,7 @@
>>>   # define CSR_STATUS    CSR_SSTATUS
>>>   # define CSR_IE        CSR_SIE
>>>   # define CSR_TVEC    CSR_STVEC
>>> +# define CSR_ENVCFG    CSR_SENVCFG
>>>   # define CSR_SCRATCH    CSR_SSCRATCH
>>>   # define CSR_EPC    CSR_SEPC
>>>   # define CSR_CAUSE    CSR_SCAUSE
>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>> index 89920f84d0a3..c5b13f7dd482 100644
>>> --- a/arch/riscv/kernel/cpufeature.c
>>> +++ b/arch/riscv/kernel/cpufeature.c
>>> @@ -950,7 +950,7 @@ arch_initcall(check_unaligned_access_all_cpus);
>>>   void riscv_user_isa_enable(void)
>>>   {
>>>       if (riscv_cpu_has_extension_unlikely(smp_processor_id(),
>>> RISCV_ISA_EXT_ZICBOZ))
>>> -        csr_set(CSR_SENVCFG, ENVCFG_CBZE);
>>> +        csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>>>   }
>>>     #ifdef CONFIG_RISCV_ALTERNATIVE
>>> -- 
>>> 2.43.0
>>>
>> After our back and forth on how we determine the existence of the *envcfg
>> CSRs, I wonder if we shouldn't put a comment above this
>> riscv_user_isa_enable() function capturing the [current] decision.
>>
>> Something like
>>
>>   /*
>>    * While the [ms]envcfg CSRs weren't defined until priv spec 1.12,
>>    * they're assumed to be present when an extension is present which
>>    * specifies [ms]envcfg bit(s). Hence, we don't do any additional
>>    * priv spec version checks or CSR probes here.
>>    */
> 
> 
> I was about to read the whole discussion in v2 to understand the v3...thank you
> Drew :) I think it really makes sense to add this comment, do you intend to do
> so Samuel?

Yes, I am about to send a v4 with the changes from the previous discussion,
including a RISCV_ISA_EXT_XLINUXENVCFG bit specifically for the presence of the
[ms]envcfg CSR and a comment like the above.

Regards,
Samuel


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv