Message-ID: <20240313174434.GM935089@ls.amr.corp.intel.com>
Date: Wed, 13 Mar 2024 10:44:34 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"Zhang, Tina" <tina.zhang@...el.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>,
	"Yuan, Hang" <hang.yuan@...el.com>,
	"Huang, Kai" <kai.huang@...el.com>, "Chen, Bo2" <chen.bo@...el.com>,
	"sagis@...gle.com" <sagis@...gle.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>,
	isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 032/130] KVM: TDX: Add helper functions to
 allocate/free TDX private host key id

On Wed, Mar 13, 2024 at 12:44:14AM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@...el.com> wrote:

> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@...el.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@...el.com>
> > 
> > Add helper functions to allocate/free TDX private host key id (HKID).
> > 
> > The memory controller encrypts TDX memory with the assigned TDX
> > HKIDs.  The
> > global TDX HKID is to encrypt the TDX module, its memory, and some
> > dynamic
> > data (TDR). 
> 
> I don't see any code about the global key id.
> 
> >  The private TDX HKID is assigned to guest TD to encrypt guest
> > memory and the related data.  When VMM releases an encrypted page for
> > reuse, the page needs a cache flush with the used HKID.
> 
> Not sure the cache part is pertinent to this patch. Sounds good for
> some other patch.
> 
> >   VMM needs the
> > global TDX HKID and the private TDX HKIDs to flush encrypted pages.
> 
> I think the commit log could have a bit more about what code is added.
> What about adding something like this (some verbiage from Kai's setup
> patch):
> 
> The memory controller encrypts TDX memory with the assigned TDX
> HKIDs. Each TDX guest must be protected by its own unique TDX HKID.
> 
> The HW has a fixed set of these HKID keys. Out of those, some are set
> aside for use by for other TDX components, but most are saved for guest
> use. The code that does this partitioning, records the range chosen to
> be available for guest use in the tdx_guest_keyid_start and
> tdx_nr_guest_keyids variables.
> 
> Use this range of HKIDs reserved for guest use with the kernel's IDA
> allocator library helper to create a mini TDX HKID allocator that can
> be called when setting up a TD. This way it can have an exclusive HKID,
> as is required. This allocator will be used in future changes.
> 
> 
> > 
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > ---
> > v19:
> > - Removed stale comment in tdx_guest_keyid_alloc() by Binbin
> > - Update sanity check in tdx_guest_keyid_free() by Binbin
> > 
> > v18:
> > - Moved the functions to kvm tdx from arch/x86/virt/vmx/tdx/
> > - Drop exporting symbols as the host tdx does.
> > 
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > ---
> >  arch/x86/kvm/vmx/tdx.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index a7e096fd8361..cde971122c1e 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -11,6 +11,34 @@
> >  #undef pr_fmt
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> > +/*
> > + * Key id globally used by TDX module: TDX module maps TDR with this
> > TDX global
> > + * key id.  TDR includes key id assigned to the TD.  Then TDX module
> > maps other
> > + * TD-related pages with the assigned key id.  TDR requires this TDX
> > global key
> > + * id for cache flush unlike other TD-related pages.
> > + */
> 
> The above comment is about tdx_global_keyid, which is unrelated to the
> patch and code.

Will delete this comment as it was moved into the host tdx patch series.

> 
> > +/* TDX KeyID pool */
> > +static DEFINE_IDA(tdx_guest_keyid_pool);
> > +
> > +static int __used tdx_guest_keyid_alloc(void)
> > +{
> > +       if (WARN_ON_ONCE(!tdx_guest_keyid_start ||
> > !tdx_nr_guest_keyids))
> > +               return -EINVAL;
> 
> I think the idea of this warnings is to check if TDX failed to init? It
> could check X86_FEATURE_TDX_HOST_PLATFORM or enable_tdx, but that seems
> to be a weird thing to check in a low level function that is called in
> the middle of in progress setup.
> 
> Don't know, I'd probably drop this warning.
> 
> > +
> > +       return ida_alloc_range(&tdx_guest_keyid_pool,
> > tdx_guest_keyid_start,
> > +                              tdx_guest_keyid_start +
> > tdx_nr_guest_keyids - 1,
> > +                              GFP_KERNEL);
> > +}
> > +
> > +static void __used tdx_guest_keyid_free(int keyid)
> > +{
> > +       if (WARN_ON_ONCE(keyid < tdx_guest_keyid_start ||
> > +                        keyid > tdx_guest_keyid_start +
> > tdx_nr_guest_keyids - 1))
> > +               return;
> 
> This seems like a more useful warning, but still not sure it's that
> risky. I guess the point is to check for returning garbage. Because a
> double free would not be caught, but would be possible to using
> idr_find(). I would think if we are worried we should do the full
> check, but I'm not sure we can't just drop this. There are very limited
> callers or things that change the checked configuration (1 of each).

The related code is stable now and I don't hit them recently.  I'll drop both
of them.
-- 
Isaku Yamahata <isaku.yamahata@...el.com>

Powered by blists - more mailing lists