Re: [PATCH v6 02/25] x86/fpu/xstate: Fix guest fpstate allocation size calculation

From: "Yang, Weijiang" <weijiang.yang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <dave.hansen@intel.com>,
	<peterz@infradead.org>, <chao.gao@intel.com>,
	<rick.p.edgecombe@intel.com>, <john.allen@amd.com>
Subject: Re: [PATCH v6 02/25] x86/fpu/xstate: Fix guest fpstate allocation size calculation
Date: Wed, 25 Oct 2023 21:49:45 +0800	[thread overview]
Message-ID: <b867fba0-ccdd-7020-7bf9-6572aa9dd408@intel.com> (raw)
In-Reply-To: <ZTfxly8573xdnruS@google.com>

On 10/25/2023 12:32 AM, Sean Christopherson wrote:
> On Tue, Oct 24, 2023, Weijiang Yang wrote:
>> On 10/21/2023 8:39 AM, Sean Christopherson wrote:
>>> On Thu, Sep 14, 2023, Yang Weijiang wrote:
>>>> Fix guest xsave area allocation size from fpu_user_cfg.default_size to
>>>> fpu_kernel_cfg.default_size so that the xsave area size is consistent
>>>> with fpstate->size set in __fpstate_reset().
>>>>
>>>> With the fix, guest fpstate size is sufficient for KVM supported guest
>>>> xfeatures.
>>>>
>>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>>>> ---
>>>>    arch/x86/kernel/fpu/core.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
>>>> index a86d37052a64..a42d8ad26ce6 100644
>>>> --- a/arch/x86/kernel/fpu/core.c
>>>> +++ b/arch/x86/kernel/fpu/core.c
>>>> @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
>>>>    	struct fpstate *fpstate;
>>>>    	unsigned int size;
>>>> -	size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
>>>> +	size = fpu_kernel_cfg.default_size +
>>>> +	       ALIGN(offsetof(struct fpstate, regs), 64);
>>> Shouldn't all the other calculations in this function also switch to fpu_kernel_cfg?
>>> At the very least, this looks wrong when paired with the above:
>>>
>>> 	gfpu->uabi_size		= sizeof(struct kvm_xsave);
>>> 	if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size))
>>> 		gfpu->uabi_size = fpu_user_cfg.default_size;
>> Hi, Sean,
>> Not sure what's your concerns.
>>  From my understanding fpu_kernel_cfg.default_size should include all enabled
>> xfeatures in host (XCR0 | XSS), this is also expected for supporting all
>> guest enabled xfeatures. gfpu->uabi_size only includes enabled user xfeatures
>> which are operated via KVM uABIs(KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2),
>> so the two sizes are relatively independent since guest supervisor xfeatures
>> are saved/restored via GET/SET_MSRS interfaces.
> Ah, right, I keep forgetting that KVM's ABI can't use XRSTOR because it forces
> the compacted format.
>
> This part still looks odd to me:
>
> 	gfpu->xfeatures		= fpu_user_cfg.default_features;
> 	gfpu->perm		= fpu_user_cfg.default_features;

I guess when the kernel FPU code was overhauled, the supervisor xstates were not taken into
account for guest supported xfeaures, so the first line looks reasonable until supervisor xfeatures
are landing. And for the second line, per current design, the user mode can only control user
xfeatures via arch_prctl() kernel uAPI, so it also makes sense to initialize perm with
fpu_user_cfg.default_features too.

But in this CET KVM series I'd like to expand the former to support all guest enabled xfeatures, i.e.,
both user and supervisor xfeaures, and keep the latter as-is since there seems no reason
to allow userspace to alter supervisor xfeatures.

> but I'm probably just not understanding something in the other patches changes yet.