[RFC PATCH 1/2] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[RFC PATCH 1/2] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Yang Weijiang]

Enable KVM_{G,S}ET_ONE_REG uAPIs so that userspace can access HW MSR or
KVM synthetic MSR throught it.

In CET KVM series [*], 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.

[*]:
https://lore.kernel.org/all/20240219074733.122080-18-weijiang.yang@intel.com/

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/include/uapi/asm/kvm.h | 10 ++++++
 arch/x86/kvm/x86.c              | 62 +++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index ef11aa4cab42..ca2a47a85fa1 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -410,6 +410,16 @@ struct kvm_xcrs {
 	__u64 padding[16];
 };
 
+#define KVM_X86_REG_MSR			(1 << 2)
+#define KVM_X86_REG_SYNTHETIC_MSR	(1 << 3)
+
+struct kvm_x86_reg_id {
+	__u32 index;
+	__u8 type;
+	__u8 rsvd;
+	__u16 rsvd16;
+};
+
 #define KVM_SYNC_X86_REGS      (1UL << 0)
 #define KVM_SYNC_X86_SREGS     (1UL << 1)
 #define KVM_SYNC_X86_EVENTS    (1UL << 2)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 91478b769af0..d0054c52f24b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2244,6 +2244,31 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 	return kvm_set_msr_ignored_check(vcpu, index, *data, true);
 }
 
+static int kvm_get_one_msr(struct kvm_vcpu *vcpu, u32 msr, u64 __user *value)
+{
+	u64 val;
+	int r;
+
+	r = do_get_msr(vcpu, msr, &val);
+	if (r)
+		return r;
+
+	if (put_user(val, value))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int kvm_set_one_msr(struct kvm_vcpu *vcpu, u32 msr, u64 __user *value)
+{
+	u64 val;
+
+	if (get_user(val, value))
+		return -EFAULT;
+
+	return do_set_msr(vcpu, msr, &val);
+}
+
 #ifdef CONFIG_X86_64
 struct pvclock_clock {
 	int vclock_mode;
@@ -5859,6 +5884,11 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 	}
 }
 
+static int kvm_translate_synthetic_msr(u32 *index)
+{
+	return 0;
+}
+
 long kvm_arch_vcpu_ioctl(struct file *filp,
 			 unsigned int ioctl, unsigned long arg)
 {
@@ -5976,6 +6006,38 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		srcu_read_unlock(&vcpu->kvm->srcu, idx);
 		break;
 	}
