diff mbox series

[v3,04/10] x86/fpu/xstate: Correct guest fpstate size calculation

Message ID 20250307164123.1613414-5-chao.gao@intel.com (mailing list archive)
State New
Headers show
Series Introduce CET supervisor state support | expand

Commit Message

Chao Gao March 7, 2025, 4:41 p.m. UTC 
From: Yang Weijiang <weijiang.yang@intel.com>

The guest fpstate size is calculated based on fpu_user_cfg, while
fpstate->xfeatures is set to fpu_kernel_cfg.default_features in
fpu_alloc_guest_fpstate(). In other words, the guest fpstate doesn't
allocate memory for all supervisor states, even though they are enabled.

Correct the calculation of the guest fpstate size.

Note that this issue does not cause any functional problems because the
guest fpstate is allocated using vmalloc(), which aligns the size to a
full page, providing enough space for all existing supervisor components.
On Emerald Rapids CPUs, the guest fpstate after this correction is ~2880
bytes.

Link: https://lore.kernel.org/kvm/20230914063325.85503-3-weijiang.yang@intel.com/
Fixes: 69f6ed1d14c6 ("x86/fpu: Provide infrastructure for KVM FPU cleanup")
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kernel/fpu/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Hansen March 7, 2025, 5:53 p.m. UTC | #1 
On 3/7/25 08:41, Chao Gao wrote:
> Note that this issue does not cause any functional problems because the
> guest fpstate is allocated using vmalloc(), which aligns the size to a
> full page, providing enough space for all existing supervisor components.
> On Emerald Rapids CPUs, the guest fpstate after this correction is ~2880
> bytes.

How about we move up the fpstate pointers at allocation time so they
just scrape the end of the vmalloc buffer? Basically, move the
page-alignment padding to the beginning of the first page instead of the
end of the last page.
Chang S. Bae March 7, 2025, 9:37 p.m. UTC | #2 
On 3/7/2025 8:41 AM, Chao Gao wrote:
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 6166a928d3f5..adc34914634e 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -218,7 +218,7 @@ 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);
>   	fpstate = vzalloc(size);
>   	if (!fpstate)
>   		return false;

BTW, did you ever base this series on the tip/master branch? The fix has 
already been merged there:

   1937e18cc3cf ("x86/fpu: Fix guest FPU state buffer allocation size")

Thanks,
Chang
Chao Gao March 8, 2025, 2:49 a.m. UTC | #3 
On Fri, Mar 07, 2025 at 01:37:15PM -0800, Chang S. Bae wrote:
>On 3/7/2025 8:41 AM, Chao Gao wrote:
>> 
>> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
>> index 6166a928d3f5..adc34914634e 100644
>> --- a/arch/x86/kernel/fpu/core.c
>> +++ b/arch/x86/kernel/fpu/core.c
>> @@ -218,7 +218,7 @@ 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);
>>   	fpstate = vzalloc(size);
>>   	if (!fpstate)
>>   		return false;
>
>BTW, did you ever base this series on the tip/master branch? The fix has
>already been merged there:
>
>  1937e18cc3cf ("x86/fpu: Fix guest FPU state buffer allocation size")

Thanks for the information. I will remove this patch.

This series is currently based on 6.14-rc5. I should have used the tip/master
branch as the base and will do so next time.
Chao Gao March 8, 2025, 2:56 a.m. UTC | #4 
On Fri, Mar 07, 2025 at 09:53:40AM -0800, Dave Hansen wrote:
>On 3/7/25 08:41, Chao Gao wrote:
>> Note that this issue does not cause any functional problems because the
>> guest fpstate is allocated using vmalloc(), which aligns the size to a
>> full page, providing enough space for all existing supervisor components.
>> On Emerald Rapids CPUs, the guest fpstate after this correction is ~2880
>> bytes.
>
>How about we move up the fpstate pointers at allocation time so they
>just scrape the end of the vmalloc buffer? Basically, move the
>page-alignment padding to the beginning of the first page instead of the
>end of the last page.

That sounds like a good way to detect similar errors and might be helpful for
all other vmalloc'ed buffers.

I can try to implement this for the fpstate pointers. The patch will be put
at the end of the series or even in a separate series.
Chang S. Bae March 9, 2025, 10:06 p.m. UTC | #5 
On 3/7/2025 6:49 PM, Chao Gao wrote:
> On Fri, Mar 07, 2025 at 01:37:15PM -0800, Chang S. Bae wrote:
>> On 3/7/2025 8:41 AM, Chao Gao wrote:
>>>
>>> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
>>> index 6166a928d3f5..adc34914634e 100644
>>> --- a/arch/x86/kernel/fpu/core.c
>>> +++ b/arch/x86/kernel/fpu/core.c
>>> @@ -218,7 +218,7 @@ 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);
>>>    	fpstate = vzalloc(size);
>>>    	if (!fpstate)
>>>    		return false;
>>
>> BTW, did you ever base this series on the tip/master branch? The fix has
>> already been merged there:
>>
>>   1937e18cc3cf ("x86/fpu: Fix guest FPU state buffer allocation size")
> 
> Thanks for the information. I will remove this patch.

But, I think there is a fallout that someone should follow up:

The merged patch ensures size consistency between 
fpu_alloc_guest_fpstate() and fpstate_realloc(), maintaining a 
consistent reference to the kernel buffer size. However, within 
fpu_alloc_guest_fpstate(), fpu_guest->xfeatures should also be adjusted 
accordingly for consistency. Instead of referencing fpu_user_cfg, it 
should reference fpu_kernel_cfg.

