[PATCH 0/3] riscv: Per-thread envcfg CSR support

[PATCH 0/3] riscv: Per-thread envcfg CSR support

[Samuel Holland]

This series (or equivalent) is a prerequisite for both user-mode pointer
masking and CFI support, as those are per-thread features are controlled
by fields in the envcfg CSR. These patches are based on v1 of the
pointer masking series[1], with significant input from both Deepak and
Andrew. By sending this as a separate series, hopefully we can converge
on a single implementation of this functionality.

[1]: https://lore.kernel.org/linux-riscv/20240319215915.832127-6-samuel.holland@sifive.com/


Samuel Holland (3):
  riscv: Enable cbo.zero only when all harts support Zicboz
  riscv: Add support for per-thread envcfg CSR values
  riscv: Call riscv_user_isa_enable() only on the boot hart

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/include/asm/processor.h  |  1 +
 arch/riscv/include/asm/switch_to.h  |  8 ++++++++
 arch/riscv/kernel/cpufeature.c      | 13 +++++++++----
 arch/riscv/kernel/smpboot.c         |  2 --
 arch/riscv/kernel/suspend.c         |  4 ++--
 6 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.44.1

[PATCH 1/3] riscv: Enable cbo.zero only when all harts support Zicboz

[Samuel Holland]

Currently, we enable cbo.zero for usermode on each hart that supports
the Zicboz extension. This means that the [ms]envcfg CSR value may
differ between harts. Other features, such as pointer masking and CFI,
require setting [ms]envcfg bits on a per-thread basis. The combination
of these two adds quite some complexity and overhead to context
switching, as we would need to maintain two separate masks for the
per-hart and per-thread bits. Andrew Jones, who originally added Zicboz
support, writes[1][2]:

  I've approached Zicboz the same way I would approach all
  extensions, which is to be per-hart. I'm not currently aware of
  a platform that is / will be composed of harts where some have
  Zicboz and others don't, but there's nothing stopping a platform
  like that from being built.

  So, how about we add code that confirms Zicboz is on all harts.
  If any hart does not have it, then we complain loudly and disable
  it on all the other harts. If it was just a hardware description
  bug, then it'll get fixed. If there's actually a platform which
  doesn't have Zicboz on all harts, then, when the issue is reported,
  we can decide to not support it, support it with defconfig, or
  support it under a Kconfig guard which must be enabled by the user.

Let's follow his suggested solution and require the extension to be
available on all harts, so the envcfg CSR value does not need to change
when a thread migrates between harts. Since we are doing this for all
extensions with fields in envcfg, the CSR itself only needs to be saved/
restored when it is present on all harts.

This should not be a regression as no known hardware has asymmetric
Zicboz support, but if anyone reports seeing the warning, we will
re-evaluate our solution.

Link: https://lore.kernel.org/linux-riscv/20240322-168f191eeb8479b2ea169a5e@orel/ [1]
Link: https://lore.kernel.org/linux-riscv/20240323-28943722feb57a41fb0ff488@orel/ [2]
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

 arch/riscv/kernel/cpufeature.c | 7 ++++++-
 arch/riscv/kernel/suspend.c    | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 5ef48cb20ee1..2879e26dbcd8 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -27,6 +27,8 @@
 
 #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
 
+static bool any_cpu_has_zicboz;
+
 unsigned long elf_hwcap __read_mostly;
 
 /* Host ISA bitmap */
@@ -92,6 +94,7 @@ static bool riscv_isa_extension_check(int id)
 			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
 			return false;
 		}
+		any_cpu_has_zicboz = true;
 		return true;
 	case RISCV_ISA_EXT_INVALID:
 		return false;
@@ -724,8 +727,10 @@ unsigned long riscv_get_elf_hwcap(void)
 
 void riscv_user_isa_enable(void)
 {
-	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
 		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
+	else if (any_cpu_has_zicboz)
+		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
 }
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index c8cec0cc5833..9a8a0dc035b2 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -14,7 +14,7 @@
 
 void suspend_save_csrs(struct suspend_context *context)
 {
-	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_XLINUXENVCFG))
 		context->envcfg = csr_read(CSR_ENVCFG);
 	context->tvec = csr_read(CSR_TVEC);
 	context->ie = csr_read(CSR_IE);
@@ -37,7 +37,7 @@ void suspend_save_csrs(struct suspend_context *context)
 void suspend_restore_csrs(struct suspend_context *context)
 {
 	csr_write(CSR_SCRATCH, 0);
-	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_XLINUXENVCFG))
 		csr_write(CSR_ENVCFG, context->envcfg);
 	csr_write(CSR_TVEC, context->tvec);
 	csr_write(CSR_IE, context->ie);
