Re: [PATCH 0/7] KVM: x86: APX reg prep work

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

[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]

On 4/2/2026 4:07 PM, Sean Christopherson wrote:
> On Wed, Mar 25, 2026, Chang S. Bae wrote:
>> On 3/12/2026 10:47 AM, Sean Christopherson wrote:
>>> On Thu, Mar 12, 2026, Chang S. Bae wrote:
>>>>
>>>> However, that is sort of what-if scenarios at best. The host kernel still
>>>> manages EGPR context switching through XSAVE. Saving EGPRs into regs[] would
>>>> introduce an oddity to synchronize between two buffers: regs[] and
>>>> gfpu->fpstate, which looks like unnecessary complexity.
>>
>> No, this looks ugly.
> 
> Sorry, you lost me.  What looks ugly?

Oh, this is against my comment above. Keeping regs[] <-> guest fpstate 
in sync will be unnecessarily complex without clear usage (continues below).

>> If guest EGPR state is saved in vcpu->arch.regs[], the APX area there isn't
>> necessary:
>>
>> When the KVM API exposes state in XSAVE format, the frontend can handle this
>> separately. Alongside uABI <-> guest fpstate copy functions, new copy
>> functions may deal with the state between uABI <-> VCPU cache.
>>
>> Further, one could think of exclusion as such:
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 76153dfb58c9..5404f9399eea 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -794,9 +794,10 @@ static u64 __init guest_default_mask(void)
>> {
>> 	/*
>> 	 * Exclude dynamic features, which require userspace opt-in even
>> -	 * for KVM guests.
>> +	 * for KVM guests, and APX as extended general-purpose register
>> +	 * states are saved in the KVM cache separately.
>> 	 */
>> -	return ~(u64)XFEATURE_MASK_USER_DYNAMIC;
>> +	return ~((u64)XFEATURE_MASK_USER_DYNAMIC | XFEATURE_MASK_APX);
>> }
>>
>> But this default bitmask feeds into the permission bits:
>>
>> 	fpu->guest_perm.__state_perm    = guest_default_cfg.features;
>> 	fpu->guest_perm.__state_size    = guest_default_cfg.size;
>>
>> This policy looks clear and sensible: permission is granted only if space is
>> reserved to save the state. If there is a strong desire to save memory, I
>> think it should go through a more thorough review to revisit this policy.
> 
> And I'm lost again.

Here I made myself pursuing the approach saving/restoring EGPRs via 
regs[] on VM entry/exit. Then a couple of follow-up questions:

   1. What about APX area in guest fpstate?
   2. How to support the state for KVM ABI?

It surely departs from the "XSAVE - the single source of truth" model. 
Then,

   Leave the APX area in guest fpstate unused.

   Copying APX state directly between regs[] and uABI to preserve XSAVE-
   based ABI like in the attached diff.

That's all I'm saying.

[-- Attachment #2: kvmapi-apx.diff --]
[-- Type: text/plain, Size: 4470 bytes --]

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index fffbf087937d..b3ab2ac827e6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -59,6 +59,16 @@ void __init kvm_init_xstate_sizes(void)
 	}
 }
 
+u32 xstate_size(unsigned int xfeature)
+{
+	return xstate_sizes[xfeature].eax;
+}
+
+u32 xstate_offset(unsigned int xfeature)
+{
+	return xstate_sizes[xfeature].ebx;
+}
+
 u32 xstate_required_size(u64 xstate_bv, bool compacted)
 {
 	u32 ret = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 039b8e6f40ba..5ace99dd152b 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -64,6 +64,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 
 void __init kvm_init_xstate_sizes(void);
 u32 xstate_required_size(u64 xstate_bv, bool compacted);
+u32 xstate_size(unsigned int xfeature);
+u32 xstate_offset(unsigned int xfeature);
 
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
 int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c1e1b3030786..1f064a32b8b7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -108,6 +108,12 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_host);
 #define emul_to_vcpu(ctxt) \
 	((struct kvm_vcpu *)(ctxt)->vcpu)
 
