Re: [PATCH v7 025/102] KVM: TDX: initialize VM with TDX specific parameters

Xiaoyao Li
2022-06-28T08:31:02Z

On 6/28/2022 5:53 AM, 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
> 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/kvm_host.h | 2 +
> arch/x86/include/asm/tdx.h | 3 +
> arch/x86/include/uapi/asm/kvm.h | 33 +++++
> arch/x86/kvm/vmx/tdx.c | 206 ++++++++++++++++++++++++++
> arch/x86/kvm/vmx/tdx.h | 23 +++
> tools/arch/x86/include/uapi/asm/kvm.h | 33 +++++
> 6 files changed, 300 insertions(+)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 342decc69649..81638987cdb9 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1338,6 +1338,8 @@ struct kvm_arch {
> * the global KVM_MAX_VCPU_IDS may lead to significant memory waste.
> */
> u32 max_vcpu_ids;
> +
> + gfn_t gfn_shared_mask;

I think it's better to put in a seperate patch or the patch that
consumes it.

> };
>
...

> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 2a9dfd54189f..1273b60a1a00 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -438,6 +438,209 @@ int tdx_dev_ioctl(void __user *argp)
> return 0;
> }
>
> +/*
> + * cpuid entry lookup in TDX cpuid config way.
> + * The difference is how to specify index(subleaves).
> + * Specify index to TDX_CPUID_NO_SUBLEAF for CPUID leaf with no-subleaves.
> + */
> +static const struct kvm_cpuid_entry2 *tdx_find_cpuid_entry(
> + const struct kvm_cpuid2 *cpuid, u32 function, u32 index)
> +{
> + int i;
> +
> +

superfluous line

> + /* In TDX CPU CONFIG, TDX_CPUID_NO_SUBLEAF means index = 0. */
> + if (index == TDX_CPUID_NO_SUBLEAF)
> + index = 0;
> +
> + for (i = 0; i < cpuid->nent; i++) {
> + const struct kvm_cpuid_entry2 *e = &cpuid->entries[i];
> +
> + if (e->function == function &&
> + (e->index == index ||
> + !(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)))
> + return e;
> + }
> + return NULL;
> +}

no need for kvm_tdx->tsc_khz field. We have kvm->arch.default_tsc_khz.
It seems kvm_tdx->tsc_khz is not used in the following patches.

...

> +
> + kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> + kvm_tdx->attributes = td_params->attributes;
> + kvm_tdx->xfam = td_params->xfam;
> + kvm_tdx->tsc_khz = TDX_TSC_25MHZ_TO_KHZ(td_params->tsc_frequency);
> + kvm->max_vcpus = td_params->max_vcpus;
> +
> + if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> + kvm->arch.gfn_shared_mask = gpa_to_gfn(BIT_ULL(51));
> + else
> + kvm->arch.gfn_shared_mask = gpa_to_gfn(BIT_ULL(47));
> +

....

> diff --git a/tools/arch/x86/include/uapi/asm/kvm.h b/tools/arch/x86/include/uapi/asm/kvm.h
> index a9ea3573be1b..779dfd683d66 100644
> --- a/tools/arch/x86/include/uapi/asm/kvm.h
> +++ b/tools/arch/x86/include/uapi/asm/kvm.h
> @@ -531,6 +531,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,
> };
> @@ -576,4 +577,36 @@ struct kvm_tdx_capabilities {
> struct kvm_tdx_cpuid_config cpuid_configs[0];
> };
>
> +struct kvm_tdx_init_vm {
> + __u64 attributes;
> + __u32 max_vcpus;
> + __u32 tsc_khz;

it needs to align with arch/x86/include/uapi/asm/kvm.h that @tsc_khz
needs to be removed.