+	case KVM_GET_ONE_REG:
+	case KVM_SET_ONE_REG: {
+		struct kvm_x86_reg_id *id;
+		struct kvm_one_reg reg;
+		u64 __user *value;
+
+		r = -EFAULT;
+		if (copy_from_user(&reg, argp, sizeof(reg)))
+			break;
+
+		r = -EINVAL;
+		id = (struct kvm_x86_reg_id *)&reg.id;
+		if (id->rsvd || id->rsvd16)
+			break;
+
+		if (id->type != KVM_X86_REG_MSR &&
+		    id->type != KVM_X86_REG_SYNTHETIC_MSR)
+			break;
+
+		if (id->type == KVM_X86_REG_SYNTHETIC_MSR) {
+			r = kvm_translate_synthetic_msr(&id->index);
+			if (r)
+				break;
+		}
+
+		value = u64_to_user_ptr(reg.addr);
+		if (ioctl == KVM_GET_ONE_REG)
+			r = kvm_get_one_msr(vcpu, id->index, value);
+		else
+			r = kvm_set_one_msr(vcpu, id->index, value);
+		break;
+	}
 	case KVM_TPR_ACCESS_REPORTING: {
 		struct kvm_tpr_access_ctl tac;
 
-- 
2.43.0

[RFC PATCH 2/2] KVM: x86: Enable guest SSP read/write interface with new uAPIs

[Yang Weijiang]

Enable guest shadow stack pointer(SSP) access interface with new uAPIs.
CET guest SSP is HW register which has corresponding VMCS field to save
/restore guest values when VM-{Exit,Entry} happens. KVM handles SSP as
a synthetic MSR for userspace access.

Use a translation helper to set up mapping for SSP synthetic index and
KVM-internal MSR index so that userspace doesn't need to take care of
KVM's management for synthetic MSRs and avoid conflicts.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/include/uapi/asm/kvm.h |  3 +++
 arch/x86/kvm/x86.c              |  7 +++++++
 arch/x86/kvm/x86.h              | 10 ++++++++++
 3 files changed, 20 insertions(+)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index ca2a47a85fa1..81c8d9ea2e58 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -420,6 +420,9 @@ struct kvm_x86_reg_id {
 	__u16 rsvd16;
 };
 
+/* KVM synthetic MSR index staring from 0 */
+#define MSR_KVM_GUEST_SSP	0
+
 #define KVM_SYNC_X86_REGS      (1UL << 0)
 #define KVM_SYNC_X86_SREGS     (1UL << 1)
 #define KVM_SYNC_X86_EVENTS    (1UL << 2)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d0054c52f24b..a970bd26ce2c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5886,6 +5886,13 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 
 static int kvm_translate_synthetic_msr(u32 *index)
 {
+	switch (*index) {
+	case MSR_KVM_GUEST_SSP:
+		*index = MSR_KVM_INTERNAL_GUEST_SSP;
+		break;
+	default:
+		return -EINVAL;
+	}
 	return 0;
 }
 
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index a8b71803777b..6ac86a75aedc 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -57,6 +57,16 @@ void kvm_spurious_fault(void);
 #define KVM_SVM_DEFAULT_PLE_WINDOW_MAX	USHRT_MAX
 #define KVM_SVM_DEFAULT_PLE_WINDOW	3000
 
+/*
+ * KVM's internal, non-ABI indices for synthetic MSRs. The values themselves
+ * are arbitrary and have no meaning, the only requirement is that they don't
+ * conflict with "real" MSRs that KVM supports. Use values at the uppper end
+ * of KVM's reserved paravirtual MSR range to minimize churn, i.e. these values
+ * will be usable until KVM exhausts its supply of paravirtual MSR indices.
+ */
+
+#define MSR_KVM_INTERNAL_GUEST_SSP	0x4b564dff
+
 static inline unsigned int __grow_ple_window(unsigned int val,
 		unsigned int base, unsigned int modifier, unsigned int max)
 {
-- 
2.43.0

Re: [RFC PATCH 2/2] KVM: x86: Enable guest SSP read/write interface with new uAPIs

[Sean Christopherson]

On Thu, May 09, 2024, Yang Weijiang wrote:
> Enable guest shadow stack pointer(SSP) access interface with new uAPIs.
> CET guest SSP is HW register which has corresponding VMCS field to save
> /restore guest values when VM-{Exit,Entry} happens. KVM handles SSP as
> a synthetic MSR for userspace access.
> 
> Use a translation helper to set up mapping for SSP synthetic index and
> KVM-internal MSR index so that userspace doesn't need to take care of
> KVM's management for synthetic MSRs and avoid conflicts.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/include/uapi/asm/kvm.h |  3 +++
>  arch/x86/kvm/x86.c              |  7 +++++++
>  arch/x86/kvm/x86.h              | 10 ++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index ca2a47a85fa1..81c8d9ea2e58 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -420,6 +420,9 @@ struct kvm_x86_reg_id {
>  	__u16 rsvd16;
>  };
>  
> +/* KVM synthetic MSR index staring from 0 */
> +#define MSR_KVM_GUEST_SSP	0

Do we want to have "SYNTHETIC" in the name?  E.g. to try and differentiate from
KVM's paravirtual MSRs?

Hmm, but the PV MSRs are synthetic too.  Maybe it's the MSR part that's bad, e.g.
the whole point of these shenanigans is to let KVM use its internal MSR framework
without exposing those details to userspace.

So rather than, KVM_X86_REG_SYNTHETIC_MSR, what if we go with KVM_X86_REG_SYNTHETIC?
And then this becomes something like KVM_SYNTHETIC_GUEST_SSP?

Aha!  And then to prepare for a future where we add synthetic registers that
aren't routed through the MSR framework (which seems unlikely, but its trivially
easy to handle, so why not):

static int kvm_translate_synthetic_reg(struct kvm_x86_reg_id *reg)
{
	switch (reg->index) {
	case MSR_KVM_GUEST_SSP:
		reg->type = KVM_X86_REG_MSR;
		reg->index = MSR_KVM_INTERNAL_GUEST_SSP;
		break;
	default:
		return -EINVAL;
	}
	return 0;
}

and then the caller would have slightly different ordering:

        if (id->type == KVM_X86_REG_SYNTHETIC_MSR) {
                r = kvm_translate_synthetic_msr(&id->index);
                if (r)
                        break;
        }

        r = -EINVAL;
        if (id->type != KVM_X86_REG_MSR)
                break;

Re: [RFC PATCH 2/2] KVM: x86: Enable guest SSP read/write interface with new uAPIs

[Yang, Weijiang]

On 6/11/2024 9:17 AM, Sean Christopherson wrote:
> On Thu, May 09, 2024, Yang Weijiang wrote:
>> Enable guest shadow stack pointer(SSP) access interface with new uAPIs.
>> CET guest SSP is HW register which has corresponding VMCS field to save
>> /restore guest values when VM-{Exit,Entry} happens. KVM handles SSP as
>> a synthetic MSR for userspace access.
>>
>> Use a translation helper to set up mapping for SSP synthetic index and
>> KVM-internal MSR index so that userspace doesn't need to take care of
>> KVM's management for synthetic MSRs and avoid conflicts.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>   arch/x86/include/uapi/asm/kvm.h |  3 +++
>>   arch/x86/kvm/x86.c              |  7 +++++++
>>   arch/x86/kvm/x86.h              | 10 ++++++++++
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
>> index ca2a47a85fa1..81c8d9ea2e58 100644
>> --- a/arch/x86/include/uapi/asm/kvm.h
>> +++ b/arch/x86/include/uapi/asm/kvm.h
>> @@ -420,6 +420,9 @@ struct kvm_x86_reg_id {
>>   	__u16 rsvd16;
>>   };
>>   
>> +/* KVM synthetic MSR index staring from 0 */
>> +#define MSR_KVM_GUEST_SSP	0
> Do we want to have "SYNTHETIC" in the name?  E.g. to try and differentiate from
> KVM's paravirtual MSRs?
>
> Hmm, but the PV MSRs are synthetic too.  Maybe it's the MSR part that's bad, e.g.
> the whole point of these shenanigans is to let KVM use its internal MSR framework
> without exposing those details to userspace.
>
> So rather than, KVM_X86_REG_SYNTHETIC_MSR, what if we go with KVM_X86_REG_SYNTHETIC?
> And then this becomes something like KVM_SYNTHETIC_GUEST_SSP?

Yes, makes sense for me.

>
> Aha!  And then to prepare for a future where we add synthetic registers that
> aren't routed through the MSR framework (which seems unlikely, but its trivially
> easy to handle, so why not):
>
> static int kvm_translate_synthetic_reg(struct kvm_x86_reg_id *reg)
> {
> 	switch (reg->index) {
> 	case MSR_KVM_GUEST_SSP:
> 		reg->type = KVM_X86_REG_MSR;
> 		reg->index = MSR_KVM_INTERNAL_GUEST_SSP;
> 		break;
> 	default:
> 		return -EINVAL;
> 	}
> 	return 0;
> }
>
> and then the caller would have slightly different ordering:
>
>          if (id->type == KVM_X86_REG_SYNTHETIC_MSR) {
>                  r = kvm_translate_synthetic_msr(&id->index);
>                  if (r)
>                          break;
>          }
>
>          r = -EINVAL;
>          if (id->type != KVM_X86_REG_MSR)
>                  break;
I assume reg->type translation for GUEST_SSP is due to the fact it relies on CET common checking
stuffs underneath for the register, i.e., it goes through existing MSR framework. But for future other
synthetic MSRs, it needs to refactor the code here so that it could be routed into new handling.
e.g.:

