From: Isaku Yamahata <[email protected]>
To use TDX functionality, TDX module needs to be loaded and initialized.
A TDX host patch series[1] implements the detection of the TDX module,
tdx_detect() and its initialization, tdx_init().
This patch is to call those functions, tdx_detect() and tdx_init(), when
loading kvm_intel.ko.
Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
while hardware is enabled, i.e. after hardware_enable_all() and before
hardware_disable_all(). Because TDX requires all present CPUs to enable
VMX (VMXON).
[1] https://lore.kernel.org/lkml/[email protected]/
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Isaku Yamahata <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/vmx/main.c | 11 ++++++
arch/x86/kvm/vmx/tdx.c | 60 +++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/tdx.h | 4 +++
arch/x86/kvm/x86.c | 8 +++++
5 files changed, 84 insertions(+)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 62dec97f6607..aa11525500d3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1639,6 +1639,7 @@ struct kvm_x86_init_ops {
int (*cpu_has_kvm_support)(void);
int (*disabled_by_bios)(void);
int (*hardware_setup)(void);
+ int (*post_hardware_enable_setup)(void);
unsigned int (*handle_intel_pt_intr)(void);
struct kvm_x86_ops *runtime_ops;
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 349534412216..ac788af17d92 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -23,6 +23,16 @@ static __init int vt_hardware_setup(void)
return 0;
}
+static int __init vt_post_hardware_enable_setup(void)
+{
+ enable_tdx = enable_tdx && !tdx_module_setup();
+ /*
+ * Even if it failed to initialize TDX module, conventional VMX is
+ * available. Keep VMX usable.
+ */
+ return 0;
+}
+
struct kvm_x86_ops vt_x86_ops __initdata = {
.name = "kvm_intel",
@@ -165,6 +175,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
.cpu_has_kvm_support = vmx_cpu_has_kvm_support,
.disabled_by_bios = vmx_disabled_by_bios,
.hardware_setup = vt_hardware_setup,
+ .post_hardware_enable_setup = vt_post_hardware_enable_setup,
.handle_intel_pt_intr = NULL,
.runtime_ops = &vt_x86_ops,
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 2617389ef466..9cb36716b0f3 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -13,6 +13,66 @@
static u64 hkid_mask __ro_after_init;
static u8 hkid_start_pos __ro_after_init;
+#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. */
+static struct tdx_capabilities tdx_caps;
+
+int __init 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_init();
+ if (ret) {
+ pr_info("Failed to initialize TDX module.\n");
+ return ret;
+ }
+
+ tdsysinfo = tdx_get_sysinfo();
+ if (tdsysinfo->num_cpuid_config > 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 __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 060bf48ec3d6..54d7a26ed9ee 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;
/* TDX specific members follow. */
@@ -37,6 +39,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 kvm kvm;
};
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 30af2bd0b4d5..fb7a33fbc136 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11792,6 +11792,14 @@ int kvm_arch_hardware_setup(void *opaque)
return 0;
}
+int kvm_arch_post_hardware_enable_setup(void *opaque)
+{
+ struct kvm_x86_init_ops *ops = opaque;
+ if (ops->post_hardware_enable_setup)
+ return ops->post_hardware_enable_setup();
+ return 0;
+}
+
void kvm_arch_hardware_unsetup(void)
{
kvm_unregister_perf_callbacks();
--
2.25.1
On Mon, 2022-06-27 at 14:53 -0700, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> To use TDX functionality, TDX module needs to be loaded and initialized.
> A TDX host patch series[1] implements the detection of the TDX module,
> tdx_detect() and its initialization, tdx_init().
"A TDX host patch series[1]" really isn't a commit message material. You can
put it to the cover letter, but not here.
Also tdx_detect() is removed in latest code.
>
> This patch is to call those functions, tdx_detect() and tdx_init(), when
> loading kvm_intel.ko.
>
> Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
> while hardware is enabled, i.e. after hardware_enable_all() and before
> hardware_disable_all(). Because TDX requires all present CPUs to enable
> VMX (VMXON).
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Suggested-by: Sean Christopherson <[email protected]>
> Signed-off-by: Isaku Yamahata <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/vmx/main.c | 11 ++++++
> arch/x86/kvm/vmx/tdx.c | 60 +++++++++++++++++++++++++++++++++
> arch/x86/kvm/vmx/tdx.h | 4 +++
> arch/x86/kvm/x86.c | 8 +++++
> 5 files changed, 84 insertions(+)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 62dec97f6607..aa11525500d3 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1639,6 +1639,7 @@ struct kvm_x86_init_ops {
> int (*cpu_has_kvm_support)(void);
> int (*disabled_by_bios)(void);
> int (*hardware_setup)(void);
> + int (*post_hardware_enable_setup)(void);
> unsigned int (*handle_intel_pt_intr)(void);
>
> struct kvm_x86_ops *runtime_ops;
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 349534412216..ac788af17d92 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -23,6 +23,16 @@ static __init int vt_hardware_setup(void)
> return 0;
> }
>
> +static int __init vt_post_hardware_enable_setup(void)
> +{
> + enable_tdx = enable_tdx && !tdx_module_setup();
> + /*
> + * Even if it failed to initialize TDX module, conventional VMX is
> + * available. Keep VMX usable.
> + */
> + return 0;
> +}
> +
> struct kvm_x86_ops vt_x86_ops __initdata = {
> .name = "kvm_intel",
>
> @@ -165,6 +175,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
> .cpu_has_kvm_support = vmx_cpu_has_kvm_support,
> .disabled_by_bios = vmx_disabled_by_bios,
> .hardware_setup = vt_hardware_setup,
> + .post_hardware_enable_setup = vt_post_hardware_enable_setup,
> .handle_intel_pt_intr = NULL,
>
> .runtime_ops = &vt_x86_ops,
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 2617389ef466..9cb36716b0f3 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -13,6 +13,66 @@
> static u64 hkid_mask __ro_after_init;
> static u8 hkid_start_pos __ro_after_init;
>
> +#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. */
> +static struct tdx_capabilities tdx_caps;
> +
> +int __init 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_init();
> + if (ret) {
> + pr_info("Failed to initialize TDX module.\n");
> + return ret;
> + }
> +
> + tdsysinfo = tdx_get_sysinfo();
> + if (tdsysinfo->num_cpuid_config > 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 __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 060bf48ec3d6..54d7a26ed9ee 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;
> /* TDX specific members follow. */
> @@ -37,6 +39,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 kvm kvm;
> };
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 30af2bd0b4d5..fb7a33fbc136 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11792,6 +11792,14 @@ int kvm_arch_hardware_setup(void *opaque)
> return 0;
> }
>
> +int kvm_arch_post_hardware_enable_setup(void *opaque)
> +{
> + struct kvm_x86_init_ops *ops = opaque;
> + if (ops->post_hardware_enable_setup)
> + return ops->post_hardware_enable_setup();
> + return 0;
> +}
> +
Where is this kvm_arch_post_hardware_enable_setup() called?
Shouldn't the code change which calls it be part of this patch?
> void kvm_arch_hardware_unsetup(void)
> {
> kvm_unregister_perf_callbacks();
On Tue, Jun 28, 2022 at 04:31:35PM +1200,
Kai Huang <[email protected]> wrote:
> On Mon, 2022-06-27 at 14:53 -0700, [email protected] wrote:
> > From: Isaku Yamahata <[email protected]>
> >
> > To use TDX functionality, TDX module needs to be loaded and initialized.
> > A TDX host patch series[1] implements the detection of the TDX module,
> > tdx_detect() and its initialization, tdx_init().
>
> "A TDX host patch series[1]" really isn't a commit message material. You can
> put it to the cover letter, but not here.
>
> Also tdx_detect() is removed in latest code.
How about the followings?
KVM: TDX: Initialize TDX module when loading kvm_intel.ko
To use TDX functionality, TDX module needs to be loaded and initialized.
This patch is to call a function, tdx_init(), when loading kvm_intel.ko.
Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
while hardware is enabled, i.e. after hardware_enable_all() and before
hardware_disable_all(). Because TDX requires all present CPUs to enable
VMX (VMXON).
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 30af2bd0b4d5..fb7a33fbc136 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11792,6 +11792,14 @@ int kvm_arch_hardware_setup(void *opaque)
> > return 0;
> > }
> >
> > +int kvm_arch_post_hardware_enable_setup(void *opaque)
> > +{
> > + struct kvm_x86_init_ops *ops = opaque;
> > + if (ops->post_hardware_enable_setup)
> > + return ops->post_hardware_enable_setup();
> > + return 0;
> > +}
> > +
>
> Where is this kvm_arch_post_hardware_enable_setup() called?
>
> Shouldn't the code change which calls it be part of this patch?
The patch of "4/102 KVM: Refactor CPU compatibility check on module
initialiization" introduces it. Because the patch affects multiple archs
(mips, x86, poerpc, s390, and arm), I deliberately put it in early.
--
Isaku Yamahata <[email protected]>
On Mon, 2022-07-11 at 17:46 -0700, Isaku Yamahata wrote:
> On Tue, Jun 28, 2022 at 04:31:35PM +1200,
> Kai Huang <[email protected]> wrote:
>
> > On Mon, 2022-06-27 at 14:53 -0700, [email protected] wrote:
> > > From: Isaku Yamahata <[email protected]>
> > >
> > > To use TDX functionality, TDX module needs to be loaded and initialized.
> > > A TDX host patch series[1] implements the detection of the TDX module,
> > > tdx_detect() and its initialization, tdx_init().
> >
> > "A TDX host patch series[1]" really isn't a commit message material. You can
> > put it to the cover letter, but not here.
> >
> > Also tdx_detect() is removed in latest code.
>
> How about the followings?
>
> KVM: TDX: Initialize TDX module when loading kvm_intel.ko
Personally don't like kvm_intel.ko in title (or changelog), but will leave to
maintainers.
>
> To use TDX functionality, TDX module needs to be loaded and initialized.
> This patch is to call a function, tdx_init(), when loading kvm_intel.ko.
Could you add explain why we need to init TDX module when loading KVM module?
You don't have to say "call a function, tdx_init()", which can be easily seen in
the code.
>
> Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
> while hardware is enabled, i.e. after hardware_enable_all() and before
> hardware_disable_all(). Because TDX requires all present CPUs to enable
> VMX (VMXON).
Please explicitly say it is a replacement of the default __weak version, so
people can know there's already a default one. Otherwise people may wonder why
this isn't called in this patch (i.e. I skipped patch 03 as it looks not
directly related to TDX).
That being said, why cannot you send out that patch separately but have to
include it into TDX series?
Looking at it, the only thing that is related to TDX is an empty
kvm_arch_post_hardware_enable_setup() with a comment saying TDX needs to do
something there. This logic has nothing to do with the actual job in that
patch.
So why cannot we introduce that __weak version in this patch, so that the rest
of it can be non-TDX related at all and can be upstreamed separately?
>
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 30af2bd0b4d5..fb7a33fbc136 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -11792,6 +11792,14 @@ int kvm_arch_hardware_setup(void *opaque)
> > > return 0;
> > > }
> > >
> > > +int kvm_arch_post_hardware_enable_setup(void *opaque)
> > > +{
> > > + struct kvm_x86_init_ops *ops = opaque;
> > > + if (ops->post_hardware_enable_setup)
> > > + return ops->post_hardware_enable_setup();
> > > + return 0;
> > > +}
> > > +
> >
> > Where is this kvm_arch_post_hardware_enable_setup() called?
> >
> > Shouldn't the code change which calls it be part of this patch?
>
> The patch of "4/102 KVM: Refactor CPU compatibility check on module
> initialiization" introduces it. Because the patch affects multiple archs
> (mips, x86, poerpc, s390, and arm), I deliberately put it in early.
It's patch 03, but not 04. And see above.
On Tue, Jul 12, 2022 at 01:13:10PM +1200,
Kai Huang <[email protected]> wrote:
> > To use TDX functionality, TDX module needs to be loaded and initialized.
> > This patch is to call a function, tdx_init(), when loading kvm_intel.ko.
>
> Could you add explain why we need to init TDX module when loading KVM module?
Makes sense. Added a paragraph for it.
> > Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
> > while hardware is enabled, i.e. after hardware_enable_all() and before
> > hardware_disable_all(). Because TDX requires all present CPUs to enable
> > VMX (VMXON).
>
> Please explicitly say it is a replacement of the default __weak version, so
> people can know there's already a default one. Otherwise people may wonder why
> this isn't called in this patch (i.e. I skipped patch 03 as it looks not
> directly related to TDX).
>
> That being said, why cannot you send out that patch separately but have to
> include it into TDX series?
>
> Looking at it, the only thing that is related to TDX is an empty
> kvm_arch_post_hardware_enable_setup() with a comment saying TDX needs to do
> something there. This logic has nothing to do with the actual job in that
> patch.
>
> So why cannot we introduce that __weak version in this patch, so that the rest
> of it can be non-TDX related at all and can be upstreamed separately?
The patch that adds weak kvm_arch_post_hardware_enable_setup() doesn't make
sense without the hook because it only enable_hardware and then disable hardware
immediately.
The patch touches multiple kvm arch. and I split out TDX specific part in this
patch. Ideally those two patch should be near. But I move it early to draw
attention for reviewers from multiple kvm arch.
Here is the updated version.
KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module
To use TDX, the TDX module needs to be loaded and initialized. This patch
is to call a function to initialize the TDX module when loading KVM intel
kernel module.
There are several options on when to initialize the TDX module. A.)
kernel boot time as builtin, B.) kernel module loading time, C.) the first
guest TD creation time. B.) was chosen. A.) causes unnecessary overhead
(boot time and memory) even when TDX isn't used. With C.), a user may hit
an error of the TDX initialization when trying to create the first guest
TD. The machine that fails to initialize the TDX module can't boot any
guest TD further. Such failure is undesirable. B.) has a good balance
between them.
Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
while hardware is enabled, i.e. after hardware_enable_all() and before
hardware_disable_all(). Because TDX requires all present CPUs to enable
VMX (VMXON). The x86 specific kvm_arch_post_hardware_enable_setup overrides
the existing weak symbol of kvm_arch_post_hardware_enable_setup which is
called at the KVM module initialization.
--
Isaku Yamahata <[email protected]>
On Tue, 2022-07-26 at 17:39 -0700, Isaku Yamahata wrote:
> On Tue, Jul 12, 2022 at 01:13:10PM +1200,
> Kai Huang <[email protected]> wrote:
>
> > > To use TDX functionality, TDX module needs to be loaded and initialized.
> > > This patch is to call a function, tdx_init(), when loading kvm_intel.ko.
> >
> > Could you add explain why we need to init TDX module when loading KVM module?
>
> Makes sense. Added a paragraph for it.
>
>
> > > Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
> > > while hardware is enabled, i.e. after hardware_enable_all() and before
> > > hardware_disable_all(). Because TDX requires all present CPUs to enable
> > > VMX (VMXON).
> >
> > Please explicitly say it is a replacement of the default __weak version, so
> > people can know there's already a default one. Otherwise people may wonder why
> > this isn't called in this patch (i.e. I skipped patch 03 as it looks not
> > directly related to TDX).
> >
> > That being said, why cannot you send out that patch separately but have to
> > include it into TDX series?
> >
> > Looking at it, the only thing that is related to TDX is an empty
> > kvm_arch_post_hardware_enable_setup() with a comment saying TDX needs to do
> > something there. This logic has nothing to do with the actual job in that
> > patch.
> >
> > So why cannot we introduce that __weak version in this patch, so that the rest
> > of it can be non-TDX related at all and can be upstreamed separately?
>
> The patch that adds weak kvm_arch_post_hardware_enable_setup() doesn't make
> sense without the hook because it only enable_hardware and then disable hardware
> immediately.
It's not a disaster if you describe the reason to do so in the changelog, but no
strong opinion here.
But I do think you need a comment to explain why disable hardware is called
immediately. Is it because we want to maintain the current behaviour that we
want to allow out-of-tree driver, i.e. virtualbox to be loaded when KVM is
loaded?
> The patch touches multiple kvm arch. and I split out TDX specific part in this
> patch. Ideally those two patch should be near. But I move it early to draw
> attention for reviewers from multiple kvm arch.
Explicitly say this is the replacement of the default __weak version is fine.
>
> Here is the updated version.
>
> KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module
>
> To use TDX, the TDX module needs to be loaded and initialized. This patch
> is to call a function to initialize the TDX module when loading KVM intel
> kernel module.
>
> There are several options on when to initialize the TDX module. A.)
> kernel boot time as builtin, B.) kernel module loading time, C.) the first
> guest TD creation time. B.) was chosen. A.) causes unnecessary overhead
> (boot time and memory) even when TDX isn't used. With C.), a user may hit
> an error of the TDX initialization when trying to create the first guest
> TD. The machine that fails to initialize the TDX module can't boot any
> guest TD further. Such failure is undesirable. B.) has a good balance
> between them.
You don't need to mention A. When this patch is merged, the host series must
have been merged already. In another words, this is already a fact, but not an
option.
>
> Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization
> while hardware is enabled, i.e. after hardware_enable_all() and before
> hardware_disable_all().
>
You don't need to say "add a hook ..., i.e. after hardware_enable_all() and
before hardware_disable_all()". Where the function is called is already a fact.
We have a __weak version already.
> Because TDX requires all present CPUs to enable
> VMX (VMXON). The x86 specific kvm_arch_post_hardware_enable_setup overrides
> the existing weak symbol of kvm_arch_post_hardware_enable_setup which is
> called at the KVM module initialization.
>