Received-SPF: pass (google.com: domain of linux-kernel+bounces-103583-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 14 Mar 2024 09:37:53 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Binbin Wu <binbin.wu@linux.intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
	chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 024/130] KVM: TDX: Add placeholders for TDX VM/vcpu
 structure
Message-ID: <20240314163753.GP935089@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <c857863a346e692837b0c35da8a0e03c45311496.1708933498.git.isaku.yamahata@intel.com>
 <1ae483bc-b279-44ca-b396-04aa480e3781@linux.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: <1ae483bc-b279-44ca-b396-04aa480e3781@linux.intel.com>

On Thu, Mar 14, 2024 at 02:21:04PM +0800,
Binbin Wu <binbin.wu@linux.intel.com> wrote:

> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 18aef6e23aab..e11edbd19e7c 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -5,6 +5,7 @@
> >   #include "vmx.h"
> >   #include "nested.h"
> >   #include "pmu.h"
> > +#include "tdx.h"
> >   static bool enable_tdx __ro_after_init;
> >   module_param_named(tdx, enable_tdx, bool, 0444);
> > @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void)
> >   		return ret;
> >   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> > +	if (enable_tdx)
> > +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> > +					   sizeof(struct kvm_tdx));
> >   	return 0;
> >   }
> > @@ -215,8 +219,18 @@ static int __init vt_init(void)
> >   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
> >   	 * exposed to userspace!
> >   	 */
> > +	/*
> > +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> > +	 * be set before kvm_x86_vendor_init().
> 
> The comment is not right?
> In this patch, vt_x86_ops.vm_size is set in  vt_hardware_setup(),
> which is called in kvm_x86_vendor_init().
> 
> Since kvm_x86_ops is updated by kvm_ops_update() with the fields of
> vt_x86_ops. I guess you wanted to say vt_x86_ops.vm_size must be set
> before kvm_ops_update()?

Correct. Here's an updated version.

       /*
        * vt_hardware_setup() updates vt_x86_ops.  Because kvm_ops_update()
        * copies vt_x86_ops to kvm_x86_op, vt_x86_ops must be updated before
        * kvm_ops_update() called by kvm_x86_vendor_init().
        */
-- 
Isaku Yamahata <isaku.yamahata@intel.com>