Message-ID: <b6f505bec9984febab84964c6422b4195938ff62.camel@intel.com>
Date:   Tue, 22 Nov 2022 23:21:41 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Luck, Tony" <tony.luck@...el.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v7 02/20] x86/virt/tdx: Detect TDX during kernel boot

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.

Powered by blists - more mailing lists