Re: [PATCH v6 06/25] x86/fpu/xstate: Opt-in kernel dynamic bits when calculate guest xstate size

[Yang, Weijiang]

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.