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@xxxxxxxxx/
> 
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>

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