Re: [PATCH v7 025/102] KVM: TDX: initialize VM with TDX specific parameters
From: Isaku Yamahata
Date: Tue Jul 12 2022 - 03:11:41 EST
On Tue, Jun 28, 2022 at 04:30:53PM +0800,
Xiaoyao Li <xiaoyao.li@xxxxxxxxx> wrote:
> On 6/28/2022 5:53 AM, isaku.yamahata@xxxxxxxxx wrote:
> > From: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > ---
> > 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.
Thanks, I fixed this patch as follows.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 81638987cdb9..342decc69649 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1338,8 +1338,6 @@ struct kvm_arch {
* the global KVM_MAX_VCPU_IDS may lead to significant memory waste.
*/
u32 max_vcpu_ids;
-
- gfn_t gfn_shared_mask;
};
struct kvm_vm_stat {
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 190b77f9cdd1..570127d4e566 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -441,7 +441,6 @@ static const struct kvm_cpuid_entry2 *tdx_find_cpuid_entry(
{
int i;
-
/* In TDX CPU CONFIG, TDX_CPUID_NO_SUBLEAF means index = 0. */
if (index == TDX_CPUID_NO_SUBLEAF)
index = 0;
@@ -619,7 +618,6 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
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)
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
index 8a0793fcc3ab..3e5782438dc9 100644
--- a/arch/x86/kvm/vmx/tdx.h
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -25,7 +25,6 @@ struct kvm_tdx {
int hkid;
u64 tsc_offset;
- unsigned long tsc_khz;
};
struct vcpu_tdx {
diff --git a/tools/arch/x86/include/uapi/asm/kvm.h b/tools/arch/x86/include/uapi/asm/kvm.h
index 18654ba2ee87..965a1c2e347d 100644
--- a/tools/arch/x86/include/uapi/asm/kvm.h
+++ b/tools/arch/x86/include/uapi/asm/kvm.h
@@ -581,7 +581,7 @@ struct kvm_tdx_capabilities {
struct kvm_tdx_init_vm {
__u64 attributes;
__u32 max_vcpus;
- __u32 tsc_khz;
+ __u32 padding;
__u64 mrconfigid[6]; /* sha384 digest */
__u64 mrowner[6]; /* sha384 digest */
__u64 mrownerconfig[6]; /* sha348 digest */
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>