Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific parameters

[Huang, Kai]

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.
"

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.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>

Missing Co-developed-by tag for Xiaoyao.

> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>

[...]

>   
> +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 ...

> +
>   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?

> 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.

>   #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.

[...]