Received-SPF: pass (google.com: domain of linux-kernel+bounces-147234-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 16 Apr 2024 09:28:29 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: Isaku Yamahata <isaku.yamahata@linux.intel.com>,
	Yan Zhao <yan.y.zhao@intel.com>, 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>, chen.bo@intel.com,
	hang.yuan@intel.com, tina.zhang@intel.com,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Xiaoyao Li <xiaoyao.li@intel.com>
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural
 definitions
Message-ID: <20240416162829.GX3039520@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <522cbfe6e5a351f88480790fe3c3be36c82ca4b1.1708933498.git.isaku.yamahata@intel.com>
 <ZeGC64sAzg4EN3G5@yzhao56-desk.sh.intel.com>
 <20240305082138.GD10568@ls.amr.corp.intel.com>
 <34d64c12-9ed5-4c63-8465-29f7fdce20dc@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <34d64c12-9ed5-4c63-8465-29f7fdce20dc@intel.com>

On Tue, Apr 16, 2024 at 12:55:33PM +1200,
"Huang, Kai" <kai.huang@intel.com> wrote:

> 
> 
> On 5/03/2024 9:21 pm, Isaku Yamahata wrote:
> > On Fri, Mar 01, 2024 at 03:25:31PM +0800,
> > Yan Zhao <yan.y.zhao@intel.com> wrote:
> > 
> > > > + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B.
> > > > + */
> > > > +#define TDX_MAX_VCPUS	(~(u16)0)
> > > This value will be treated as -1 in tdx_vm_init(),
> > > 	"kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);"
> > > 
> > > This will lead to kvm->max_vcpus being -1 by default.
> > > Is this by design or just an error?
> > > If it's by design, why not set kvm->max_vcpus = -1 in tdx_vm_init() directly.
> > > If an unexpected error, may below is better?
> > > 
> > > #define TDX_MAX_VCPUS   (int)((u16)(~0UL))
> > > or
> > > #define TDX_MAX_VCPUS 65536
> > 
> > You're right. I'll use ((int)U16_MAX).
> > As TDX 1.5 introduced metadata MAX_VCPUS_PER_TD, I'll update to get the value
> > and trim it further. Something following.
> > 
> 
> [...]
> 
> > +	u16 max_vcpus_per_td;
> > +
> 
> [...]
> 
> > -	kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);
> > +	kvm->max_vcpus = min3(kvm->max_vcpus, tdx_info->max_vcpus_per_td,
> > +			     TDX_MAX_VCPUS);
> 
> [...]
> 
> > -#define TDX_MAX_VCPUS	(~(u16)0)
> > +#define TDX_MAX_VCPUS	((int)U16_MAX)
> 
> Why do you even need TDX_MAX_VCPUS, given it cannot exceed U16_MAX and you
> will have the 'u16 max_vcpus_per_td' anyway?
> 
> IIUC, in KVM_ENABLE_CAP(KVM_CAP_MAX_VCPUS), we can overwrite the
> kvm->max_vcpus to the 'max_vcpus' provided by the userspace, and make sure
> it doesn't exceed tdx_info->max_vcpus_per_td.
> 
> Anything I am missing?

With the latest TDX 1.5 module, we don't need TDX_MAX_VCPUS.

The metadata MD_FIELD_ID_MAX_VCPUS_PER_TD was introduced at the middle version
of TDX 1.5. (I don't remember the exact version.), the logic was something
like as follows.  Now if we fail to read the metadata, disable TDX.

read metadata MD_FIELD_ID_MAX_VCPUS_PER_TD;
if success
  tdx_info->max_vcpu_per_td = the value read metadata
else
  tdx_info->max_vcpu_per_td = TDX_MAX_VCPUS;

-- 
Isaku Yamahata <isaku.yamahata@intel.com>