Re: [PATCH v10 17/27] KVM: x86: Report KVM supported CET MSRs as to-be-saved

[Sean Christopherson <seanjc@google.com>]

On Mon, May 06, 2024, Weijiang Yang wrote:
> On 5/2/2024 6:40 AM, Sean Christopherson wrote:
> > On Sun, Feb 18, 2024, Yang Weijiang wrote:
> > > Add CET MSRs to the list of MSRs reported to userspace if the feature,
> > > i.e. IBT or SHSTK, associated with the MSRs is supported by KVM.
> > > 
> > > SSP can only be read via RDSSP. Writing even requires destructive and
> > > potentially faulting operations such as SAVEPREVSSP/RSTORSSP or
> > > SETSSBSY/CLRSSBSY. Let the host use a pseudo-MSR that is just a wrapper
> > > for the GUEST_SSP field of the VMCS.
> > > 
> > > Suggested-by: Chao Gao <chao.gao@intel.com>
> > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > > ---
> > >   arch/x86/include/uapi/asm/kvm_para.h |  1 +
> > >   arch/x86/kvm/vmx/vmx.c               |  2 ++
> > >   arch/x86/kvm/x86.c                   | 18 ++++++++++++++++++
> > >   3 files changed, 21 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > index 605899594ebb..9d08c0bec477 100644
> > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > @@ -58,6 +58,7 @@
> > >   #define MSR_KVM_ASYNC_PF_INT	0x4b564d06
> > >   #define MSR_KVM_ASYNC_PF_ACK	0x4b564d07
> > >   #define MSR_KVM_MIGRATION_CONTROL	0x4b564d08
> > > +#define MSR_KVM_SSP	0x4b564d09
> > We never resolved the conservation from v6[*], but I still agree with Maxim's
> > view that defining a synthetic MSR, which "steals" an MSR from KVM's MSR address
> > space, is a bad idea.
> > 
> > And I still also think that KVM_SET_ONE_REG is the best way forward.  Completely
> > untested, but I think this is all that is needed to wire up KVM_{G,S}ET_ONE_REG
> > to support MSRs, and carve out room for 250+ other register types, plus room for
> > more future stuff as needed.
> 
> Got your point now.
> 
> > 
> > We'll still need a KVM-defined MSR for SSP, but it can be KVM internal, not uAPI,
> > e.g. the "index" exposed to userspace can simply be '0' for a register type of
> > KVM_X86_REG_SYNTHETIC_MSR, and then the translated internal index can be any
> > value that doesn't conflict.
> 
> Let me try to understand it, for your reference code below, id.type is to separate normal
> MSR (HW defined) namespace and synthetic MSR namespace, right?

Yep.

> For the latter, IIUC KVM still needs to expose the index within the synthetic
> namespace so that userspace can read/write the intended MSRs, of course not
> expose the synthetic MSR index via existing uAPI,  But you said the "index"
> exposed to userspace can simply  be '0' in this case, then how to distinguish
> the synthetic MSRs in userspace and KVM? And how userspace can be aware of
> the synthetic MSR index allocation in KVM?

The idea is to have a synthetic index that is exposed to userspace, and a separate
KVM-internal index for emulating accesses.  The value that is exposed to userspace
can start at 0 and be a simple incrementing value as we add synthetic MSRs, as the
.type == SYNTHETIC makes it impossible for the value to collide with a "real" MSR.

Translating to a KVM-internal index is a hack to avoid having to plumb a 64-bit
index into all the MSR code.  We could do that, i.e. pass the full kvm_x86_reg_id
into the MSR helpers, but I'm not convinced it'd be worth the churn.  That said,
I'm not opposed to the idea either, if others prefer that approach.

E.g.

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 738c449e4f9e..21152796238a 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -420,6 +420,8 @@ struct kvm_x86_reg_id {
        __u16 rsvd16;
 };
 
+#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 f45cdd9d8c1f..1a9e1e0c9f49 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5990,6 +5990,19 @@ 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;
+}
+
 long kvm_arch_vcpu_ioctl(struct file *filp,
                         unsigned int ioctl, unsigned long arg)
 {
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index cc585051d24b..3b5a038f5260 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -49,6 +49,15 @@ void kvm_spurious_fault(void);
 #define KVM_FIRST_EMULATED_VMX_MSR     MSR_IA32_VMX_BASIC
 #define KVM_LAST_EMULATED_VMX_MSR      MSR_IA32_VMX_VMFUNC
 
+/*
+ * 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
+
 #define KVM_DEFAULT_PLE_GAP            128
 #define KVM_VMX_DEFAULT_PLE_WINDOW     4096
 #define KVM_DEFAULT_PLE_WINDOW_GROW    2