Re: [PATCH v12 06/24] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Chao Gao <chao.gao@xxxxxxxxx>]

>> +#define KVM_X86_REG_MSR			(1 << 2)
>> +#define KVM_X86_REG_SYNTHETIC		(1 << 3)
>> +
>> +struct kvm_x86_reg_id {
>> +	__u32 index;
>> +	__u8 type;
>> +	__u8 rsvd;
>> +	__u16 rsvd16;
>> +};
>
>Some feedback from a while back never got addressed[*].  That feedback still
>looks sane/good, so this for the uAPI:

I missed that comment. Below is the diff I end up with. I moved struct
kvm_x86_reg_id to x86.c and added checks for ARCH (i.e., x86) and size.

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index e72d9e6c1739..bb17b7a85159 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -411,15 +411,23 @@ struct kvm_xcrs {
	__u64 padding[16];
 };
 
-#define KVM_X86_REG_MSR			(1 << 2)
-#define KVM_X86_REG_SYNTHETIC		(1 << 3)
-
-struct kvm_x86_reg_id {
-	__u32 index;
-	__u8 type;
-	__u8 rsvd;
-	__u16 rsvd16;
-};
+#define KVM_X86_REG_TYPE_MSR		2
+#define KVM_X86_REG_TYPE_SYNTHETIC_MSR	3
+
+#define KVM_x86_REG_TYPE_SIZE(type)						\
+{(										\
+	__u64 type_size = type;							\
+										\
+	type_size |= type == KVM_X86_REG_TYPE_MSR ? KVM_REG_SIZE_U64 :		\
+		     type == KVM_X86_REG_TYPE_SYNTHETIC_MSR ? KVM_REG_SIZE_U64 :\
+		     0;								\
+	type_size;								\
+})
+
+#define KVM_X86_REG_ENCODE(type, index)				\
+	(KVM_REG_X86 | KVM_X86_REG_TYPE_SIZE(type) | index)
+
+#define KVM_X86_REG_MSR(index) KVM_X86_REG_ENCODE(KVM_X86_REG_TYPE_MSR, index)
 
 #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 3bf098a1183a..28e33269c1e9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5940,6 +5940,15 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
	}
 }
 
+struct kvm_x86_reg_id {
+	__u32 index;
+	__u8  type;
+	__u8  rsvd;
+	__u8  rsvd4:4;
+	__u8  size:4;
+	__u8  x86;
+};
+
 static int kvm_translate_synthetic_msr(struct kvm_x86_reg_id *reg)
 {
	return -EINVAL;
@@ -6072,22 +6081,28 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
			break;
 
		r = -EINVAL;
+		if ((reg.id & KVM_REG_ARCH_MASK) != KVM_REG_X86)
+			break;
+
		id = (struct kvm_x86_reg_id *)&reg.id;
-		if (id->rsvd || id->rsvd16)
+		if (id->rsvd || id->rsvd4)
+			break;
+
+		if (id->type != KVM_X86_REG_TYPE_MSR &&
+		    id->type != KVM_X86_REG_TYPE_SYNTHETIC_MSR)
			break;
 
-		if (id->type != KVM_X86_REG_MSR &&
-		    id->type != KVM_X86_REG_SYNTHETIC)
+		if ((reg.id & KVM_REG_SIZE_MASK) != KVM_REG_SIZE_U64)
			break;
 
-		if (id->type == KVM_X86_REG_SYNTHETIC) {
+		if (id->type == KVM_X86_REG_TYPE_SYNTHETIC_MSR) {
			r = kvm_translate_synthetic_msr(id);
			if (r)
				break;
		}
 
		r = -EINVAL;
-		if (id->type != KVM_X86_REG_MSR)
+		if (id->type != KVM_X86_REG_TYPE_MSR)
			break;
 
		value = u64_to_user_ptr(reg.addr);


>
>--
>#define KVM_X86_REG_TYPE_MSR	2ull
>
>#define KVM_x86_REG_TYPE_SIZE(type) 						\
>{(										\
>	__u64 type_size = type;							\
>										\
>	type_size |= type == KVM_X86_REG_TYPE_MSR ? KVM_REG_SIZE_U64 :		\
>		     type == KVM_X86_REG_TYPE_SYNTHETIC_MSR ? KVM_REG_SIZE_U64 :\
>		     0;								\
>	type_size;								\
>})
>
>#define KVM_X86_REG_ENCODE(type, index)				\
>	(KVM_REG_X86 | KVM_X86_REG_TYPE_SIZE(type) | index)
>
>#define KVM_X86_REG_MSR(index) KVM_X86_REG_ENCODE(KVM_X86_REG_TYPE_MSR, index)
>--
>
>And then the kernel-only struct overlay becomes:
>
>--
>struct kvm_x86_reg_id {
>	__u32 index;
>	__u8  type;
>	__u8  rsvd;
>	__u8  rsvd4:4;
>	__u8  size:4;
>	__u8  x86;
>}
>--
>
>[*] https://lore.kernel.org/all/ZuGpJtEPv1NtdYwM@xxxxxxxxxx