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

[Dave Hansen]

On 11/22/22 03:28, Huang, Kai wrote:
>>> +	/*
>>> +	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
>>> +	 * KeyIDs start after the last MKTME KeyID.
>>> +	 */
>>
>> Is the TME key a "MKTME KeyID"?
> 
> I don't think so.  Hardware handles TME KeyID 0 differently from non-0 MKTME
> KeyIDs.  And PCONFIG only accept non-0 KeyIDs.

Let's say we have 4 MKTME hardware bits, we'd have:

   0: TME Key
1->3: MKTME Keys
4->7: TDX Private Keys

First, the MSR values:

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

These would be:

	Bit [ 31:0] = 3
	Bit [63:22] = 4

And in the end the variables:

	tdx_keyid_start would be 4 and tdx_keyid_num would be 4.

Right?

That's a bit wonky for my brain because I guess I know too much about
the internal implementation and how the key space is split up.  I guess
I (wrongly) expected Bit[31:0]==Bit[63:22].



>>> +static void __init clear_tdx(void)
>>> +{
>>> +	tdx_keyid_start = tdx_keyid_num = 0;
>>> +}
>>
>> This is where a comment is needed and can actually help.
>>
>> /*
>>  * tdx_keyid_start/num indicate that TDX is uninitialized.  This
>>  * is used in TDX initialization error paths to take it from
>>  * initialized -> uninitialized.
>>  */
> 
> Just want to point out after removing the !x2apic_enabled() check, the only
> thing need to do here is to detect/record the TDX KeyIDs.
> 
> And the purpose of this TDX boot-time initialization code is to provide
> platform_tdx_enabled() function so that kexec() can use.
> 
> To distinguish boot-time TDX initialization from runtime TDX module
> initialization, how about change the comment to below?
> 
> static void __init clear_tdx(void)
> {
>         /*
>          * tdx_keyid_start and nr_tdx_keyids indicate that TDX is not
>          * enabled by the BIOS.  This is used in TDX boot-time
>          * initializatiton error paths to take it from enabled to not
>          * enabled.
>          */
>         tdx_keyid_start = nr_tdx_keyids = 0;
> }
> 
> [...]

I honestly have no idea what "boot-time TDX initialization" is versus
"runtime TDX module initialization".  This doesn't hel.

> And below is the updated patch.  How does it look to you?

Let's see...

...
> +static u32 tdx_keyid_start __ro_after_init;
> +static u32 nr_tdx_keyids __ro_after_init;
> +
> +static int __init record_keyid_partitioning(void)
> +{
> +       u32 nr_mktme_keyids;
> +       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, &nr_mktme_keyids,
> +                       &nr_tdx_keyids);
> +       if (ret)
> +               return -ENODEV;
> +
> +       if (!nr_tdx_keyids)
> +               return -ENODEV;
> +
> +       /* TDX KeyIDs start after the last MKTME KeyID. */
> +       tdx_keyid_start++;

tdx_keyid_start is uniniitalized here.  So, it'd be 0, then ++'d.

Kai, please take a moment and slow down.  This isn't a race.  I offered
some replacement code here, which you've discarded, missed or ignored
and in the process broken this code.

This approach just wastes reviewer time.  It's not working for me.

I'm going to make a suggestion (aka. a demand): You can post these
patches at most once a week.  You get a whole week to (carefully)
incorporate reviewer feedback, make the patch better, and post a new
version.  Need more time?  Go ahead and take it.  Take as much time as
you want.