On Tue, 2022-11-22 at 08:50 -0800, Dave Hansen wrote:
> 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?
Yes.
>
> 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].
The spec says the The Bit[31:0] only reports the number of MKTME KeyIDs, and it
does exclude KeyID 0.
My machine has 6 hardware bits in total (that is KeyID 0 ~ 63), and the upper 48
KeyIDs are reserved to TDX. In my case:
[Bit 31:0] = 15
[Bit 63:32] = 48
And tdx_keyid_start and nr_tdx_keyids are 16 and 48.
The TDX KeyID range: [16, 63], or [16, 64).
So [Bit 31:0] reports only "NUM_MKTME_KIDS", which excludes KeyID 0.
>
>
>
> > > > +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.
I'll use your original comment.
>
> > 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.
Apology. I missed it this time.
>
> 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.
>
Yes will follow.
