On Wed, Mar 13, 2024 at 12:44:14AM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@xxxxxxxxx> wrote:
> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@xxxxxxxxx wrote:
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
