Re: [PATCH v7 012/102] KVM: x86: Introduce vm_type to differentiate default VMs from confidential VMs

[Kai Huang]

On Mon, 2022-07-11 at 18:01 -0700, Isaku Yamahata wrote:
> On Tue, Jun 28, 2022 at 02:52:28PM +1200,
> Kai Huang <kai.huang@xxxxxxxxx> wrote:
> 
> > On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@xxxxxxxxx wrote:
> > > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > 
> > > Unlike default VMs, confidential VMs (Intel TDX and AMD SEV-ES) don't allow
> > > some operations (e.g., memory read/write, register state access, etc).
> > > 
> > > Introduce vm_type to track the type of the VM to x86 KVM.  Other arch KVMs
> > > already use vm_type, KVM_INIT_VM accepts vm_type, and x86 KVM callback
> > > vm_init accepts vm_type.  So follow them.  Further, a different policy can
> > > be made based on vm_type.  Define KVM_X86_DEFAULT_VM for default VM as
> > > default and define KVM_X86_TDX_VM for Intel TDX VM.  The wrapper function
> > > will be defined as "bool is_td(kvm) { return vm_type == VM_TYPE_TDX; }"
> > > 
> > > Add a capability KVM_CAP_VM_TYPES to effectively allow device model,
> > > e.g. qemu, to query what VM types are supported by KVM.  This (introduce a
> > > new capability and add vm_type) is chosen to align with other arch KVMs
> > > that have VM types already.  Other arch KVMs uses different name to query
> > > supported vm types and there is no common name for it, so new name was
> > > chosen.
> > > 
> > > Co-developed-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > > Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > ---
> > >  Documentation/virt/kvm/api.rst        | 21 +++++++++++++++++++++
> > >  arch/x86/include/asm/kvm-x86-ops.h    |  1 +
> > >  arch/x86/include/asm/kvm_host.h       |  2 ++
> > >  arch/x86/include/uapi/asm/kvm.h       |  3 +++
> > >  arch/x86/kvm/svm/svm.c                |  6 ++++++
> > >  arch/x86/kvm/vmx/main.c               |  1 +
> > >  arch/x86/kvm/vmx/tdx.h                |  6 +-----
> > >  arch/x86/kvm/vmx/vmx.c                |  5 +++++
> > >  arch/x86/kvm/vmx/x86_ops.h            |  1 +
> > >  arch/x86/kvm/x86.c                    |  9 ++++++++-
> > >  include/uapi/linux/kvm.h              |  1 +
> > >  tools/arch/x86/include/uapi/asm/kvm.h |  3 +++
> > >  tools/include/uapi/linux/kvm.h        |  1 +
> > >  13 files changed, 54 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > > index 9cbbfdb663b6..b9ab598883b2 100644
> > > --- a/Documentation/virt/kvm/api.rst
> > > +++ b/Documentation/virt/kvm/api.rst
> > > @@ -147,10 +147,31 @@ described as 'basic' will be available.
> > >  The new VM has no virtual cpus and no memory.
> > >  You probably want to use 0 as machine type.
> > >  
> > > +X86:
> > > +^^^^
> > > +
> > > +Supported vm type can be queried from KVM_CAP_VM_TYPES, which returns the
> > > +bitmap of supported vm types. The 1-setting of bit @n means vm type with
> > > +value @n is supported.
> > 
> > 
> > Perhaps I am missing something, but I don't understand how the below changes
> > (except the x86 part above) in Documentation are related to this patch.
> 
> This is to summarize divergence of archs.  Those archs (s390, mips, and
> arm64) introduce essentially same KVM capabilities, but different names. This
> patch makes things worse.  So I thought it's good idea to summarize it. Probably
> this documentation part can be split out into its own patch. thoughts?

I will leave to maintainers here.  Thought personally I would split different
things into different patches.

> 
> 
> > > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > > index 54d7a26ed9ee..2f43db5bbefb 100644
> > > --- a/arch/x86/kvm/vmx/tdx.h
> > > +++ b/arch/x86/kvm/vmx/tdx.h
> > > @@ -17,11 +17,7 @@ struct vcpu_tdx {
> > >  
> > >  static inline bool is_td(struct kvm *kvm)
> > >  {
> > > -	/*
> > > -	 * TDX VM type isn't defined yet.
> > > -	 * return kvm->arch.vm_type == KVM_X86_TDX_VM;
> > > -	 */
> > > -	return false;
> > > +	return kvm->arch.vm_type == KVM_X86_TDX_VM;
> > >  }
> > 
> > If you put this patch before patch:
> > 
> > 	[PATCH v7 009/102] KVM: TDX: Add placeholders for TDX VM/vcpu structure
> > 
> > Then you don't need to introduce this chunk in above patch and then remove it
> > here, which is unnecessary and ugly.
> > 
> > And you can even only introduce KVM_X86_DEFAULT_VM but not KVM_X86_TDX_VM in
> > this patch, so you can make this patch as a infrastructural patch to report VM
> > type.  The KVM_X86_TDX_VM can come with the patch where is_td() is introduced
> > (in your above patch 9).  
> > 
> > To me, it's more clean way to write patch.  For instance, this infrastructural
> > patch can be theoretically used by other series if they have similar thing to
> > support, but doesn't need to carry is_td() and KVM_X86_TDX_VM burden that you
> > made.
> 
> There are two choices. One is to put this patch before 9 as you suggested, other
> is to  put it here right before the patch 13 that uses vm_type_supported().
> 
> Thanks,

To me this belongs to category of "infrastructural patch", which does "Add new
ABI to support reporting VM types".  It can originally support default VM only.
TDX VM can come later.  But will leave to maintainers.