| Message ID | 20240213033744.4069020-5-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: cbo.zero fixes | expand |
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
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
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 --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);
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(+)