Re: [PATCH v8 03/20] KVM: x86: Support APX state for XSAVE ABI

[Jim Mattson]

On Wed, Sep 30, 2026 at 4:02 PM Chang S. Bae <chang.seok.bae@xxxxxxxxx> wrote:
>
> Introduce a facility to copy APX state between the VCPU cache and the
> userspace buffer as the VCPU cache is the single source of truth.
>
> The existing fpstate copy functions historically sync all XSTATEs in
> between userspace and kernel buffers [1]. In this regard, any additional
> state handling logic should be consistent with them -- i.e. validation of
> XSTATE_BV against the supported XCR0 mask.
>
> Now with the two copy paths, their invocations require to take care of
> these facts:
>
>   * When exporting to userspace, the fpstate function should run first
>     since it zeros out the area of components either not present or
>     inactive. Then the VCPU cache function ensures copying APX state.
>
>   * On the opposite way, both will copy the state to both storages. This
>     duplication is avoidable by tweaking either the generic function or
>     XSTATE_BV/XCR0. But the optimization while at a slow path looks to
>     rather add complication. So keep the implementation simple.
>
> [1] Except for PKRU state, as stored in struct thread_struct.
>
> Tested-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
> Signed-off-by: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> ---

[ ... ]

> +static int kvm_copy_uabi_to_vcpu_regs(struct kvm_vcpu *vcpu, void *buf, u64 supported_xcr0)
> +{
> +       union fpregs_state *xstate = (union fpregs_state *)buf;
> +
> +       if (!(xstate->xsave.header.xfeatures & XFEATURE_MASK_APX))
> +               return 0;
> +
> +       if (!(supported_xcr0 & XFEATURE_MASK_APX))
> +               return -EINVAL;

Sashiko is right that this early return leaves stale values in R16-R31 when
XSTATE_BV[19] is 0.

In your reply to Sashiko, you wrote:

  This matches the existing XSAVE uABI semantics. XSTATE_BV specifies
  which state components are being updated. If userspace wants to zero
  the APX state, it must include the APX bit in XSTATE_BV; omitting the
  bit means "leave this component unchanged", just like the generic
  XSAVE copy path.

That is not how XRSTOR or KVM_SET_XSAVE works. In the XSAVE architecture,
XSTATE_BV[i] = 0 indicates that state component i is in its initial
configuration (typically zeros), not that it should be left unchanged. In
copy_uabi_to_xstate(), clearing bit i in xsave->header.xfeatures causes the
subsequent XRSTOR[S] in restore_fpregs_from_fpstate() to re-initialize
component i to its hardware INIT state. As the comment in
fpu_copy_uabi_to_guest_fpstate() notes, PKRU is the *only* exception to
this rule, for historical reasons:

    /*
     * Nullify @vpkru to preserve its current value if PKRU's bit isn't set
     * in the header.  KVM's odd ABI is to leave PKRU untouched in this
     * case (all other components are eventually re-initialized).
     */

Because R16-R31 are loaded into hardware from vcpu->arch.regs rather than
from guest_fpu via XRSTOR[S], kvm_copy_uabi_to_vcpu_regs() must explicitly
zero vcpu->arch.regs[VCPU_REGS_R16..VCPU_REGS_R31] when XFEATURE_MASK_APX
is clear in xstate->xsave.header.xfeatures.

[ ... ]

> @@ -3189,6 +3240,16 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>          */
>         xstate->xsave.header.xfeatures &= ~vcpu->arch.guest_fpu.fpstate->xfd;
>
> +       /*
> +        * This and the following copy functions copy APX state into each
> +        * storage. The VCPU cache is the single source of truth but avoiding
> +        * this redundant copy tends to introduce special-case handling. Since
> +        * this lies in a slow path, keep the implementation simple.
> +        */
> +       err = kvm_copy_uabi_to_vcpu_regs(vcpu, guest_xsave->region, kvm_caps.supported_xcr0);
> +       if (err)
> +               return err;
> +
>         return fpu_copy_uabi_to_guest_fpstate(&vcpu->arch.guest_fpu,
>                                               guest_xsave->region,
>                                               kvm_caps.supported_xcr0,

If fpu_copy_uabi_to_guest_fpstate() fails, vcpu->arch.regs[R16..R31] will
have already been clobbered.

Moreover, if you call kvm_copy_uabi_to_vcpu_regs() *after*
fpu_copy_uabi_to_guest_fpstate() succeeds, you don't need to check
supported_xcr0 in kvm_copy_uabi_to_vcpu_regs(), since
fpu_copy_uabi_to_guest_fpstate() rejects any xfeatures bits not in
kvm_caps.supported_xcr0.