if (id->type == KVM_X86_REG_MSR)
         go through MSR framework;
else
         go through other new handling;

But currently the new uAPIs are only for GUEST_SSP, so above suggested id->type check works.
Does it make sense?

Re: [RFC PATCH 2/2] KVM: x86: Enable guest SSP read/write interface with new uAPIs

[Sean Christopherson]

On Tue, Jun 11, 2024, Weijiang Yang wrote:
> On 6/11/2024 9:17 AM, Sean Christopherson wrote:
> > On Thu, May 09, 2024, Yang Weijiang wrote:
> > Aha!  And then to prepare for a future where we add synthetic registers that
> > aren't routed through the MSR framework (which seems unlikely, but its trivially
> > easy to handle, so why not):
> > 
> > static int kvm_translate_synthetic_reg(struct kvm_x86_reg_id *reg)
> > {
> > 	switch (reg->index) {
> > 	case MSR_KVM_GUEST_SSP:
> > 		reg->type = KVM_X86_REG_MSR;
> > 		reg->index = MSR_KVM_INTERNAL_GUEST_SSP;
> > 		break;
> > 	default:
> > 		return -EINVAL;
> > 	}
> > 	return 0;
> > }
> > 
> > and then the caller would have slightly different ordering:
> > 
> >          if (id->type == KVM_X86_REG_SYNTHETIC_MSR) {
> >                  r = kvm_translate_synthetic_msr(&id->index);
> >                  if (r)
> >                          break;
> >          }
> > 
> >          r = -EINVAL;
> >          if (id->type != KVM_X86_REG_MSR)
> >                  break;
> I assume reg->type translation for GUEST_SSP is due to the fact it relies on
> CET common checking stuffs underneath for the register, i.e., it goes through
> existing MSR framework. But for future other synthetic MSRs, it needs to

