Re: [PATCH v18 025/121] KVM: TDX: initialize VM with TDX specific parameters

[Isaku Yamahata]

On Thu, Jan 25, 2024 at 10:19:30AM +0800,
Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx> wrote:

> 
> 
> On 1/23/2024 7:53 AM, 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.  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.  Guest
> > TDs can trust those CPUIDs and sha384 values for measurement.
> > 
> > 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.  The device model, say
> > qemu, passes per-VM parameters for the TDX guest.  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.
> > 
> > Call this subcommand before creating vcpu and KVM_SET_CPUID2, i.e.  CPUID
> > configurations aren't available yet.  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.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > 
> > ---
> > v18:
> > - remove the change of tools/arch/x86/include/uapi/asm/kvm.h
> > - typo in comment. sha348 => sha384
> > - updated comment in setup_tdparams_xfam()
> > - fix setup_tdparams_xfam() to use init_vm instead of td_params
> > 
> > v15 -> v16:
> > - Removed AMX check as the KVM upstream supports AMX.
> > - Added CET flag to guest supported xss
> > 
> > v14 -> v15:
> > - add check if the reserved area of init_vm is zero
> > ---
> >   arch/x86/include/uapi/asm/kvm.h |  27 ++++
> >   arch/x86/kvm/cpuid.c            |   7 +
> >   arch/x86/kvm/cpuid.h            |   2 +
> >   arch/x86/kvm/vmx/tdx.c          | 261 ++++++++++++++++++++++++++++++--
> >   arch/x86/kvm/vmx/tdx.h          |  18 +++
> >   arch/x86/kvm/vmx/tdx_arch.h     |   6 +
> >   6 files changed, 311 insertions(+), 10 deletions(-)
> > 
> [...]
> > +
> > +static int setup_tdparams_xfam(struct kvm_cpuid2 *cpuid, struct td_params *td_params)
> > +{
> > +	const struct kvm_cpuid_entry2 *entry;
> > +	u64 guest_supported_xcr0;
> > +	u64 guest_supported_xss;
> > +
> > +	/* Setup td_params.xfam */
> > +	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0xd, 0);
> > +	if (entry)
> > +		guest_supported_xcr0 = (entry->eax | ((u64)entry->edx << 32));
> > +	else
> > +		guest_supported_xcr0 = 0;
> > +	guest_supported_xcr0 &= kvm_caps.supported_xcr0;
> > +
> > +	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0xd, 1);
> > +	if (entry)
> > +		guest_supported_xss = (entry->ecx | ((u64)entry->edx << 32));
> > +	else
> > +		guest_supported_xss = 0;
> > +
> > +	/*
> > +	 * PT can be exposed to TD guest regardless of KVM's XSS and CET
> > +	 * support.
> > +	 */
> According to the code below, it seems that both PT and CET can be exposed to
> TD
> guest regardless of KVM's XSS support?

Yes, updated the comment.

> > +	guest_supported_xss &=
> > +		(kvm_caps.supported_xss | XFEATURE_MASK_PT | TDX_TD_XFAM_CET);
> > +
> > +	td_params->xfam = guest_supported_xcr0 | guest_supported_xss;
> > +	if (td_params->xfam & XFEATURE_MASK_LBR) {
> > +		/*
> > +		 * TODO: once KVM supports LBR(save/restore LBR related
> > +		 * registers around TDENTER), remove this guard.
> > +		 */
> > +#define MSG_LBR	"TD doesn't support LBR yet. KVM needs to save/restore IA32_LBR_DEPTH properly.\n"
> > +		pr_warn(MSG_LBR);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int setup_tdparams(struct kvm *kvm, struct td_params *td_params,
> > +			struct kvm_tdx_init_vm *init_vm)
> > +{
> > +	struct kvm_cpuid2 *cpuid = &init_vm->cpuid;
> > +	int ret;
> > +
> > +	if (kvm->created_vcpus)
> > +		return -EBUSY;
> > +
> > +	if (init_vm->attributes & TDX_TD_ATTRIBUTE_PERFMON) {
> > +		/*
> > +		 * TODO: save/restore PMU related registers around TDENTER.
> > +		 * Once it's done, remove this guard.
> > +		 */
> > +#define MSG_PERFMON	"TD doesn't support perfmon yet. KVM needs to save/restore host perf registers properly.\n"
> > +		pr_warn(MSG_PERFMON);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	td_params->max_vcpus = kvm->max_vcpus;
> Can the max vcpu number be passed by KVM_TDX_INIT_VM?
> So that no need to add KVM_CAP_MAX_VCPUS in patch 23/121.

Please see the comment there.
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxxxxxxxx>