On Thu, Apr 04, 2024 at 12:59:45PM +1300,
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
>
> On 26/02/2024 9:25 pm, isaku.yamahata@xxxxxxxxx wrote:
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
