| Message ID | 20231124055330.138870-3-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Fri, Nov 24, 2023 at 12:53:06AM -0500, Yang Weijiang wrote: > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > reflect true dependency between CET features and the user xstate bit. > Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is > available. > > Both user mode shadow stack and indirect branch tracking features depend > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted > from CET KVM series which synthesizes guest CPUIDs based on userspace > settings,in real world the case is rare. In other words, the exitings > dependency check is correct when only user mode SHSTK is available. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index 73f6bc00d178..6e50a4251e2b 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > }; > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > + /* > + * CET user mode xstate bit has been cleared by above sanity check. > + * Now pick it up if either SHSTK or IBT is available. Either feature > + * depends on the xstate bit to save/restore user mode states. > + */ > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); So booting a host with "ibt=off" will clear the FEATURE_IBT, this was fine before this patch-set, but possibly not with. That kernel argument really only wants to tell the kernel not to use IBT itself, but not inhibit IBT from being used by guests.
On 11/24/2023 5:40 PM, Peter Zijlstra wrote: > On Fri, Nov 24, 2023 at 12:53:06AM -0500, Yang Weijiang wrote: >> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >> reflect true dependency between CET features and the user xstate bit. >> Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is >> available. >> >> Both user mode shadow stack and indirect branch tracking features depend >> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >> >> Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted >> from CET KVM series which synthesizes guest CPUIDs based on userspace >> settings,in real world the case is rare. In other words, the exitings >> dependency check is correct when only user mode SHSTK is available. >> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> --- >> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >> index 73f6bc00d178..6e50a4251e2b 100644 >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >> }; >> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> + /* >> + * CET user mode xstate bit has been cleared by above sanity check. >> + * Now pick it up if either SHSTK or IBT is available. Either feature >> + * depends on the xstate bit to save/restore user mode states. >> + */ >> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > So booting a host with "ibt=off" will clear the FEATURE_IBT, this was > fine before this patch-set, but possibly not with. > > That kernel argument really only wants to tell the kernel not to use IBT > itself, but not inhibit IBT from being used by guests. Thanks for pointing this out! If ibt=off actually causes XFEATURE_CET_USER unset in fpu_kernel_cfg.max_features, in KVM part (patch 24), we already have below check to disable SHSTK/IBT in this cases, so looks like it won't bring issues. +if ((kvm_caps.supported_xss & (XFEATURE_MASK_CET_USER | +XFEATURE_MASK_CET_KERNEL)) != +(XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL)) { +kvm_cpu_cap_clear(X86_FEATURE_SHSTK); +kvm_cpu_cap_clear(X86_FEATURE_IBT); +kvm_caps.supported_xss &= ~XFEATURE_CET_USER; +kvm_caps.supported_xss &= ~XFEATURE_CET_KERNEL; +} + >
On Fri, 2023-11-24 at 10:40 +0100, Peter Zijlstra wrote: > So booting a host with "ibt=off" will clear the FEATURE_IBT, this was > fine before this patch-set, but possibly not with. > > That kernel argument really only wants to tell the kernel not to use > IBT > itself, but not inhibit IBT from being used by guests. Should we add a SW bit for it then? ibt=off sounds like it should be turning off the whole feature though. It doesn't sound like kernel IBT specifically.
On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
> settings,in real world the case is rare. In other words, the exitings
^existing
On 11/28/2023 9:31 AM, Edgecombe, Rick P wrote: > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >> settings,in real world the case is rare. In other words, the exitings > ^existing Thank you! :-)
On Tue, Nov 28, 2023 at 01:31:14AM +0000, Edgecombe, Rick P wrote: > On Fri, 2023-11-24 at 10:40 +0100, Peter Zijlstra wrote: > > So booting a host with "ibt=off" will clear the FEATURE_IBT, this was > > fine before this patch-set, but possibly not with. > > > > That kernel argument really only wants to tell the kernel not to use > > IBT > > itself, but not inhibit IBT from being used by guests. > > Should we add a SW bit for it then? Don't think we need a feature flag for this, some boolean state should be enough. > ibt=off sounds like it should be > turning off the whole feature though. It doesn't sound like kernel IBT > specifically. Well, the intent really was to just not enable IBT, clearing the feature was the simplest way to make that happen.
On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > reflect true dependency between CET features and the user xstate bit. > Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is > available. > > Both user mode shadow stack and indirect branch tracking features depend > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted > from CET KVM series which synthesizes guest CPUIDs based on userspace > settings,in real world the case is rare. In other words, the exitings > dependency check is correct when only user mode SHSTK is available. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index 73f6bc00d178..6e50a4251e2b 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > }; > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > + /* > + * CET user mode xstate bit has been cleared by above sanity check. > + * Now pick it up if either SHSTK or IBT is available. Either feature > + * depends on the xstate bit to save/restore user mode states. > + */ > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > + > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > I am curious: Any reason why my review feedback was not applied even though you did agree that it is reasonable? https://lore.kernel.org/lkml/c72dfaac-1622-94cf-a81d-9d7ed81b2f55@intel.com/ Best regards, Maxim Levitsky
On 12/1/2023 1:26 AM, Maxim Levitsky wrote: > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >> reflect true dependency between CET features and the user xstate bit. >> Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is >> available. >> >> Both user mode shadow stack and indirect branch tracking features depend >> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >> >> Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted >> from CET KVM series which synthesizes guest CPUIDs based on userspace >> settings,in real world the case is rare. In other words, the exitings >> dependency check is correct when only user mode SHSTK is available. >> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> --- >> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >> index 73f6bc00d178..6e50a4251e2b 100644 >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >> }; >> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> + /* >> + * CET user mode xstate bit has been cleared by above sanity check. >> + * Now pick it up if either SHSTK or IBT is available. Either feature >> + * depends on the xstate bit to save/restore user mode states. >> + */ >> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); >> + >> if (!cpu_feature_enabled(X86_FEATURE_XFD)) >> fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; >> > I am curious: > > Any reason why my review feedback was not applied even though you did agree > that it is reasonable? My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't work before sending out v7 version, after a close look at the existing code: for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { unsigned short cid = xsave_cpuid_features[i]; /* Careful: X86_FEATURE_FPU is 0! */ if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) fpu_kernel_cfg.max_features &= ~BIT_ULL(i); } With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually above check will clear the bit from fpu_kernel_cfg.max_features. So now I need to add it back conditionally. Your sample code is more consistent with existing code in style, but I don't want to hack into the loop and handle XFEATURE_CET_USER specifically. Just keep the handling and rewording the comments which is also straightforward. > > > https://lore.kernel.org/lkml/c72dfaac-1622-94cf-a81d-9d7ed81b2f55@intel.com/ > > Best regards, > Maxim Levitsky >
On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: > On 12/1/2023 1:26 AM, Maxim Levitsky wrote: > > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > > > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > > > reflect true dependency between CET features and the user xstate bit. > > > Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is > > > available. > > > > > > Both user mode shadow stack and indirect branch tracking features depend > > > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > > > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > > > > > Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted > > > from CET KVM series which synthesizes guest CPUIDs based on userspace > > > settings,in real world the case is rare. In other words, the exitings > > > dependency check is correct when only user mode SHSTK is available. > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > --- > > > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > index 73f6bc00d178..6e50a4251e2b 100644 > > > --- a/arch/x86/kernel/fpu/xstate.c > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > > > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > > > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > > > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > > > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > > > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > > > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > > > }; > > > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > } > > > > > > + /* > > > + * CET user mode xstate bit has been cleared by above sanity check. > > > + * Now pick it up if either SHSTK or IBT is available. Either feature > > > + * depends on the xstate bit to save/restore user mode states. > > > + */ > > > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > > > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > > > + > > > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > > > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > > > > I am curious: > > > > Any reason why my review feedback was not applied even though you did agree > > that it is reasonable? > > My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't > work before sending out v7 version, after a close look at the existing code: > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > unsigned short cid = xsave_cpuid_features[i]; > > /* Careful: X86_FEATURE_FPU is 0! */ > if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually > above check will clear the bit from fpu_kernel_cfg.max_features. Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features array. What I suggested was that we remove the XFEATURE_CET_USER from the xsave_cpuid_features and instead do this after the above loop. if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); Which is pretty much just a manual iteration of the loop, just instead of checking for absence of single feature, it checks that both features are absent. Best regards, Maxim Levitsky > So now I need > to add it back conditionally. > Your sample code is more consistent with existing code in style, but I don't want to > hack into the loop and handle XFEATURE_CET_USER specifically. Just keep the handling > and rewording the comments which is also straightforward. > > > > > https://lore.kernel.org/lkml/c72dfaac-1622-94cf-a81d-9d7ed81b2f55@intel.com/ > > > > Best regards, > > Maxim Levitsky > >
On 12/5/2023 5:53 PM, Maxim Levitsky wrote: > On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: >> On 12/1/2023 1:26 AM, Maxim Levitsky wrote: >>> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >>>> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >>>> reflect true dependency between CET features and the user xstate bit. >>>> Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is >>>> available. >>>> >>>> Both user mode shadow stack and indirect branch tracking features depend >>>> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >>>> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >>>> >>>> Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted >>>> from CET KVM series which synthesizes guest CPUIDs based on userspace >>>> settings,in real world the case is rare. In other words, the exitings >>>> dependency check is correct when only user mode SHSTK is available. >>>> >>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >>>> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>> --- >>>> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >>>> index 73f6bc00d178..6e50a4251e2b 100644 >>>> --- a/arch/x86/kernel/fpu/xstate.c >>>> +++ b/arch/x86/kernel/fpu/xstate.c >>>> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >>>> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >>>> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >>>> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >>>> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >>>> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >>>> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >>>> }; >>>> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >>>> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >>>> } >>>> >>>> + /* >>>> + * CET user mode xstate bit has been cleared by above sanity check. >>>> + * Now pick it up if either SHSTK or IBT is available. Either feature >>>> + * depends on the xstate bit to save/restore user mode states. >>>> + */ >>>> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >>>> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); >>>> + >>>> if (!cpu_feature_enabled(X86_FEATURE_XFD)) >>>> fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; >>>> >>> I am curious: >>> >>> Any reason why my review feedback was not applied even though you did agree >>> that it is reasonable? >> My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't >> work before sending out v7 version, after a close look at the existing code: >> >> for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { >> unsigned short cid = xsave_cpuid_features[i]; >> >> /* Careful: X86_FEATURE_FPU is 0! */ >> if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually >> above check will clear the bit from fpu_kernel_cfg.max_features. > Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, > then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features > array. No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. > What I suggested was that we remove the XFEATURE_CET_USER from the xsave_cpuid_features > and instead do this after the above loop. > > if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); > > Which is pretty much just a manual iteration of the loop, just instead of checking > for absence of single feature, it checks that both features are absent. > > Best regards, > Maxim Levitsky > > >> So now I need >> to add it back conditionally. >> Your sample code is more consistent with existing code in style, but I don't want to >> hack into the loop and handle XFEATURE_CET_USER specifically. Just keep the handling >> and rewording the comments which is also straightforward. >> >>> https://lore.kernel.org/lkml/c72dfaac-1622-94cf-a81d-9d7ed81b2f55@intel.com/ >>> >>> Best regard >>> Maxim Levitsky >>> > > >
On Wed, 2023-12-06 at 09:03 +0800, Yang, Weijiang wrote: > On 12/5/2023 5:53 PM, Maxim Levitsky wrote: > > On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: > > > On 12/1/2023 1:26 AM, Maxim Levitsky wrote: > > > > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > > > > > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > > > > > reflect true dependency between CET features and the user xstate bit. > > > > > Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is > > > > > available. > > > > > > > > > > Both user mode shadow stack and indirect branch tracking features depend > > > > > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > > > > > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > > > > > > > > > Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted > > > > > from CET KVM series which synthesizes guest CPUIDs based on userspace > > > > > settings,in real world the case is rare. In other words, the exitings > > > > > dependency check is correct when only user mode SHSTK is available. > > > > > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > > > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > > > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > > > --- > > > > > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > > > index 73f6bc00d178..6e50a4251e2b 100644 > > > > > --- a/arch/x86/kernel/fpu/xstate.c > > > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > > > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > > > > > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > > > > > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > > > > > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > > > > > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > > > > > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > > > > > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > > > > > }; > > > > > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > > > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > > > } > > > > > > > > > > + /* > > > > > + * CET user mode xstate bit has been cleared by above sanity check. > > > > > + * Now pick it up if either SHSTK or IBT is available. Either feature > > > > > + * depends on the xstate bit to save/restore user mode states. > > > > > + */ > > > > > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > > > > > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > > > > > + > > > > > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > > > > > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > > > > > > > > I am curious: > > > > > > > > Any reason why my review feedback was not applied even though you did agree > > > > that it is reasonable? > > > My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't > > > work before sending out v7 version, after a close look at the existing code: > > > > > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > > > unsigned short cid = xsave_cpuid_features[i]; > > > > > > /* Careful: X86_FEATURE_FPU is 0! */ > > > if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > } > > > > > > With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually > > > above check will clear the bit from fpu_kernel_cfg.max_features. > > Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, > > then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features > > array. > > No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. Oh I see now. IMHO the current code is broken, or at least it violates the 'Clear XSAVE features that are disabled in the normal CPUID' comment, because it also clears all xfeatures which have no CPUID bit in the table (except FPU, for which we have a workaround). How about we do this instead: for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { unsigned short cid = xsave_cpuid_features[i]; if (cid && !boot_cpu_has(cid)) fpu_kernel_cfg.max_features &= ~BIT_ULL(i); } The only side effect is that this code will not clear features bits for which kernel knows no CPUID bit but kernel anyway clears _all_ features that it knows nothing about so there is no net change in behavior. (That is kernel only allows XFEATURE_MASK_USER_SUPPORTED | XFEATURE_MASK_SUPERVISOR_SUPPORTED) Best regards, Maxim Levitsky > > What I suggested was that we remove the XFEATURE_CET_USER from the xsave_cpuid_features > > and instead do this after the above loop. > > > > if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) > > fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); > > > > Which is pretty much just a manual iteration of the loop, just instead of checking > > for absence of single feature, it checks that both features are absent. > > > > Best regards, > > Maxim Levitsky > > > > > > > So now I need > > > to add it back conditionally. > > > Your sample code is more consistent with existing code in style, but I don't want to > > > hack into the loop and handle XFEATURE_CET_USER specifically. Just keep the handling > > > and rewording the comments which is also straightforward. > > > > > > > https://lore.kernel.org/lkml/c72dfaac-1622-94cf-a81d-9d7ed81b2f55@intel.com/ > > > > > > > > Best regard > > > > Maxim Levitsky > > > > > > > >
On 12/6/2023 11:57 PM, Maxim Levitsky wrote: > On Wed, 2023-12-06 at 09:03 +0800, Yang, Weijiang wrote: >> On 12/5/2023 5:53 PM, Maxim Levitsky wrote: >>> On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: >>>> On 12/1/2023 1:26 AM, Maxim Levitsky wrote: >>>>> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >>>>>> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >>>>>> reflect true dependency between CET features and the user xstate bit. >>>>>> Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is >>>>>> available. >>>>>> >>>>>> Both user mode shadow stack and indirect branch tracking features depend >>>>>> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >>>>>> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >>>>>> >>>>>> Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted >>>>>> from CET KVM series which synthesizes guest CPUIDs based on userspace >>>>>> settings,in real world the case is rare. In other words, the exitings >>>>>> dependency check is correct when only user mode SHSTK is available. >>>>>> >>>>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >>>>>> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>>>> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>>>> --- >>>>>> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >>>>>> index 73f6bc00d178..6e50a4251e2b 100644 >>>>>> --- a/arch/x86/kernel/fpu/xstate.c >>>>>> +++ b/arch/x86/kernel/fpu/xstate.c >>>>>> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >>>>>> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >>>>>> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >>>>>> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >>>>>> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >>>>>> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >>>>>> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >>>>>> }; >>>>>> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >>>>>> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >>>>>> } >>>>>> >>>>>> + /* >>>>>> + * CET user mode xstate bit has been cleared by above sanity check. >>>>>> + * Now pick it up if either SHSTK or IBT is available. Either feature >>>>>> + * depends on the xstate bit to save/restore user mode states. >>>>>> + */ >>>>>> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >>>>>> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); >>>>>> + >>>>>> if (!cpu_feature_enabled(X86_FEATURE_XFD)) >>>>>> fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; >>>>>> >>>>> I am curious: >>>>> >>>>> Any reason why my review feedback was not applied even though you did agree >>>>> that it is reasonable? >>>> My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't >>>> work before sending out v7 version, after a close look at the existing code: >>>> >>>> for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { >>>> unsigned short cid = xsave_cpuid_features[i]; >>>> >>>> /* Careful: X86_FEATURE_FPU is 0! */ >>>> if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) >>>> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >>>> } >>>> >>>> With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually >>>> above check will clear the bit from fpu_kernel_cfg.max_features. >>> Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, >>> then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features >>> array. >> No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. > Oh I see now. IMHO the current code is broken, or at least it violates the > 'Clear XSAVE features that are disabled in the normal CPUID' comment, because > it also clears all xfeatures which have no CPUID bit in the table (except FPU, > for which we have a workaround). > > > How about we do this instead: > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > unsigned short cid = xsave_cpuid_features[i]; > > if (cid && !boot_cpu_has(cid)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } I think existing code is more reasonable, the side-effect of current code, i.e., masking out the unclaimed xfeature bits, sanitizes the bits in max_features at the first place, then following calculations derived from it become reasonable too. It also forces developers add explicit dependency claim between xfeature bit and its CPUID so that make it clear why one bit is needed in max_features. Regarding CET_USER bit handling, it could be a singular case, hope it can be tolerated :-) > The only side effect is that this code will not clear features bits for which kernel knows > no CPUID bit but kernel anyway clears _all_ features that it knows nothing about so there > is no net change in behavior. > > (That is kernel only allows XFEATURE_MASK_USER_SUPPORTED | XFEATURE_MASK_SUPERVISOR_SUPPORTED) IMHO, the bits in the macros are just xfeature candidate bits, the relevant CPUID features could be disabled for any reason, in this case, the bits should not be appear in fpu_kernel_cfg.max_features.
On Fri, 2023-12-08 at 22:57 +0800, Yang, Weijiang wrote: > On 12/6/2023 11:57 PM, Maxim Levitsky wrote: > > On Wed, 2023-12-06 at 09:03 +0800, Yang, Weijiang wrote: > > > On 12/5/2023 5:53 PM, Maxim Levitsky wrote: > > > > On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: > > > > > On 12/1/2023 1:26 AM, Maxim Levitsky wrote: > > > > > > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > > > > > > > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > > > > > > > reflect true dependency between CET features and the user xstate bit. > > > > > > > Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is > > > > > > > available. > > > > > > > > > > > > > > Both user mode shadow stack and indirect branch tracking features depend > > > > > > > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > > > > > > > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > > > > > > > > > > > > > Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted > > > > > > > from CET KVM series which synthesizes guest CPUIDs based on userspace > > > > > > > settings,in real world the case is rare. In other words, the exitings > > > > > > > dependency check is correct when only user mode SHSTK is available. > > > > > > > > > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > > > > > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > > > > > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > > > > > --- > > > > > > > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > > > > > index 73f6bc00d178..6e50a4251e2b 100644 > > > > > > > --- a/arch/x86/kernel/fpu/xstate.c > > > > > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > > > > > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > > > > > > > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > > > > > > > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > > > > > > > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > > > > > > > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > > > > > > > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > > > > > > > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > > > > > > > }; > > > > > > > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > > > > > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > > > > > } > > > > > > > > > > > > > > + /* > > > > > > > + * CET user mode xstate bit has been cleared by above sanity check. > > > > > > > + * Now pick it up if either SHSTK or IBT is available. Either feature > > > > > > > + * depends on the xstate bit to save/restore user mode states. > > > > > > > + */ > > > > > > > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > > > > > > > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > > > > > > > + > > > > > > > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > > > > > > > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > > > > > > > > > > > > I am curious: > > > > > > > > > > > > Any reason why my review feedback was not applied even though you did agree > > > > > > that it is reasonable? > > > > > My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't > > > > > work before sending out v7 version, after a close look at the existing code: > > > > > > > > > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > > > > > unsigned short cid = xsave_cpuid_features[i]; > > > > > > > > > > /* Careful: X86_FEATURE_FPU is 0! */ > > > > > if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > > > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > > > } > > > > > > > > > > With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually > > > > > above check will clear the bit from fpu_kernel_cfg.max_features. > > > > Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, > > > > then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features > > > > array. > > > No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. > > Oh I see now. IMHO the current code is broken, or at least it violates the > > 'Clear XSAVE features that are disabled in the normal CPUID' comment, because > > it also clears all xfeatures which have no CPUID bit in the table (except FPU, > > for which we have a workaround). > > > > > > How about we do this instead: > > > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > > unsigned short cid = xsave_cpuid_features[i]; > > > > if (cid && !boot_cpu_has(cid)) > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > } > > I think existing code is more reasonable, the side-effect of current code, i.e., masking out > the unclaimed xfeature bits, sanitizes the bits in max_features at the first place, then following calculations derived from it become reasonable too. I strongly disagree with that. Kernel already removes all features bits which it knows nothing about. There is no need to also remove the xfeatures that it knows about but knows nothing about a CPUID bit. For such features the kernel needs either to accept it (like FPU) or remove the feature from set of supported features. > It also forces developers add explicit dependency claim between xfeature bit and its CPUID so that make it clear why one bit is needed in max_features. A static compile time assert for this is a good idea but not some undocumented cleaning of the bits that was not obvious to either you not me. Best regards, Maxim Levitsky > Regarding CET_USER bit handling, it could be a singular case, hope it can be tolerated :-) > > The only side effect is that this code will not clear features bits for which kernel knows > > no CPUID bit but kernel anyway clears _all_ features that it knows nothing about so there > > is no net change in behavior. > > > > (That is kernel only allows XFEATURE_MASK_USER_SUPPORTED | XFEATURE_MASK_SUPERVISOR_SUPPORTED) > > IMHO, the bits in the macros are just xfeature candidate bits, the relevant CPUID features could be disabled > for any reason, in this case, the bits should not be appear in fpu_kernel_cfg.max_features. >
On 12/8/2023 11:15 PM, Maxim Levitsky wrote: > On Fri, 2023-12-08 at 22:57 +0800, Yang, Weijiang wrote: >> On 12/6/2023 11:57 PM, Maxim Levitsky wrote: >>> On Wed, 2023-12-06 at 09:03 +0800, Yang, Weijiang wrote: >>>> On 12/5/2023 5:53 PM, Maxim Levitsky wrote: >>>>> On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: >>>>>> On 12/1/2023 1:26 AM, Maxim Levitsky wrote: >>>>>>> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >>>>>>>> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >>>>>>>> reflect true dependency between CET features and the user xstate bit. >>>>>>>> Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is >>>>>>>> available. >>>>>>>> >>>>>>>> Both user mode shadow stack and indirect branch tracking features depend >>>>>>>> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >>>>>>>> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >>>>>>>> >>>>>>>> Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted >>>>>>>> from CET KVM series which synthesizes guest CPUIDs based on userspace >>>>>>>> settings,in real world the case is rare. In other words, the exitings >>>>>>>> dependency check is correct when only user mode SHSTK is available. >>>>>>>> >>>>>>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >>>>>>>> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>>>>>> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >>>>>>>> --- >>>>>>>> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >>>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >>>>>>>> index 73f6bc00d178..6e50a4251e2b 100644 >>>>>>>> --- a/arch/x86/kernel/fpu/xstate.c >>>>>>>> +++ b/arch/x86/kernel/fpu/xstate.c >>>>>>>> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >>>>>>>> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >>>>>>>> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >>>>>>>> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >>>>>>>> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >>>>>>>> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >>>>>>>> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >>>>>>>> }; >>>>>>>> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >>>>>>>> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >>>>>>>> } >>>>>>>> >>>>>>>> + /* >>>>>>>> + * CET user mode xstate bit has been cleared by above sanity check. >>>>>>>> + * Now pick it up if either SHSTK or IBT is available. Either feature >>>>>>>> + * depends on the xstate bit to save/restore user mode states. >>>>>>>> + */ >>>>>>>> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >>>>>>>> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); >>>>>>>> + >>>>>>>> if (!cpu_feature_enabled(X86_FEATURE_XFD)) >>>>>>>> fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; >>>>>>>> >>>>>>> I am curious: >>>>>>> >>>>>>> Any reason why my review feedback was not applied even though you did agree >>>>>>> that it is reasonable? >>>>>> My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't >>>>>> work before sending out v7 version, after a close look at the existing code: >>>>>> >>>>>> for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { >>>>>> unsigned short cid = xsave_cpuid_features[i]; >>>>>> >>>>>> /* Careful: X86_FEATURE_FPU is 0! */ >>>>>> if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) >>>>>> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >>>>>> } >>>>>> >>>>>> With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually >>>>>> above check will clear the bit from fpu_kernel_cfg.max_features. >>>>> Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, >>>>> then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features >>>>> array. >>>> No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. >>> Oh I see now. IMHO the current code is broken, or at least it violates the >>> 'Clear XSAVE features that are disabled in the normal CPUID' comment, because >>> it also clears all xfeatures which have no CPUID bit in the table (except FPU, >>> for which we have a workaround). >>> >>> >>> How about we do this instead: >>> >>> for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { >>> unsigned short cid = xsave_cpuid_features[i]; >>> >>> if (cid && !boot_cpu_has(cid)) >>> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >>> } >> I think existing code is more reasonable, the side-effect of current code, i.e., masking out >> the unclaimed xfeature bits, sanitizes the bits in max_features at the first place, then following calculations derived from it become reasonable too. > > I strongly disagree with that. Kernel already removes all features bits which it knows nothing about. > > There is no need to also remove the xfeatures that it knows about but knows nothing about a CPUID bit. > For such features the kernel needs either to accept it (like FPU) or remove the feature from set of supported features. Let me involve Chang, the author of the code in question. Hi, Chang, In commit 70c3f1671b0c ("x86/fpu/xstate: Prepare XSAVE feature table for gaps in state component numbers"), you modified the loop as below: for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { - if (!boot_cpu_has(xsave_cpuid_features[i])) + unsigned short cid = xsave_cpuid_features[i]; + + /* Careful: X86_FEATURE_FPU is 0! */ + if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) fpu_kernel_cfg.max_features &= ~BIT_ULL(i); } IMHO the change resulted functional change of the loop, i.e., before that only it only clears the bits without CPUIDs, but after the change, the side-effect of the loop will clear the bits of blank entries ( where xsave_cpuid_features[i] == 0 ) since the loop hits (i != XFEATURE_FP && !cid), is it intended or something else?
On Wed, 2023-12-13 at 17:30 +0800, Yang, Weijiang wrote: > On 12/8/2023 11:15 PM, Maxim Levitsky wrote: > > On Fri, 2023-12-08 at 22:57 +0800, Yang, Weijiang wrote: > > > On 12/6/2023 11:57 PM, Maxim Levitsky wrote: > > > > On Wed, 2023-12-06 at 09:03 +0800, Yang, Weijiang wrote: > > > > > On 12/5/2023 5:53 PM, Maxim Levitsky wrote: > > > > > > On Fri, 2023-12-01 at 14:51 +0800, Yang, Weijiang wrote: > > > > > > > On 12/1/2023 1:26 AM, Maxim Levitsky wrote: > > > > > > > > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > > > > > > > > > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > > > > > > > > > reflect true dependency between CET features and the user xstate bit. > > > > > > > > > Enable the bit in fpu_kernel_cfg.max_features when either SHSTK or IBT is > > > > > > > > > available. > > > > > > > > > > > > > > > > > > Both user mode shadow stack and indirect branch tracking features depend > > > > > > > > > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > > > > > > > > > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > > > > > > > > > > > > > > > > > Note, the issue, i.e., CPUID only enumerates IBT but no SHSTK is resulted > > > > > > > > > from CET KVM series which synthesizes guest CPUIDs based on userspace > > > > > > > > > settings,in real world the case is rare. In other words, the exitings > > > > > > > > > dependency check is correct when only user mode SHSTK is available. > > > > > > > > > > > > > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > > > > > > > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > > > > > > > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > > > > > > > > --- > > > > > > > > > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > > > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > > > > > > > index 73f6bc00d178..6e50a4251e2b 100644 > > > > > > > > > --- a/arch/x86/kernel/fpu/xstate.c > > > > > > > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > > > > > > > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > > > > > > > > > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > > > > > > > > > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > > > > > > > > > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > > > > > > > > > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > > > > > > > > > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > > > > > > > > > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > > > > > > > > > }; > > > > > > > > > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > > > > > > > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > > > > > > > } > > > > > > > > > > > > > > > > > > + /* > > > > > > > > > + * CET user mode xstate bit has been cleared by above sanity check. > > > > > > > > > + * Now pick it up if either SHSTK or IBT is available. Either feature > > > > > > > > > + * depends on the xstate bit to save/restore user mode states. > > > > > > > > > + */ > > > > > > > > > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > > > > > > > > > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > > > > > > > > > + > > > > > > > > > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > > > > > > > > > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > > > > > > > > > > > > > > > > I am curious: > > > > > > > > > > > > > > > > Any reason why my review feedback was not applied even though you did agree > > > > > > > > that it is reasonable? > > > > > > > My apology! I changed the patch per you feedback but found XFEATURE_CET_USER didn't > > > > > > > work before sending out v7 version, after a close look at the existing code: > > > > > > > > > > > > > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > > > > > > > unsigned short cid = xsave_cpuid_features[i]; > > > > > > > > > > > > > > /* Careful: X86_FEATURE_FPU is 0! */ > > > > > > > if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > > > > > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > > > > > } > > > > > > > > > > > > > > With removal of XFEATURE_CET_USER entry from xsave_cpuid_features, actually > > > > > > > above check will clear the bit from fpu_kernel_cfg.max_features. > > > > > > Are you sure about this? If we remove the XFEATURE_CET_USER from the xsave_cpuid_features, > > > > > > then the above loop will not touch it - it loops only over the items in the xsave_cpuid_features > > > > > > array. > > > > > No, the code is a bit tricky, the actual array size is XFEATURE_XTILE_DATA( ie, 18) + 1, those xfeature bits not listed in init code leave a blank entry with xsave_cpuid_features[i] == 0, so for the blank elements, the loop hits (i != XFEATURE_FP && !cid) then the relevant xfeature bit for i is cleared in fpu_kernel_cfg.max_features. I had the same illusion at first when I replied your comments in v6, and modified the code as you suggested but found the issue during tests. Please double check it. > > > > Oh I see now. IMHO the current code is broken, or at least it violates the > > > > 'Clear XSAVE features that are disabled in the normal CPUID' comment, because > > > > it also clears all xfeatures which have no CPUID bit in the table (except FPU, > > > > for which we have a workaround). > > > > > > > > > > > > How about we do this instead: > > > > > > > > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > > > > unsigned short cid = xsave_cpuid_features[i]; > > > > > > > > if (cid && !boot_cpu_has(cid)) > > > > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > > > > } > > > I think existing code is more reasonable, the side-effect of current code, i.e., masking out > > > the unclaimed xfeature bits, sanitizes the bits in max_features at the first place, then following calculations derived from it become reasonable too. > > > > I strongly disagree with that. Kernel already removes all features bits which it knows nothing about. > > > > There is no need to also remove the xfeatures that it knows about but knows nothing about a CPUID bit. > > For such features the kernel needs either to accept it (like FPU) or remove the feature from set of supported features. > > Let me involve Chang, the author of the code in question. > > Hi, Chang, > In commit 70c3f1671b0c ("x86/fpu/xstate: Prepare XSAVE feature table for gaps in state component numbers"), > you modified the loop as below: > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > - if (!boot_cpu_has(xsave_cpuid_features[i])) > + unsigned short cid = xsave_cpuid_features[i]; > + > + /* Careful: X86_FEATURE_FPU is 0! */ > + if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > IMHO the change resulted functional change of the loop, i.e., before that only it only clears the bits without CPUIDs, > but after the change, the side-effect of the loop will clear the bits of blank entries ( where xsave_cpuid_features[i] == 0 ) > since the loop hits (i != XFEATURE_FP && !cid), is it intended or something else? > > 100% agree. Best regards, Maxim Levitsky
On 12/13/2023 1:30 AM, Yang, Weijiang wrote: > > Let me involve Chang, the author of the code in question. > > Hi, Chang, > In commit 70c3f1671b0c ("x86/fpu/xstate: Prepare XSAVE feature table for > gaps in state component numbers"), > you modified the loop as below: > for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { > - if (!boot_cpu_has(xsave_cpuid_features[i])) > + unsigned short cid = xsave_cpuid_features[i]; > + > + /* Careful: X86_FEATURE_FPU is 0! */ > + if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > IMHO the change resulted functional change of the loop, i.e., before > that only it only clears the bits without CPUIDs, > but after the change, the side-effect of the loop will clear the bits of > blank entries ( where xsave_cpuid_features[i] == 0 ) > since the loop hits (i != XFEATURE_FP && !cid), is it intended or > something else? The code was considered as much *simpler* than the other [1]. Yes, it clears those not listed in the table but they were either non-existed or disabled at that moment. Thanks, Chang [1] https://lore.kernel.org/lkml/87y2eha576.fsf@nanos.tec.linutronix.de/
On 12/14/2023 1:01 AM, Chang S. Bae wrote: > On 12/13/2023 1:30 AM, Yang, Weijiang wrote: >> >> Let me involve Chang, the author of the code in question. >> >> Hi, Chang, >> In commit 70c3f1671b0c ("x86/fpu/xstate: Prepare XSAVE feature table for gaps in state component numbers"), >> you modified the loop as below: >> for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) { >> - if (!boot_cpu_has(xsave_cpuid_features[i])) >> + unsigned short cid = xsave_cpuid_features[i]; >> + >> + /* Careful: X86_FEATURE_FPU is 0! */ >> + if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid)) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> IMHO the change resulted functional change of the loop, i.e., before that only it only clears the bits without CPUIDs, >> but after the change, the side-effect of the loop will clear the bits of blank entries ( where xsave_cpuid_features[i] == 0 ) >> since the loop hits (i != XFEATURE_FP && !cid), is it intended or something else? > > The code was considered as much *simpler* than the other [1]. Yes, it clears those not listed in the table but they were either non-existed or disabled at that moment. Thanks Chang, so I assume the "side-effect" is intended. > > Thanks, > Chang > > [1] https://lore.kernel.org/lkml/87y2eha576.fsf@nanos.tec.linutronix.de/ >
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index 73f6bc00d178..6e50a4251e2b 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, [XFEATURE_PKRU] = X86_FEATURE_OSPKE, [XFEATURE_PASID] = X86_FEATURE_ENQCMD, - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, }; @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) fpu_kernel_cfg.max_features &= ~BIT_ULL(i); } + /* + * CET user mode xstate bit has been cleared by above sanity check. + * Now pick it up if either SHSTK or IBT is available. Either feature + * depends on the xstate bit to save/restore user mode states. + */ + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); + if (!cpu_feature_enabled(X86_FEATURE_XFD)) fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC;