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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> > 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.
