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
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: <[email protected]> # v6.7+
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Signed-off-by: Samuel Holland <[email protected]>
---
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
When the kernel is running in M-mode, the CBZE bit must be set in the
menvcfg CSR, not in senvcfg.
Cc: <[email protected]>
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Reviewed-by: Andrew Jones <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---
(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
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: <[email protected]>
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Reviewed-by: Andrew Jones <[email protected]>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> (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
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: <[email protected]> # v6.7+
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> 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 <[email protected]>
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: <[email protected]>
>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>> Reviewed-by: Andrew Jones <[email protected]>
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>>
>> (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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
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: <[email protected]>
>>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>>> Reviewed-by: Andrew Jones <[email protected]>
>>> Signed-off-by: Samuel Holland <[email protected]>
>>> ---
>>>
>>> (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