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

Commit Message

Rick Edgecombe Aug. 12, 2024, 10:48 p.m. UTC

From: Kai Huang <[e-mail]>

Each TDX guest must be protected by its own unique TDX KeyID.  KVM will
need to tell the TDX module the unique KeyID for a TDX guest when KVM
creates it.

Export the TDX KeyID range that can be used by TDX guests for KVM to
use.  KVM can then manage these KeyIDs and assign one for each TDX guest
when it is created.

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.

Also export the TDX global KeyID for KVM to tear down the TDR.

Signed-off-by: Kai Huang <[e-mail]>
Signed-off-by: Rick Edgecombe <[e-mail]>
---
uAPI breakout v1:
 - New patch
---
 arch/x86/include/asm/tdx.h  |  4 ++++
 arch/x86/virt/vmx/tdx/tdx.c | 11 ++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

Dave Hansen Aug. 30, 2024, 6:45 p.m. UTC | #1

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?

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

> -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.

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.

Rick Edgecombe Aug. 30, 2024, 7:16 p.m. UTC | #2

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.

Dave Hansen Aug. 30, 2024, 9:18 p.m. UTC | #3

On 8/30/24 12:16, Edgecombe, Rick P wrote:
...>> 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.

I do like the idea of exporting a couple of helpers that are quite hard
to misuse instead of exporting the variable.

> 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.

Yeah, do what's cleanest.  This is all super cold code.

Paolo Bonzini Sept. 10, 2024, 4:26 p.m. UTC | #4

On 8/30/24 23:18, Dave Hansen wrote:
> I do like the idea of exporting a couple of helpers that are quite hard
> to misuse instead of exporting the variable.

Yeah, that's nicer.

Paolo

>> 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.
> Yeah, do what's cleanest.  This is all super cold code.

Patch

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 56c3a5512c22..8e0eef4f74f5 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -176,6 +176,10 @@  struct tdx_sysinfo {
 
 const struct tdx_sysinfo *tdx_get_sysinfo(void);
 
+extern u32 tdx_global_keyid;
+extern u32 tdx_guest_keyid_start;
+extern u32 tdx_nr_guest_keyids;
+
 u64 __seamcall(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 478d894f46a2..96640cfb1830 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -39,9 +39,14 @@ 
 #include <asm/mce.h>
 #include "tdx.h"
 
-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);
 
 static DEFINE_PER_CPU(bool, tdx_lp_initialized);