Re: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module init/exit functions

[Isaku Yamahata <isaku.yamahata@gmail.com>]

On Mon, Mar 13, 2023 at 02:49:03PM +0000,
"Wang, Wei W" <wei.w.wang@intel.com> wrote:

> On Monday, March 13, 2023 1:55 AM, isaku.yamahata@intel.com wrote:
> > 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,
> > 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
> > 
> > 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().
> > 
> > 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 TDX specific kvm
> > and vcpu structures.
> > 
> > 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.
> > 
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  arch/x86/kvm/vmx/main.c    | 51 +++++++++++++++++++++++++++++++++++
> >  arch/x86/kvm/vmx/vmx.c     | 54 +++++---------------------------------
> >  arch/x86/kvm/vmx/x86_ops.h | 13 ++++++++-
> >  3 files changed, 69 insertions(+), 49 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c index
> > a59559ff140e..3f49e8e38b6b 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -165,3 +165,54 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
> >  	.runtime_ops = &vt_x86_ops,
> >  	.pmu_ops = &intel_pmu_ops,
> >  };
> > +
> > +static int __init vt_init(void)
> > +{
> > +	unsigned int vcpu_size, vcpu_align;
> > +	int r;
> > +
> > +	if (!kvm_is_vmx_supported())
> > +		return -EOPNOTSUPP;
> > +
> > +	/*
> > +	 * Note, hv_init_evmcs() touches only VMX knobs, i.e. there's nothing
> > +	 * to unwind if a later step fails.
> > +	 */
> > +	hv_init_evmcs();
> > +
> > +	r = kvm_x86_vendor_init(&vt_init_ops);
> > +	if (r)
> > +		return r;
> > +
> > +	r = vmx_init();
> > +	if (r)
> > +		goto err_vmx_init;
> > +
> > +	/*
> > +	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
> > +	 * exposed to userspace!
> > +	 */
> > +	vt_x86_ops.vm_size = sizeof(struct kvm_vmx);
> > +	vcpu_size = sizeof(struct vcpu_vmx);
> > +	vcpu_align = __alignof__(struct vcpu_vmx);
> > +	r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
> > +	if (r)
> > +		goto err_kvm_init;
> > +
> > +	return 0;
> > +
> > +err_kvm_init:
> > +	vmx_exit();
> > +err_vmx_init:
> > +	kvm_x86_vendor_exit();
> > +	return r;
> > +}
> > +module_init(vt_init);
> 
> I had a patch to fix a bug here, maybe you can take it:
> 
> kvm_x86_vendor_init copies vt_x86_ops to kvm_x86_ops. vt_x86_ops.vm_size
> needs to be updated before calling kvm_x86_vendor_init so that kvm_x86_ops
> can get the correct vm_size.

Thanks for catching it.  With your patch, vm_size is always
max(sizeof struct kvm_vmx, sizeof strut kvm_tdx) even when the admin sets
kvm_intel.tdx=true and tdx is disabled by error.

option 1: Ignore such waste. Your patch. The difference is small and it's only
          the error case. Locally I have the following values.
          sizeof(struct kvm_vmx) = 44576
          sizeof(struct vcpu_vmx) = 10432
          sizeof(struct kvm_tdx)= 44632
          sizeof(struct vcpu_tdx) = 8192
          I suspect the actual allocation size for struct kvm is same.  That's
          the reason why I didn't hit problem.

option 2: Explicitly update vm_size after kvm_x86_vendor_init()
          struct kvm_x86_ops isn't exported.  It would be ugly.

option 3: Allow setup_hardware() to update vm_size.
          setup_hardware(void) => setup_hardware(unsigned int *vm_size)
          It's confusing because kvm_x86_ops.vm_size is already initialized.

Let's go with option 1(your patch).
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>