-- 
2.44.1

Re: [PATCH 1/3] riscv: Enable cbo.zero only when all harts support Zicboz

[Deepak Gupta]

On Wed, Jun 05, 2024 at 01:56:45PM -0700, Samuel Holland wrote:
>Currently, we enable cbo.zero for usermode on each hart that supports
>the Zicboz extension. This means that the [ms]envcfg CSR value may
>differ between harts. Other features, such as pointer masking and CFI,
>require setting [ms]envcfg bits on a per-thread basis. The combination
>of these two adds quite some complexity and overhead to context
>switching, as we would need to maintain two separate masks for the
>per-hart and per-thread bits. Andrew Jones, who originally added Zicboz
>support, writes[1][2]:
>
>  I've approached Zicboz the same way I would approach all
>  extensions, which is to be per-hart. I'm not currently aware of
>  a platform that is / will be composed of harts where some have
>  Zicboz and others don't, but there's nothing stopping a platform
>  like that from being built.
>
>  So, how about we add code that confirms Zicboz is on all harts.
>  If any hart does not have it, then we complain loudly and disable
>  it on all the other harts. If it was just a hardware description
>  bug, then it'll get fixed. If there's actually a platform which
>  doesn't have Zicboz on all harts, then, when the issue is reported,
>  we can decide to not support it, support it with defconfig, or
>  support it under a Kconfig guard which must be enabled by the user.
>
>Let's follow his suggested solution and require the extension to be
>available on all harts, so the envcfg CSR value does not need to change
>when a thread migrates between harts. Since we are doing this for all
>extensions with fields in envcfg, the CSR itself only needs to be saved/
>restored when it is present on all harts.
>
>This should not be a regression as no known hardware has asymmetric
>Zicboz support, but if anyone reports seeing the warning, we will
>re-evaluate our solution.
>
>Link: https://lore.kernel.org/linux-riscv/20240322-168f191eeb8479b2ea169a5e@orel/ [1]
>Link: https://lore.kernel.org/linux-riscv/20240323-28943722feb57a41fb0ff488@orel/ [2]
>Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>---
>
> arch/riscv/kernel/cpufeature.c | 7 ++++++-
> arch/riscv/kernel/suspend.c    | 4 ++--
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
>diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>index 5ef48cb20ee1..2879e26dbcd8 100644
>--- a/arch/riscv/kernel/cpufeature.c
>+++ b/arch/riscv/kernel/cpufeature.c
>@@ -27,6 +27,8 @@
>
> #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>
>+static bool any_cpu_has_zicboz;
>+
> unsigned long elf_hwcap __read_mostly;
>
> /* Host ISA bitmap */
>@@ -92,6 +94,7 @@ static bool riscv_isa_extension_check(int id)
> 			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
> 			return false;
> 		}
>+		any_cpu_has_zicboz = true;
> 		return true;
> 	case RISCV_ISA_EXT_INVALID:
> 		return false;
>@@ -724,8 +727,10 @@ unsigned long riscv_get_elf_hwcap(void)
>
> void riscv_user_isa_enable(void)
> {
>-	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
>+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
> 		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>+	else if (any_cpu_has_zicboz)
>+		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");

`riscv_has_extension_unlikely` will check bitmap `riscv_isa[0]` which I think gets populated
by boot cpu (correct me if I am wrong here). So as long boot processor has the extension, it'll
try to set it on CPU which doesn't have it.

How about doing this

`riscv_fill_hwcap_from_isa_string` checks and enables bitmap for all CPUs.
So make a check there and if any of the CPU dont have `Zicboz`, then set a global bool
`zicboz_cpu_not_homogenous`.

Now in `riscv_user_isa_enable`, check following

If `zicboz_cpu_not_homogenous` is set, then you already detected that some of the CPUs don't
have support for `Zicboz` and thus you wouldn't set for CPU which even has the support and
print a warning message.

If `zicboz_cpu_not_homogenous` is clear, then that means all CPUs support the feature.
You simply enable it on hart.

> }
>
> #ifdef CONFIG_RISCV_ALTERNATIVE
>diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
>index c8cec0cc5833..9a8a0dc035b2 100644
>--- a/arch/riscv/kernel/suspend.c
>+++ b/arch/riscv/kernel/suspend.c
>@@ -14,7 +14,7 @@
>
> void suspend_save_csrs(struct suspend_context *context)
> {
>-	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
>+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_XLINUXENVCFG))
> 		context->envcfg = csr_read(CSR_ENVCFG);
> 	context->tvec = csr_read(CSR_TVEC);
> 	context->ie = csr_read(CSR_IE);
>@@ -37,7 +37,7 @@ void suspend_save_csrs(struct suspend_context *context)
> void suspend_restore_csrs(struct suspend_context *context)
> {
> 	csr_write(CSR_SCRATCH, 0);
>-	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
>+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_XLINUXENVCFG))
> 		csr_write(CSR_ENVCFG, context->envcfg);
> 	csr_write(CSR_TVEC, context->tvec);
> 	csr_write(CSR_IE, context->ie);
>-- 
>2.44.1
>

