Re: [PATCH v7 02/20] x86/virt/tdx: Detect TDX during kernel boot

From: "Huang, Kai" <kai.huang@intel.com>
To: "sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "Hansen, Dave" <dave.hansen@intel.com>,
	"Luck, Tony" <tony.luck@intel.com>,
	"bagasdotme@gmail.com" <bagasdotme@gmail.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v7 02/20] x86/virt/tdx: Detect TDX during kernel boot
Date: Mon, 21 Nov 2022 09:37:14 +0000	[thread overview]
Message-ID: <827adb3d1610f43ce71b1491db8c680dbe095035.camel@intel.com> (raw)
In-Reply-To: <3e4644e4-d939-7f15-02e1-a6be1ed7f932@linux.intel.com>

> > +static u32 tdx_keyid_start __ro_after_init;
> > +static u32 tdx_keyid_num __ro_after_init;
> > +
> > +/*
> > + * Detect TDX private KeyIDs to see whether TDX has been enabled by the
> > + * BIOS.  Both initializing the TDX module and running TDX guest require
> > + * TDX private KeyID.
> > + *
> > + * TDX doesn't trust BIOS.  TDX verifies all configurations from BIOS
> > + * are correct before enabling TDX on any core.  TDX requires the BIOS
> > + * to correctly and consistently program TDX private KeyIDs on all CPU
> > + * packages.  Unless there is a BIOS bug, detecting a valid TDX private
> > + * KeyID range on BSP indicates TDX has been enabled by the BIOS.  If
> > + * there's such BIOS bug, it will be caught later when initializing the
> > + * TDX module.
> > + */
> > +static int __init detect_tdx(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * IA32_MKTME_KEYID_PARTIONING:
> > +	 *   Bit [31:0]:	Number of MKTME KeyIDs.
> > +	 *   Bit [63:32]:	Number of TDX private KeyIDs.
> > +	 */
> > +	ret = rdmsr_safe(MSR_IA32_MKTME_KEYID_PARTITIONING, &tdx_keyid_start,
> > +			&tdx_keyid_num);
> > +	if (ret)
> > +		return -ENODEV;
> > +
> > +	if (!tdx_keyid_num)
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
> > +	 * KeyIDs start after the last MKTME KeyID.
> > +	 */
> > +	tdx_keyid_start++;
> 
> It is not very clear why you increment tdx_keyid_start. 
> 

Please see above comment around rdmsr_safe():

	/*
	 * IA32_MKTME_KEYID_PARTIONING:
	 *   Bit [31:0]:	Number of MKTME KeyIDs.
	 *   Bit [63:32]:	Number of TDX private KeyIDs.
	 */

And the comment right above 'tdx_keyid_start++':

	/*
	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
	 * KeyIDs start after the last MKTME KeyID.
	 */

Do the above two comments answer the question?

Or I can be more explicit, such as below?

"
Now tdx_start_keyid is the last MKTME KeyID.  TDX private KeyIDs start after the
last MKTME KeyID.  Increase tdx_start_keyid by 1 to set it to the first TDX
private KeyID.
"

> What we read from
> MSR_IA32_MKTME_KEYID_PARTITIONING is not the correct start keyid?

TDX verifies TDX private KeyID range is configured consistently across all
packages.  Any wrong KeyID range means BIOS bug, and such bug will cause TDX
being not enabled -- later TDX module initialization will catch this.

> 
> Also why is this global variable? At least in this patch, there seems to
> be no use case.

Platform_tdx_enabled() uses tdx_keyid_num to determine whether TDX is enabled by
BIOS.

Also, in the changlog I can add "both initializing the TDX module and creating
TDX guest will need to use TDX private KeyID".

But I also have a comment saying something similar around ...

> 
> > +	/*
> > +	 * Initializing the TDX module requires one TDX private KeyID.
> > +	 * If there's only one TDX KeyID then after module initialization
> > +	 * KVM won't be able to run any TDX guest, which makes the whole
> > +	 * thing worthless.  Just disable TDX in this case.
> > +	 */
> > +	if (tdx_keyid_num < 2) {
> > +		pr_info("Disable TDX as there's only one TDX private KeyID available.\n");
> > +		goto no_tdx;
> > +	}
> > +

... here.