| Message ID | 20241025091527.57825-1-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Enable Zicbom in usermode | expand |
On Fri, Oct 25, 2024 at 05:15:27PM +0800, Yunhui Cui wrote: > Like Zicboz, by enabling the corresponding bits of senvcfg, > the instructions cbo.clean, cbo.flush, and cbo.inval can be > executed normally in user mode. > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/cpufeature.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 1992ea64786e..bc850518ab41 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -924,7 +924,7 @@ unsigned long riscv_get_elf_hwcap(void) > void __init riscv_user_isa_enable(void) > { > if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) > - current->thread.envcfg |= ENVCFG_CBZE; > + current->thread.envcfg |= ENVCFG_CBIE | ENVCFG_CBCFE | ENVCFG_CBZE; I believe we previously decided that userspace should not be allowed to use zicbom, but that not withstanding - this is wrong. It should be checking for Zicbom, not Zicboz.
On 25 Oct 2024, at 11:16, Conor Dooley <conor@kernel.org> wrote: > On Fri, Oct 25, 2024 at 05:15:27PM +0800, Yunhui Cui wrote: >> Like Zicboz, by enabling the corresponding bits of senvcfg, >> the instructions cbo.clean, cbo.flush, and cbo.inval can be >> executed normally in user mode. >> >> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> >> --- >> arch/riscv/kernel/cpufeature.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> index 1992ea64786e..bc850518ab41 100644 >> --- a/arch/riscv/kernel/cpufeature.c >> +++ b/arch/riscv/kernel/cpufeature.c >> @@ -924,7 +924,7 @@ unsigned long riscv_get_elf_hwcap(void) >> void __init riscv_user_isa_enable(void) >> { >> if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) >> - current->thread.envcfg |= ENVCFG_CBZE; >> + current->thread.envcfg |= ENVCFG_CBIE | ENVCFG_CBCFE | ENVCFG_CBZE; > > I believe we previously decided that userspace should not be allowed to > use zicbom, but that not withstanding - this is wrong. It should be > checking for Zicbom, not Zicboz. Allowing clean/flush is safe but has the same problems as fence.i with regards to migrating between harts. Allowing invalidate, unless mapped to flush, is not safe in general unless the kernel does a lot of flushing to avoid userspace accessing data it shouldn’t be able to see. Also, ENVCFG_CBIE is a mask for a multi-bit field, which happens to have the same value as ENVCFG_CBIE_INV (i.e. really is making cbo.inval be an invalidate). I note that the KVM code, which this likely copied from(?), makes the same mistake, but there that is the intended behaviour, if misleading about what the field really is. So, with suitable caveats, allowing clean/flush could be a reasonable thing to do (maybe useful for userspace drivers so long as they pin themselves to a specific hart?), but invalidate should only ever be allowed if mapped to flush. Jess
On Fri, Oct 25, 2024 at 11:16:44AM +0100, Conor Dooley wrote: >On Fri, Oct 25, 2024 at 05:15:27PM +0800, Yunhui Cui wrote: >> Like Zicboz, by enabling the corresponding bits of senvcfg, >> the instructions cbo.clean, cbo.flush, and cbo.inval can be >> executed normally in user mode. >> >> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> >> --- >> arch/riscv/kernel/cpufeature.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> index 1992ea64786e..bc850518ab41 100644 >> --- a/arch/riscv/kernel/cpufeature.c >> +++ b/arch/riscv/kernel/cpufeature.c >> @@ -924,7 +924,7 @@ unsigned long riscv_get_elf_hwcap(void) >> void __init riscv_user_isa_enable(void) >> { >> if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) >> - current->thread.envcfg |= ENVCFG_CBZE; >> + current->thread.envcfg |= ENVCFG_CBIE | ENVCFG_CBCFE | ENVCFG_CBZE; > >I believe we previously decided that userspace should not be allowed to >use zicbom, but that not withstanding - this is wrong. It should be >checking for Zicbom, not Zicboz. Additional comment: It would be good to have this (flush/clean/inval) disabled for seccomped process or at least some sort of user abi to disable it (whenever use decides to seccomp current task). So either - by default disable when task is strict seccomped OR - introduce user abi (prctl) to disable it. so that any task trying to lockdown itself should be able to do that. This is particularly useful for sandbox hosting in same address space.
Hi Jessica, On Sat, Oct 26, 2024 at 12:32 AM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 25 Oct 2024, at 11:16, Conor Dooley <conor@kernel.org> wrote: > > On Fri, Oct 25, 2024 at 05:15:27PM +0800, Yunhui Cui wrote: > >> Like Zicboz, by enabling the corresponding bits of senvcfg, > >> the instructions cbo.clean, cbo.flush, and cbo.inval can be > >> executed normally in user mode. > >> > >> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > >> --- > >> arch/riscv/kernel/cpufeature.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > >> index 1992ea64786e..bc850518ab41 100644 > >> --- a/arch/riscv/kernel/cpufeature.c > >> +++ b/arch/riscv/kernel/cpufeature.c > >> @@ -924,7 +924,7 @@ unsigned long riscv_get_elf_hwcap(void) > >> void __init riscv_user_isa_enable(void) > >> { > >> if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) > >> - current->thread.envcfg |= ENVCFG_CBZE; > >> + current->thread.envcfg |= ENVCFG_CBIE | ENVCFG_CBCFE | ENVCFG_CBZE; > > > > I believe we previously decided that userspace should not be allowed to > > use zicbom, but that not withstanding - this is wrong. It should be > > checking for Zicbom, not Zicboz. > > Allowing clean/flush is safe but has the same problems as fence.i with > regards to migrating between harts. Allowing invalidate, unless mapped > to flush, is not safe in general unless the kernel does a lot of > flushing to avoid userspace accessing data it shouldn’t be able to see. > > Also, ENVCFG_CBIE is a mask for a multi-bit field, which happens to > have the same value as ENVCFG_CBIE_INV (i.e. really is making cbo.inval > be an invalidate). I note that the KVM code, which this likely copied > from(?), makes the same mistake, but there that is the intended > behaviour, if misleading about what the field really is. > > So, with suitable caveats, allowing clean/flush could be a reasonable > thing to do (maybe useful for userspace drivers so long as they pin > themselves to a specific hart?), but invalidate should only ever be > allowed if mapped to flush. > > Jess > Yes. The original intention is to enable clean/flush/invalid. So ENVCFG_CBIE | ENVCFG_CBCFE is added. When one core initiates an invalidation, other cores will also invalidate the corresponding cache line. So do we not need to worry about this problem? Moreover, invalidation is not found in the logic of disabling preemption in the kernel. Or perhaps binding cores belongs to the user-space's own logic. Can this patch be fixed as RISCV_ISA_EXT_ZICBOM and then a v2 be sent? Thanks, Yunhui
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 1992ea64786e..bc850518ab41 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -924,7 +924,7 @@ unsigned long riscv_get_elf_hwcap(void) void __init riscv_user_isa_enable(void) { if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ)) - current->thread.envcfg |= ENVCFG_CBZE; + current->thread.envcfg |= ENVCFG_CBIE | ENVCFG_CBCFE | ENVCFG_CBZE; else if (any_cpu_has_zicboz) pr_warn("Zicboz disabled as it is unavailable on some harts\n"); }
Like Zicboz, by enabling the corresponding bits of senvcfg, the instructions cbo.clean, cbo.flush, and cbo.inval can be executed normally in user mode. Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> --- arch/riscv/kernel/cpufeature.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)