| Message ID | 20230914063325.85503-7-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On 9/13/23 23:33, Yang Weijiang wrote: > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > > /* Calculate the resulting kernel state size */ > mask = permitted | requested; > - /* Take supervisor states into account on the host */ > + /* > + * Take supervisor states into account on the host. And add > + * kernel dynamic xfeatures to guest since guest kernel may > + * enable corresponding CPU feaures and the xstate registers > + * need to be saved/restored properly. > + */ > if (!guest) > mask |= xfeatures_mask_supervisor(); > + else > + mask |= fpu_kernel_dynamic_xfeatures; > + > ksize = xstate_calculate_size(mask, compacted); Heh, you changed the "guest" naming in "fpu_kernel_dynamic_xfeatures" but didn't change the logic. As it's coded at the moment *ALL* "fpu_kernel_dynamic_xfeatures" are guest xfeatures. So, they're different in name only. If you want to change the rules for guests, we have *ONE* place that's done: fpstate_reset(). It establishes the permissions and the sizes for the default guest FPU. Start there. If you want to make the guest defaults include XFEATURE_CET_USER, then you need to put the bit in *there*. The other option is to have the KVM code actually go and "request" that the dynamic states get added to 'fpu->guest_perm'. Would there ever be any reason for KVM to be on a system which supports a dynamic kernel feature but where it doesn't get enabled for guest use, or at least shouldn't have the FPU space allocated?
On 9/15/2023 1:40 AM, Dave Hansen wrote: > On 9/13/23 23:33, Yang Weijiang wrote: >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) >> >> /* Calculate the resulting kernel state size */ >> mask = permitted | requested; >> - /* Take supervisor states into account on the host */ >> + /* >> + * Take supervisor states into account on the host. And add >> + * kernel dynamic xfeatures to guest since guest kernel may >> + * enable corresponding CPU feaures and the xstate registers >> + * need to be saved/restored properly. >> + */ >> if (!guest) >> mask |= xfeatures_mask_supervisor(); >> + else >> + mask |= fpu_kernel_dynamic_xfeatures; >> + >> ksize = xstate_calculate_size(mask, compacted); > Heh, you changed the "guest" naming in "fpu_kernel_dynamic_xfeatures" > but didn't change the logic. > > As it's coded at the moment *ALL* "fpu_kernel_dynamic_xfeatures" are > guest xfeatures. So, they're different in name only. > > If you want to change the rules for guests, we have *ONE* place that's > done: fpstate_reset(). It establishes the permissions and the sizes for > the default guest FPU. Start there. If you want to make the guest > defaults include XFEATURE_CET_USER, then you need to put the bit in *there*. Yeah, fpstate_reset() is the right place to hold the guest init permits and propagate them here, thanks for the suggestion! Nit, did you actually mean XFEATURE_CET_KERNEL instead of XFEATURE_CET_USER above? because the latter is already supported by upstream kernel. > The other option is to have the KVM code actually go and "request" that > the dynamic states get added to 'fpu->guest_perm'. Yes, compared with above option, it will change current userspace handling logic, i.e., only user xstates are dynamically requested. So I'd try above option first. > Would there ever be > any reason for KVM to be on a system which supports a dynamic kernel > feature but where it doesn't get enabled for guest use, or at least > shouldn't have the FPU space allocated? I haven't heard of that kind of usage for other features so far, CET supervisor xstate is the only dynamic kernel feature now, not sure whether other CPU features having supervisor xstate would share the handling logic like CET does one day.
On Fri, Sep 15, 2023, Weijiang Yang wrote: > On 9/15/2023 1:40 AM, Dave Hansen wrote: > > On 9/13/23 23:33, Yang Weijiang wrote: > > > --- a/arch/x86/kernel/fpu/xstate.c > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > > > /* Calculate the resulting kernel state size */ > > > mask = permitted | requested; > > > - /* Take supervisor states into account on the host */ > > > + /* > > > + * Take supervisor states into account on the host. And add > > > + * kernel dynamic xfeatures to guest since guest kernel may > > > + * enable corresponding CPU feaures and the xstate registers > > > + * need to be saved/restored properly. > > > + */ > > > if (!guest) > > > mask |= xfeatures_mask_supervisor(); > > > + else > > > + mask |= fpu_kernel_dynamic_xfeatures; This looks wrong. Per commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions"), mask at this point only contains user features, which somewhat unintuitively doesn't include CET_USER (I get that they're MSRs and thus supervisor state, it's just the name that's odd). IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_supervisor() conatins PASID, CET_USER, and CET_KERNEL. PASID isn't virtualized by KVM, but doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/XTILE enabling? The existing code also seems odd, but I might be missing something. Won't the kernel drop PASID if the guest request AMX/XTILE? I'm not at all familiar with what PASID state is managed via XSAVE, so I've no idea if that's an actual problem or just an oddity. > > > ksize = xstate_calculate_size(mask, compacted); > > Heh, you changed the "guest" naming in "fpu_kernel_dynamic_xfeatures" > > but didn't change the logic. > > > > As it's coded at the moment *ALL* "fpu_kernel_dynamic_xfeatures" are > > guest xfeatures. So, they're different in name only. ... > > Would there ever be any reason for KVM to be on a system which supports a > > dynamic kernel feature but where it doesn't get enabled for guest use, or > > at least shouldn't have the FPU space allocated? > > I haven't heard of that kind of usage for other features so far, CET > supervisor xstate is the only dynamic kernel feature now, not sure whether > other CPU features having supervisor xstate would share the handling logic > like CET does one day. There are definitely scenarios where CET will not be exposed to KVM guests, but I don't see any reason to make the guest FPU space dynamically sized for CET. It's what, 40 bytes? I would much prefer to avoid the whole "dynamic" thing and instead make CET explicitly guest-only. E.g. fpu_kernel_guest_only_xfeatures? Or even better if it doesn't cause weirdness elsewhere, a dedicated fpu_guest_cfg. For me at least, a fpu_guest_cfg would make it easier to understand what all is going on.
On 10/25/2023 1:07 AM, Sean Christopherson wrote: > On Fri, Sep 15, 2023, Weijiang Yang wrote: >> On 9/15/2023 1:40 AM, Dave Hansen wrote: >>> On 9/13/23 23:33, Yang Weijiang wrote: >>>> --- a/arch/x86/kernel/fpu/xstate.c >>>> +++ b/arch/x86/kernel/fpu/xstate.c >>>> @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) >>>> /* Calculate the resulting kernel state size */ >>>> mask = permitted | requested; >>>> - /* Take supervisor states into account on the host */ >>>> + /* >>>> + * Take supervisor states into account on the host. And add >>>> + * kernel dynamic xfeatures to guest since guest kernel may >>>> + * enable corresponding CPU feaures and the xstate registers >>>> + * need to be saved/restored properly. >>>> + */ >>>> if (!guest) >>>> mask |= xfeatures_mask_supervisor(); >>>> + else >>>> + mask |= fpu_kernel_dynamic_xfeatures; > This looks wrong. Per commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor > states in XSTATE permissions"), mask at this point only contains user features, > which somewhat unintuitively doesn't include CET_USER (I get that they're MSRs > and thus supervisor state, it's just the name that's odd). I think the user-only boundary becomes unclear when fpstate_reset() introduce below line: fpu->perm.__state_perm = fpu_kernel_cfg.default_features; Then in xstate_request_perm(), it re-uses above reset value for __xstate_request_perm(), so in the latter, the mask is already mixed with supervisor xfeatures. > IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_supervisor() > conatins PASID, CET_USER, and CET_KERNEL. PASID isn't virtualized by KVM, but > doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/XTILE > enabling? Yes, __state_size is correct for guest enabled xfeatures, including CET_USER, and it gets removed from __state_perm. IIUC, from current qemu/kernel interaction for guest permission settings, __xstate_request_perm() is called only _ONCE_ to set AMX/XTILE for every vCPU thread, so the removal of guest supervisor xfeatures won't hurt guest! ;-/ > The existing code also seems odd, but I might be missing something. Won't the > kernel drop PASID if the guest request AMX/XTILE? Yeah, dropped after the first invocation. > I'm not at all familiar with > what PASID state is managed via XSAVE, so I've no idea if that's an actual problem > or just an oddity. > >>>> ksize = xstate_calculate_size(mask, compacted); >>> Heh, you changed the "guest" naming in "fpu_kernel_dynamic_xfeatures" >>> but didn't change the logic. >>> >>> As it's coded at the moment *ALL* "fpu_kernel_dynamic_xfeatures" are >>> guest xfeatures. So, they're different in name only. > ... > >>> Would there ever be any reason for KVM to be on a system which supports a >>> dynamic kernel feature but where it doesn't get enabled for guest use, or >>> at least shouldn't have the FPU space allocated? >> I haven't heard of that kind of usage for other features so far, CET >> supervisor xstate is the only dynamic kernel feature now, not sure whether >> other CPU features having supervisor xstate would share the handling logic >> like CET does one day. > There are definitely scenarios where CET will not be exposed to KVM guests, but > I don't see any reason to make the guest FPU space dynamically sized for CET. > It's what, 40 bytes? Could it also be xsave/xrstor operation efficiency for non-guest threads? > I would much prefer to avoid the whole "dynamic" thing and instead make CET > explicitly guest-only. E.g. fpu_kernel_guest_only_xfeatures? Or even better > if it doesn't cause weirdness elsewhere, a dedicated fpu_guest_cfg. For me at > least, a fpu_guest_cfg would make it easier to understand what all is going on. Agree, guess non-kernel-generic designs are not very much welcome for kernel...
On Wed, Oct 25, 2023, Weijiang Yang wrote: > On 10/25/2023 1:07 AM, Sean Christopherson wrote: > > On Fri, Sep 15, 2023, Weijiang Yang wrote: > > IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_supervisor() > > conatins PASID, CET_USER, and CET_KERNEL. PASID isn't virtualized by KVM, but > > doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/XTILE > > enabling? > > Yes, __state_size is correct for guest enabled xfeatures, including CET_USER, > and it gets removed from __state_perm. > > IIUC, from current qemu/kernel interaction for guest permission settings, > __xstate_request_perm() is called only _ONCE_ to set AMX/XTILE for every vCPU > thread, so the removal of guest supervisor xfeatures won't hurt guest! ;-/ Huh? I don't follow. What does calling __xstate_request_perm() only once have to do with anything? /me stares more OMG, hell no. First off, this code is a nightmare to follow. The existing comment is useless. No shit the code is adding in supervisor states for the host. What's not AT ALL clear is *why*. The commit says it's necessary because the "permission bitmap is only relevant for user states": commit 781c64bfcb735960717d1cb45428047ff6a5030c Author: Thomas Gleixner <tglx@linutronix.de> Date: Thu Mar 24 14:47:14 2022 +0100 x86/fpu/xstate: Handle supervisor states in XSTATE permissions The size calculation in __xstate_request_perm() fails to take supervisor states into account because the permission bitmap is only relevant for user states. But @permitted comes from: permitted = xstate_get_group_perm(guest); which is either fpu->guest_perm.__state_perm or fpu->perm.__state_perm. And __state_perm is initialized to: fpu->perm.__state_perm = fpu_kernel_cfg.default_features; where fpu_kernel_cfg.default_features contains everything except the dynamic xfeatures, i.e. everything except XFEATURE_MASK_XTILE_DATA: fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features; fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; So why on earth does this code to force back xfeatures_mask_supervisor()? Because the code just below drops the supervisor bits to compute the user xstate size and then clobbers __state_perm. /* Calculate the resulting user state size */ mask &= XFEATURE_MASK_USER_SUPPORTED; usize = xstate_calculate_size(mask, false); ... WRITE_ONCE(perm->__state_perm, mask); That is beyond asinine. IIUC, the intent is to apply the permission bitmap only for user states, because the only dynamic states are user states. Bbut the above creates an inconsistent mess. If userspace doesn't request XTILE_DATA, __state_perm will contain supervisor states, but once userspace does request XTILE_DATA, __state_perm will be lost. And because that's not confusing enough, clobbering __state_perm would also drop FPU_GUEST_PERM_LOCKED, except that __xstate_request_perm() can' be reached with said LOCKED flag set. fpu_xstate_prctl() already strips out supervisor features: case ARCH_GET_XCOMP_PERM: /* * Lockless snapshot as it can also change right after the * dropping the lock. */ permitted = xstate_get_host_group_perm(); permitted &= XFEATURE_MASK_USER_SUPPORTED; return put_user(permitted, uptr); case ARCH_GET_XCOMP_GUEST_PERM: permitted = xstate_get_guest_group_perm(); permitted &= XFEATURE_MASK_USER_SUPPORTED; return put_user(permitted, uptr); and while KVM doesn't apply the __state_perm to supervisor states, if it did there would be zero harm in doing so. case 0xd: { u64 permitted_xcr0 = kvm_get_filtered_xcr0(); u64 permitted_xss = kvm_caps.supported_xss; Second, the relying on QEMU to only trigger __xstate_request_perm() is not acceptable. It "works" for the current code, but only because there's only a single dynamic feature, i.e. this will short circuit and prevent computing a bad ksize. /* Check whether fully enabled */ if ((permitted & requested) == requested) return 0; I don't know how I can possibly make it any clearer: KVM absolutely must not assume userspace behavior. So rather than continue with the current madness, which will break if/when the next dynamic feature comes along, just preserve non-user xfeatures/flags in __guest_perm. If there are no objections, I'll test the below and write a proper changelog. -- From: Sean Christopherson <seanjc@google.com> Date: Thu, 26 Oct 2023 10:17:33 -0700 Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index ef6906107c54..73f6bc00d178 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) if ((permitted & requested) == requested) return 0; - /* Calculate the resulting kernel state size */ + /* + * Calculate the resulting kernel state size. Note, @permitted also + * contains supervisor xfeatures even though supervisor are always + * permitted for kernel and guest FPUs, and never permitted for user + * FPUs. + */ mask = permitted | requested; - /* Take supervisor states into account on the host */ - if (!guest) - mask |= xfeatures_mask_supervisor(); ksize = xstate_calculate_size(mask, compacted); - /* Calculate the resulting user state size */ - mask &= XFEATURE_MASK_USER_SUPPORTED; - usize = xstate_calculate_size(mask, false); + /* + * Calculate the resulting user state size. Take care not to clobber + * the supervisor xfeatures in the new mask! + */ + usize = xstate_calculate_size(mask & XFEATURE_MASK_USER_SUPPORTED, false); if (!guest) { ret = validate_sigaltstack(usize); base-commit: c076acf10c78c0d7e1aa50670e9cc4c91e8d59b4 --
On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: > + /* > + * Calculate the resulting kernel state size. Note, > @permitted also > + * contains supervisor xfeatures even though supervisor are > always > + * permitted for kernel and guest FPUs, and never permitted > for user > + * FPUs. What is a user FPU vs kernel FPU in this context? By user FPU do you mean, like user FPU state in a sigframe or something? Or a kernel task's FPU? If the former I think this comment could be made more clear. Maybe just drop the bit about user FPUs. At least the comment makes more sense to me without it.
On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: > On Wed, Oct 25, 2023, Weijiang Yang wrote: > > On 10/25/2023 1:07 AM, Sean Christopherson wrote: > > > On Fri, Sep 15, 2023, Weijiang Yang wrote: > > > IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_supervisor() > > > conatins PASID, CET_USER, and CET_KERNEL. PASID isn't virtualized by KVM, but > > > doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/XTILE > > > enabling? > > > > Yes, __state_size is correct for guest enabled xfeatures, including CET_USER, > > and it gets removed from __state_perm. > > > > IIUC, from current qemu/kernel interaction for guest permission settings, > > __xstate_request_perm() is called only _ONCE_ to set AMX/XTILE for every vCPU > > thread, so the removal of guest supervisor xfeatures won't hurt guest! ;-/ > > Huh? I don't follow. What does calling __xstate_request_perm() only once have > to do with anything? > > /me stares more > > OMG, hell no. First off, this code is a nightmare to follow. The existing comment > is useless. No shit the code is adding in supervisor states for the host. What's > not AT ALL clear is *why*. > > The commit says it's necessary because the "permission bitmap is only relevant > for user states": > > commit 781c64bfcb735960717d1cb45428047ff6a5030c > Author: Thomas Gleixner <tglx@linutronix.de> > Date: Thu Mar 24 14:47:14 2022 +0100 > > x86/fpu/xstate: Handle supervisor states in XSTATE permissions > > The size calculation in __xstate_request_perm() fails to take supervisor > states into account because the permission bitmap is only relevant for user > states. > > But @permitted comes from: > > permitted = xstate_get_group_perm(guest); > > which is either fpu->guest_perm.__state_perm or fpu->perm.__state_perm. And > __state_perm is initialized to: > > fpu->perm.__state_perm = fpu_kernel_cfg.default_features; Not anymore after patch 5, and patch 5 does make sense in the regard to the fact that we might not want to save/restore kernel CET state for nothing for regular kernel threads. > > where fpu_kernel_cfg.default_features contains everything except the dynamic > xfeatures, i.e. everything except XFEATURE_MASK_XTILE_DATA: > > fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features; > fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > So why on earth does this code to force back xfeatures_mask_supervisor()? Because > the code just below drops the supervisor bits to compute the user xstate size and > then clobbers __state_perm. > > /* Calculate the resulting user state size */ > mask &= XFEATURE_MASK_USER_SUPPORTED; > usize = xstate_calculate_size(mask, false); > > ... > > WRITE_ONCE(perm->__state_perm, mask); > > That is beyond asinine. IIUC, the intent is to apply the permission bitmap only > for user states, because the only dynamic states are user states. Bbut the above > creates an inconsistent mess. If userspace doesn't request XTILE_DATA, > __state_perm will contain supervisor states, but once userspace does request > XTILE_DATA, __state_perm will be lost. > > And because that's not confusing enough, clobbering __state_perm would also drop > FPU_GUEST_PERM_LOCKED, except that __xstate_request_perm() can' be reached with > said LOCKED flag set. > > fpu_xstate_prctl() already strips out supervisor features: > > case ARCH_GET_XCOMP_PERM: > /* > * Lockless snapshot as it can also change right after the > * dropping the lock. > */ > permitted = xstate_get_host_group_perm(); > permitted &= XFEATURE_MASK_USER_SUPPORTED; > return put_user(permitted, uptr); > > case ARCH_GET_XCOMP_GUEST_PERM: > permitted = xstate_get_guest_group_perm(); > permitted &= XFEATURE_MASK_USER_SUPPORTED; > return put_user(permitted, uptr); > > and while KVM doesn't apply the __state_perm to supervisor states, if it did > there would be zero harm in doing so. > > case 0xd: { > u64 permitted_xcr0 = kvm_get_filtered_xcr0(); > u64 permitted_xss = kvm_caps.supported_xss; > > Second, the relying on QEMU to only trigger __xstate_request_perm() is not acceptable. > It "works" for the current code, but only because there's only a single dynamic > feature, i.e. this will short circuit and prevent computing a bad ksize. > > /* Check whether fully enabled */ > if ((permitted & requested) == requested) > return 0; > > I don't know how I can possibly make it any clearer: KVM absolutely must not assume > userspace behavior. > > So rather than continue with the current madness, which will break if/when the > next dynamic feature comes along, just preserve non-user xfeatures/flags in > __guest_perm. I more or less agree with you, however I would like to discuss the FPU permissions in more depth: First of all we have two things at play here: 1. On demand resize of the thread's FPU state buffer to avoid penalty of context switching the AMX state. 2. The fact that allowing this on demand resize of this state buffer breaks the x86_64 ABI, because FPU state has to be saved on the signal stack and ABI allows the stack size to be smaller than what is needed to save the FPU state with AMX features enabled. Thus a two tiered approach was done: first application asks for a permission to use the dynamic features, and then when it actually uses it, the FPU state buffer is resized. Otherwise if an AMX instruction is used by the app but the permission was not asked by the app for its xstate component, the application is terminated. (I might not 100% understand this correctly, please correct me if I am wrong). However IMHO the 'fpu permission' name is a bit misleading, This feature is not really about security/permissions but more like opt-in to use newer ABI, for example KVM capabilities API, and the kernel will never refuse the permission request (except if the signal stack size is too small but the userspace can adjust it before asking for the permission) On top of that I think that applying the same permission approach to guest's FPU state is not a good fit, because of two reasons: 1. The guest FPU state will never be pushed on the signal stack - KVM swaps back the host FPU state before it returns from the KVM_RUN ioctl. Also I think (not sure) that ptrace can only access (FPU) state of a stopped process, and a stopped vCPU process will also first return to userspace. Again I might be mistaken here, I never researched this in depth. Assuming that I am correct on these assumptions, the guest FPU state can only be accessed via KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2 ioctls, which also returns the userspace portion of the state including optionally the AMX state, but this ioctl doesn't really need FPU permission framework, because it is a KVM ABI, and in fact KVM_GET_XSAVE2 was added exactly because of that: to make sure that userspace is aware that larger than 4K buffer can be returned. 2. Guest FPU state is not even on demand resized (but I can imagine that in the future we will do this). And of course, adding permissions for kernel features, that is even worse idea, which we really shouldn't do. > > If there are no objections, I'll test the below and write a proper changelog. > > -- > From: Sean Christopherson <seanjc@google.com> > Date: Thu, 26 Oct 2023 10:17:33 -0700 > Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in > __state_perm > > Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index ef6906107c54..73f6bc00d178 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > if ((permitted & requested) == requested) > return 0; > > - /* Calculate the resulting kernel state size */ > + /* > + * Calculate the resulting kernel state size. Note, @permitted also > + * contains supervisor xfeatures even though supervisor are always > + * permitted for kernel and guest FPUs, and never permitted for user > + * FPUs. > + */ > mask = permitted | requested; > - /* Take supervisor states into account on the host */ > - if (!guest) > - mask |= xfeatures_mask_supervisor(); > ksize = xstate_calculate_size(mask, compacted); This might not work with kernel dynamic features, because xfeatures_mask_supervisor() will return all supported supervisor features. Therefore at least until we have an actual kernel dynamic feature (a feature used by the host kernel and not KVM, and which has to be dynamic like AMX), I suggest that KVM stops using the permission API completely for the guest FPU state, and just gives all the features it wants to enable right to __fpu_alloc_init_guest_fpstate() (Guest FPU permission API IMHO should be deprecated and ignored) > > - /* Calculate the resulting user state size */ > - mask &= XFEATURE_MASK_USER_SUPPORTED; > - usize = xstate_calculate_size(mask, false); > + /* > + * Calculate the resulting user state size. Take care not to clobber > + * the supervisor xfeatures in the new mask! > + */ > + usize = xstate_calculate_size(mask & XFEATURE_MASK_USER_SUPPORTED, false); > > if (!guest) { > ret = validate_sigaltstack(usize); Best regards, Maxim Levitsky > > base-commit: c076acf10c78c0d7e1aa50670e9cc4c91e8d59b4
On Tue, Oct 31, 2023, Maxim Levitsky wrote: > On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: > > On Wed, Oct 25, 2023, Weijiang Yang wrote: > On top of that I think that applying the same permission approach to guest's > FPU state is not a good fit, because of two reasons: > > 1. The guest FPU state will never be pushed on the signal stack - KVM swaps > back the host FPU state before it returns from the KVM_RUN ioctl. > > Also I think (not sure) that ptrace can only access (FPU) state of a > stopped process, and a stopped vCPU process will also first return to > userspace. Again I might be mistaken here, I never researched this in > depth. > > Assuming that I am correct on these assumptions, the guest FPU state can > only be accessed via KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2 ioctls, > which also returns the userspace portion of the state including optionally > the AMX state, but this ioctl doesn't really need FPU permission > framework, because it is a KVM ABI, and in fact KVM_GET_XSAVE2 was added > exactly because of that: to make sure that userspace is aware that larger > than 4K buffer can be returned. > > 2. Guest FPU state is not even on demand resized (but I can imagine that in > the future we will do this). Just because guest FPU state isn't resized doesn't mean there's no value in requiring userspace to opt-in to allocating 8KiB of data per-vCPU. > And of course, adding permissions for kernel features, that is even worse > idea, which we really shouldn't do. > > > > > If there are no objections, I'll test the below and write a proper changelog. > > > > -- > > From: Sean Christopherson <seanjc@google.com> > > Date: Thu, 26 Oct 2023 10:17:33 -0700 > > Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in > > __state_perm > > > > Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > index ef6906107c54..73f6bc00d178 100644 > > --- a/arch/x86/kernel/fpu/xstate.c > > +++ b/arch/x86/kernel/fpu/xstate.c > > @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > > if ((permitted & requested) == requested) > > return 0; > > > > - /* Calculate the resulting kernel state size */ > > + /* > > + * Calculate the resulting kernel state size. Note, @permitted also > > + * contains supervisor xfeatures even though supervisor are always > > + * permitted for kernel and guest FPUs, and never permitted for user > > + * FPUs. > > + */ > > mask = permitted | requested; > > - /* Take supervisor states into account on the host */ > > - if (!guest) > > - mask |= xfeatures_mask_supervisor(); > > ksize = xstate_calculate_size(mask, compacted); > > This might not work with kernel dynamic features, because > xfeatures_mask_supervisor() will return all supported supervisor features. I don't understand what you mean by "This". Somewhat of a side topic, I feel very strongly that we should use "guest only" terminology instead of "dynamic". There is nothing dynamic about whether or not XFEATURE_CET_KERNEL is allowed; there's not even a real "decision" beyond checking wheter or not CET is supported. > Therefore at least until we have an actual kernel dynamic feature (a feature > used by the host kernel and not KVM, and which has to be dynamic like AMX), > I suggest that KVM stops using the permission API completely for the guest > FPU state, and just gives all the features it wants to enable right to By "it", I assume you mean userspace? > __fpu_alloc_init_guest_fpstate() (Guest FPU permission API IMHO should be > deprecated and ignored) KVM allocates guest FPU state during KVM_CREATE_VCPU, so not using prctl() would either require KVM to defer allocating guest FPU state until KVM_SET_CPUID{,2}, or would require a VM-scoped KVM ioctl() to let userspace opt-in to Allocating guest FPU state during KVM_SET_CPUID{,2} would get messy, as KVM allows multiple calls to KVM_SET_CPUID{,2} so long as the vCPU hasn't done KVM_RUN. E.g. KVM would need to support actually resizing guest FPU state, which would be extra complexity without any meaningful benefit. The only benefit I can think of for a VM-scoped ioctl() is that it would allow a single process to host multiple VMs with different dynamic xfeature requirements. But such a setup is mostly theoretical. Maybe it'll affect the SEV migration helper at some point? But even that isn't guaranteed. So while I agree that ARCH_GET_XCOMP_GUEST_PERM isn't ideal, practically speaking it's sufficient for all current use cases. Unless a concrete use case comes along, deprecating ARCH_GET_XCOMP_GUEST_PERM in favor of a KVM ioctl() would be churn for both the kernel and userspace without any meaningful benefit, or really even any true change in behavior.
On Wed, 2023-11-01 at 07:16 -0700, Sean Christopherson wrote: > On Tue, Oct 31, 2023, Maxim Levitsky wrote: > > On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: > > > On Wed, Oct 25, 2023, Weijiang Yang wrote: > > On top of that I think that applying the same permission approach to guest's > > FPU state is not a good fit, because of two reasons: > > > > 1. The guest FPU state will never be pushed on the signal stack - KVM swaps > > back the host FPU state before it returns from the KVM_RUN ioctl. > > > > Also I think (not sure) that ptrace can only access (FPU) state of a > > stopped process, and a stopped vCPU process will also first return to > > userspace. Again I might be mistaken here, I never researched this in > > depth. > > > > Assuming that I am correct on these assumptions, the guest FPU state can > > only be accessed via KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2 ioctls, > > which also returns the userspace portion of the state including optionally > > the AMX state, but this ioctl doesn't really need FPU permission > > framework, because it is a KVM ABI, and in fact KVM_GET_XSAVE2 was added > > exactly because of that: to make sure that userspace is aware that larger > > than 4K buffer can be returned. > > > > 2. Guest FPU state is not even on demand resized (but I can imagine that in > > the future we will do this). > > Just because guest FPU state isn't resized doesn't mean there's no value in > requiring userspace to opt-in to allocating 8KiB of data per-vCPU. See my response below: > > > And of course, adding permissions for kernel features, that is even worse > > idea, which we really shouldn't do. > > > > > > > > If there are no objections, I'll test the below and write a proper changelog. > > > > > > -- > > > From: Sean Christopherson <seanjc@google.com> > > > Date: Thu, 26 Oct 2023 10:17:33 -0700 > > > Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in > > > __state_perm > > > > > > Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- > > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > index ef6906107c54..73f6bc00d178 100644 > > > --- a/arch/x86/kernel/fpu/xstate.c > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > > > if ((permitted & requested) == requested) > > > return 0; > > > > > > - /* Calculate the resulting kernel state size */ > > > + /* > > > + * Calculate the resulting kernel state size. Note, @permitted also > > > + * contains supervisor xfeatures even though supervisor are always > > > + * permitted for kernel and guest FPUs, and never permitted for user > > > + * FPUs. > > > + */ > > > mask = permitted | requested; > > > - /* Take supervisor states into account on the host */ > > > - if (!guest) > > > - mask |= xfeatures_mask_supervisor(); > > > ksize = xstate_calculate_size(mask, compacted); > > > > This might not work with kernel dynamic features, because > > xfeatures_mask_supervisor() will return all supported supervisor features. > > I don't understand what you mean by "This". > > Somewhat of a side topic, I feel very strongly that we should use "guest only" > terminology instead of "dynamic". There is nothing dynamic about whether or not > XFEATURE_CET_KERNEL is allowed; there's not even a real "decision" beyond checking > wheter or not CET is supported. > > Therefore at least until we have an actual kernel dynamic feature (a feature > > used by the host kernel and not KVM, and which has to be dynamic like AMX), > > I suggest that KVM stops using the permission API completely for the guest > > FPU state, and just gives all the features it wants to enable right to > > By "it", I assume you mean userspace? > > > __fpu_alloc_init_guest_fpstate() (Guest FPU permission API IMHO should be > > deprecated and ignored) > > KVM allocates guest FPU state during KVM_CREATE_VCPU, so not using prctl() would > either require KVM to defer allocating guest FPU state until KVM_SET_CPUID{,2}, > or would require a VM-scoped KVM ioctl() to let userspace opt-in to > > Allocating guest FPU state during KVM_SET_CPUID{,2} would get messy, > as KVM allows > multiple calls to KVM_SET_CPUID{,2} so long as the vCPU hasn't done KVM_RUN. E.g. > KVM would need to support actually resizing guest FPU state, which would be extra > complexity without any meaningful benefit. OK, I understand you now. What you claim is that it is legal to do this: - KVM_SET_XSAVE - KVM_SET_CPUID (with AMX enabled) KVM_SET_CPUID will have to resize the xstate which is already valid. Your patch to fix the __xstate_request_perm() does seem to be correct in a sense that it will preserve the kernel fpu components in the fpu permissions. However note that kernel fpu permissions come from 'fpu_kernel_cfg.default_features' which don't include the dynamic kernel xfeatures (added a few patches before this one). Therefore an attempt to resize the xstate to include a kernel dynamic feature by __xfd_enable_feature will fail. If kvm on the other hand includes all the kernel dynamic features in the initial allocation of FPU state (not optimal but possible), then later call to __xstate_request_perm for a userspace dynamic feature (which can still happen) will mess the the xstate, because again the permission code assumes that only default kernel features were granted the permissions. This has to be solved this way or another. > > The only benefit I can think of for a VM-scoped ioctl() is that it would allow a > single process to host multiple VMs with different dynamic xfeature requirements. > But such a setup is mostly theoretical. Maybe it'll affect the SEV migration > helper at some point? But even that isn't guaranteed. > > So while I agree that ARCH_GET_XCOMP_GUEST_PERM isn't ideal, practically speaking > it's sufficient for all current use cases. Unless a concrete use case comes along, > deprecating ARCH_GET_XCOMP_GUEST_PERM in favor of a KVM ioctl() would be churn for > both the kernel and userspace without any meaningful benefit, or really even any > true change in behavior. ARCH_GET_XCOMP_GUEST_PERM/ARCH_SET_XCOMP_GUEST_PERM is not a good API from usability POV, because it is redundant. KVM already has API called KVM_SET_CPUID2, by which the qemu/userspace instructs the KVM, how much space to allocate, to support a VM with *this* CPUID. For example if qemu asks for nested SVM/VMX, then kvm will allocate on demand state for it (also at least 8K/vCPU btw). The same should apply for AMX - Qemu sets AMX xsave bit in CPUID - that permits KVM to allocate the extra state when needed. I don't see why we need an extra and non KVM API for that. Best regards, Maxim Levitsky >
On Thu, Nov 02, 2023, Maxim Levitsky wrote: > On Wed, 2023-11-01 at 07:16 -0700, Sean Christopherson wrote: > > On Tue, Oct 31, 2023, Maxim Levitsky wrote: > > > On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: > > > > -- > > > > From: Sean Christopherson <seanjc@google.com> > > > > Date: Thu, 26 Oct 2023 10:17:33 -0700 > > > > Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in > > > > __state_perm > > > > > > > > Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > --- > > > > arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- > > > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > > index ef6906107c54..73f6bc00d178 100644 > > > > --- a/arch/x86/kernel/fpu/xstate.c > > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > > @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > > > > if ((permitted & requested) == requested) > > > > return 0; > > > > > > > > - /* Calculate the resulting kernel state size */ > > > > + /* > > > > + * Calculate the resulting kernel state size. Note, @permitted also > > > > + * contains supervisor xfeatures even though supervisor are always > > > > + * permitted for kernel and guest FPUs, and never permitted for user > > > > + * FPUs. > > > > + */ > > > > mask = permitted | requested; > > > > - /* Take supervisor states into account on the host */ > > > > - if (!guest) > > > > - mask |= xfeatures_mask_supervisor(); > > > > ksize = xstate_calculate_size(mask, compacted); > > > > > > This might not work with kernel dynamic features, because > > > xfeatures_mask_supervisor() will return all supported supervisor features. > > > > I don't understand what you mean by "This". > > > > > Somewhat of a side topic, I feel very strongly that we should use "guest only" > > terminology instead of "dynamic". There is nothing dynamic about whether or not > > XFEATURE_CET_KERNEL is allowed; there's not even a real "decision" beyond checking > > wheter or not CET is supported. > > > > Therefore at least until we have an actual kernel dynamic feature (a feature > > > used by the host kernel and not KVM, and which has to be dynamic like AMX), > > > I suggest that KVM stops using the permission API completely for the guest > > > FPU state, and just gives all the features it wants to enable right to > > > > By "it", I assume you mean userspace? > > > > > __fpu_alloc_init_guest_fpstate() (Guest FPU permission API IMHO should be > > > deprecated and ignored) > > > > KVM allocates guest FPU state during KVM_CREATE_VCPU, so not using prctl() would > > either require KVM to defer allocating guest FPU state until KVM_SET_CPUID{,2}, > > or would require a VM-scoped KVM ioctl() to let userspace opt-in to > > > > Allocating guest FPU state during KVM_SET_CPUID{,2} would get messy, > > > as KVM allows > > multiple calls to KVM_SET_CPUID{,2} so long as the vCPU hasn't done KVM_RUN. E.g. > > KVM would need to support actually resizing guest FPU state, which would be extra > > complexity without any meaningful benefit. > > > OK, I understand you now. What you claim is that it is legal to do this: > > - KVM_SET_XSAVE > - KVM_SET_CPUID (with AMX enabled) > > KVM_SET_CPUID will have to resize the xstate which is already valid. I was actually talking about KVM_SET_CPUID2 (with dynamic user feature #1) KVM_SET_CPUID2 (with dynamic user feature #2) The second call through __xstate_request_perm() will be done with only user xfeatures in @permitted and so the kernel will compute the wrong ksize. > Your patch to fix the __xstate_request_perm() does seem to be correct in a > sense that it will preserve the kernel fpu components in the fpu permissions. > > However note that kernel fpu permissions come from > 'fpu_kernel_cfg.default_features' which don't include the dynamic kernel > xfeatures (added a few patches before this one). CET_KERNEL isn't dynamic! It's guest-only. There are no runtime decisions as to whether or not CET_KERNEL is allowed. All guest FPU get CET_KERNEL, no kernel FPUs get CET_KERNEL. That matters because I am also proposing that we add a dedicated, defined-at-boot fpu_guest_cfg instead of bolting on a "dynamic", which is what I meant by this: : Or even better if it doesn't cause weirdness elsewhere, a dedicated : fpu_guest_cfg. For me at least, a fpu_guest_cfg would make it easier to : understand what all is going on. That way, initialization of permissions is simply fpu->guest_perm = fpu_guest_cfg.default_features; and there's no need to differentiate between guest and kernel FPUs when reallocating for dynamic user xfeatures because guest_perm.__state_perm already holds the correct data. > Therefore an attempt to resize the xstate to include a kernel dynamic feature by > __xfd_enable_feature will fail. > > If kvm on the other hand includes all the kernel dynamic features in the > initial allocation of FPU state (not optimal but possible), This is what I am suggesting. : There are definitely scenarios where CET will not be exposed to KVM guests, but : I don't see any reason to make the guest FPU space dynamically sized for CET. : It's what, 40 bytes? > then later call to __xstate_request_perm for a userspace dynamic feature > (which can still happen) will mess the the xstate, because again the > permission code assumes that only default kernel features were granted the > permissions. > > > This has to be solved this way or another. > > > > > The only benefit I can think of for a VM-scoped ioctl() is that it would allow a > > single process to host multiple VMs with different dynamic xfeature requirements. > > But such a setup is mostly theoretical. Maybe it'll affect the SEV migration > > helper at some point? But even that isn't guaranteed. > > > > So while I agree that ARCH_GET_XCOMP_GUEST_PERM isn't ideal, practically speaking > > it's sufficient for all current use cases. Unless a concrete use case comes along, > > deprecating ARCH_GET_XCOMP_GUEST_PERM in favor of a KVM ioctl() would be churn for > > both the kernel and userspace without any meaningful benefit, or really even any > > true change in behavior. > > > ARCH_GET_XCOMP_GUEST_PERM/ARCH_SET_XCOMP_GUEST_PERM is not a good API from > usability POV, because it is redundant. > > KVM already has API called KVM_SET_CPUID2, by which the qemu/userspace > instructs the KVM, how much space to allocate, to support a VM with *this* > CPUID. > > For example if qemu asks for nested SVM/VMX, then kvm will allocate on demand > state for it (also at least 8K/vCPU btw). The same should apply for AMX - > Qemu sets AMX xsave bit in CPUID - that permits KVM to allocate the extra > state when needed. > > I don't see why we need an extra and non KVM API for that. I don't necessarily disagree, but what's done is done. We missed our chance to propose a different mechanism, and at this point undoing all of that without good cause is unlikely to benefit anyone. If a use comes along that needs something "better" than the prctl() API, then I agree it'd be worth revisiting.
On Fri, 2023-11-03 at 07:33 -0700, Sean Christopherson wrote: > On Thu, Nov 02, 2023, Maxim Levitsky wrote: > > On Wed, 2023-11-01 at 07:16 -0700, Sean Christopherson wrote: > > > On Tue, Oct 31, 2023, Maxim Levitsky wrote: > > > > On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: > > > > > -- > > > > > From: Sean Christopherson <seanjc@google.com> > > > > > Date: Thu, 26 Oct 2023 10:17:33 -0700 > > > > > Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in > > > > > __state_perm > > > > > > > > > > Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > > --- > > > > > arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- > > > > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > > > > > index ef6906107c54..73f6bc00d178 100644 > > > > > --- a/arch/x86/kernel/fpu/xstate.c > > > > > +++ b/arch/x86/kernel/fpu/xstate.c > > > > > @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) > > > > > if ((permitted & requested) == requested) > > > > > return 0; > > > > > > > > > > - /* Calculate the resulting kernel state size */ > > > > > + /* > > > > > + * Calculate the resulting kernel state size. Note, @permitted also > > > > > + * contains supervisor xfeatures even though supervisor are always > > > > > + * permitted for kernel and guest FPUs, and never permitted for user > > > > > + * FPUs. > > > > > + */ > > > > > mask = permitted | requested; > > > > > - /* Take supervisor states into account on the host */ > > > > > - if (!guest) > > > > > - mask |= xfeatures_mask_supervisor(); > > > > > ksize = xstate_calculate_size(mask, compacted); > > > > > > > > This might not work with kernel dynamic features, because > > > > xfeatures_mask_supervisor() will return all supported supervisor features. > > > > > > I don't understand what you mean by "This". > > > Somewhat of a side topic, I feel very strongly that we should use "guest only" > > > terminology instead of "dynamic". There is nothing dynamic about whether or not > > > XFEATURE_CET_KERNEL is allowed; there's not even a real "decision" beyond checking > > > wheter or not CET is supported. > > > > Therefore at least until we have an actual kernel dynamic feature (a feature > > > > used by the host kernel and not KVM, and which has to be dynamic like AMX), > > > > I suggest that KVM stops using the permission API completely for the guest > > > > FPU state, and just gives all the features it wants to enable right to > > > > > > By "it", I assume you mean userspace? > > > > > > > __fpu_alloc_init_guest_fpstate() (Guest FPU permission API IMHO should be > > > > deprecated and ignored) > > > > > > KVM allocates guest FPU state during KVM_CREATE_VCPU, so not using prctl() would > > > either require KVM to defer allocating guest FPU state until KVM_SET_CPUID{,2}, > > > or would require a VM-scoped KVM ioctl() to let userspace opt-in to > > > > > > Allocating guest FPU state during KVM_SET_CPUID{,2} would get messy, > > > as KVM allows > > > multiple calls to KVM_SET_CPUID{,2} so long as the vCPU hasn't done KVM_RUN. E.g. > > > KVM would need to support actually resizing guest FPU state, which would be extra > > > complexity without any meaningful benefit. > > > > OK, I understand you now. What you claim is that it is legal to do this: > > > > - KVM_SET_XSAVE > > - KVM_SET_CPUID (with AMX enabled) > > > > KVM_SET_CPUID will have to resize the xstate which is already valid. > > I was actually talking about > > KVM_SET_CPUID2 (with dynamic user feature #1) > KVM_SET_CPUID2 (with dynamic user feature #2) > > The second call through __xstate_request_perm() will be done with only user > xfeatures in @permitted and so the kernel will compute the wrong ksize. > > > Your patch to fix the __xstate_request_perm() does seem to be correct in a > > sense that it will preserve the kernel fpu components in the fpu permissions. > > > > However note that kernel fpu permissions come from > > 'fpu_kernel_cfg.default_features' which don't include the dynamic kernel > > xfeatures (added a few patches before this one). > > CET_KERNEL isn't dynamic! It's guest-only. There are no runtime decisions as to > whether or not CET_KERNEL is allowed. All guest FPU get CET_KERNEL, no kernel FPUs > get CET_KERNEL. > > That matters because I am also proposing that we add a dedicated, defined-at-boot > fpu_guest_cfg instead of bolting on a "dynamic", which is what I meant by this: Seems fair. > > : Or even better if it doesn't cause weirdness elsewhere, a dedicated > : fpu_guest_cfg. For me at least, a fpu_guest_cfg would make it easier to > : understand what all is going on. This is a very good idea. > > That way, initialization of permissions is simply > > fpu->guest_perm = fpu_guest_cfg.default_features; > > and there's no need to differentiate between guest and kernel FPUs when reallocating > for dynamic user xfeatures because guest_perm.__state_perm already holds the correct > data. > > > Therefore an attempt to resize the xstate to include a kernel dynamic feature by > > __xfd_enable_feature will fail. > > > > If kvm on the other hand includes all the kernel dynamic features in the > > initial allocation of FPU state (not optimal but possible), > > This is what I am suggesting. This is a valid solution. > > : There are definitely scenarios where CET will not be exposed to KVM guests, but > : I don't see any reason to make the guest FPU space dynamically sized for CET. > : It's what, 40 bytes? I don't disagree with this. Allocating all guest kernel features is a valid solution for now although this can change in the future if a 'heavy' kernel feature comes. Also IMHO its not a question of space but more question of run-time overhead. I don't know how well the INIT/MODIFIED ucode state tracking works (on Intel and AMD) and what are the costs of saving/restoring an unused feature. But again this is a valid solution and as long as the code works, I don't have anything against it. > > > then later call to __xstate_request_perm for a userspace dynamic feature > > (which can still happen) will mess the the xstate, because again the > > permission code assumes that only default kernel features were granted the > > permissions. > > > > > > This has to be solved this way or another. > > > > > The only benefit I can think of for a VM-scoped ioctl() is that it would allow a > > > single process to host multiple VMs with different dynamic xfeature requirements. > > > But such a setup is mostly theoretical. Maybe it'll affect the SEV migration > > > helper at some point? But even that isn't guaranteed. > > > > > > So while I agree that ARCH_GET_XCOMP_GUEST_PERM isn't ideal, practically speaking > > > it's sufficient for all current use cases. Unless a concrete use case comes along, > > > deprecating ARCH_GET_XCOMP_GUEST_PERM in favor of a KVM ioctl() would be churn for > > > both the kernel and userspace without any meaningful benefit, or really even any > > > true change in behavior. > > > > ARCH_GET_XCOMP_GUEST_PERM/ARCH_SET_XCOMP_GUEST_PERM is not a good API from > > usability POV, because it is redundant. > > > > KVM already has API called KVM_SET_CPUID2, by which the qemu/userspace > > instructs the KVM, how much space to allocate, to support a VM with *this* > > CPUID. > > > > For example if qemu asks for nested SVM/VMX, then kvm will allocate on demand > > state for it (also at least 8K/vCPU btw). The same should apply for AMX - > > Qemu sets AMX xsave bit in CPUID - that permits KVM to allocate the extra > > state when needed. > > > > I don't see why we need an extra and non KVM API for that. > > I don't necessarily disagree, but what's done is done. We missed our chance to > propose a different mechanism, and at this point undoing all of that without good > cause is unlikely to benefit anyone. If a use comes along that needs something > "better" than the prctl() API, then I agree it'd be worth revisiting. I do think that it is not too late to deprecate the ARCH_GET_XCOMP_GUEST_PERM/ARCH_SET_XCOMP_GUEST_PERM, and just ignore it, instead taking the guest CPUID as the source of truth. That API was out only for a few releases and only has to be used for AMX which is a very new feature. Also if we let the guest call the deprecated API but ignore it (allow everything regardless if the userspace called the permission API) that will not break the existing code IMHO. Best regards, Maxim Levitsky >
On 11/8/2023 2:04 AM, Maxim Levitsky wrote: > On Fri, 2023-11-03 at 07:33 -0700, Sean Christopherson wrote: >> On Thu, Nov 02, 2023, Maxim Levitsky wrote: >>> On Wed, 2023-11-01 at 07:16 -0700, Sean Christopherson wrote: >>>> On Tue, Oct 31, 2023, Maxim Levitsky wrote: >>>>> On Thu, 2023-10-26 at 10:24 -0700, Sean Christopherson wrote: >>>>>> -- >>>>>> From: Sean Christopherson <seanjc@google.com> >>>>>> Date: Thu, 26 Oct 2023 10:17:33 -0700 >>>>>> Subject: [PATCH] x86/fpu/xstate: Always preserve non-user xfeatures/flags in >>>>>> __state_perm >>>>>> >>>>>> Fixes: 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE permissions") >>>>>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>>>>> --- >>>>>> arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- >>>>>> 1 file changed, 11 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >>>>>> index ef6906107c54..73f6bc00d178 100644 >>>>>> --- a/arch/x86/kernel/fpu/xstate.c >>>>>> +++ b/arch/x86/kernel/fpu/xstate.c >>>>>> @@ -1601,16 +1601,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) >>>>>> if ((permitted & requested) == requested) >>>>>> return 0; >>>>>> >>>>>> - /* Calculate the resulting kernel state size */ >>>>>> + /* >>>>>> + * Calculate the resulting kernel state size. Note, @permitted also >>>>>> + * contains supervisor xfeatures even though supervisor are always >>>>>> + * permitted for kernel and guest FPUs, and never permitted for user >>>>>> + * FPUs. >>>>>> + */ >>>>>> mask = permitted | requested; >>>>>> - /* Take supervisor states into account on the host */ >>>>>> - if (!guest) >>>>>> - mask |= xfeatures_mask_supervisor(); >>>>>> ksize = xstate_calculate_size(mask, compacted); >>>>> This might not work with kernel dynamic features, because >>>>> xfeatures_mask_supervisor() will return all supported supervisor features. >>>> I don't understand what you mean by "This". >>>> Somewhat of a side topic, I feel very strongly that we should use "guest only" >>>> terminology instead of "dynamic". There is nothing dynamic about whether or not >>>> XFEATURE_CET_KERNEL is allowed; there's not even a real "decision" beyond checking >>>> wheter or not CET is supported. >>>>> Therefore at least until we have an actual kernel dynamic feature (a feature >>>>> used by the host kernel and not KVM, and which has to be dynamic like AMX), >>>>> I suggest that KVM stops using the permission API completely for the guest >>>>> FPU state, and just gives all the features it wants to enable right to >>>> By "it", I assume you mean userspace? >>>> >>>>> __fpu_alloc_init_guest_fpstate() (Guest FPU permission API IMHO should be >>>>> deprecated and ignored) >>>> KVM allocates guest FPU state during KVM_CREATE_VCPU, so not using prctl() would >>>> either require KVM to defer allocating guest FPU state until KVM_SET_CPUID{,2}, >>>> or would require a VM-scoped KVM ioctl() to let userspace opt-in to >>>> >>>> Allocating guest FPU state during KVM_SET_CPUID{,2} would get messy, >>>> as KVM allows >>>> multiple calls to KVM_SET_CPUID{,2} so long as the vCPU hasn't done KVM_RUN. E.g. >>>> KVM would need to support actually resizing guest FPU state, which would be extra >>>> complexity without any meaningful benefit. >>> OK, I understand you now. What you claim is that it is legal to do this: >>> >>> - KVM_SET_XSAVE >>> - KVM_SET_CPUID (with AMX enabled) >>> >>> KVM_SET_CPUID will have to resize the xstate which is already valid. >> I was actually talking about >> >> KVM_SET_CPUID2 (with dynamic user feature #1) >> KVM_SET_CPUID2 (with dynamic user feature #2) >> >> The second call through __xstate_request_perm() will be done with only user >> xfeatures in @permitted and so the kernel will compute the wrong ksize. >> >>> Your patch to fix the __xstate_request_perm() does seem to be correct in a >>> sense that it will preserve the kernel fpu components in the fpu permissions. >>> >>> However note that kernel fpu permissions come from >>> 'fpu_kernel_cfg.default_features' which don't include the dynamic kernel >>> xfeatures (added a few patches before this one). >> CET_KERNEL isn't dynamic! It's guest-only. There are no runtime decisions as to >> whether or not CET_KERNEL is allowed. All guest FPU get CET_KERNEL, no kernel FPUs >> get CET_KERNEL. >> >> That matters because I am also proposing that we add a dedicated, defined-at-boot >> fpu_guest_cfg instead of bolting on a "dynamic", which is what I meant by this: > Seems fair. > >> : Or even better if it doesn't cause weirdness elsewhere, a dedicated >> : fpu_guest_cfg. For me at least, a fpu_guest_cfg would make it easier to >> : understand what all is going on. > This is a very good idea. > >> That way, initialization of permissions is simply >> >> fpu->guest_perm = fpu_guest_cfg.default_features; >> >> and there's no need to differentiate between guest and kernel FPUs when reallocating >> for dynamic user xfeatures because guest_perm.__state_perm already holds the correct >> data. >> >>> Therefore an attempt to resize the xstate to include a kernel dynamic feature by >>> __xfd_enable_feature will fail. >>> >>> If kvm on the other hand includes all the kernel dynamic features in the >>> initial allocation of FPU state (not optimal but possible), >> This is what I am suggesting. > This is a valid solution. Sorry for delayed response!! I favor adding new fpu_guest_cfg to make things clearer. Maybe you're talking about some patch like below: (not tested) From 19c77aad196efe7eab4a10c5882166453de287b9 Mon Sep 17 00:00:00 2001 From: Yang Weijiang <weijiang.yang@intel.com> Date: Fri, 22 Sep 2023 00:37:20 -0400 Subject: [PATCH] x86/fpu/xstate: Introduce fpu_guest_cfg for guest FPU configuration 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 | 9 +++++++++ 3 files changed, 21 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 a86d37052a64..c70dad9894f0 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, @@ -535,8 +536,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 1b7bc03968c5..bebabace628b 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,9 @@ 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 &= ~XFEATURE_MASK_USER_DYNAMIC; + /* Store it for paranoia check at the end */ xfeatures = fpu_kernel_cfg.max_features; -- 2.27.0 [...]
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index 4753c677e2e1..c5d903b4df4d 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest) /* Calculate the resulting kernel state size */ mask = permitted | requested; - /* Take supervisor states into account on the host */ + /* + * Take supervisor states into account on the host. And add + * kernel dynamic xfeatures to guest since guest kernel may + * enable corresponding CPU feaures and the xstate registers + * need to be saved/restored properly. + */ if (!guest) mask |= xfeatures_mask_supervisor(); + else + mask |= fpu_kernel_dynamic_xfeatures; + ksize = xstate_calculate_size(mask, compacted); /* Calculate the resulting user state size */
When user space requests guest xstate permits, the sufficient xstate size is calculated from permitted mask. Currently the max guest permits are set to fpu_kernel_cfg.default_features, and the latter doesn't include kernel dynamic xfeatures, so add them back for correct guest fpstate size. If guest dynamic xfeatures are enabled, KVM re-allocates guest fpstate area with above resulting size before launches VM. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kernel/fpu/xstate.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)