| Message ID | 20231124055330.138870-6-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > + /* > + * Set guest's __user_state_size to fpu_user_cfg.default_size > so that > + * existing uAPIs can still work. > + */ > + fpu->guest_perm.__user_state_size = > fpu_user_cfg.default_size; It seems like an appropriate value, but where does this come into play exactly for guest FPUs?
On 11/28/2023 10:58 PM, Edgecombe, Rick P wrote: > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >> + /* >> + * Set guest's __user_state_size to fpu_user_cfg.default_size >> so that >> + * existing uAPIs can still work. >> + */ >> + fpu->guest_perm.__user_state_size = >> fpu_user_cfg.default_size; > It seems like an appropriate value, but where does this come into play > exactly for guest FPUs? I don't see there's special usage of this field for vCPU in VMM userspace(QEMU). Maybe it's mainly for AMX resulted usespace fault handling? For vCPU thread, it's only referenced when AMX is enabled via __xfd_enable_feature() .
On Wed, 2023-11-29 at 22:12 +0800, Yang, Weijiang wrote: > On 11/28/2023 10:58 PM, Edgecombe, Rick P wrote: > > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > > > + /* > > > + * Set guest's __user_state_size to > > > fpu_user_cfg.default_size > > > so that > > > + * existing uAPIs can still work. > > > + */ > > > + fpu->guest_perm.__user_state_size = > > > fpu_user_cfg.default_size; > > It seems like an appropriate value, but where does this come into > > play > > exactly for guest FPUs? > > I don't see there's special usage of this field for vCPU in VMM > userspace(QEMU). > Maybe it's mainly for AMX resulted usespace fault handling? For vCPU > thread, > it's only referenced when AMX is enabled via __xfd_enable_feature() > . > In that case the "so that existing uAPIs can still work" comment seems misleading. Maybe "this doesn't come into play for guest FPUs, but set it to a reasonable value"?
On 11/30/2023 1:08 AM, Edgecombe, Rick P wrote: > On Wed, 2023-11-29 at 22:12 +0800, Yang, Weijiang wrote: >> On 11/28/2023 10:58 PM, Edgecombe, Rick P wrote: >>> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: >>>> + /* >>>> + * Set guest's __user_state_size to >>>> fpu_user_cfg.default_size >>>> so that >>>> + * existing uAPIs can still work. >>>> + */ >>>> + fpu->guest_perm.__user_state_size = >>>> fpu_user_cfg.default_size; >>> It seems like an appropriate value, but where does this come into >>> play >>> exactly for guest FPUs? >> I don't see there's special usage of this field for vCPU in VMM >> userspace(QEMU). >> Maybe it's mainly for AMX resulted usespace fault handling? For vCPU >> thread, >> it's only referenced when AMX is enabled via __xfd_enable_feature() >> . >> > In that case the "so that existing uAPIs can still work" comment seems > misleading. Maybe "this doesn't come into play for guest FPUs, but set > it to a reasonable value"? Ah, I mistook it for uabi_size and added comments. Will reword it properly. Thank you for bringing it up!
On Tue, 2023-11-28 at 14:58 +0000, Edgecombe, Rick P wrote: > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > > + /* > > + * Set guest's __user_state_size to fpu_user_cfg.default_size > > so that > > + * existing uAPIs can still work. > > + */ > > + fpu->guest_perm.__user_state_size = > > fpu_user_cfg.default_size; > > It seems like an appropriate value, but where does this come into play > exactly for guest FPUs? It is used because permission API is used for guest fpu state as well (for user features), and it affects two things: 1. If permission is not asked, then KVM will fail to resize the FPU state to match guest CPUID. 2. It will affect output size of the KVM_GET_XSAVE2 ioctl, which outputs buffer similar to other FPU state buffers exposed to userspace (like one saved on signal stack, or one obtained via ptrace). Best regards, Maxim Levitsky
On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote: > Define new fpu_guest_cfg to hold all guest FPU settings so that it can > differ from generic kernel FPU settings, e.g., enabling CET supervisor > xstate by default for guest fpstate while it's remained disabled in > kernel FPU config. > > The kernel dynamic xfeatures are specifically used by guest fpstate now, > add the mask for guest fpstate so that guest_perm.__state_permit == > (fpu_kernel_cfg.default_xfeature | XFEATURE_MASK_KERNEL_DYNAMIC). And > if guest fpstate is re-allocated to hold user dynamic xfeatures, the > resulting permissions are consumed before calculate new guest fpstate. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/include/asm/fpu/types.h | 2 +- > arch/x86/kernel/fpu/core.c | 14 +++++++++++--- > arch/x86/kernel/fpu/xstate.c | 10 ++++++++++ > 3 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h > index c6fd13a17205..306825ad6bc0 100644 > --- a/arch/x86/include/asm/fpu/types.h > +++ b/arch/x86/include/asm/fpu/types.h > @@ -602,6 +602,6 @@ struct fpu_state_config { > }; > > /* FPU state configuration information */ > -extern struct fpu_state_config fpu_kernel_cfg, fpu_user_cfg; > +extern struct fpu_state_config fpu_kernel_cfg, fpu_user_cfg, fpu_guest_cfg; > > #endif /* _ASM_X86_FPU_H */ > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c > index a21a4d0ecc34..516af626bf6a 100644 > --- a/arch/x86/kernel/fpu/core.c > +++ b/arch/x86/kernel/fpu/core.c > @@ -33,9 +33,10 @@ DEFINE_STATIC_KEY_FALSE(__fpu_state_size_dynamic); > DEFINE_PER_CPU(u64, xfd_state); > #endif > > -/* The FPU state configuration data for kernel and user space */ > +/* The FPU state configuration data for kernel, user space and guest. */ > struct fpu_state_config fpu_kernel_cfg __ro_after_init; > struct fpu_state_config fpu_user_cfg __ro_after_init; > +struct fpu_state_config fpu_guest_cfg __ro_after_init; > > /* > * Represents the initial FPU state. It's mostly (but not completely) zeroes, > @@ -536,8 +537,15 @@ void fpstate_reset(struct fpu *fpu) > fpu->perm.__state_perm = fpu_kernel_cfg.default_features; > fpu->perm.__state_size = fpu_kernel_cfg.default_size; > fpu->perm.__user_state_size = fpu_user_cfg.default_size; > - /* Same defaults for guests */ > - fpu->guest_perm = fpu->perm; > + > + /* Guest permission settings */ > + fpu->guest_perm.__state_perm = fpu_guest_cfg.default_features; > + fpu->guest_perm.__state_size = fpu_guest_cfg.default_size; > + /* > + * Set guest's __user_state_size to fpu_user_cfg.default_size so that > + * existing uAPIs can still work. > + */ > + fpu->guest_perm.__user_state_size = fpu_user_cfg.default_size; > } > > static inline void fpu_inherit_perms(struct fpu *dst_fpu) > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index ba4172172afd..aa8f8595cd41 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -681,6 +681,7 @@ static int __init init_xstate_size(void) > { > /* Recompute the context size for enabled features: */ > unsigned int user_size, kernel_size, kernel_default_size; > + unsigned int guest_default_size; > bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED); > > /* Uncompacted user space size */ > @@ -702,13 +703,18 @@ static int __init init_xstate_size(void) > kernel_default_size = > xstate_calculate_size(fpu_kernel_cfg.default_features, compacted); > > + guest_default_size = > + xstate_calculate_size(fpu_guest_cfg.default_features, compacted); > + > if (!paranoid_xstate_size_valid(kernel_size)) > return -EINVAL; > > fpu_kernel_cfg.max_size = kernel_size; > fpu_user_cfg.max_size = user_size; > + fpu_guest_cfg.max_size = kernel_size; > > fpu_kernel_cfg.default_size = kernel_default_size; > + fpu_guest_cfg.default_size = guest_default_size; > fpu_user_cfg.default_size = > xstate_calculate_size(fpu_user_cfg.default_features, false); > > @@ -829,6 +835,10 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > fpu_user_cfg.default_features = fpu_user_cfg.max_features; > fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > + fpu_guest_cfg.max_features = fpu_kernel_cfg.max_features; > + fpu_guest_cfg.default_features = fpu_guest_cfg.max_features; > + fpu_guest_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; > + > /* Store it for paranoia check at the end */ > xfeatures = fpu_kernel_cfg.max_features; > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Thu, 2023-11-30 at 19:29 +0200, Maxim Levitsky wrote: > 2. It will affect output size of the KVM_GET_XSAVE2 ioctl, which > outputs buffer similar to > other FPU state buffers exposed to userspace (like one saved on > signal stack, or one obtained via ptrace). Ah! I missed this part. Thanks for the correction. So the original comment is important and correct.
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h index c6fd13a17205..306825ad6bc0 100644 --- a/arch/x86/include/asm/fpu/types.h +++ b/arch/x86/include/asm/fpu/types.h @@ -602,6 +602,6 @@ struct fpu_state_config { }; /* FPU state configuration information */ -extern struct fpu_state_config fpu_kernel_cfg, fpu_user_cfg; +extern struct fpu_state_config fpu_kernel_cfg, fpu_user_cfg, fpu_guest_cfg; #endif /* _ASM_X86_FPU_H */ diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index a21a4d0ecc34..516af626bf6a 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -33,9 +33,10 @@ DEFINE_STATIC_KEY_FALSE(__fpu_state_size_dynamic); DEFINE_PER_CPU(u64, xfd_state); #endif -/* The FPU state configuration data for kernel and user space */ +/* The FPU state configuration data for kernel, user space and guest. */ struct fpu_state_config fpu_kernel_cfg __ro_after_init; struct fpu_state_config fpu_user_cfg __ro_after_init; +struct fpu_state_config fpu_guest_cfg __ro_after_init; /* * Represents the initial FPU state. It's mostly (but not completely) zeroes, @@ -536,8 +537,15 @@ void fpstate_reset(struct fpu *fpu) fpu->perm.__state_perm = fpu_kernel_cfg.default_features; fpu->perm.__state_size = fpu_kernel_cfg.default_size; fpu->perm.__user_state_size = fpu_user_cfg.default_size; - /* Same defaults for guests */ - fpu->guest_perm = fpu->perm; + + /* Guest permission settings */ + fpu->guest_perm.__state_perm = fpu_guest_cfg.default_features; + fpu->guest_perm.__state_size = fpu_guest_cfg.default_size; + /* + * Set guest's __user_state_size to fpu_user_cfg.default_size so that + * existing uAPIs can still work. + */ + fpu->guest_perm.__user_state_size = fpu_user_cfg.default_size; } static inline void fpu_inherit_perms(struct fpu *dst_fpu) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index ba4172172afd..aa8f8595cd41 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -681,6 +681,7 @@ static int __init init_xstate_size(void) { /* Recompute the context size for enabled features: */ unsigned int user_size, kernel_size, kernel_default_size; + unsigned int guest_default_size; bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED); /* Uncompacted user space size */ @@ -702,13 +703,18 @@ static int __init init_xstate_size(void) kernel_default_size = xstate_calculate_size(fpu_kernel_cfg.default_features, compacted); + guest_default_size = + xstate_calculate_size(fpu_guest_cfg.default_features, compacted); + if (!paranoid_xstate_size_valid(kernel_size)) return -EINVAL; fpu_kernel_cfg.max_size = kernel_size; fpu_user_cfg.max_size = user_size; + fpu_guest_cfg.max_size = kernel_size; fpu_kernel_cfg.default_size = kernel_default_size; + fpu_guest_cfg.default_size = guest_default_size; fpu_user_cfg.default_size = xstate_calculate_size(fpu_user_cfg.default_features, false); @@ -829,6 +835,10 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) fpu_user_cfg.default_features = fpu_user_cfg.max_features; fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; + fpu_guest_cfg.max_features = fpu_kernel_cfg.max_features; + fpu_guest_cfg.default_features = fpu_guest_cfg.max_features; + fpu_guest_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; + /* Store it for paranoia check at the end */ xfeatures = fpu_kernel_cfg.max_features;
Define new fpu_guest_cfg to hold all guest FPU settings so that it can differ from generic kernel FPU settings, e.g., enabling CET supervisor xstate by default for guest fpstate while it's remained disabled in kernel FPU config. The kernel dynamic xfeatures are specifically used by guest fpstate now, add the mask for guest fpstate so that guest_perm.__state_permit == (fpu_kernel_cfg.default_xfeature | XFEATURE_MASK_KERNEL_DYNAMIC). And if guest fpstate is re-allocated to hold user dynamic xfeatures, the resulting permissions are consumed before calculate new guest fpstate. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/include/asm/fpu/types.h | 2 +- arch/x86/kernel/fpu/core.c | 14 +++++++++++--- arch/x86/kernel/fpu/xstate.c | 10 ++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-)