Re: [PATCH v7 011/102] KVM: TDX: Initialize TDX module when loading kvm_intel.ko

Kai Huang
2022-07-27T04:38:37Z

On Tue, 2022-07-26 at 17:39 -0700, Isaku Yamahata wrote:
> On Tue, Jul 12, 2022 at 01:13:10PM +1200,
> Kai Huang <kai.huang@intel.com> 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.
>