Thanks,
CHang
Chao Gao March 10, 2025, 1:33 a.m. UTC | #6 
On Sun, Mar 09, 2025 at 03:06:19PM -0700, Chang S. Bae wrote:
>On 3/7/2025 6:49 PM, Chao Gao wrote:
>> On Fri, Mar 07, 2025 at 01:37:15PM -0800, Chang S. Bae wrote:
>> > On 3/7/2025 8:41 AM, Chao Gao wrote:
>> > > 
>> > > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
>> > > index 6166a928d3f5..adc34914634e 100644
>> > > --- a/arch/x86/kernel/fpu/core.c
>> > > +++ b/arch/x86/kernel/fpu/core.c
>> > > @@ -218,7 +218,7 @@ 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);
>> > >    	fpstate = vzalloc(size);
>> > >    	if (!fpstate)
>> > >    		return false;
>> > 
>> > BTW, did you ever base this series on the tip/master branch? The fix has
>> > already been merged there:
>> > 
>> >   1937e18cc3cf ("x86/fpu: Fix guest FPU state buffer allocation size")
>> 
>> Thanks for the information. I will remove this patch.
>
>But, I think there is a fallout that someone should follow up:
>
>The merged patch ensures size consistency between fpu_alloc_guest_fpstate()
>and fpstate_realloc(), maintaining a consistent reference to the kernel
>buffer size. However, within fpu_alloc_guest_fpstate(), fpu_guest->xfeatures
>should also be adjusted accordingly for consistency. Instead of referencing
>fpu_user_cfg, it should reference fpu_kernel_cfg.

This is fixed by the patch 3.

>
>Thanks,
>CHang
>
Chang S. Bae March 10, 2025, 5:21 a.m. UTC | #7 
On 3/9/2025 6:33 PM, Chao Gao wrote:
> 
> This is fixed by the patch 3.

Well, take a look at your changelog — the context is quite different. I 
don't think it'S mergeable without a rewrite. Also, this should be a 
standalone fix to complement the recent tip-tree changes.

Thanks,
Chang
Chao Gao March 10, 2025, 7:06 a.m. UTC | #8 
On Sun, Mar 09, 2025 at 10:21:12PM -0700, Chang S. Bae wrote:
>On 3/9/2025 6:33 PM, Chao Gao wrote:
>> 
>> This is fixed by the patch 3.
>
>Well, take a look at your changelog — the context is quite different. I don't
>think it'S mergeable without a rewrite. Also, this should be a standalone fix
>to complement the recent tip-tree changes.

Should patch 2 be posted separately?

Because current tip/master branch has:

	gfpu->xfeatures		= fpu_user_cfg.default_features;
	gfpu->perm		= fpu_user_cfg.default_features;

Adjusting only fpu_guest->features raises the question: why isn't gfpu->perm
adjusted as well?

If patch 2 should go first, I don't think it's necessary to post patches 2-3
separately as maintainers can easily pick up patches 1-3 when they are in good
shape.

Regarding the changelog, I am uncertain what's quite different in the context.
It seems both you and I are talking about the inconsistency between
gfpu->xfeatures and fpstate->xfeatures. Did I miss something obvious?
Chang S. Bae March 10, 2025, 5:33 p.m. UTC | #9 
On 3/10/2025 12:06 AM, Chao Gao wrote:
> 
> Should patch 2 be posted separately?

gfpu->perm has been somewhat overlooked, as __xstate_request_perm() does 
not update this field. However, I see that as a separate issue. The 
options are either to fix it so that it remains in sync with 
fpu->guest_perm consistently or to remove it entirely, as you proposed, 
if it has no actual use.

There hasn’t been any relevant change that would justify a quick 
follow-up like the other case. So, I'd assume it as part of this series.

But yes, I think gfpu->perm is also going to be 
fpu_kernel_cfg.default_features at the moment.

> Regarding the changelog, I am uncertain what's quite different in the context.
> It seems both you and I are talking about the inconsistency between
> gfpu->xfeatures and fpstate->xfeatures. Did I miss something obvious?

I saw a distinction between inconsistencies within a function and 
inconsistencies across functions.

Stepping back a bit, the approach for defining the VCPU xfeature set was 
originally intended to include only user features, but it now appears 
somewhat inconsistent:

(a) In fpu_alloc_guest_fpstate(), fpu_user_cfg is used.
(b) However, __fpstate_reset() references fpu_kernel_cfg to set storage
     attributes.
(c) Additionally, fpu->guest_perm takes fpu_kernel_cfg, which affects
     fpstate_realloc().

To maintain a consistent VCPU xfeature set, (b) and (c) should be corrected.

Alternatively, the VCPU xfeature set could be reconsidered to align with 
how other tasks handle it. This might offer better maintainability 
across functions. In that case, another option would be simply updating 
fpu_alloc_guest_fpstate().

The recent tip-tree change seems somewhat incomplete — perhaps in 
hindsight. If following up on this, the changelog should specifically 
address inconsistencies within a function. I saw this as a way to 
solidify an upcoming change, where addressing it sooner rather than 
later would be beneficial.

In patch 3, you've pointed out the inconsistency between (a) and (b), 
which is a valid point. However, the fix is only partial and does not 
fully address the issue either. Moreover, the patch does not reference 
the recent tip-tree change as it didn't have any context at that time.

Thanks,
Chang
diff mbox series

Patch

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 6166a928d3f5..adc34914634e 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -218,7 +218,7 @@  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);
 	fpstate = vzalloc(size);
 	if (!fpstate)
 		return false;