Nit, other synthetic *registers*.

> refactor the code here so that it could be routed into new handling.  e.g.:
> 
> if (id->type == KVM_X86_REG_MSR)
>         go through MSR framework;
> else
>         go through other new handling;
> 
> But currently the new uAPIs are only for GUEST_SSP, so above suggested
> id->type check works.  Does it make sense?

Yep, we're on the same page.

Re: [RFC PATCH 1/2] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Sean Christopherson]

On Thu, May 09, 2024, Yang Weijiang wrote:
> @@ -5859,6 +5884,11 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +static int kvm_translate_synthetic_msr(u32 *index)
> +{
> +	return 0;

This needs to be -EINVAL.

> +}
> +
>  long kvm_arch_vcpu_ioctl(struct file *filp,
>  			 unsigned int ioctl, unsigned long arg)
>  {

Re: [RFC PATCH 1/2] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Yang, Weijiang]

On 6/11/2024 9:04 AM, Sean Christopherson wrote:
> On Thu, May 09, 2024, Yang Weijiang wrote:
>> @@ -5859,6 +5884,11 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>>   	}
>>   }
>>   
>> +static int kvm_translate_synthetic_msr(u32 *index)
>> +{
>> +	return 0;
> This needs to be -EINVAL.

OK, I'll change it, thanks!

Re: [RFC PATCH 1/2] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Nikolas Wipper]

On Thu May  9, 2024 at 09:54 AM UTC+0200, Yang Weijiang wrote:
> Enable KVM_{G,S}ET_ONE_REG uAPIs so that userspace can access HW MSR or
> KVM synthetic MSR throught it.
> 
> In CET KVM series [*], 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.
> 
> [*]:
> https://lore.kernel.org/all/20240219074733.122080-18-weijiang.yang@intel.com/
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>