+#ifdef CONFIG_KVM_APX
+#define VCPU_EGPRS_PTR(vcpu)   &(vcpu)->arch.regs[VCPU_REGS_R16]
+#else
+#define VCPU_EGPRS_PTR(vcpu)   NULL
+#endif
+
 /* EFER defaults:
  * - enable syscall per default because its emulated by KVM
  * - enable LME and LMA per default on 64 bit KVM
@@ -5804,10 +5810,33 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static void kvm_copy_vcpu_regs_to_uabi(struct kvm_vcpu *vcpu, struct kvm_xsave *uabi_xsave)
+{
+	union fpregs_state *xstate = (union fpregs_state *)uabi_xsave->region;
+	void *uabi_apx = (void*)uabi_xsave->region + xstate_offset(XFEATURE_APX);
+	void *vcpu_egprs = VCPU_EGPRS_PTR(vcpu);
+
+	if (!vcpu_egprs)
+		return;
 
-static int kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
-					 u8 *state, unsigned int size)
+	memcpy(uabi_apx, vcpu_egprs, xstate_size(XFEATURE_APX));
+	xstate->xsave.header.xfeatures |= XFEATURE_MASK_APX;
+}
+
+static void kvm_copy_uabi_to_vcpu_regs(struct kvm_vcpu *vcpu, struct kvm_xsave *uabi_xsave)
 {
+	union fpregs_state *xstate = (union fpregs_state *)uabi_xsave->region;
+	void *uabi_apx = (void*)uabi_xsave->region + xstate_offset(XFEATURE_APX);
+	void *vcpu_egprs = VCPU_EGPRS_PTR(vcpu);
+
+	if (vcpu_egprs && xstate->xsave.header.xfeatures & XFEATURE_MASK_APX)
+		memcpy(vcpu_egprs, uabi_apx, xstate_size(XFEATURE_APX));
+}
+
+static int kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu, struct kvm_xsave *guest_xsave,
+					 unsigned int size)
+{
+
 	/*
 	 * Only copy state for features that are enabled for the guest.  The
 	 * state itself isn't problematic, but setting bits in the header for
@@ -5826,15 +5855,23 @@ static int kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
 	if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
 		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
 
-	fpu_copy_guest_fpstate_to_uabi(&vcpu->arch.guest_fpu, state, size,
+	/*
+	 * The generic XSAVE copy function zeros out areas not present in
+	 * guest fpstate. Those not in fpstate but in somewhere else,
+	 * like EGPRs, should be copied after this.
+	 */
+	fpu_copy_guest_fpstate_to_uabi(&vcpu->arch.guest_fpu, guest_xsave->region, size,
 				       supported_xcr0, vcpu->arch.pkru);
+
+	kvm_copy_vcpu_regs_to_uabi(vcpu, guest_xsave);
+
 	return 0;
 }
 
 static int kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
 					struct kvm_xsave *guest_xsave)
 {
-	return kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
+	return kvm_vcpu_ioctl_x86_get_xsave2(vcpu, guest_xsave,
 					     sizeof(guest_xsave->region));
 }
 
@@ -5853,6 +5890,8 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
 	 */
 	xstate->xsave.header.xfeatures &= ~vcpu->arch.guest_fpu.fpstate->xfd;
 
+	kvm_copy_uabi_to_vcpu_regs(vcpu, guest_xsave);
+
 	return fpu_copy_uabi_to_guest_fpstate(&vcpu->arch.guest_fpu,
 					      guest_xsave->region,
 					      kvm_caps.supported_xcr0,
@@ -6464,7 +6503,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		if (!u.xsave)
 			break;
 
-		r = kvm_vcpu_ioctl_x86_get_xsave2(vcpu, u.buffer, size);
+		r = kvm_vcpu_ioctl_x86_get_xsave2(vcpu, u.xsave, size);
 		if (r < 0)
 			break;
 
-- 
2.51.0