Re: [PATCH 1/3] riscv: Enable cbo.zero only when all harts support Zicboz

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4669 bytes --]

On Fri, Jun 07, 2024 at 01:35:00PM -0700, Deepak Gupta wrote:
> On Wed, Jun 05, 2024 at 01:56:45PM -0700, Samuel Holland wrote:
> > Currently, we enable cbo.zero for usermode on each hart that supports
> > the Zicboz extension. This means that the [ms]envcfg CSR value may
> > differ between harts. Other features, such as pointer masking and CFI,
> > require setting [ms]envcfg bits on a per-thread basis. The combination
> > of these two adds quite some complexity and overhead to context
> > switching, as we would need to maintain two separate masks for the
> > per-hart and per-thread bits. Andrew Jones, who originally added Zicboz
> > support, writes[1][2]:
> > 
> >  I've approached Zicboz the same way I would approach all
> >  extensions, which is to be per-hart. I'm not currently aware of
> >  a platform that is / will be composed of harts where some have
> >  Zicboz and others don't, but there's nothing stopping a platform
> >  like that from being built.
> > 
> >  So, how about we add code that confirms Zicboz is on all harts.
> >  If any hart does not have it, then we complain loudly and disable
> >  it on all the other harts. If it was just a hardware description
> >  bug, then it'll get fixed. If there's actually a platform which
> >  doesn't have Zicboz on all harts, then, when the issue is reported,
> >  we can decide to not support it, support it with defconfig, or
> >  support it under a Kconfig guard which must be enabled by the user.
> > 
> > Let's follow his suggested solution and require the extension to be
> > available on all harts, so the envcfg CSR value does not need to change
> > when a thread migrates between harts. Since we are doing this for all
> > extensions with fields in envcfg, the CSR itself only needs to be saved/
> > restored when it is present on all harts.
> > 
> > This should not be a regression as no known hardware has asymmetric
> > Zicboz support, but if anyone reports seeing the warning, we will
> > re-evaluate our solution.
> > 
> > Link: https://lore.kernel.org/linux-riscv/20240322-168f191eeb8479b2ea169a5e@orel/ [1]
> > Link: https://lore.kernel.org/linux-riscv/20240323-28943722feb57a41fb0ff488@orel/ [2]
> > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > ---
> > 
> > arch/riscv/kernel/cpufeature.c | 7 ++++++-
> > arch/riscv/kernel/suspend.c    | 4 ++--
> > 2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 5ef48cb20ee1..2879e26dbcd8 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -27,6 +27,8 @@
> > 
> > #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> > 
> > +static bool any_cpu_has_zicboz;
> > +
> > unsigned long elf_hwcap __read_mostly;
> > 
> > /* Host ISA bitmap */
> > @@ -92,6 +94,7 @@ static bool riscv_isa_extension_check(int id)
> > 			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
> > 			return false;
> > 		}
> > +		any_cpu_has_zicboz = true;
> > 		return true;
> > 	case RISCV_ISA_EXT_INVALID:
> > 		return false;
> > @@ -724,8 +727,10 @@ unsigned long riscv_get_elf_hwcap(void)
> > 
> > void riscv_user_isa_enable(void)
> > {
> > -	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
> > +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
> > 		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
> > +	else if (any_cpu_has_zicboz)
> > +		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
> 
> `riscv_has_extension_unlikely` will check bitmap `riscv_isa[0]` which I think gets populated
> by boot cpu (correct me if I am wrong here). So as long boot processor has the extension, it'll
> try to set it on CPU which doesn't have it.
> 
> How about doing this
> 
> `riscv_fill_hwcap_from_isa_string` checks and enables bitmap for all CPUs.
> So make a check there and if any of the CPU dont have `Zicboz`, then set a global bool
> `zicboz_cpu_not_homogenous`.

That is what riscv_fill_hwcap.*() already does, we track both what each
cpu has and what is common across all cpus.
riscv_has_extension_[un]likely() is a test for whether all cpus have the
extension.

> Now in `riscv_user_isa_enable`, check following
> 
> If `zicboz_cpu_not_homogenous` is set, then you already detected that some of the CPUs don't
> have support for `Zicboz` and thus you wouldn't set for CPU which even has the support and
> print a warning message.
> 
> If `zicboz_cpu_not_homogenous` is clear, then that means all CPUs support the feature.
> You simply enable it on hart.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] riscv: Enable cbo.zero only when all harts support Zicboz

