linux-kernel - RE: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module init/exit functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DS0PR11MB63735A7EEC68894923925ECEDCB99@DS0PR11MB6373.namprd11.prod.outlook.com>
Date:   Mon, 13 Mar 2023 14:49:03 +0000
From:   "Wang, Wei W" <wei.w.wang@...el.com>
To:     "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        David Matlack <dmatlack@...gle.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        Zhi Wang <zhi.wang.linux@...il.com>
Subject: RE: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module
 init/exit functions

On Monday, March 13, 2023 1:55 AM, isaku.yamahata@...el.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@...el.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.

diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 41f07d92aaf6..37a8c212c653 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -1126,18 +1126,6 @@ static int __init vt_init(void)
         */
        hv_init_evmcs();

-       r = kvm_x86_vendor_init(&vt_init_ops);
-       if (r)
-               return r;
-
-       r = vmx_init();
-       if (r)
-               goto err_vmx_init;
-
-       r = tdx_init();
-       if (r)
-               goto err_tdx_init;
-
        /*
         * Common KVM initialization _must_ come last, after this, /dev/kvm is
         * exposed to userspace!
@@ -1153,6 +1141,19 @@ static int __init vt_init(void)
                vcpu_align = max_t(unsigned int, vcpu_align,
                                   __alignof__(struct vcpu_tdx));
        }
+
+       r = kvm_x86_vendor_init(&vt_init_ops);
+       if (r)
+               return r;
+
+       r = vmx_init();
+       if (r)
+               goto err_vmx_init;
+
+       r = tdx_init();
+       if (r)
+               goto err_tdx_init;
+
        r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
        if (r)
                goto err_kvm_init;
--