linux-kernel - Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural definitions

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]

Date: Tue, 16 Apr 2024 09:28:29 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: Isaku Yamahata <isaku.yamahata@...ux.intel.com>,
	Yan Zhao <yan.y.zhao@...el.com>, isaku.yamahata@...el.com,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
	erdemaktas@...gle.com, Sean Christopherson <seanjc@...gle.com>,
	Sagi Shahar <sagis@...gle.com>, chen.bo@...el.com,
	hang.yuan@...el.com, tina.zhang@...el.com,
	Sean Christopherson <sean.j.christopherson@...el.com>,
	Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural
 definitions

On Tue, Apr 16, 2024 at 12:55:33PM +1200,
"Huang, Kai" <kai.huang@...el.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@...el.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@...el.com>