[Deepak Gupta]

On Fri, Jun 07, 2024 at 09:39:50PM +0100, Conor Dooley wrote:
>On Fri, Jun 07, 2024 at 01:35:00PM -0700, Deepak Gupta wrote:
>> On Wed, Jun 05, 2024 at 01:56:45PM -0700, Samuel Holland wrote:
>> > Currently, we enable cbo.zero for usermode on each hart that supports
>> > the Zicboz extension. This means that the [ms]envcfg CSR value may
>> > differ between harts. Other features, such as pointer masking and CFI,
>> > require setting [ms]envcfg bits on a per-thread basis. The combination
>> > of these two adds quite some complexity and overhead to context
>> > switching, as we would need to maintain two separate masks for the
>> > per-hart and per-thread bits. Andrew Jones, who originally added Zicboz
>> > support, writes[1][2]:
>> >
>> >  I've approached Zicboz the same way I would approach all
>> >  extensions, which is to be per-hart. I'm not currently aware of
>> >  a platform that is / will be composed of harts where some have
>> >  Zicboz and others don't, but there's nothing stopping a platform
>> >  like that from being built.
>> >
>> >  So, how about we add code that confirms Zicboz is on all harts.
>> >  If any hart does not have it, then we complain loudly and disable
>> >  it on all the other harts. If it was just a hardware description
>> >  bug, then it'll get fixed. If there's actually a platform which
>> >  doesn't have Zicboz on all harts, then, when the issue is reported,
>> >  we can decide to not support it, support it with defconfig, or
>> >  support it under a Kconfig guard which must be enabled by the user.
>> >
>> > Let's follow his suggested solution and require the extension to be
>> > available on all harts, so the envcfg CSR value does not need to change
>> > when a thread migrates between harts. Since we are doing this for all
>> > extensions with fields in envcfg, the CSR itself only needs to be saved/
>> > restored when it is present on all harts.
>> >
>> > This should not be a regression as no known hardware has asymmetric
>> > Zicboz support, but if anyone reports seeing the warning, we will
>> > re-evaluate our solution.
>> >
>> > Link: https://lore.kernel.org/linux-riscv/20240322-168f191eeb8479b2ea169a5e@orel/ [1]
>> > Link: https://lore.kernel.org/linux-riscv/20240323-28943722feb57a41fb0ff488@orel/ [2]
>> > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> > ---
>> >
>> > arch/riscv/kernel/cpufeature.c | 7 ++++++-
>> > arch/riscv/kernel/suspend.c    | 4 ++--
>> > 2 files changed, 8 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> > index 5ef48cb20ee1..2879e26dbcd8 100644
>> > --- a/arch/riscv/kernel/cpufeature.c
>> > +++ b/arch/riscv/kernel/cpufeature.c
>> > @@ -27,6 +27,8 @@
>> >
>> > #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>> >
>> > +static bool any_cpu_has_zicboz;
>> > +
>> > unsigned long elf_hwcap __read_mostly;
>> >
>> > /* Host ISA bitmap */
>> > @@ -92,6 +94,7 @@ static bool riscv_isa_extension_check(int id)
>> > 			pr_err("Zicboz disabled as cboz-block-size present, but is not a power-of-2\n");
>> > 			return false;
>> > 		}
>> > +		any_cpu_has_zicboz = true;
>> > 		return true;
>> > 	case RISCV_ISA_EXT_INVALID:
>> > 		return false;
>> > @@ -724,8 +727,10 @@ unsigned long riscv_get_elf_hwcap(void)
>> >
>> > void riscv_user_isa_enable(void)
>> > {
>> > -	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
>> > +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
>> > 		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>> > +	else if (any_cpu_has_zicboz)
>> > +		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
>>
>> `riscv_has_extension_unlikely` will check bitmap `riscv_isa[0]` which I think gets populated
>> by boot cpu (correct me if I am wrong here). So as long boot processor has the extension, it'll
>> try to set it on CPU which doesn't have it.
>>
>> How about doing this
>>
>> `riscv_fill_hwcap_from_isa_string` checks and enables bitmap for all CPUs.
>> So make a check there and if any of the CPU dont have `Zicboz`, then set a global bool
>> `zicboz_cpu_not_homogenous`.
>
>That is what riscv_fill_hwcap.*() already does, we track both what each
>cpu has and what is common across all cpus.
>riscv_has_extension_[un]likely() is a test for whether all cpus have the
>extension.
>

Thanks for clarifying that.

Samuel,

Ignore my comment then.
This patch lgtm.

>> Now in `riscv_user_isa_enable`, check following
>>
>> If `zicboz_cpu_not_homogenous` is set, then you already detected that some of the CPUs don't
>> have support for `Zicboz` and thus you wouldn't set for CPU which even has the support and
>> print a warning message.
>>
>> If `zicboz_cpu_not_homogenous` is clear, then that means all CPUs support the feature.
>> You simply enable it on hart.

[PATCH 2/3] riscv: Add support for per-thread envcfg CSR values

[Samuel Holland]

Some bits in the [ms]envcfg CSR, such as the CFI state and pointer
masking mode, need to be controlled on a per-thread basis. Support this
by keeping a copy of the CSR value in struct thread_struct and writing
it during context switches. It is safe to discard the old CSR value
during the context switch because the CSR is modified only by software,
so the CSR will remain in sync with the copy in thread_struct.

Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to
minimize branchiness in the context switching code.

Since thread_struct is copied during fork(), setting the value for the
init task sets the default value for all other threads.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

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

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 68c3432dc6ea..0838922bd1c8 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -118,6 +118,7 @@ struct thread_struct {
 	unsigned long s[12];	/* s[0]: frame pointer */
 	struct __riscv_d_ext_state fstate;
 	unsigned long bad_cause;
+	unsigned long envcfg;
 	u32 riscv_v_flags;
 	u32 vstate_ctrl;
 	struct __riscv_v_ext_state vstate;
diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
index 7594df37cc9f..9685cd85e57c 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; }
 #define __switch_to_fpu(__prev, __next) do { } while (0)
 #endif
 
+static inline void __switch_to_envcfg(struct task_struct *next)
+{
+	asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0",
+				  0, RISCV_ISA_EXT_XLINUXENVCFG, 1)
+			:: "r" (next->thread.envcfg) : "memory");
+}
+
 extern struct task_struct *__switch_to(struct task_struct *,
 				       struct task_struct *);
 
