Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <21d982b56e3958ae85c308defd13ec5f5e2edd39.camel@intel.com>
Subject: Re: [PATCH v7 012/102] KVM: x86: Introduce vm_type to differentiate
 default VMs from confidential VMs
From:   Kai Huang <kai.huang@intel.com>
To:     Isaku Yamahata <isaku.yamahata@gmail.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Xiaoyao Li <xiaoyao.li@intel.com>
Date:   Tue, 12 Jul 2022 13:24:08 +1200
In-Reply-To: <20220712010115.GE1379820@ls.amr.corp.intel.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
         <5979d880dc074c7fa57e02da34a41a6905ebd89d.1656366338.git.isaku.yamahata@intel.com>
         <3c5d4e38b631a921006e44551fe1249339393e41.camel@intel.com>
         <20220712010115.GE1379820@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

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@intel.com> wrote:
>=20
> > On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@intel.com wrote:
> > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > >=20
> > > 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)=
.
> > >=20
> > > 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 callbac=
k
> > > vm_init accepts vm_type.  So follow them.  Further, a different polic=
y can
> > > be made based on vm_type.  Define KVM_X86_DEFAULT_VM for default VM a=
s
> > > default and define KVM_X86_TDX_VM for Intel TDX VM.  The wrapper func=
tion
> > > will be defined as "bool is_td(kvm) { return vm_type =3D=3D VM_TYPE_T=
DX; }"
> > >=20
> > > 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 (introd=
uce a
> > > new capability and add vm_type) is chosen to align with other arch KV=
Ms
> > > that have VM types already.  Other arch KVMs uses different name to q=
uery
> > > supported vm types and there is no common name for it, so new name wa=
s
> > > chosen.
> > >=20
> > > Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  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(-)
> > >=20
> > > 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.
> > > =20
> > > +X86:
> > > +^^^^
> > > +
> > > +Supported vm type can be queried from KVM_CAP_VM_TYPES, which return=
s the
> > > +bitmap of supported vm types. The 1-setting of bit @n means vm type =
with
> > > +value @n is supported.
> >=20
> >=20
> > Perhaps I am missing something, but I don't understand how the below ch=
anges
> > (except the x86 part above) in Documentation are related to this patch.
>=20
> 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. P=
robably
> this documentation part can be split out into its own patch. thoughts?

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

>=20
>=20
> > > 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 {
> > > =20
> > >  static inline bool is_td(struct kvm *kvm)
> > >  {
> > > -	/*
> > > -	 * TDX VM type isn't defined yet.
> > > -	 * return kvm->arch.vm_type =3D=3D KVM_X86_TDX_VM;
> > > -	 */
> > > -	return false;
> > > +	return kvm->arch.vm_type =3D=3D KVM_X86_TDX_VM;
> > >  }
> >=20
> > If you put this patch before patch:
> >=20
> > 	[PATCH v7 009/102] KVM: TDX: Add placeholders for TDX VM/vcpu structur=
e
> >=20
> > Then you don't need to introduce this chunk in above patch and then rem=
ove it
> > here, which is unnecessary and ugly.
> >=20
> > 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 re=
port VM
> > type.  The KVM_X86_TDX_VM can come with the patch where is_td() is intr=
oduced
> > (in your above patch 9). =C2=A0
> >=20
> > To me, it's more clean way to write patch.  For instance, this infrastr=
uctural
> > patch can be theoretically used by other series if they have similar th=
ing to
> > support, but doesn't need to carry is_td() and KVM_X86_TDX_VM burden th=
at you
> > made.
>=20
> 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(=
).
>=20
> 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 o=
nly.
TDX VM can come later.  But will leave to maintainers.