diff mbox series

[07/13] x86/virt/tdx: Add SEAMCALL wrapper tdh_mem_sept_add() to add SEPT pages

Message ID 20250101074959.412696-8-pbonzini@redhat.com (mailing list archive)
State New
Headers show
Series x86/virt/tdx: Add SEAMCALL wrappers for KVM | expand

Commit Message

Paolo Bonzini Jan. 1, 2025, 7:49 a.m. UTC 
From: Isaku Yamahata <isaku.yamahata@intel.com>

TDX architecture introduces the concept of private GPA vs shared GPA,
depending on the GPA.SHARED bit. The TDX module maintains a Secure EPT
(S-EPT or SEPT) tree per TD for private GPA to HPA translation. Wrap the
TDH.MEM.SEPT.ADD SEAMCALL with tdh_mem_sept_add() to provide pages to the
TDX module for building a TD's SEPT tree. (Refer to these pages as SEPT
pages).

Callers need to allocate and provide a normal page to tdh_mem_sept_add(),
which then passes the page to the TDX module via the SEAMCALL
TDH.MEM.SEPT.ADD. The TDX module then installs the page into SEPT tree and
encrypts this SEPT page with the TD's guest keyID. The kernel cannot use
the SEPT page until after reclaiming it via TDH.MEM.SEPT.REMOVE or
TDH.PHYMEM.PAGE.RECLAIM.

Before passing the page to the TDX module, tdh_mem_sept_add() performs a
CLFLUSH on the page mapped with keyID 0 to ensure that any dirty cache
lines don't write back later and clobber TD memory or control structures.
Don't worry about the other MK-TME keyIDs because the kernel doesn't use
them. The TDX docs specify that this flush is not needed unless the TDX
module exposes the CLFLUSH_BEFORE_ALLOC feature bit. Do the CLFLUSH
unconditionally for two reasons: make the solution simpler by having a
single path that can handle both !CLFLUSH_BEFORE_ALLOC and
CLFLUSH_BEFORE_ALLOC cases. Avoid wading into any correctness uncertainty
by going with a conservative solution to start.

Callers should specify "GPA" and "level" for the TDX module to install the
SEPT page at the specified position in the SEPT. Do not include the root
page level in "level" since TDH.MEM.SEPT.ADD can only add non-root pages to
the SEPT. Ensure "level" is between 1 and 3 for a 4-level SEPT or between 1
and 4 for a 5-level SEPT.

Call tdh_mem_sept_add() during the TD's build time or during the TD's
runtime. Check for errors from the function return value and retrieve
extended error info from the function output parameters.

The TDX module has many internal locks. To avoid staying in SEAM mode for
too long, SEAMCALLs returns a BUSY error code to the kernel instead of
spinning on the locks. Depending on the specific SEAMCALL, the caller
may need to handle this error in specific ways (e.g., retry). Therefore,
return the SEAMCALL error code directly to the caller. Don't attempt to
handle it in the core kernel.

TDH.MEM.SEPT.ADD effectively manages two internal resources of the TDX
module: it installs page table pages in the SEPT tree and also updates the
TDX module's page metadata (PAMT). Don't add a wrapper for the matching
SEAMCALL for removing a SEPT page (TDH.MEM.SEPT.REMOVE) because KVM, as the
only in-kernel user, will only tear down the SEPT tree when the TD is being
torn down. When this happens it can just do other operations that reclaim
the SEPT pages for the host kernels to use, update the PAMT and let the
SEPT get trashed.

[Kai: Switched from generic seamcall export]
[Yan: Re-wrote the changelog]
Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Message-ID: <20241112073624.22114-1-yan.y.zhao@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/tdx.h  |  1 +
 arch/x86/virt/vmx/tdx/tdx.c | 19 +++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h |  1 +
 3 files changed, 21 insertions(+)

Comments