@@ -103,6 +110,7 @@ do {							\
 		__switch_to_vector(__prev, __next);	\
 	if (switch_to_should_flush_icache(__next))	\
 		local_flush_icache_all();		\
+	__switch_to_envcfg(__next);			\
 	((last) = __switch_to(__prev, __next));		\
 } while (0)
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 2879e26dbcd8..1153b96346ae 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void)
 void riscv_user_isa_enable(void)
 {
 	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
-		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
+		current->thread.envcfg |= ENVCFG_CBZE;
 	else if (any_cpu_has_zicboz)
 		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
 }
-- 
2.44.1

Re: [PATCH 2/3] riscv: Add support for per-thread envcfg CSR values

[Deepak Gupta]

On Wed, Jun 05, 2024 at 01:56:46PM -0700, Samuel Holland wrote:
>Some bits in the [ms]envcfg CSR, such as the CFI state and pointer
>masking mode, need to be controlled on a per-thread basis. Support this
>by keeping a copy of the CSR value in struct thread_struct and writing
>it during context switches. It is safe to discard the old CSR value
>during the context switch because the CSR is modified only by software,
>so the CSR will remain in sync with the copy in thread_struct.
>
>Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to
>minimize branchiness in the context switching code.
>
>Since thread_struct is copied during fork(), setting the value for the
>init task sets the default value for all other threads.
>
>Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>---
>
> arch/riscv/include/asm/processor.h | 1 +
> arch/riscv/include/asm/switch_to.h | 8 ++++++++
> arch/riscv/kernel/cpufeature.c     | 2 +-
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
>diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
>index 68c3432dc6ea..0838922bd1c8 100644
>--- a/arch/riscv/include/asm/processor.h
>+++ b/arch/riscv/include/asm/processor.h
>@@ -118,6 +118,7 @@ struct thread_struct {
> 	unsigned long s[12];	/* s[0]: frame pointer */
> 	struct __riscv_d_ext_state fstate;
> 	unsigned long bad_cause;
>+	unsigned long envcfg;
> 	u32 riscv_v_flags;
> 	u32 vstate_ctrl;
> 	struct __riscv_v_ext_state vstate;
>diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
>index 7594df37cc9f..9685cd85e57c 100644
>--- a/arch/riscv/include/asm/switch_to.h
>+++ b/arch/riscv/include/asm/switch_to.h
>@@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; }
> #define __switch_to_fpu(__prev, __next) do { } while (0)
> #endif
>
>+static inline void __switch_to_envcfg(struct task_struct *next)
>+{
>+	asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0",
>+				  0, RISCV_ISA_EXT_XLINUXENVCFG, 1)
>+			:: "r" (next->thread.envcfg) : "memory");
>+}
>+
> extern struct task_struct *__switch_to(struct task_struct *,
> 				       struct task_struct *);
>
>@@ -103,6 +110,7 @@ do {							\
> 		__switch_to_vector(__prev, __next);	\
> 	if (switch_to_should_flush_icache(__next))	\
> 		local_flush_icache_all();		\
>+	__switch_to_envcfg(__next);			\
> 	((last) = __switch_to(__prev, __next));		\
> } while (0)