Having this API, and specifically having a definite kvm_one_reg structure 
for x86 registers, would be interesting for register pinning/intercepts.
With one_reg for x86 the API could be platform agnostic and possible even
replace MSR filters for x86. I do have a couple of questions about these
patches.

> ---
>  arch/x86/include/uapi/asm/kvm.h | 10 ++++++
>  arch/x86/kvm/x86.c              | 62 +++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index ef11aa4cab42..ca2a47a85fa1 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -410,6 +410,16 @@ struct kvm_xcrs {
>  	__u64 padding[16];
>  };
>  
> +#define KVM_X86_REG_MSR			(1 << 2)
> +#define KVM_X86_REG_SYNTHETIC_MSR	(1 << 3)

Why is this a bitfield? As opposed to just counting up?

#define KVM_X86_REG_MSR			2
#define KVM_X86_REG_SYNTHETIC_MSR	3

> +
> +struct kvm_x86_reg_id {
> +	__u32 index;
> +	__u8 type;
> +	__u8 rsvd;
> +	__u16 rsvd16;
> +};

This struct is opposite to what other architectures do, where they have
an architecture ID in the upper 32 bits, and the lower 32 bits actually
identify the register. This would probably make sense for x86 too, to
avoid conflicts with other IDs (I think MIPS core registers can have IDs
with the lower 32 bits all zero) so that the IDs are actually unique,
right?

Best,
Nikolas



Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

Re: [RFC PATCH 1/2] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

[Sean Christopherson]

On Wed, Sep 11, 2024, Nikolas Wipper wrote:
> On Thu May  9, 2024 at 09:54 AM UTC+0200, Yang Weijiang wrote:
> > Enable KVM_{G,S}ET_ONE_REG uAPIs so that userspace can access HW MSR or
> > KVM synthetic MSR throught it.
> > 
> > In CET KVM series [*], 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.
> > 
> > [*]:
> > https://lore.kernel.org/all/20240219074733.122080-18-weijiang.yang@intel.com/
> > 
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> 
> Having this API, and specifically having a definite kvm_one_reg structure 
> for x86 registers, would be interesting for register pinning/intercepts.
> With one_reg for x86 the API could be platform agnostic and possible even
> replace MSR filters for x86.

I don't follow.  MSR filters let userspace intercept accesses for a variety of
reasons, these APIs simply provide a way to read/write a register value that is
stored in KVM.  I don't see how this could replace MSR filters.  

> I do have a couple of questions about these patches.
> 
> > ---
> >  arch/x86/include/uapi/asm/kvm.h | 10 ++++++
> >  arch/x86/kvm/x86.c              | 62 +++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index ef11aa4cab42..ca2a47a85fa1 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -410,6 +410,16 @@ struct kvm_xcrs {
> >  	__u64 padding[16];
> >  };
> >  
> > +#define KVM_X86_REG_MSR			(1 << 2)
> > +#define KVM_X86_REG_SYNTHETIC_MSR	(1 << 3)
> 
> Why is this a bitfield? As opposed to just counting up?

Hmm, good question.  This came from my initial sketch, and it would seem that I
something specific in mind since starting at (1 << 2) is oddly specific, but for
the life of me I can't remember what the plan was.  Best guest is that I was
leaving space for '0' and '1' to be regs and sregs?  But that still doesn't
explain/justify using a bitfield.

[*] https://lore.kernel.org/all/ZjLE7giCsEI4Sftp@google.com

> 
> #define KVM_X86_REG_MSR			2
> #define KVM_X86_REG_SYNTHETIC_MSR	3
> 
> > +
> > +struct kvm_x86_reg_id {
> > +	__u32 index;
> > +	__u8 type;
> > +	__u8 rsvd;
> > +	__u16 rsvd16;
> > +};
> 
> This struct is opposite to what other architectures do, where they have
> an architecture ID in the upper 32 bits, and the lower 32 bits actually
> identify the register. This would probably make sense for x86 too, to
> avoid conflicts with other IDs (I think MIPS core registers can have IDs
> with the lower 32 bits all zero) so that the IDs are actually unique,
> right?

