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

[Chang S. Bae]

On 11/5/26 16:13, Jim Mattson wrote:
> On Wed, Sep 30, 2026 at 4:02 PM Chang S. Bae <chang.seok.bae@xxxxxxxxx> wrote:
> >
>
> > +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 

But copy_uabi_to_xstate() does not overwrite XSTATE_BV. Instead,

	/*
	 * Add back in the features that came in from userspace:
	 */
	xsave->header.xfeatures |= hdr.xfeatures;

This means that even if userspace clears a bit, the bit in the kernel 
XSAVE buffer remains set if it was already set. Consequently, clearing 
XSTATE_BV[i] in the userspace buffer does not necessarily reset the 
state component.

I'd follow up separately with update the wording, if needed, to match 
with this behavior.

> > @@ -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.

I think this is a good point. Since it was decided to let APX state go 
through the fpstate anyway, it looks rather simplifying if switching the 
order.

Thanks,
Chang