Suggestion:
Probably make this patch 1

>
>diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>index 2879e26dbcd8..1153b96346ae 100644
>--- a/arch/riscv/kernel/cpufeature.c
>+++ b/arch/riscv/kernel/cpufeature.c
>@@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void)
> void riscv_user_isa_enable(void)
> {
> 	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
>-		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>+		current->thread.envcfg |= ENVCFG_CBZE;

Suggestion:
Squash this with current patch 1 and call it patch 2.

> 	else if (any_cpu_has_zicboz)
> 		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
> }
>-- 
>2.44.1
>

Re: [PATCH 2/3] riscv: Add support for per-thread envcfg CSR values

[Samuel Holland]

Hi Deepak,

On 2024-06-07 4:59 PM, Deepak Gupta wrote:
> On Wed, Jun 05, 2024 at 01:56:46PM -0700, Samuel Holland wrote:
>> Some bits in the [ms]envcfg CSR, such as the CFI state and pointer
>> masking mode, need to be controlled on a per-thread basis. Support this
>> by keeping a copy of the CSR value in struct thread_struct and writing
>> it during context switches. It is safe to discard the old CSR value
>> during the context switch because the CSR is modified only by software,
>> so the CSR will remain in sync with the copy in thread_struct.
>>
>> Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to
>> minimize branchiness in the context switching code.
>>
>> Since thread_struct is copied during fork(), setting the value for the
>> init task sets the default value for all other threads.
>>
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>> arch/riscv/include/asm/processor.h | 1 +
>> arch/riscv/include/asm/switch_to.h | 8 ++++++++
>> arch/riscv/kernel/cpufeature.c     | 2 +-
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/processor.h
>> b/arch/riscv/include/asm/processor.h
>> index 68c3432dc6ea..0838922bd1c8 100644
>> --- a/arch/riscv/include/asm/processor.h
>> +++ b/arch/riscv/include/asm/processor.h
>> @@ -118,6 +118,7 @@ struct thread_struct {
>>     unsigned long s[12];    /* s[0]: frame pointer */
>>     struct __riscv_d_ext_state fstate;
>>     unsigned long bad_cause;
>> +    unsigned long envcfg;
>>     u32 riscv_v_flags;
>>     u32 vstate_ctrl;
>>     struct __riscv_v_ext_state vstate;
>> diff --git a/arch/riscv/include/asm/switch_to.h
>> b/arch/riscv/include/asm/switch_to.h
>> index 7594df37cc9f..9685cd85e57c 100644
>> --- a/arch/riscv/include/asm/switch_to.h
>> +++ b/arch/riscv/include/asm/switch_to.h
>> @@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; }
>> #define __switch_to_fpu(__prev, __next) do { } while (0)
>> #endif
>>
>> +static inline void __switch_to_envcfg(struct task_struct *next)
>> +{
>> +    asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0",
>> +                  0, RISCV_ISA_EXT_XLINUXENVCFG, 1)
>> +            :: "r" (next->thread.envcfg) : "memory");
>> +}
>> +
>> extern struct task_struct *__switch_to(struct task_struct *,
>>                        struct task_struct *);
>>
>> @@ -103,6 +110,7 @@ do {                            \
>>         __switch_to_vector(__prev, __next);    \
>>     if (switch_to_should_flush_icache(__next))    \
>>         local_flush_icache_all();        \
>> +    __switch_to_envcfg(__next);            \
>>     ((last) = __switch_to(__prev, __next));        \
>> } while (0)
> 
> Suggestion:
> Probably make this patch 1
> 
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 2879e26dbcd8..1153b96346ae 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void)
>> void riscv_user_isa_enable(void)
>> {
>>     if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
>> -        csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>> +        current->thread.envcfg |= ENVCFG_CBZE;
> 
> Suggestion:
> Squash this with current patch 1 and call it patch 2.

This reorganization doesn't work. If I add __switch_to_envcfg() first without
this change, then the CSR would get zeroed out during the first context switch,
so userspace cbo.zero would be broken after the first patch.

Regards,
Samuel

>>     else if (any_cpu_has_zicboz)
>>         pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
>> }
>> -- 
>> 2.44.1
>>

