Re: [PATCH v8 12/16] x86/virt/tdx: Designate the global KeyID and configure the TDX module

From: "Huang, Kai" <kai.huang@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "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>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v8 12/16] x86/virt/tdx: Designate the global KeyID and configure the TDX module
Date: Tue, 10 Jan 2023 23:33:47 +0000	[thread overview]
Message-ID: <c4e5eadae12c72e7b6d97c1802773694ee04ba86.camel@intel.com> (raw)
In-Reply-To: <755f94aa-a0cc-b7d6-ce8a-a81ff4f598da@intel.com>

On Tue, 2023-01-10 at 08:25 -0800, Dave Hansen wrote:
> On 1/10/23 02:48, Huang, Kai wrote:
> > > > 
> > > > +   /*
> > > > +    * Use the first private KeyID as the global KeyID, and pass
> > > > +    * it along with the TDMRs to the TDX module.
> > > > +    */
> > > > +   ret = config_tdx_module(&tdmr_list, tdx_keyid_start);
> > > > +   if (ret)
> > > > +           goto out_free_pamts;
> > > This is "consuming" tdx_keyid_start.  Does it need to get incremented
> > > since the first guest can't use this KeyID now?
> > 
> > It depends on how we treat 'tdx_keyid_start'.  If it means the first _usable_
> > KeyID for KVM, then we should increase it; but if it only used for the hardware-
> > enabled TDX KeyID range, then we don't need to increase it.
> > 
> > Currently it is marked as __ro_after_init so my intention is the latter (also in
> > the spirit of keeping this series minimal).
> > 
> > Eventually we will need to have functions to allocate/free TDX KeyIDs anyway for
> > KVM, but in that we can just treat 'tdx_keyid_start + 1' as the first usable
> > KeyID.
> 
> So, basically, you're going to depend on the KVM code (which isn't in
> this series) to magically know exactly what this series did?  Then,
> you're expecting that this code will never change in a way that breaks
> this random KVM code?
> 
> That's frankly awful.

Sorry I should have said this in my previous reply:  The two functions will be
implemented in here together with 'tdx_keyid_start' and 'nr_tdx_keyids'
variables, so they are not KVM code, although they will only be used by KVM for
now:

https://lore.kernel.org/lkml/Y19NzlQcwhV%2F2wl3@debian.me/T/#m0735de9e60138da8fa69828b755f1387e031d08d

Another benefit of putting here (not in KVM) is just in case in the future other
kernel components might need to allocate TDX KeyID too.

Btw this series itself is not enough for KVM to support TDX.  There will be some
minor x86 patches based on this series for that (exposing tdsysinfo_struct to
KVM, and this KeyID allocation, etc). 

(Or should I just include them in this series?)

> 
> Make the variable read/write.  Call it tdx_guest_keyid_start, and
> increment it when you make a keyid unavailable for guest use.
> 

Yes I can do if you prefer.

One minor thing is 'tdx_keyid_start' is introduced in the second patch in this
series ("x86/virt/tdx: Detect TDX during kernel boot").  IMHO it would be a
little weird to call it 'tdx_guest_keyid_start' in that patch.