| Message ID | 20240605205658.184399-2-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Per-thread envcfg CSR support | expand |
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 >
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.
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.
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);
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(-)