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;
Date:   Wed, 11 Jan 2023 19:12:01 -0800
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     "Wang, Lei" <lei4.wang@intel.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
        Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
        Sean Christopherson <seanjc@google.com>,
        Sagi Shahar <sagis@google.com>,
        David Matlack <dmatlack@google.com>,
        Xiaoyao Li <xiaoyao.li@intel.com>
Subject: Re: [PATCH v10 021/108] KVM: TDX: initialize VM with TDX specific
 parameters
Message-ID: <20230112031201.GB2034518@ls.amr.corp.intel.com>
References: <cover.1667110240.git.isaku.yamahata@intel.com>
 <ebb4beadff8e117d0fb643af4ee310b6608679bf.1667110240.git.isaku.yamahata@intel.com>
 <f9d7c13e-e96c-2afd-4c8b-cb7f17d61445@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <f9d7c13e-e96c-2afd-4c8b-cb7f17d61445@intel.com>
Precedence: bulk

On Wed, Jan 04, 2023 at 03:59:08PM +0800,
"Wang, Lei" <lei4.wang@intel.com> wrote:

> On 10/30/2022 2:22 PM, isaku.yamahata@intel.com wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > 
> > TDX requires additional parameters for TDX VM for confidential execution to
> > protect its confidentiality of its memory contents and its CPU state from
> > any other software, including VMM. When creating guest TD VM before
> > creating vcpu, the number of vcpu, TSC frequency (that is same among
> > vcpus. and it can't be changed.)  CPUIDs which is emulated by the TDX
> > module. It means guest can trust those CPUIDs. and sha384 values for
> > measurement.
> > 
> > Add new subcommand, KVM_TDX_INIT_VM, to pass parameters for TDX guest.  It
> > assigns encryption key to the TDX guest for memory encryption.  TDX
> > encrypts memory per-guest bases.  It assigns device model passes per-VM
> > parameters for the TDX guest.  The maximum number of vcpus, tsc frequency
> > (TDX guest has fised VM-wide TSC frequency. not per-vcpu.  The TDX guest
> > can not change it.), attributes (production or debug), available extended
> > features (which is reflected into guest XCR0, IA32_XSS MSR), cpuids, sha384
> > measurements, and etc.
> > 
> > This subcommand is called before creating vcpu and KVM_SET_CPUID2, i.e.
> > cpuids configurations aren't available yet.  So CPUIDs configuration values
> > needs to be passed in struct kvm_init_vm.  It's device model responsibility
> 
> I suppose this should be kvm_tdx_init_vm.
> 
> > to make this cpuid config for KVM_TDX_INIT_VM and KVM_SET_CPUID2.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  arch/x86/include/asm/tdx.h            |   3 +
> >  arch/x86/include/uapi/asm/kvm.h       |  31 +++
> >  arch/x86/kvm/vmx/tdx.c                | 296 ++++++++++++++++++++++----
> >  arch/x86/kvm/vmx/tdx.h                |  22 ++
> >  tools/arch/x86/include/uapi/asm/kvm.h |  33 +++
> >  5 files changed, 347 insertions(+), 38 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index cd304d323d33..05ac4bfc8f8a 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -131,6 +131,9 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
> >  #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
> >  
> >  #ifdef CONFIG_INTEL_TDX_HOST
> > +
> > +/* -1 indicates CPUID leaf with no sub-leaves. */
> > +#define TDX_CPUID_NO_SUBLEAF	((u32)-1)
> >  struct tdx_cpuid_config {
> >  	u32	leaf;
> >  	u32	sub_leaf;
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 2ad9666e02a5..26661879c031 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -538,6 +538,7 @@ struct kvm_pmu_event_filter {
> >  /* Trust Domain eXtension sub-ioctl() commands. */
> >  enum kvm_tdx_cmd_id {
> >  	KVM_TDX_CAPABILITIES = 0,
> > +	KVM_TDX_INIT_VM,
> >  
> >  	KVM_TDX_CMD_NR_MAX,
> >  };
> > @@ -583,4 +584,34 @@ struct kvm_tdx_capabilities {
> >  	struct kvm_tdx_cpuid_config cpuid_configs[0];
> >  };
> >  
> > +struct kvm_tdx_init_vm {
> > +	__u64 attributes;
> > +	__u64 mrconfigid[6];	/* sha384 digest */
> > +	__u64 mrowner[6];	/* sha384 digest */
> > +	__u64 mrownerconfig[6];	/* sha348 digest */
> > +	union {
> > +		/*
> > +		 * KVM_TDX_INIT_VM is called before vcpu creation, thus before
> > +		 * KVM_SET_CPUID2.  CPUID configurations needs to be passed.
> > +		 *
> > +		 * This configuration supersedes KVM_SET_CPUID{,2}.
> > +		 * The user space VMM, e.g. qemu, should make them consistent
> > +		 * with this values.
> > +		 * sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES(256)
> > +		 * = 8KB.
> > +		 */
> > +		struct {
> > +			struct kvm_cpuid2 cpuid;
> > +			/* 8KB with KVM_MAX_CPUID_ENTRIES. */
> > +			struct kvm_cpuid_entry2 entries[];
> > +		};
> > +		/*
> > +		 * For future extensibility.
> > +		 * The size(struct kvm_tdx_init_vm) = 16KB.
> > +		 * This should be enough given sizeof(TD_PARAMS) = 1024
> > +		 */
> > +		__u64 reserved[2029];
> > +	};
> > +};
> > +
> >  #endif /* _ASM_X86_KVM_H */
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index d77709a6da51..54045e0576e7 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -284,6 +284,205 @@ static int tdx_do_tdh_mng_key_config(void *param)
> >  int tdx_vm_init(struct kvm *kvm)
> >  {
> >  	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > +
> > +	kvm_tdx->hkid = -1;
> > +
> > +	/*
> > +	 * This function initializes only KVM software construct.  It doesn't
> > +	 * initialize TDX stuff, e.g. TDCS, TDR, TDCX, HKID etc.
> > +	 * It is handled by KVM_TDX_INIT_VM, __tdx_td_init().
> > +	 */
> > +
> > +	return 0;
> > +}
> > +
> > +int tdx_dev_ioctl(void __user *argp)
> > +{
> > +	struct kvm_tdx_capabilities __user *user_caps;
> > +	struct kvm_tdx_capabilities caps;
> > +	struct kvm_tdx_cmd cmd;
> > +
> > +	BUILD_BUG_ON(sizeof(struct kvm_tdx_cpuid_config) !=
> > +		     sizeof(struct tdx_cpuid_config));
> > +
> > +	if (copy_from_user(&cmd, argp, sizeof(cmd)))
> > +		return -EFAULT;
> > +	if (cmd.flags || cmd.error || cmd.unused)
> > +		return -EINVAL;
> > +	/*
> > +	 * Currently only KVM_TDX_CAPABILITIES is defined for system-scoped
> > +	 * mem_enc_ioctl().
> > +	 */
> > +	if (cmd.id != KVM_TDX_CAPABILITIES)
> > +		return -EINVAL;
> > +
> > +	user_caps = (void __user *)cmd.data;
> > +	if (copy_from_user(&caps, user_caps, sizeof(caps)))
> > +		return -EFAULT;
> > +
> > +	if (caps.nr_cpuid_configs < tdx_caps.nr_cpuid_configs)
> > +		return -E2BIG;
> > +
> > +	caps = (struct kvm_tdx_capabilities) {
> > +		.attrs_fixed0 = tdx_caps.attrs_fixed0,
> > +		.attrs_fixed1 = tdx_caps.attrs_fixed1,
> > +		.xfam_fixed0 = tdx_caps.xfam_fixed0,
> > +		.xfam_fixed1 = tdx_caps.xfam_fixed1,
> > +		.nr_cpuid_configs = tdx_caps.nr_cpuid_configs,
> > +		.padding = 0,
> > +	};
> > +
> > +	if (copy_to_user(user_caps, &caps, sizeof(caps)))
> > +		return -EFAULT;
> > +	if (copy_to_user(user_caps->cpuid_configs, &tdx_caps.cpuid_configs,
> > +			 tdx_caps.nr_cpuid_configs *
> > +			 sizeof(struct tdx_cpuid_config)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> 
> tdx_dev_ioctl() is introduced in previous patch with the same code added here,
> which means this is just a place change and it will confuse reviewers. Is it
> neccesary to do so?

Right. Somehow diff was also confused. I'll try to twist the previous patch to
introduce __tdx_td_init() with the previous patch.


...

> > +static inline bool is_td_initialized(struct kvm *kvm)
> > +{
> > +	return to_kvm_tdx(kvm)->hkid > 0;
> > +}
> 
> There is a similar function, is_hkid_assigned(), which is previously defined. Do
> you think redefining a new function here will bring code redundency?

I'll drop this function and replace it with is_hkid_assigned().
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>