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

["Chang S. Bae" <chang.seok.bae@intel.com>]

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