[PATCH 3/3] riscv: Call riscv_user_isa_enable() only on the boot hart

[Samuel Holland]

Now that the [ms]envcfg CSR value is maintained per thread, not per
hart, riscv_user_isa_enable() only needs to be called once during boot,
to set the value for the init task. This also allows it to be marked as
__init. riscv_isa_extension_check() sets any_cpu_has_zicboz, so it also
needs to be marked __init; it could have had this annotation already.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

 arch/riscv/include/asm/cpufeature.h | 2 +-
 arch/riscv/kernel/cpufeature.c      | 8 ++++----
 arch/riscv/kernel/smpboot.c         | 2 --
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index 347805446151..4bf7b7ebf6b3 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -31,7 +31,7 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void riscv_user_isa_enable(void);
+void __init riscv_user_isa_enable(void);
 
 #if defined(CONFIG_RISCV_MISALIGNED)
 bool check_unaligned_access_emulated_all_cpus(void);
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 1153b96346ae..bfe8550c0aae 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -27,7 +27,7 @@
 
 #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
 
-static bool any_cpu_has_zicboz;
+static bool any_cpu_has_zicboz __initdata;
 
 unsigned long elf_hwcap __read_mostly;
 
@@ -74,7 +74,7 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
 }
 EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
 
-static bool riscv_isa_extension_check(int id)
+static bool __init riscv_isa_extension_check(int id)
 {
 	switch (id) {
 	case RISCV_ISA_EXT_ZICBOM:
@@ -725,12 +725,12 @@ unsigned long riscv_get_elf_hwcap(void)
 	return hwcap;
 }
 
-void riscv_user_isa_enable(void)
+void __init riscv_user_isa_enable(void)
 {
 	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
 		current->thread.envcfg |= ENVCFG_CBZE;
 	else if (any_cpu_has_zicboz)
-		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
+		pr_warn("Zicboz disabled as it is unavailable on some harts\n");
 }
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 1319b29ce3b5..3e8ece31c30d 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -231,8 +231,6 @@ asmlinkage __visible void smp_callin(void)
 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
 	}
 
