Re: [PATCH v13 01/21] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 29, 2025, Chao Gao wrote:
> On Thu, Aug 28, 2025 at 08:58:07PM +0800, Xiaoyao Li wrote:
> >On 8/21/2025 9:30 PM, Chao Gao wrote:
> >> From: Yang Weijiang <weijiang.yang@intel.com>
> >> 
> >> Enable KVM_{G,S}ET_ONE_REG uAPIs so that userspace can access HW MSR or
> >> KVM synthetic MSR through it.
> >> 
> >> In CET KVM series [1], KVM "steals" an MSR from PV MSR space and access
> >> it via KVM_{G,S}ET_MSRs uAPIs, but the approach pollutes PV MSR space
> >> and hides the difference of synthetic MSRs and normal HW defined MSRs.
> >> 
> >> Now carve out a separate room in KVM-customized MSR address space for
> >> synthetic MSRs. The synthetic MSRs are not exposed to userspace via
> >> KVM_GET_MSR_INDEX_LIST, instead userspace complies with KVM's setup and
> >> composes the uAPI params. KVM synthetic MSR indices start from 0 and
> >> increase linearly. Userspace caller should tag MSR type correctly in
> >> order to access intended HW or synthetic MSR.
> >
> >The old feedback[*]

Good sleuthing!  I completely forgot about that...

> was to introduce support for SYNTHETIC registers instead of limiting it to MSR.
> 
> GUEST_SSP is a real register but not an MSR, 

Eh, yes and no.  The vCPU's current SSP is a real register, but there's no
interface to access it.  The "synthetic" part here the interface that KVM is
defining.  Though I can see where that gets confusing since "synthetic" in KVM
usually means a completely made up register/MSR.

> so it's ok to treat it as a synthetic MSR. But, it's probably
> inappropriate/inaccurate to call it a synthetic register.
> 
> I think we need a clear definition for "synthetic register" to determine what
> should be included in this category. "synthetic MSR" is clear - it refers to
> something that isn't an MSR in hardware but is handled by KVM as an MSR.

Ah, but I specifically want to avoid bleeding those details into userspace.
I.e. I don't want userspace to know that internally, KVM handles GUEST_SSP via
the MSR infrastructure, so that if for whatever reason we want to do something
different (very unlikely), then we don't create a contradiction.

> That said, I'm still fine with renaming all "synthetic MSR" to "synthetic
> register" in this series. :)
> 
> Sean, which do you prefer with fresh eyes?
> 
> If we make this change, I suppose KVM_x86_REG_TYPE_SIZE() should be dropped, as
> we can't guarantee that the size will remain constant for the "synthetic
> register" type, since it could be extended to include registers with different
> sizes in the future.

Ooh, good catch.  We can keep it, we just need to be more conservative and only
define a valid size for GUEST_SSP.

To avoid confusion with "synthetic", what if we go with a more literal type of
"KVM"?  That's vague enough that it gives KVM a lot of flexibility, but specific
enough that it'll be intuitive for readers, hopefully.

---
 arch/x86/include/uapi/asm/kvm.h | 21 +++++++++++++--------
 arch/x86/kvm/x86.c              | 18 +++++++-----------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 478d9b63a9db..eeefc03120fc 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -412,28 +412,33 @@ struct kvm_xcrs {
 };
 
 #define KVM_X86_REG_TYPE_MSR		2
-#define KVM_X86_REG_TYPE_SYNTHETIC_MSR	3
+#define KVM_X86_REG_TYPE_KVM		3
 
-#define KVM_X86_REG_TYPE_SIZE(type)						\
+#define KVM_X86_KVM_REG_SIZE(reg)						\
+({										\
+	reg == KVM_REG_GUEST_SSP ? KVM_REG_SIZE_U64 : 0;			\
+})
+
+#define KVM_X86_REG_TYPE_SIZE(type, reg)					\
 ({										\
 	__u64 type_size = (__u64)type << 32;					\
 										\
 	type_size |= type == KVM_X86_REG_TYPE_MSR ? KVM_REG_SIZE_U64 :		\
-		     type == KVM_X86_REG_TYPE_SYNTHETIC_MSR ? KVM_REG_SIZE_U64 :\
+		     type == KVM_X86_REG_TYPE_KVM ? KVM_X86_KVM_REG_SIZE(reg) :	\
 		     0;								\
 	type_size;								\
 })
 
 #define KVM_X86_REG_ENCODE(type, index)				\
-	(KVM_REG_X86 | KVM_X86_REG_TYPE_SIZE(type) | index)
+	(KVM_REG_X86 | KVM_X86_REG_TYPE_SIZE(type, index) | index)
 
 #define KVM_X86_REG_MSR(index)					\
 	KVM_X86_REG_ENCODE(KVM_X86_REG_TYPE_MSR, index)
-#define KVM_X86_REG_SYNTHETIC_MSR(index)			\
-	KVM_X86_REG_ENCODE(KVM_X86_REG_TYPE_SYNTHETIC_MSR, index)
+#define KVM_X86_REG_KVM(index)			\
+	KVM_X86_REG_ENCODE(KVM_X86_REG_TYPE_KVM, index)
 
-/* KVM synthetic MSR index staring from 0 */
-#define KVM_SYNTHETIC_GUEST_SSP 0
+/* KVM-defined registers staring from 0 */
+#define KVM_REG_GUEST_SSP 0
 
 #define KVM_SYNC_X86_REGS      (1UL << 0)
 #define KVM_SYNC_X86_SREGS     (1UL << 1)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9930678f5a3b..e0d7298aea94 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6057,10 +6057,10 @@ struct kvm_x86_reg_id {
 	__u8  x86;
 };
 
-static int kvm_translate_synthetic_msr(struct kvm_x86_reg_id *reg)
+static int kvm_translate_kvm_reg(struct kvm_x86_reg_id *reg)
 {
 	switch (reg->index) {
-	case KVM_SYNTHETIC_GUEST_SSP:
+	case KVM_REG_GUEST_SSP:
 		reg->type = KVM_X86_REG_TYPE_MSR;
 		reg->index = MSR_KVM_INTERNAL_GUEST_SSP;
 		break;
@@ -6204,15 +6204,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		if (id->rsvd || id->rsvd4)
 			break;
 
-		if (id->type != KVM_X86_REG_TYPE_MSR &&
-		    id->type != KVM_X86_REG_TYPE_SYNTHETIC_MSR)
-			break;
-
-		if ((reg.id & KVM_REG_SIZE_MASK) != KVM_REG_SIZE_U64)
-			break;
-
-		if (id->type == KVM_X86_REG_TYPE_SYNTHETIC_MSR) {
-			r = kvm_translate_synthetic_msr(id);
+		if (id->type == KVM_X86_REG_TYPE_KVM) {
+			r = kvm_translate_kvm_reg(id);
 			if (r)
 				break;
 		}
@@ -6221,6 +6214,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		if (id->type != KVM_X86_REG_TYPE_MSR)
 			break;
 
+		if ((reg.id & KVM_REG_SIZE_MASK) != KVM_REG_SIZE_U64)
+			break;
+
 		value = u64_to_user_ptr(reg.addr);
 		if (ioctl == KVM_GET_ONE_REG)
 			r = kvm_get_one_msr(vcpu, id->index, value);

base-commit: eb92644efe58ecb8d00a83ef1788871404116001
--