Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

[Edgecombe, Rick P]

On Fri, 2024-08-30 at 11:45 -0700, Dave Hansen wrote:
> On 8/12/24 15:48, Rick Edgecombe wrote:
> > Each TDX guest has a root control structure called "Trust Domain
> > Root" (TDR).  Unlike the rest of the TDX guest, the TDR is protected
> > by the TDX global KeyID.  When tearing down the TDR, KVM will need to
> > pass the TDX global KeyID explicitly to the TDX module to flush cache
> > associated to the TDR.
> 
> What does that end up looking like?

The global key id callers looks like:
	err = tdh_phymem_page_wbinvd(set_hkid_to_hpa(kvm_tdx->tdr_pa,
						     tdx_global_keyid));
	if (KVM_BUG_ON(err, kvm)) {
		pr_tdx_error(TDH_PHYMEM_PAGE_WBINVD, err);
		return;
	}

The TD keyid callers looks like:
	hpa_with_hkid = set_hkid_to_hpa(hpa, (u16)kvm_tdx->hkid);
	do {
		/*
		 * TDX_OPERAND_BUSY can happen on locking PAMT entry.  Because
		 * this page was removed above, other thread shouldn't be
		 * repeatedly operating on this page.  Just retry loop.
		 */
		err = tdh_phymem_page_wbinvd(hpa_with_hkid);
	} while (unlikely(err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX)));

> 
> In other words, should we export the global KeyID, or export a function
> to do the flush and then never actually expose the KeyID?

We could split it into two helpers if we wanted to remove the export of
tdx_global_keyid. One for global key id and one that only takes TD range key
ids. Adding more layers is a downside.

Separate from Dave's question, I wonder if we should open code set_hkid_to_hpa()
inside tdh_phymem_page_wbinvd(). The signature could change to
tdh_phymem_page_wbinvd(hpa_t pa, u16 hkid). set_hkid_to_hpa() is very
lightweight, so I don't think doing it outside the loop is much gain. It makes
the code cleaner.

> 
> > -static u32 tdx_global_keyid __ro_after_init;
> > -static u32 tdx_guest_keyid_start __ro_after_init;
> > -static u32 tdx_nr_guest_keyids __ro_after_init;
> > +u32 tdx_global_keyid __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_global_keyid);
> > +
> > +u32 tdx_guest_keyid_start __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_guest_keyid_start);
> > +
> > +u32 tdx_nr_guest_keyids __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_nr_guest_keyids);
> 
> I know the KVM folks aren't maniacs that will start writing to these or
> anything.

Yea. ro_after_init would stop most mischief as well.

> 
> But, in general, just exporting global variables isn't super nice.  If
> these are being used to set up the key allocator, I'd kinda just rather
> that the allocator be in core code and have its alloc/free functions
> exported.
> 

Makes sense. We could remove tdx_guest_keyid_start/tdx_nr_guest_keyids then in
any case. But if we want to remove tdx_global_keyid too, we could add a
tdh_phymem_page_wbinvd_global_keyid(void). I'm split on that one, but I'd lean
towards doing it.