Message-ID: <ZUJdohf6wLE5LrCN@google.com>
Date:   Wed, 1 Nov 2023 07:16:02 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     Weijiang Yang <weijiang.yang@...el.com>,
        Dave Hansen <dave.hansen@...el.com>, pbonzini@...hat.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        peterz@...radead.org, chao.gao@...el.com,
        rick.p.edgecombe@...el.com, john.allen@....com
Subject: Re: [PATCH v6 06/25] x86/fpu/xstate: Opt-in kernel dynamic bits when
 calculate guest xstate size

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

Powered by blists - more mailing lists