Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 26 Jul 2022 17:39:38 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     Kai Huang <kai.huang@intel.com>
Cc:     Isaku Yamahata <isaku.yamahata@gmail.com>,
        isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>
Subject: Re: [PATCH v7 011/102] KVM: TDX: Initialize TDX module when loading
 kvm_intel.ko
Message-ID: <20220727003938.GG1379820@ls.amr.corp.intel.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
 <d933e5f16ff8cb58020f1479b7af35196f0ef61e.1656366338.git.isaku.yamahata@intel.com>
 <81ea5068b890400ca4064781f7d2221826701020.camel@intel.com>
 <20220712004640.GD1379820@ls.amr.corp.intel.com>
 <d495a777f31df86271f1c4511b2f521adfa867d1.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <d495a777f31df86271f1c4511b2f521adfa867d1.camel@intel.com>
Precedence: bulk

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.
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 <isaku.yamahata@gmail.com>