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

From: "Yang, Weijiang" <weijiang.yang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: <pbonzini@redhat.com>, <dave.hansen@intel.com>, <x86@kernel.org>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<peterz@infradead.org>, <chao.gao@intel.com>,
	<rick.p.edgecombe@intel.com>, <mlevitsk@redhat.com>,
	<john.allen@amd.com>
Subject: Re: [PATCH v10 17/27] KVM: x86: Report KVM supported CET MSRs as to-be-saved
Date: Wed, 8 May 2024 15:00:12 +0800	[thread overview]
Message-ID: <5a8c31bd-0fe3-48f2-8ec3-b143fe289827@intel.com> (raw)
In-Reply-To: <Zjpkl0U23qEOO3DY@google.com>

On 5/8/2024 1:27 AM, Sean Christopherson wrote:
> 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:

[...]

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

OK, I'll post an RFC patch for this change, thanks a lot!