Edgecombe, Rick P Jan. 2, 2025, 9:59 p.m. UTC | #1 
On Wed, 2025-01-01 at 02:49 -0500, Paolo Bonzini wrote:
> 
> +u64 tdh_mem_sept_add(struct tdx_td *td, u64 gpa, u64 level, u64 hpa, u64 *rcx, u64 *rdx)
> +{
> +	struct tdx_module_args args = {
> +		.rcx = gpa | level,

The bit packing here is a bit magic. Yan had been looking at adding a union like
below to use internally in tdh_mem_sept_add() to pack the rcx register.

union tdx_sept_gpa_mapping_info {
	struct {
		u64 level	: 3;
		u64 reserved1	: 9;
		u64 gfn		: 40;
		u64 reserved2	: 12;
	};
	u64 full;
};

It might be a bit verbose, but I agree something should be done.


> +		.rdx = tdx_tdr_pa(td),
> +		.r8 = hpa,
> +	};
> +	u64 ret;
> +
> +	clflush_cache_range(__va(hpa), PAGE_SIZE);
> +	ret = seamcall_ret(TDH_MEM_SEPT_ADD, &args);
> +
> +	*rcx = args.rcx;
> +	*rdx = args.rdx;

This was changed to this:
	*extended_err1 = args.rcx;
	*extended_err2 = args.rdx;

The reasoning is rcx and rdx are not very clear names. The caller only uses them
for printing raw error codes, so don't bother creating a union or anything
fancy. The print statements should print the raw error code to help in debugging
situations where new bits are defined and returned by a future buggy TDX module.

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(tdh_mem_sept_add);
> +
>  u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page)
>  {
>  	struct tdx_module_args args = {
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 62cb7832c42d..308d3aa565d7 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -16,6 +16,7 @@
>   * TDX module SEAMCALL leaf functions
>   */
>  #define TDH_MNG_ADDCX			1
> +#define TDH_MEM_SEPT_ADD		3
>  #define TDH_VP_ADDCX			4
>  #define TDH_MNG_KEY_CONFIG		8
>  #define TDH_MNG_CREATE			9
Yan Zhao Jan. 7, 2025, 5:27 a.m. UTC | #2 
Here's the proposed new version with changelog:
SEAMCALL RFC:
    - Use struct tdx_td instead of raw TDR u64.
    - Use "struct page *" for sept_page instead of raw hpa.
    - Change "u64 level" to "int level".
    - Rename "level" to "tdx_level" as it's tdx specfic. (Rick)
    - Change "u64 gpa" to "gfn_t gfn". (Reinette)
    - Introduce a union tdx_sept_gpa_mapping_info to initialize args.rcx.
      (Reinette)
    - Call tdx_clflush_page() instead of clflush_cache_range().
    - Use extended_err1/2 instead of rcx/rdx for output.


diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index c6d0668de167..ea5a98fd1dd5 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -105,6 +105,20 @@ struct tdx_module_args {
        u64 rsi;
 };
 
+/*
+ * Used to specify SEPT GPA mapping information for SEPT related SEAMCALLs and
+ * TDCALLs.
+ */
+union tdx_sept_gpa_mapping_info {
+       struct {
+               u64 level       : 3;
+               u64 reserved1   : 9;
+               u64 gfn         : 40;
+               u64 reserved2   : 12;
+       };
+       u64 full;
+};
+
 /* Used to communicate with the TDX module */
 u64 __tdcall(u64 fn, struct tdx_module_args *args);
 u64 __tdcall_ret(u64 fn, struct tdx_module_args *args);
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index bbb8f0bae9ba..787c359a5fc9 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -33,6 +33,7 @@
 #ifndef __ASSEMBLY__
 
 #include <uapi/asm/mce.h>
+#include <linux/kvm_types.h>
 #include "tdx_global_metadata.h"
 
 /*
@@ -143,6 +144,8 @@ struct tdx_vp {
 };
 
 u64 tdh_mng_addcx(struct tdx_td *td, struct page *tdcs_page);
+u64 tdh_mem_sept_add(struct tdx_td *td, gfn_t gfn, int tdx_level, struct page *sept_page,
+                    u64 *extended_err1, u64 *extended_err2);
 u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page);
 u64 tdh_mng_key_config(struct tdx_td *td);
 u64 tdh_mng_create(struct tdx_td *td, u64 hkid);
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index a16c507296df..adb2059b6b5f 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1583,6 +1583,27 @@ u64 tdh_mng_addcx(struct tdx_td *td, struct page *tdcs_page)
 }
 EXPORT_SYMBOL_GPL(tdh_mng_addcx);
 
+u64 tdh_mem_sept_add(struct tdx_td *td, gfn_t gfn, int tdx_level, struct page *sept_page,
+                    u64 *extended_err1, u64 *extended_err2)
+{
+       union tdx_sept_gpa_mapping_info gpa_info = { .level = tdx_level, .gfn = gfn, };
+       struct tdx_module_args args = {
+               .rcx = gpa_info.full,
+               .rdx = tdx_tdr_pa(td),
+               .r8 = page_to_phys(sept_page),
+       };
+       u64 ret;
+
+       tdx_clflush_page(sept_page);
+       ret = seamcall_ret(TDH_MEM_SEPT_ADD, &args);
+
+       *extended_err1 = args.rcx;
+       *extended_err2 = args.rdx;
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(tdh_mem_sept_add);
+
 u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page)
 {
        struct tdx_module_args args = {
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 08b01b7fe7c2..0d1ba0d0ac82 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -18,6 +18,7 @@
  * TDX module SEAMCALL leaf functions
  */
 #define TDH_MNG_ADDCX                  1
+#define TDH_MEM_SEPT_ADD               3
 #define TDH_VP_ADDCX                   4
 #define TDH_MNG_KEY_CONFIG             8
 #define TDH_MNG_CREATE                 9
Dave Hansen Jan. 7, 2025, 7:48 p.m. UTC | #3 
On 1/2/25 13:59, Edgecombe, Rick P wrote:
> union tdx_sept_gpa_mapping_info {
> 	struct {
> 		u64 level	: 3;
> 		u64 reserved1	: 9;
> 		u64 gfn		: 40;
> 		u64 reserved2	: 12;
> 	};
> 	u64 full;
> };

This is functionally OK, but seeing bitfields on a value that's probably
going to get shifted around makes me nervous because of:

> https://lore.kernel.org/lkml/20231111020019.553664-1-michael.roth@amd.com/
I wouldn't NAK it just for this, but it's also not how I would code it up.
Yan Zhao Jan. 8, 2025, 1:12 a.m. UTC | #4 
On Tue, Jan 07, 2025 at 11:48:12AM -0800, Dave Hansen wrote:
> On 1/2/25 13:59, Edgecombe, Rick P wrote:
> > union tdx_sept_gpa_mapping_info {
> > 	struct {
> > 		u64 level	: 3;
> > 		u64 reserved1	: 9;
> > 		u64 gfn		: 40;
> > 		u64 reserved2	: 12;
> > 	};
> > 	u64 full;
> > };
> 
> This is functionally OK, but seeing bitfields on a value that's probably
> going to get shifted around makes me nervous because of:
This is defined according to the TDX spec.
e.g. in TDH.MEM.SEPT.ADD:

RCX | EPT mapping information:
----|---------------------------------------------------------------------------
    | Bits |  Name    | Description
    |------|----------|---------------------------------------------------------
    | 2:0  |  Level   | Level of the non-leaf Secure EPT entry that will map the
    |      |          | new Secure EPT page - see 3.6.1
    |      |          | Level must between 1 and 3 for a 4-level EPT or between
    |      |          | 1 and 4 for a 5-level EPT.
    |------|----------|---------------------------------------------------------
    | 11:3 | Reserved | Reserved: must be 0
    |------|----------|---------------------------------------------------------
    | 51:12|   GPA    | Bits 51:12 of the guest physical address of to be mapped
    |      |          | for the new Secure EPT page Depending on the level, the
    |      |          | following least significant bits must be 0:
    |      |          | Level 1 (EPT): Bits 20:12
    |      |          | Level 2 (EPD): Bits 29:12
    |      |          | Level 3 (EPDPT): Bits 38:12
    |      |          | Level 4 (EPML4): Bits 47:12
    |------|----------|---------------------------------------------------------
    | 63:52| Reserved | Reserved: must be 0


So, why does this bitfields definition make things worse?

> > https://lore.kernel.org/lkml/20231111020019.553664-1-michael.roth@amd.com/
> I wouldn't NAK it just for this, but it's also not how I would code it up.
Dave Hansen Jan. 8, 2025, 1:20 a.m. UTC | #5 
On 1/7/25 17:12, Yan Zhao wrote:
> So, why does this bitfields definition make things worse?

Look at the kernel page table management. Why don't we use bitfields for
_that_? Look at the link I sent. Bitfields can cause some really goofy
unexpected behavior if you pass them around like they were a full type.
Edgecombe, Rick P Jan. 8, 2025, 1:43 a.m. UTC | #6 
On Tue, 2025-01-07 at 17:20 -0800, Dave Hansen wrote:
> On 1/7/25 17:12, Yan Zhao wrote:
> > So, why does this bitfields definition make things worse?
> 
> Look at the kernel page table management. Why don't we use bitfields for
> _that_? Look at the link I sent. Bitfields can cause some really goofy
> unexpected behavior if you pass them around like they were a full type.

Huh, so this enum is unsafe for reading out the individual fields because if
shifting them, it will perform the shift with the size of the source bit field
size. It is safe in the way it is being used in these patches, which is to
encode a u64. But if we ever started to use tdx_sept_gpa_mapping_info to process
output from a SEAMCALL, or something, we could set ourselves up for the same
problem as the SEV bug.

Let's not open code the encoding in each SEAMCALL though. What about replacing
it with just a helper that encodes the u64 gpa from two args: gfn and tdx_level.
We could add some specific over-size behavior for the fields, but I'd think it
would be ok to keep it simple. Maybe something like this:

static u64 encode_gpa_mapping_info(gfn_t gfn, unsigned int tdx_level)
{
	u64 val = 0;

	val |= level;
	val |= gfn << TDX_MAPPING_INFO_GFN_SHIFT;

	return val;
}
Yan Zhao Jan. 8, 2025, 2:14 a.m. UTC | #7 
On Wed, Jan 08, 2025 at 09:43:37AM +0800, Edgecombe, Rick P wrote:
> On Tue, 2025-01-07 at 17:20 -0800, Dave Hansen wrote:
> > On 1/7/25 17:12, Yan Zhao wrote:
> > > So, why does this bitfields definition make things worse?
> > 
> > Look at the kernel page table management. Why don't we use bitfields for
> > _that_? Look at the link I sent. Bitfields can cause some really goofy
> > unexpected behavior if you pass them around like they were a full type.
> 
> Huh, so this enum is unsafe for reading out the individual fields because if
> shifting them, it will perform the shift with the size of the source bit field
> size. It is safe in the way it is being used in these patches, which is to
> encode a u64. But if we ever started to use tdx_sept_gpa_mapping_info to process
> output from a SEAMCALL, or something, we could set ourselves up for the same
> problem as the SEV bug.
Thanks for the explanation!
Sorry that I didn't clearly explain the usage to Dave.

> Let's not open code the encoding in each SEAMCALL though. What about replacing
> it with just a helper that encodes the u64 gpa from two args: gfn and tdx_level.
> We could add some specific over-size behavior for the fields, but I'd think it
> would be ok to keep it simple. Maybe something like this:
> 
> static u64 encode_gpa_mapping_info(gfn_t gfn, unsigned int tdx_level)
> {
> 	u64 val = 0;
> 
> 	val |= level;
> 	val |= gfn << TDX_MAPPING_INFO_GFN_SHIFT;
> 
> 	return val;
> }
That's a clever alternative :)
diff mbox series

Patch

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 8fc1836c5d09..9e0c60a41d5d 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -138,6 +138,7 @@  struct tdx_vp {
 };
 
 u64 tdh_mng_addcx(struct tdx_td *td, struct page *tdcs_page);
+u64 tdh_mem_sept_add(struct tdx_td *td, u64 gpa, u64 level, u64 hpa, u64 *rcx, u64 *rdx);
 u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page);
 u64 tdh_mng_key_config(struct tdx_td *td);
 u64 tdh_mng_create(struct tdx_td *td, u64 hkid);
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 986b58b63121..a97a470dda23 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1491,6 +1491,25 @@  u64 tdh_mng_addcx(struct tdx_td *td, struct page *tdcs_page)
 }
 EXPORT_SYMBOL_GPL(tdh_mng_addcx);
 
+u64 tdh_mem_sept_add(struct tdx_td *td, u64 gpa, u64 level, u64 hpa, u64 *rcx, u64 *rdx)
+{
+	struct tdx_module_args args = {
+		.rcx = gpa | level,
+		.rdx = tdx_tdr_pa(td),
+		.r8 = hpa,
+	};
+	u64 ret;
+
+	clflush_cache_range(__va(hpa), PAGE_SIZE);
+	ret = seamcall_ret(TDH_MEM_SEPT_ADD, &args);
+
+	*rcx = args.rcx;
+	*rdx = args.rdx;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tdh_mem_sept_add);
+
 u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page)
 {
 	struct tdx_module_args args = {
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 62cb7832c42d..308d3aa565d7 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -16,6 +16,7 @@ 
  * TDX module SEAMCALL leaf functions
  */
 #define TDH_MNG_ADDCX			1
+#define TDH_MEM_SEPT_ADD		3
 #define TDH_VP_ADDCX			4
 #define TDH_MNG_KEY_CONFIG		8
 #define TDH_MNG_CREATE			9