Re: [PATCH v2 07/11] KVM: x86: define standard behavior for bits 0/1 of VM type

[Paolo Bonzini]

On Fri, Feb 23, 2024 at 5:46 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Fri, Feb 23, 2024, Paolo Bonzini wrote:
> > Some VM types have characteristics in common; in fact, the only use
> > of VM types right now is kvm_arch_has_private_mem and it assumes that
> > _all_ VM types have private memory.
> >
> > So, let the low bits specify the characteristics of the VM type.  As
> > of we have two special things: whether memory is private, and whether
> > guest state is protected.  The latter is similar to
> > kvm->arch.guest_state_protected, but the latter is only set on a fully
> > initialized VM.  If both are set, ioctls to set registers will cause
> > an error---SEV-ES did not do so, which is a problematic API.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Message-Id: <20240209183743.22030-7-pbonzini@xxxxxxxxxx>
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  9 +++-
> >  arch/x86/kvm/x86.c              | 93 +++++++++++++++++++++++++++------
> >  2 files changed, 85 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 0bcd9ae16097..15db2697863c 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2135,8 +2135,15 @@ void kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd);
> >  void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
> >                      int tdp_max_root_level, int tdp_huge_page_level);
> >
> > +
> > +/* Low bits of VM types provide confidential computing capabilities.  */
> > +#define __KVM_X86_PRIVATE_MEM_TYPE   1
> > +#define __KVM_X86_PROTECTED_STATE_TYPE       2
> > +#define __KVM_X86_VM_TYPE_FEATURES   3
> > +static_assert((KVM_X86_SW_PROTECTED_VM & __KVM_X86_VM_TYPE_FEATURES) == __KVM_X86_PRIVATE_MEM_TYPE);
>
> Aliasing bit 0 to KVM_X86_SW_PROTECTED_VM is gross, e.g. this
>
>  #define KVM_X86_DEFAULT_VM     0
>  #define KVM_X86_SW_PROTECTED_VM        1
> +#define KVM_X86_SEV_VM         8
> +#define KVM_X86_SEV_ES_VM      10
>
> is _super_ confusing and bound to cause problems.
>
> Oh, good gravy, you're also aliasing __KVM_X86_PROTECTED_STATE_TYPE into SEV_ES_VM.
> Curse you and your Rami Code induced decimal-based bitwise shenanigans!!!

v1 was less gross but Mike talked me into this one. :)

> I don't see any reason to bleed the flags into KVM's ABI.  Even shoving the flags
> into kvm->arch.vm_type is unncessary.  Aha!  As is storing vm_type as an "unsigned
> long", since (a) it can't ever be bigger than u32, and (b) in practice a u8 will
> suffice since as Mike pointed out we're effectively limited to 31 types before
> kvm_vm_ioctl_check_extension() starts returning error codes.
>
> So I vote to skip the aliasing and simply do:

Fair enough.

Paolo