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(-)
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;
--
2.46.1On 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
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.
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
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
>
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
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?
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
On Mon, Mar 10, 2025 at 10:33:20AM -0700, Chang S. Bae wrote: >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. Hi Chang, All of the above makes sense to me. Thank you for your review and suggestions. I will update the changelog to reference the recent change in tip-tree and post it separately. One thing I'm not entirely clear on is "the fix is only partial". I assume I need to update gfpu->perm to reference fpu_kernel_cfg to complement the fix. Is that correct?
On 3/11/2025 5:09 AM, Chao Gao wrote: > > One thing I'm not entirely clear on is "the fix is only partial". I assume I > need to update gfpu->perm to reference fpu_kernel_cfg to complement the fix. > Is that correct? Yes, I think so. Thanks, Chang
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.
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.
© 2016 - 2026 Red Hat, Inc.