Message-ID: <20240411185458.GD3039520@ls.amr.corp.intel.com>
Date: Thu, 11 Apr 2024 11:54:58 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
	Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
	Sean Christopherson <seanjc@...gle.com>,
	Sagi Shahar <sagis@...gle.com>, chen.bo@...el.com,
	hang.yuan@...el.com, tina.zhang@...el.com,
	Xiaoyao Li <xiaoyao.li@...el.com>, isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific
 parameters

On Thu, Apr 04, 2024 at 12:59:45PM +1300,
"Huang, Kai" <kai.huang@...el.com> wrote:

> 
> 
> On 26/02/2024 9:25 pm, isaku.yamahata@...el.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@...el.com>
> > 
> > TDX requires additional parameters for TDX VM for confidential execution to
> > protect the confidentiality of its memory contents and CPU state from any
> > other software, including VMM.
> 
> Hmm.. not only "confidentiality" but also "integrity".  And the "per-VM" TDX
> initializaiton here actually has nothing to do with "crypto-protection",
> because the establishment of the key has already been done before reaching
> here.
> 
> I would just say:
> 
> After the crypto-protection key has been configured, TDX requires a VM-scope
> initialization as a step of creating the TDX guest.  This "per-VM" TDX
> initialization does the global configurations/features that the TDX guest
> can support, such as guest's CPUIDs (emulated by the TDX module), the
> maximum number of vcpus etc.
> 
> 
> 
> 
> When creating a guest TD VM before creating
> > vcpu, the number of vcpu, TSC frequency (the values are the same among
> > vcpus, and it can't change.)  CPUIDs which the TDX module emulates.
> 
> I cannot parse this sentence.  It doesn't look like a sentence to me.
> 
> Guest
> > TDs can trust those CPUIDs and sha384 values for measurement.
> 
> Trustness is not about the "guest can trust", but the "people using the
> guest can trust".
> 
> Just remove it.
> 
> If you want to emphasize the attestation, you can add something like:
> 
> "
> It also passes the VM's measurement and hash of the signer etc and the
> hardware only allows to initialize the TDX guest when that match.
> "
> 
> > 
> > Add a new subcommand, KVM_TDX_INIT_VM, to pass parameters for the TDX
> > guest.
> 
> [...]
> 
> It assigns an encryption key to the TDX guest for memory
> > encryption.  TDX encrypts memory per guest basis.
> 
> No it doesn't.  The key has been programmed already in your previous patch.
> 
> The device model, say
> > qemu, passes per-VM parameters for the TDX guest.
> 
> This is implied by your first sentence of this paragraph.
> 
> The maximum number of
> > vcpus, TSC frequency (TDX guest has fixed VM-wide TSC frequency, not per
> > vcpu.  The TDX guest can not change it.), attributes (production or debug),
> > available extended features (which configure guest XCR0, IA32_XSS MSR),
> > CPUIDs, sha384 measurements, etc.
> 
> This is not a sentence.
> 
> > 
> > Call this subcommand before creating vcpu and KVM_SET_CPUID2, i.e.  CPUID
> > configurations aren't available yet.
> 
> "
> This "per-VM" TDX initialization must be done before any "vcpu-scope" TDX
> initialization.  To match this better, require the KVM_TDX_INIT_VM IOCTL()
> to be done before KVM creates any vcpus.
> 
> Note KVM configures the VM's CPUIDs in KVM_SET_CPUID2 via vcpu.  The
> downside of this approach is KVM will need to do some enforcement later to
> make sure the consisntency between the CPUIDs passed here and the CPUIDs
> done in KVM_SET_CPUID2.
> "

Thanks for the draft.  Let me update it.

> So CPUIDs configuration values need
> > to be passed in struct kvm_tdx_init_vm.  The device model's responsibility
> > to make this CPUID config for KVM_TDX_INIT_VM and KVM_SET_CPUID2.
> 
> And I would leave how to handle KVM_SET_CPUID2 to the patch that actually
> enforces the consisntency.

Yes, that's a different discussion.


> > +struct kvm_cpuid_entry2 *kvm_find_cpuid_entry2(
> > +	struct kvm_cpuid_entry2 *entries, int nent, u32 function, u64 index)
> > +{
> > +	return cpuid_entry2_find(entries, nent, function, index);
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry2);
> 
> Not sure whether we can export cpuid_entry2_find() directly?
> 
> No strong opinion of course.
> 
> But if we want to expose the wrapper, looks ...


Almost all KVM exported symbols have kvm_ prefix. I'm afraid that cpuid is too
common.  We can rename the function directly without wrapper.


> > +
> >   struct kvm_cpuid_entry2 *kvm_find_cpuid_entry_index(struct kvm_vcpu *vcpu,
> >   						    u32 function, u32 index)
> >   {
> > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> > index 856e3037e74f..215d1c68c6d1 100644
> > --- a/arch/x86/kvm/cpuid.h
> > +++ b/arch/x86/kvm/cpuid.h
> > @@ -13,6 +13,8 @@ void kvm_set_cpu_caps(void);
> >   void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
> >   void kvm_update_pv_runtime(struct kvm_vcpu *vcpu);
> > +struct kvm_cpuid_entry2 *kvm_find_cpuid_entry2(struct kvm_cpuid_entry2 *entries,
> > +					       int nent, u32 function, u64 index);
> >   struct kvm_cpuid_entry2 *kvm_find_cpuid_entry_index(struct kvm_vcpu *vcpu,
> >   						    u32 function, u32 index); >   struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
> 
> ... __kvm_find_cpuid_entry() would fit better?

Ok, let's rename it.


> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 1cf2b15da257..b11f105db3cd 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -8,7 +8,6 @@
> >   #include "mmu.h"
> >   #include "tdx_arch.h"
> >   #include "tdx.h"
> > -#include "tdx_ops.h"
> 
> ??
> 
> If it isn't needed, then it shouldn't be included in some previous patch.

Will fix.


> >   #include "x86.h"
> >   #undef pr_fmt
> > @@ -350,18 +349,21 @@ static int tdx_do_tdh_mng_key_config(void *param)
> >   	return 0;
> >   }
> > -static int __tdx_td_init(struct kvm *kvm);
> > -
> >   int tdx_vm_init(struct kvm *kvm)
> >   {
> > +	/*
> > +	 * 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().
> > +	 */
> > +
> >   	/*
> >   	 * TDX has its own limit of the number of vcpus in addition to
> >   	 * KVM_MAX_VCPUS.
> >   	 */
> >   	kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);
> > -	/* Place holder for TDX specific logic. */
> > -	return __tdx_td_init(kvm);
> > +	return 0;
> 
> ??
> 
> I don't quite understand.  What's wrong of still calling __tdx_td_init() in
> tdx_vm_init()?
> 
> If there's anything preventing doing __tdx_td_init() from tdx_vm_init(),
> then it's wrong to implement that in your previous patch.

Yes. As discussed the previous patch is too big, we need to break the previous
patch and this patch.
-- 
Isaku Yamahata <isaku.yamahata@...el.com>

Powered by blists - more mailing lists