Re: [PATCH v1 6/8] x86: Remove fpu_initialised/fpu_dirty

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/03/2026 1:29 pm, Ross Lagerwall wrote:
> With lazy FPU removed, fpu_initialised and fpu_dirty are always set to
> true in vcpu_restore_fpu() so remove them and adjust the code
> accordingly.
>
> No functional change intended.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
>  xen/arch/x86/domctl.c              |  3 +--
>  xen/arch/x86/hvm/emulate.c         |  6 +-----
>  xen/arch/x86/hvm/hvm.c             | 15 ++++++---------
>  xen/arch/x86/hvm/vlapic.c          |  3 ---
>  xen/arch/x86/i387.c                | 31 ++----------------------------
>  xen/arch/x86/include/asm/hvm/hvm.h |  1 -
>  xen/arch/x86/include/asm/xstate.h  | 11 -----------
>  xen/arch/x86/xstate.c              | 21 +++++---------------
>  xen/common/domain.c                |  2 --
>  xen/include/xen/sched.h            |  4 ----
>  10 files changed, 15 insertions(+), 82 deletions(-)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 942f41c584d4..d9b08182ac1d 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1409,8 +1409,7 @@ void arch_get_info_guest(struct vcpu *v, 
> vcpu_guest_context_u c)
>          c(flags = v->arch.pv.vgc_flags & ~(VGCF_i387_valid|VGCF_in_kernel));
>      else
>          c(flags = 0);
> -    if ( v->fpu_initialised )
> -        c(flags |= VGCF_i387_valid);
> +    c(flags |= VGCF_i387_valid);

This is an API/ABI change.  Previously, creating a vCPU and instantly
getting state will hand back a record with !VGCF_i387_valid.

It's fine - I've done a bunch of API/ABI changes in the FRED work, but
it at least needs calling out in the commit message.

We have had a lot of cases where calling arch_{get,set}_info_guest()
without an intervening __context_switch() would lead to subtle
differences.  Generally I've been moving in the direction of
architectural behaviour and not worrying about API/ABI changes which
would occur naturally from running the vCPU.

That said, I think d1895441b3bad (2007) was the removal of the final
consumer of VGCF_i387_valid in Xen.  We don't even have a conditional
reset of state based on it's absence, and of course it's documented in
the usual place, so it's really unclear what the purpose of this flag
ever was. [edit, see below]

> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 79697487ba90..885f5d304b2f 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -289,10 +288,8 @@ static void vlapic_init_sipi_one(struct vcpu *target, 
> uint32_t icr)
>          hvm_vcpu_down(target);
>          domain_lock(target->domain);
>          /* Reset necessary VCPU state. This does not include FPU state. */
> -        fpu_initialised = target->fpu_initialised;
>          rc = vcpu_reset(target);
>          ASSERT(!rc);
> -        target->fpu_initialised = fpu_initialised;
>          vlapic_do_init(vcpu_vlapic(target));

This whole code block irks me.  x86 has two architectural events, #RESET
and #INIT which are well defined, and this is using the former to mean
the latter.

We are going to need to fix this, and it's going to be some fairly
invasive renaming, but the result will be better. [edit, see below]

> diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
> index 88018397b1ad..5e893a2aab94 100644
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -265,7 +240,6 @@ void vcpu_reset_fpu(struct vcpu *v)
>  {
>      struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(v);
>  
> -    v->fpu_initialised = false;
>      *xsave_area = (struct xsave_struct) {
>          .xsave_hdr.xstate_bv = X86_XCR0_X87,
>      };
> @@ -282,7 +256,6 @@ void vcpu_setup_fpu(struct vcpu *v, const void *data)
>  {
>      struct xsave_struct *xsave_area = VCPU_MAP_XSAVE_AREA(v);
>  
> -    v->fpu_initialised = true;
>      *xsave_area = (struct xsave_struct) {
>          .fpu_sse = *(const fpusse_t*)data,
>          .xsave_hdr.xstate_bv = XSTATE_FP_SSE,


Hmm, looking at the callers of these two, we find that Xen has
VGCF_I387_VALID too, and does have a consumer of this flag.  (This needs
deleting for sanity reasons.)

It also means that this patch does introduce a bug here.  Calling
arch_get_info_guest() prior to scheduling will hand back a block of all
0's, claiming it to be valid.

We need to arrange for vcpu_reset_fpu() to be called during vCPU
construction (i.e. so we've never got a bad FPU state), before this
patch will be safe.

~Andrew