It's not the opposite, it's just missing fields for the arch and the size.  Ugh,
the size is unaligned.  That's annoying.  Something like this?

struct kvm_x86_reg_id {
	__u32 index;
	__u8  type;
	__u8  rsvd;
	__u8  rsvd4:4;
	__u8  size:4;
	__u8  x86;
}

Though looking at this with fresh eyes, I don't think the above structure should
be exposed to userspace.  Userspace will only ever want to encode a register; the
exact register may not be hardcoded, but I would expect the type to always be
known ahead of time, if not outright hardcoded.  The struct is really only useful
for the kernel, e.g. to easily switch on the type, extract the index, etc.

As annoying as it can be for a human to decipher the final value, the arm64/riscv
approach of providing builders is probably the way to go, though I think x86 can
be much simpler (less stuff to encode).

Oh!  Another thing I think we should do is make KVM_{G,S}ET_ONE_REG 64-bit only
so that we don't have to deal with 32-bit vs. 64-bit GPRs.  32-bit userspace
would need to manually encode the register id, but I have no problem making life
difficult for such setups.  Or KVM could reject the ioctl for .compat_ioctl(),
but that seems unnecessary.

E.g. since IIUC switch() and if() statements are off-limits in uapi headers...

#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)

Re: [RFC PATCH 1/2] KVM: x86: Introduce KVM_{G, S}ET_ONE_REG uAPIs support

[Nikolas Wipper]

On Wed Sep 11, 2024 at 04:36 PM UTC+0200, Sean Christopherson wrote:
> On Wed, Sep 11, 2024, Nikolas Wipper wrote:
>> Having this API, and specifically having a definite kvm_one_reg structure
>> for x86 registers, would be interesting for register pinning/intercepts.
>> With one_reg for x86 the API could be platform agnostic and possible even
>> replace MSR filters for x86.
> 
> I don't follow.  MSR filters let userspace intercept accesses for a variety of
> reasons, these APIs simply provide a way to read/write a register value that is
> stored in KVM.  I don't see how this could replace MSR filters.

Nope, that would be an entirely different API, but if that uses one reg IDs it
could be unified to cover CRs and MSRs all in one.




Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

Re: [RFC PATCH 1/2] KVM: x86: Introduce KVM_{G, S}ET_ONE_REG uAPIs support

[Sean Christopherson]

On Wed, Sep 11, 2024, Nikolas Wipper wrote:
> On Wed Sep 11, 2024 at 04:36 PM UTC+0200, Sean Christopherson wrote:
> > On Wed, Sep 11, 2024, Nikolas Wipper wrote:
> >> Having this API, and specifically having a definite kvm_one_reg structure
> >> for x86 registers, would be interesting for register pinning/intercepts.
> >> With one_reg for x86 the API could be platform agnostic and possible even
> >> replace MSR filters for x86.
> > 
> > I don't follow.  MSR filters let userspace intercept accesses for a variety of
> > reasons, these APIs simply provide a way to read/write a register value that is
> > stored in KVM.  I don't see how this could replace MSR filters.
> 
> Nope, that would be an entirely different API, but if that uses one reg IDs it
> could be unified to cover CRs and MSRs all in one.

Oooh, gotcha.  Yeah, uniquely identifiable registers would allow for a generic
filtering API, though I'm not entirely sure that's actually a good idea in the
long run.  Most x86 registers can't be intercepted; having a generic filtering
API might incur an annoyingly high maintenance cost.  Hmm, though it should be
easy enough to explicitly allow only MSR and CR types, so if/when we get to the
point where CR pinning/filtering is desirable/ready, then a unified API probably
does make sense.