-	riscv_user_isa_enable();
-
 	/*
 	 * Remote cache and TLB flushes are ignored while the CPU is offline,
 	 * so flush them both right now just in case.
-- 
2.44.1

Re: [PATCH 3/3] riscv: Call riscv_user_isa_enable() only on the boot hart

[Deepak Gupta]

On Wed, Jun 05, 2024 at 01:56:47PM -0700, Samuel Holland wrote:
>Now that the [ms]envcfg CSR value is maintained per thread, not per
>hart, riscv_user_isa_enable() only needs to be called once during boot,
>to set the value for the init task. This also allows it to be marked as
>__init. riscv_isa_extension_check() sets any_cpu_has_zicboz, so it also
>needs to be marked __init; it could have had this annotation already.
>
>Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>---
>
> arch/riscv/include/asm/cpufeature.h | 2 +-
> arch/riscv/kernel/cpufeature.c      | 8 ++++----
> arch/riscv/kernel/smpboot.c         | 2 --
> 3 files changed, 5 insertions(+), 7 deletions(-)
>
>diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>index 347805446151..4bf7b7ebf6b3 100644
>--- a/arch/riscv/include/asm/cpufeature.h
>+++ b/arch/riscv/include/asm/cpufeature.h
>@@ -31,7 +31,7 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
> /* Per-cpu ISA extensions. */
> extern struct riscv_isainfo hart_isa[NR_CPUS];
>
>-void riscv_user_isa_enable(void);
>+void __init riscv_user_isa_enable(void);
>
> #if defined(CONFIG_RISCV_MISALIGNED)
> bool check_unaligned_access_emulated_all_cpus(void);
>diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>index 1153b96346ae..bfe8550c0aae 100644
>--- a/arch/riscv/kernel/cpufeature.c
>+++ b/arch/riscv/kernel/cpufeature.c
>@@ -27,7 +27,7 @@
>
> #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>
>-static bool any_cpu_has_zicboz;
>+static bool any_cpu_has_zicboz __initdata;
>
> unsigned long elf_hwcap __read_mostly;
>
>@@ -74,7 +74,7 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
> }
> EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>
>-static bool riscv_isa_extension_check(int id)
>+static bool __init riscv_isa_extension_check(int id)
> {
> 	switch (id) {
> 	case RISCV_ISA_EXT_ZICBOM:
>@@ -725,12 +725,12 @@ unsigned long riscv_get_elf_hwcap(void)
> 	return hwcap;
> }
>
>-void riscv_user_isa_enable(void)
>+void __init riscv_user_isa_enable(void)
> {
> 	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
> 		current->thread.envcfg |= ENVCFG_CBZE;
> 	else if (any_cpu_has_zicboz)
>-		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
>+		pr_warn("Zicboz disabled as it is unavailable on some harts\n");
> }
>
> #ifdef CONFIG_RISCV_ALTERNATIVE
>diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>index 1319b29ce3b5..3e8ece31c30d 100644
>--- a/arch/riscv/kernel/smpboot.c
>+++ b/arch/riscv/kernel/smpboot.c
>@@ -231,8 +231,6 @@ asmlinkage __visible void smp_callin(void)
> 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> 	}
>
>-	riscv_user_isa_enable();
>-
> 	/*
> 	 * Remote cache and TLB flushes are ignored while the CPU is offline,
> 	 * so flush them both right now just in case.

Suggestion:
Squash with current patch 1 and call it patch 2.

>-- 
>2.44.1
>

Re: [PATCH 0/3] riscv: Per-thread envcfg CSR support

[Deepak Gupta]

Hi Samuel,

Thanks for working on these.
Patches looks good to me. I've given some suggestion for patch organization
and squashing. You can take it or leave it.

Other than that.

Reviewed-By: Deepak Gupta <debug@rivosinc.com>

On Wed, Jun 05, 2024 at 01:56:44PM -0700, Samuel Holland wrote:
>This series (or equivalent) is a prerequisite for both user-mode pointer
>masking and CFI support, as those are per-thread features are controlled
>by fields in the envcfg CSR. These patches are based on v1 of the
>pointer masking series[1], with significant input from both Deepak and
>Andrew. By sending this as a separate series, hopefully we can converge
>on a single implementation of this functionality.
>
>[1]: https://lore.kernel.org/linux-riscv/20240319215915.832127-6-samuel.holland@sifive.com/
>
>
>Samuel Holland (3):
>  riscv: Enable cbo.zero only when all harts support Zicboz
>  riscv: Add support for per-thread envcfg CSR values
>  riscv: Call riscv_user_isa_enable() only on the boot hart
>
> arch/riscv/include/asm/cpufeature.h |  2 +-
> arch/riscv/include/asm/processor.h  |  1 +
> arch/riscv/include/asm/switch_to.h  |  8 ++++++++
> arch/riscv/kernel/cpufeature.c      | 13 +++++++++----
> arch/riscv/kernel/smpboot.c         |  2 --
> arch/riscv/kernel/suspend.c         |  4 ++--
> 6 files changed, 21 insertions(+), 9 deletions(-)
>
>-- 
>2.44.1
>

Re: [PATCH 0/3] riscv: Per-thread envcfg CSR support

[Samuel Holland]

Hi Deepak,

On 2024-06-07 5:01 PM, Deepak Gupta wrote:
> Hi Samuel,
> 
> Thanks for working on these.
> Patches looks good to me. I've given some suggestion for patch organization
> and squashing. You can take it or leave it.

Thanks for the review! I'd like to keep the patches separate so it is clear
which part is the behavior change (patch 1), and which part is the new
functionality (patch 2).

Regards,
Samuel

> Other than that.
> 
> Reviewed-By: Deepak Gupta <debug@rivosinc.com>
> 
> On Wed, Jun 05, 2024 at 01:56:44PM -0700, Samuel Holland wrote:
>> This series (or equivalent) is a prerequisite for both user-mode pointer
>> masking and CFI support, as those are per-thread features are controlled
>> by fields in the envcfg CSR. These patches are based on v1 of the
>> pointer masking series[1], with significant input from both Deepak and
>> Andrew. By sending this as a separate series, hopefully we can converge
>> on a single implementation of this functionality.
>>
>> [1]:
>> https://lore.kernel.org/linux-riscv/20240319215915.832127-6-samuel.holland@sifive.com/
>>
>>
>> Samuel Holland (3):
>>  riscv: Enable cbo.zero only when all harts support Zicboz
>>  riscv: Add support for per-thread envcfg CSR values
>>  riscv: Call riscv_user_isa_enable() only on the boot hart
>>
>> arch/riscv/include/asm/cpufeature.h |  2 +-
>> arch/riscv/include/asm/processor.h  |  1 +
>> arch/riscv/include/asm/switch_to.h  |  8 ++++++++
>> arch/riscv/kernel/cpufeature.c      | 13 +++++++++----
>> arch/riscv/kernel/smpboot.c         |  2 --
>> arch/riscv/kernel/suspend.c         |  4 ++--
>> 6 files changed, 21 insertions(+), 9 deletions(-)
>>
>> -- 
>> 2.44.1
>>