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.
