From: Isaku Yamahata <[email protected]>
Memory used for TDX is encrypted with an encryption key. An encryption key
is assigned to guest TD, and TDX memory is encrypted. VMM calculates Trust
Domain Memory Range (TDMR), a range of memory pages that can hold TDX
memory encrypted with an encryption key. VMM allocates memory regions for
Physical Address Metadata Table (PAMT) which the TDX module uses to track
page states. Used for TDX memory, assigned to which guest TD, etc. VMM
gives PAMT regions to the TDX module and initializes it which is also
encrypted.
TDX requires more initialization steps in addition to VMX. As a
preparation step, check if the CPU feature is available and enable VMX
because the TDX module API requires VMX to be enabled to be functional.
The next step is basic platform initialization. Check if TDX module API is
available, call system-wide initialization API (TDH.SYS.INIT), and call LP
initialization API (TDH.SYS.LP.INIT). Lastly, get system-wide
parameters (TDH.SYS.INFO), allocate PAMT for TDX module to track page
states (TDH.SYS.CONFIG), configure encryption key (TDH.SYS.KEY.CONFIG), and
initialize PAMT (TDH.SYS.TDMR.INIT).
A TDX host patch series implements those details and it provides APIs,
seamrr_enabled() to check if CPU feature is available, init_tdx() to
initialize the TDX module, tdx_get_tdsysinfo() to get TDX system
parameters.
Add a wrapper function to initialize the TDX module and get system-wide
parameters via those APIs. Because TDX requires VMX enabled, It will be
called on-demand when the first guest TD is created via x86 KVM init_vm
callback.
Signed-off-by: Isaku Yamahata <[email protected]>
---
arch/x86/kvm/vmx/tdx.c | 89 ++++++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/tdx.h | 4 ++
2 files changed, 93 insertions(+)
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 8ed3ec342e28..8adc87ad1807 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -13,9 +13,98 @@
static bool __read_mostly enable_tdx = true;
module_param_named(tdx, enable_tdx, bool, 0644);
+#define TDX_MAX_NR_CPUID_CONFIGS \
+ ((sizeof(struct tdsysinfo_struct) - \
+ offsetof(struct tdsysinfo_struct, cpuid_configs)) \
+ / sizeof(struct tdx_cpuid_config))
+
+struct tdx_capabilities {
+ u8 tdcs_nr_pages;
+ u8 tdvpx_nr_pages;
+
+ u64 attrs_fixed0;
+ u64 attrs_fixed1;
+ u64 xfam_fixed0;
+ u64 xfam_fixed1;
+
+ u32 nr_cpuid_configs;
+ struct tdx_cpuid_config cpuid_configs[TDX_MAX_NR_CPUID_CONFIGS];
+};
+
+/* Capabilities of KVM + the TDX module. */
+struct tdx_capabilities tdx_caps;
+
static u64 hkid_mask __ro_after_init;
static u8 hkid_start_pos __ro_after_init;
+static int __tdx_module_setup(void)
+{
+ const struct tdsysinfo_struct *tdsysinfo;
+ int ret = 0;
+
+ BUILD_BUG_ON(sizeof(*tdsysinfo) != 1024);
+ BUILD_BUG_ON(TDX_MAX_NR_CPUID_CONFIGS != 37);
+
+ ret = tdx_detect();
+ if (ret) {
+ pr_info("Failed to detect TDX module.\n");
+ return ret;
+ }
+
+ ret = tdx_init();
+ if (ret) {
+ pr_info("Failed to initialize TDX module.\n");
+ return ret;
+ }
+
+ tdsysinfo = tdx_get_sysinfo();
+ if (tdx_caps.nr_cpuid_configs > TDX_MAX_NR_CPUID_CONFIGS)
+ return -EIO;
+
+ tdx_caps = (struct tdx_capabilities) {
+ .tdcs_nr_pages = tdsysinfo->tdcs_base_size / PAGE_SIZE,
+ /*
+ * TDVPS = TDVPR(4K page) + TDVPX(multiple 4K pages).
+ * -1 for TDVPR.
+ */
+ .tdvpx_nr_pages = tdsysinfo->tdvps_base_size / PAGE_SIZE - 1,
+ .attrs_fixed0 = tdsysinfo->attributes_fixed0,
+ .attrs_fixed1 = tdsysinfo->attributes_fixed1,
+ .xfam_fixed0 = tdsysinfo->xfam_fixed0,
+ .xfam_fixed1 = tdsysinfo->xfam_fixed1,
+ .nr_cpuid_configs = tdsysinfo->num_cpuid_config,
+ };
+ if (!memcpy(tdx_caps.cpuid_configs, tdsysinfo->cpuid_configs,
+ tdsysinfo->num_cpuid_config *
+ sizeof(struct tdx_cpuid_config)))
+ return -EIO;
+
+ return 0;
+}
+
+int tdx_module_setup(void)
+{
+ static DEFINE_MUTEX(tdx_init_lock);
+ static bool __read_mostly tdx_module_initialized;
+ int ret = 0;
+
+ mutex_lock(&tdx_init_lock);
+
+ if (!tdx_module_initialized) {
+ if (enable_tdx) {
+ ret = __tdx_module_setup();
+ if (ret)
+ enable_tdx = false;
+ else
+ tdx_module_initialized = true;
+ } else
+ ret = -EOPNOTSUPP;
+ }
+
+ mutex_unlock(&tdx_init_lock);
+ return ret;
+}
+
static int __init __tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
{
u32 max_pa;
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
index daf6bfc6502a..d448e019602c 100644
--- a/arch/x86/kvm/vmx/tdx.h
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -3,6 +3,8 @@
#define __KVM_X86_TDX_H
#ifdef CONFIG_INTEL_TDX_HOST
+int tdx_module_setup(void);
+
struct kvm_tdx {
struct kvm kvm;
};
@@ -35,6 +37,8 @@ static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu)
return container_of(vcpu, struct vcpu_tdx, vcpu);
}
#else
+static inline int tdx_module_setup(void) { return -ENODEV; };
+
struct kvm_tdx;
struct vcpu_tdx;
--
2.25.1
On 3/4/22 20:48, [email protected] wrote:
> +
> + if (!tdx_module_initialized) {
> + if (enable_tdx) {
> + ret = __tdx_module_setup();
> + if (ret)
> + enable_tdx = false;
"enable_tdx = false" isn't great to do only when a VM is created. Does
it make sense to anticipate this to the point when the kvm_intel.ko
module is loaded?
Paolo
> + else
> + tdx_module_initialized = true;
> + } else
> + ret = -EOPNOTSUPP;
> + }
> +
On Sun, Mar 13, 2022 at 03:03:40PM +0100,
Paolo Bonzini <[email protected]> wrote:
> On 3/4/22 20:48, [email protected] wrote:
> > +
> > + if (!tdx_module_initialized) {
> > + if (enable_tdx) {
> > + ret = __tdx_module_setup();
> > + if (ret)
> > + enable_tdx = false;
>
> "enable_tdx = false" isn't great to do only when a VM is created. Does it
> make sense to anticipate this to the point when the kvm_intel.ko module is
> loaded?
It's possible. I have the following two reasons to chose to defer TDX module
initialization until creating first TD. Given those reasons, do you still want
the initialization at loading kvm_intel.ko module? If yes, I'll change it.
- memory over head: The initialization of TDX module requires to allocate
physically contiguous memory whose size is about 0.43% of the system memory.
If user don't use TD, it will be wasted.
- VMXON on all pCPUs: The TDX module initialization requires to enable VMX
(VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
guest does it. It naturally fits with the TDX module initialization at creating
first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
--
Isaku Yamahata <[email protected]>
> >
> > - VMXON on all pCPUs: The TDX module initialization requires to enable VMX
> > (VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
> > guest does it. It naturally fits with the TDX module initialization at creating
> > first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
>
> That's a solvable problem, though making it work without exporting hardware_enable_all()
> could get messy.
Could you elaborate a little bit on how to resolve?
--
Thanks,
-Kai
On Mon, Mar 14, 2022, Isaku Yamahata wrote:
> On Sun, Mar 13, 2022 at 03:03:40PM +0100,
> Paolo Bonzini <[email protected]> wrote:
>
> > On 3/4/22 20:48, [email protected] wrote:
> > > +
> > > + if (!tdx_module_initialized) {
> > > + if (enable_tdx) {
> > > + ret = __tdx_module_setup();
> > > + if (ret)
> > > + enable_tdx = false;
> >
> > "enable_tdx = false" isn't great to do only when a VM is created. Does it
> > make sense to anticipate this to the point when the kvm_intel.ko module is
> > loaded?
>
> It's possible. I have the following two reasons to chose to defer TDX module
> initialization until creating first TD. Given those reasons, do you still want
> the initialization at loading kvm_intel.ko module? If yes, I'll change it.
Yes, TDX module setup needs to be done at load time. The loss of memory is
unfortunate, e.g. if the host is part of a pool that _might_ run TDX guests, but
the alternatives are worse. If TDX fails to initialize, e.g. due to low mem,
then the host will be unable to run TDX guests despite saying "I support TDX".
Or this gem :-)
/*
* TDH.SYS.KEY.CONFIG may fail with entropy error (which is
* a recoverable error). Assume this is exceedingly rare and
* just return error if encountered instead of retrying.
*/
The CPU overhead of initializing the TDX module is also non-trivial, and it
doesn't affect just this CPU, e.g. all CPUs need to do certain SEAMCALLs and at
least one WBINVD. The can cause noisy neighbor problems.
> - memory over head: The initialization of TDX module requires to allocate
> physically contiguous memory whose size is about 0.43% of the system memory.
> If user don't use TD, it will be wasted.
>
> - VMXON on all pCPUs: The TDX module initialization requires to enable VMX
> (VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
> guest does it. It naturally fits with the TDX module initialization at creating
> first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
That's a solvable problem, though making it work without exporting hardware_enable_all()
could get messy.
On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> Memory used for TDX is encrypted with an encryption key. An encryption key
> is assigned to guest TD, and TDX memory is encrypted. VMM calculates Trust
> Domain Memory Range (TDMR), a range of memory pages that can hold TDX
> memory encrypted with an encryption key. VMM allocates memory regions for
> Physical Address Metadata Table (PAMT) which the TDX module uses to track
> page states. Used for TDX memory, assigned to which guest TD, etc. VMM
> gives PAMT regions to the TDX module and initializes it which is also
> encrypted.
Not sure why above are related to this patch. Perhaps you can just say TDX
module is detected and initialized on demand via tdx_detect() and tdx_init().
>
> TDX requires more initialization steps in addition to VMX. As a
> preparation step, check if the CPU feature is available and enable VMX
> because the TDX module API requires VMX to be enabled to be functional.
Those are not reflected in this patch either.
> The next step is basic platform initialization. Check if TDX module API is
> available, call system-wide initialization API (TDH.SYS.INIT), and call LP
> initialization API (TDH.SYS.LP.INIT). Lastly, get system-wide
> parameters (TDH.SYS.INFO), allocate PAMT for TDX module to track page
> states (TDH.SYS.CONFIG), configure encryption key (TDH.SYS.KEY.CONFIG), and
> initialize PAMT (TDH.SYS.TDMR.INIT).
Again, not sure why those are related.
>
> A TDX host patch series implements those details and it provides APIs,
> seamrr_enabled() to check if CPU feature is available, init_tdx() to
> initialize the TDX module, tdx_get_tdsysinfo() to get TDX system
> parameters.
init_tdx() -> tdx_init().
"A TDX host patch series" should not be in the formal commit message, I suppose.
>
> Add a wrapper function to initialize the TDX module and get system-wide
> parameters via those APIs. Because TDX requires VMX enabled, It will be
> called on-demand when the first guest TD is created via x86 KVM init_vm
> callback.
Why not just merge this patch with the change where you implement the init_vm
callback? Then you can just declare this patch as "detect and initialize TDX
module when first VM is created", or something like that..
>
> Signed-off-by: Isaku Yamahata <[email protected]>
> ---
> arch/x86/kvm/vmx/tdx.c | 89 ++++++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/vmx/tdx.h | 4 ++
> 2 files changed, 93 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 8ed3ec342e28..8adc87ad1807 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -13,9 +13,98 @@
> static bool __read_mostly enable_tdx = true;
> module_param_named(tdx, enable_tdx, bool, 0644);
>
> +#define TDX_MAX_NR_CPUID_CONFIGS \
> + ((sizeof(struct tdsysinfo_struct) - \
> + offsetof(struct tdsysinfo_struct, cpuid_configs)) \
> + / sizeof(struct tdx_cpuid_config))
> +
> +struct tdx_capabilities {
> + u8 tdcs_nr_pages;
> + u8 tdvpx_nr_pages;
> +
> + u64 attrs_fixed0;
> + u64 attrs_fixed1;
> + u64 xfam_fixed0;
> + u64 xfam_fixed1;
> +
> + u32 nr_cpuid_configs;
> + struct tdx_cpuid_config cpuid_configs[TDX_MAX_NR_CPUID_CONFIGS];
> +};
> +
> +/* Capabilities of KVM + the TDX module. */
> +struct tdx_capabilities tdx_caps;
> +
> static u64 hkid_mask __ro_after_init;
> static u8 hkid_start_pos __ro_after_init;
The two seems are not used in this patch.
Please make sure each patch can compile w/o warning.
>
> +static int __tdx_module_setup(void)
> +{
> + const struct tdsysinfo_struct *tdsysinfo;
> + int ret = 0;
> +
> + BUILD_BUG_ON(sizeof(*tdsysinfo) != 1024);
> + BUILD_BUG_ON(TDX_MAX_NR_CPUID_CONFIGS != 37);
> +
> + ret = tdx_detect();
> + if (ret) {
> + pr_info("Failed to detect TDX module.\n");
> + return ret;
> + }
> +
> + ret = tdx_init();
> + if (ret) {
> + pr_info("Failed to initialize TDX module.\n");
> + return ret;
> + }
> +
> + tdsysinfo = tdx_get_sysinfo();
> + if (tdx_caps.nr_cpuid_configs > TDX_MAX_NR_CPUID_CONFIGS)
> + return -EIO;
> +
> + tdx_caps = (struct tdx_capabilities) {
> + .tdcs_nr_pages = tdsysinfo->tdcs_base_size / PAGE_SIZE,
> + /*
> + * TDVPS = TDVPR(4K page) + TDVPX(multiple 4K pages).
> + * -1 for TDVPR.
> + */
> + .tdvpx_nr_pages = tdsysinfo->tdvps_base_size / PAGE_SIZE - 1,
> + .attrs_fixed0 = tdsysinfo->attributes_fixed0,
> + .attrs_fixed1 = tdsysinfo->attributes_fixed1,
> + .xfam_fixed0 = tdsysinfo->xfam_fixed0,
> + .xfam_fixed1 = tdsysinfo->xfam_fixed1,
> + .nr_cpuid_configs = tdsysinfo->num_cpuid_config,
> + };
> + if (!memcpy(tdx_caps.cpuid_configs, tdsysinfo->cpuid_configs,
> + tdsysinfo->num_cpuid_config *
> + sizeof(struct tdx_cpuid_config)))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +int tdx_module_setup(void)
> +{
> + static DEFINE_MUTEX(tdx_init_lock);
> + static bool __read_mostly tdx_module_initialized;
> + int ret = 0;
> +
> + mutex_lock(&tdx_init_lock);
It took me a while to figure out why this mutex is needed. Please see my above
suggestion to merge this patch to the change that implements init_vm() callback.
> +
> + if (!tdx_module_initialized) {
> + if (enable_tdx) {
> + ret = __tdx_module_setup();
I think you can move tdx_detect() and tdx_init() out of your mutex. They are
internally protected by mutex.
> + if (ret)
> + enable_tdx = false;
> + else
> + tdx_module_initialized = true;
> + } else
> + ret = -EOPNOTSUPP;
> + }
> +
> + mutex_unlock(&tdx_init_lock);
> + return ret;
> +}
> +
> static int __init __tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> {
> u32 max_pa;
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index daf6bfc6502a..d448e019602c 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -3,6 +3,8 @@
> #define __KVM_X86_TDX_H
>
> #ifdef CONFIG_INTEL_TDX_HOST
> +int tdx_module_setup(void);
> +
> struct kvm_tdx {
> struct kvm kvm;
> };
> @@ -35,6 +37,8 @@ static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu)
> return container_of(vcpu, struct vcpu_tdx, vcpu);
> }
> #else
> +static inline int tdx_module_setup(void) { return -ENODEV; };
> +
> struct kvm_tdx;
> struct vcpu_tdx;
>
On Thu, Mar 31, 2022 at 04:31:10PM +1300,
Kai Huang <[email protected]> wrote:
> On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
> > From: Isaku Yamahata <[email protected]>
> > Add a wrapper function to initialize the TDX module and get system-wide
> > parameters via those APIs. Because TDX requires VMX enabled, It will be
> > called on-demand when the first guest TD is created via x86 KVM init_vm
> > callback.
>
> Why not just merge this patch with the change where you implement the init_vm
> callback? Then you can just declare this patch as "detect and initialize TDX
> module when first VM is created", or something like that..
Ok. Anyway in the next respoin, tdx module initialization will be done when
loading kvm_intel.ko. So the whole part will be changed and will be a part
of module loading.
--
Isaku Yamahata <[email protected]>
On 4/1/2022 3:41 AM, Isaku Yamahata wrote:
> On Thu, Mar 31, 2022 at 04:31:10PM +1300,
> Kai Huang <[email protected]> wrote:
>
>> On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
>>> From: Isaku Yamahata <[email protected]>
>
>>> Add a wrapper function to initialize the TDX module and get system-wide
>>> parameters via those APIs. Because TDX requires VMX enabled, It will be
>>> called on-demand when the first guest TD is created via x86 KVM init_vm
>>> callback.
>>
>> Why not just merge this patch with the change where you implement the init_vm
>> callback? Then you can just declare this patch as "detect and initialize TDX
>> module when first VM is created", or something like that..
>
> Ok. Anyway in the next respoin, tdx module initialization will be done when
> loading kvm_intel.ko. So the whole part will be changed and will be a part
> of module loading.
Will we change the GET_TDX_CAPABILITIES ioctl back to KVM scope?
On Thu, Mar 31, 2022 at 12:03:15AM +0000,
Sean Christopherson <[email protected]> wrote:
> On Mon, Mar 14, 2022, Isaku Yamahata wrote:
> > On Sun, Mar 13, 2022 at 03:03:40PM +0100,
> > Paolo Bonzini <[email protected]> wrote:
> >
> > > On 3/4/22 20:48, [email protected] wrote:
> > > > +
> > > > + if (!tdx_module_initialized) {
> > > > + if (enable_tdx) {
> > > > + ret = __tdx_module_setup();
> > > > + if (ret)
> > > > + enable_tdx = false;
> > >
> > > "enable_tdx = false" isn't great to do only when a VM is created. Does it
> > > make sense to anticipate this to the point when the kvm_intel.ko module is
> > > loaded?
> >
> > It's possible. I have the following two reasons to chose to defer TDX module
> > initialization until creating first TD. Given those reasons, do you still want
> > the initialization at loading kvm_intel.ko module? If yes, I'll change it.
>
> Yes, TDX module setup needs to be done at load time. The loss of memory is
> unfortunate, e.g. if the host is part of a pool that _might_ run TDX guests, but
> the alternatives are worse. If TDX fails to initialize, e.g. due to low mem,
> then the host will be unable to run TDX guests despite saying "I support TDX".
> Or this gem :-)
Ok.
> > - memory over head: The initialization of TDX module requires to allocate
> > physically contiguous memory whose size is about 0.43% of the system memory.
> > If user don't use TD, it will be wasted.
> >
> > - VMXON on all pCPUs: The TDX module initialization requires to enable VMX
> > (VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
> > guest does it. It naturally fits with the TDX module initialization at creating
> > first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
>
> That's a solvable problem, though making it work without exporting hardware_enable_all()
> could get messy.
Could you please explain any reason why it's bad idea to export it?
--
Isaku Yamahata <[email protected]>
+Chao Gao
On Thu, Mar 31, 2022, Isaku Yamahata wrote:
> On Thu, Mar 31, 2022 at 12:03:15AM +0000, Sean Christopherson <[email protected]> wrote:
> > On Mon, Mar 14, 2022, Isaku Yamahata wrote:
> > > - VMXON on all pCPUs: The TDX module initialization requires to enable VMX
> > > (VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
> > > guest does it. It naturally fits with the TDX module initialization at creating
> > > first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
> >
> > That's a solvable problem, though making it work without exporting hardware_enable_all()
> > could get messy.
>
> Could you please explain any reason why it's bad idea to export it?
I'd really prefer to keep the hardware enable/disable logic internal to kvm_main.c
so that all architectures share a common flow, and so that kvm_main.c is the sole
owner. I'm worried that exposing the helper will lead to other arch/vendor usage,
and that will end up with what is effectively duplicate flows. Deduplicating arch
code into generic KVM is usually very difficult.
This might also be a good opportunity to make KVM slightly more robust. Ooh, and
we can kill two birds with one stone. There's an in-flight series to add compatibility
checks to hotplug[*]. But rather than special case hotplug, what if we instead do
hardware enable/disable during module load, and move the compatibility check into
the hardware_enable path? That fixes the hotplug issue, gives TDX a window for running
post-VMXON code in kvm_init(), and makes the broadcast IPI less wasteful on architectures
that don't have compatiblity checks.
I'm thinking something like this, maybe as a modificatyion to patch 6 in Chao's
series, or more likely as a patch 7 so that the hotplug compat checks still get
in even if the early hardware enable doesn't work on all architectures for some
reason.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 69c318fdff61..c6572a056072 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4838,8 +4838,13 @@ static void hardware_enable_nolock(void *junk)
cpumask_set_cpu(cpu, cpus_hardware_enabled);
+ r = kvm_arch_check_processor_compat();
+ if (r)
+ goto out;
+
r = kvm_arch_hardware_enable();
+out:
if (r) {
cpumask_clear_cpu(cpu, cpus_hardware_enabled);
atomic_inc(&hardware_enable_failed);
@@ -5636,18 +5641,6 @@ void kvm_unregister_perf_callbacks(void)
}
#endif
-struct kvm_cpu_compat_check {
- void *opaque;
- int *ret;
-};
-
-static void check_processor_compat(void *data)
-{
- struct kvm_cpu_compat_check *c = data;
-
- *c->ret = kvm_arch_check_processor_compat(c->opaque);
-}
-
int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
struct module *module)
{
@@ -5679,13 +5672,13 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
if (r < 0)
goto out_free_1;
- c.ret = &r;
- c.opaque = opaque;
- for_each_online_cpu(cpu) {
- smp_call_function_single(cpu, check_processor_compat, &c, 1);
- if (r < 0)
- goto out_free_2;
- }
+ r = hardware_enable_all();
+ if (r)
+ goto out_free_2;
+
+ kvm_arch_post_hardware_enable_setup();
+
+ hardware_disable_all();
r = cpuhp_setup_state_nocalls(CPUHP_AP_KVM_STARTING, "kvm/cpu:starting",
kvm_starting_cpu, kvm_dying_cpu);
[*] https://lore.kernel.org/all/[email protected]
The original reply was sent to Sean only by mistake. Add others back.
On Fri, Apr 01, 2022 at 11:27:42AM +0800, Chao Gao wrote:
>On Thu, Mar 31, 2022 at 07:34:12PM +0000, Sean Christopherson wrote:
>>+Chao Gao
>>
>>On Thu, Mar 31, 2022, Isaku Yamahata wrote:
>>> On Thu, Mar 31, 2022 at 12:03:15AM +0000, Sean Christopherson <[email protected]> wrote:
>>> > On Mon, Mar 14, 2022, Isaku Yamahata wrote:
>>> > > - VMXON on all pCPUs: The TDX module initialization requires to enable VMX
>>> > > (VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
>>> > > guest does it. It naturally fits with the TDX module initialization at creating
>>> > > first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
>>> >
>>> > That's a solvable problem, though making it work without exporting hardware_enable_all()
>>> > could get messy.
>>>
>>> Could you please explain any reason why it's bad idea to export it?
>>
>>I'd really prefer to keep the hardware enable/disable logic internal to kvm_main.c
>>so that all architectures share a common flow, and so that kvm_main.c is the sole
>>owner. I'm worried that exposing the helper will lead to other arch/vendor usage,
>>and that will end up with what is effectively duplicate flows. Deduplicating arch
>>code into generic KVM is usually very difficult.
>>
>>This might also be a good opportunity to make KVM slightly more robust. Ooh, and
>>we can kill two birds with one stone. There's an in-flight series to add compatibility
>>checks to hotplug[*]. But rather than special case hotplug, what if we instead do
>>hardware enable/disable during module load, and move the compatibility check into
>>the hardware_enable path? That fixes the hotplug issue, gives TDX a window for running
>>post-VMXON code in kvm_init(), and makes the broadcast IPI less wasteful on architectures
>>that don't have compatiblity checks.
>
>Sounds good. But more time is wasted on compat checks on architectures
>that have them because they are done each time of enabling hardware.
>A solution for this is caching the result of kvm_arch_check_processor_compat().
>
>>
>>I'm thinking something like this, maybe as a modificatyion to patch 6 in Chao's
>>series, or more likely as a patch 7 so that the hotplug compat checks still get
>>in even
>
>>if the early hardware enable doesn't work on all architectures for some
>>reason.
>
>By "early", do you mean hardware enable during module loading or during CPU hotplug?
>
>And if below change is put into my series, kvm_arch_post_hardware_enable_setup()
>will be an empty function for all architectures until TDX series gets merged.
>So, I prefer to drop kvm_arch_post_hardware_enable_setup() and let TDX series
>introduce it.
On 4/2/2022 4:18 AM, Isaku Yamahata wrote:
> On Fri, Apr 01, 2022 at 02:56:40PM +0800,
> Xiaoyao Li <[email protected]> wrote:
>
>> On 4/1/2022 3:41 AM, Isaku Yamahata wrote:
>>> On Thu, Mar 31, 2022 at 04:31:10PM +1300,
>>> Kai Huang <[email protected]> wrote:
>>>
>>>> On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
>>>>> From: Isaku Yamahata <[email protected]>
>>>
>>>>> Add a wrapper function to initialize the TDX module and get system-wide
>>>>> parameters via those APIs. Because TDX requires VMX enabled, It will be
>>>>> called on-demand when the first guest TD is created via x86 KVM init_vm
>>>>> callback.
>>>>
>>>> Why not just merge this patch with the change where you implement the init_vm
>>>> callback? Then you can just declare this patch as "detect and initialize TDX
>>>> module when first VM is created", or something like that..
>>>
>>> Ok. Anyway in the next respoin, tdx module initialization will be done when
>>> loading kvm_intel.ko. So the whole part will be changed and will be a part
>>> of module loading.
>>
>> Will we change the GET_TDX_CAPABILITIES ioctl back to KVM scope?
>
> No because it system scoped KVM_TDX_CAPABILITIES requires one more callback for
> it. We can reduce the change.
>
> Or do you have any use case for system scoped KVM_TDX_CAPABILITIES?
No. Just to confirm.
on the other hand, vm-scope IOCTL seems more flexible if different
capabilities are reported per VM in the future.
On Fri, Apr 01, 2022 at 02:56:40PM +0800,
Xiaoyao Li <[email protected]> wrote:
> On 4/1/2022 3:41 AM, Isaku Yamahata wrote:
> > On Thu, Mar 31, 2022 at 04:31:10PM +1300,
> > Kai Huang <[email protected]> wrote:
> >
> > > On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
> > > > From: Isaku Yamahata <[email protected]>
> >
> > > > Add a wrapper function to initialize the TDX module and get system-wide
> > > > parameters via those APIs. Because TDX requires VMX enabled, It will be
> > > > called on-demand when the first guest TD is created via x86 KVM init_vm
> > > > callback.
> > >
> > > Why not just merge this patch with the change where you implement the init_vm
> > > callback? Then you can just declare this patch as "detect and initialize TDX
> > > module when first VM is created", or something like that..
> >
> > Ok. Anyway in the next respoin, tdx module initialization will be done when
> > loading kvm_intel.ko. So the whole part will be changed and will be a part
> > of module loading.
>
> Will we change the GET_TDX_CAPABILITIES ioctl back to KVM scope?
No because it system scoped KVM_TDX_CAPABILITIES requires one more callback for
it. We can reduce the change.
Or do you have any use case for system scoped KVM_TDX_CAPABILITIES?
--
Isaku Yamahata <[email protected]>