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

[Kai Huang]

On Mon, 2022-07-11 at 17:46 -0700, Isaku Yamahata wrote:
> On Tue, Jun 28, 2022 at 04:31:35PM +1200,
> Kai Huang <kai.huang@xxxxxxxxx> wrote:
> 
> > On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@xxxxxxxxx wrote:
> > > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > > 
> > > 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.