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

[Kai Huang]

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@xxxxxxxxx> 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.
>