Received-SPF: pass (google.com: domain of linux-kernel+bounces-111870-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 22 Mar 2024 10:39:06 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"Zhang, Tina" <tina.zhang@intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"Yuan, Hang" <hang.yuan@intel.com>, "Chen, Bo2" <chen.bo@intel.com>,
	"sagis@google.com" <sagis@google.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 022/130] KVM: x86/vmx: Refactor KVM VMX module
 init/exit functions
Message-ID: <20240322173906.GY1994522@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <11d5ae6a1102a50b0e773fc7efd949bb0bd2b776.1708933498.git.isaku.yamahata@intel.com>
 <0f466c5845e9d75b25392ecb5129c4e984052c1b.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <0f466c5845e9d75b25392ecb5129c4e984052c1b.camel@intel.com>

On Thu, Mar 21, 2024 at 11:27:46AM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > Currently, KVM VMX module initialization/exit functions are a single
> > function each.  Refactor KVM VMX module initialization functions into KVM
> > common part and VMX part so that TDX specific part can be added cleanly.
> > Opportunistically refactor module exit function as well.
> > 
> > The current module initialization flow is,
> 
> 					  ^ ',' -> ':'
> 
> And please add an empty line to make text more breathable.
> 
> > 0.) Check if VMX is supported,
> > 1.) hyper-v specific initialization,
> > 2.) system-wide x86 specific and vendor specific initialization,
> > 3.) Final VMX specific system-wide initialization,
> > 4.) calculate the sizes of VMX kvm structure and VMX vcpu structure,
> > 5.) report those sizes to the KVM common layer and KVM common
> >     initialization
> 
> Is there any difference between "KVM common layer" and "KVM common
> initialization"?  I think you can remove the former.

Ok.

> > Refactor the KVM VMX module initialization function into functions with a
> > wrapper function to separate VMX logic in vmx.c from a file, main.c, common
> > among VMX and TDX.  Introduce a wrapper function for vmx_init().
> 
> Sorry I don't quite follow what your are trying to say in the above paragraph.
> 
> You have adequately put what is the _current_ flow, and I am expecting to see
> the flow _after_ the refactor here.

Will add it.


> > The KVM architecture common layer allocates struct kvm with reported size
> > for architecture-specific code.  The KVM VMX module defines its structure
> > as struct vmx_kvm { struct kvm; VMX specific members;} and uses it as
> > struct vmx kvm.  Similar for vcpu structure. TDX KVM patches will define
> 
> 	 ^vmx_kvm.
> 
> Please be more consistent on the words.
> 
> > TDX specific kvm and vcpu structures.
> 
> Is this paragraph related to the changes in this patch?
> 
> For instance, why do you need to point out we will have TDX-specific 'kvm and
> vcpu' structures?

The point of this refactoring is to make room for TDX-specific code.  The
consideration point is data size/alignment difference and VMX-dependency.
Let me re-order the sentences.


> > The current module exit function is also a single function, a combination
> > of VMX specific logic and common KVM logic.  Refactor it into VMX specific
> > logic and KVM common logic.  
> > 
> 
> [...]
> 
> > This is just refactoring to keep the VMX
> > specific logic in vmx.c from main.c.
> 
> It's better to make this as a separate paragraph, because it is a summary to
> this patch.
> 
> And in other words: No functional change intended?

Thanks for the feedback.  Here is the revised version.

KVM: x86/vmx: Refactor KVM VMX module init/exit functions

Split KVM VMX kernel module initialization into one specific to VMX in
vmx.c and one common for VMX and TDX in main.c to make room for
TDX-specific logic to fit in.  Opportunistically, refactor module exit
function as well.

The key points are data structure difference and TDX dependency on
VMX.  The data structures for TDX are different from VMX.  So are its
size and alignment.  Because TDX depends on VMX, TDX initialization
must be after VMX initialization.  TDX cleanup must be before VMX
cleanup.

The current module initialization flow is:

0.) Check if VMX is supported,
1.) Hyper-v specific initialization,
2.) System-wide x86 specific and vendor-specific initialization,
3.) Final VMX-specific system-wide initialization,
4.) Calculate the sizes of the kvm and vcpu structure for VMX,
5.) Report those sizes to the KVM-common initialization

After refactoring and TDX, the flow will be:

0.) Check if VMX is supported (main.c),
1.) Hyper-v specific initialization (main.c),
2.) System-wide x86 specific and vendor-specific initialization,
    2.1) VMX-specific initialization (vmx.c)
    2.2) TDX-specific initialization (tdx.c)
3.) Final VMX-specific system-wide initialization (vmx.c),
    TDX doesn't need this step.
4.) Calculate the sizes of the kvm and vcpu structure for both VMX and
    TDX, (main.c)
5.) Report those sizes to the KVM-common initialization (main.c)

No functional change intended.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>