[PATCH v3 03/12] x86/tdx: Make macros of TDCALLs consistent with the spec

Kai Huang posted 12 patches 2 years, 6 months ago
There is a newer version of this series
[PATCH v3 03/12] x86/tdx: Make macros of TDCALLs consistent with the spec
Posted by Kai Huang 2 years, 6 months ago
The TDX spec names all TDCALLs with prefix "TDG".  Currently, the kernel
doesn't follow such convention for the macros of those TDCALLs but uses
prefix "TDX_" for all of them.  Although it's arguable whether the TDX
spec names those TDCALLs properly, it's better for the kernel to follow
the spec when naming those macros.

Change all macros of TDCALLs to make them consistent with the spec.  As
a bonus, they get distinguished easily from the host-side SEAMCALLs,
which all have prefix "TDH".

No functional change intended.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v2 -> v3:
 - No change.

v1 -> v2:
 - Rebase to 6.5-rc2.

---
 arch/x86/coco/tdx/tdx-shared.c    |  4 ++--
 arch/x86/coco/tdx/tdx.c           |  8 ++++----
 arch/x86/include/asm/shared/tdx.h | 10 +++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx-shared.c b/arch/x86/coco/tdx/tdx-shared.c
index ef20ddc37b58..f10cd3e4a04e 100644
--- a/arch/x86/coco/tdx/tdx-shared.c
+++ b/arch/x86/coco/tdx/tdx-shared.c
@@ -35,7 +35,7 @@ static unsigned long try_accept_one(phys_addr_t start, unsigned long len,
 	}
 
 	tdcall_rcx = start | page_size;
-	if (__tdx_module_call(TDX_ACCEPT_PAGE, tdcall_rcx, 0, 0, 0, NULL))
+	if (__tdx_module_call(TDG_MEM_PAGE_ACCEPT, tdcall_rcx, 0, 0, 0, NULL))
 		return 0;
 
 	return accept_size;
@@ -45,7 +45,7 @@ bool tdx_accept_memory(phys_addr_t start, phys_addr_t end)
 {
 	/*
 	 * For shared->private conversion, accept the page using
-	 * TDX_ACCEPT_PAGE TDX module call.
+	 * TDG_MEM_PAGE_ACCEPT TDX module call.
 	 */
 	while (start < end) {
 		unsigned long len = end - start;
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 1d6b863c42b0..05785df66b1c 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -91,7 +91,7 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
 {
 	u64 ret;
 
-	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
+	ret = __tdx_module_call(TDG_MR_REPORT, virt_to_phys(tdreport),
 				virt_to_phys(reportdata), TDREPORT_SUBTYPE_0,
 				0, NULL);
 	if (ret) {
@@ -152,7 +152,7 @@ static void tdx_parse_tdinfo(u64 *cc_mask)
 	 * Guest-Host-Communication Interface (GHCI), section 2.4.2 TDCALL
 	 * [TDG.VP.INFO].
 	 */
-	tdx_module_call(TDX_GET_INFO, 0, 0, 0, 0, &out);
+	tdx_module_call(TDG_VP_INFO, 0, 0, 0, 0, &out);
 
 	/*
 	 * The highest bit of a guest physical address is the "sharing" bit.
@@ -594,7 +594,7 @@ void tdx_get_ve_info(struct ve_info *ve)
 	 * Note, the TDX module treats virtual NMIs as inhibited if the #VE
 	 * valid flag is set. It means that NMI=>#VE will not result in a #DF.
 	 */
-	tdx_module_call(TDX_GET_VEINFO, 0, 0, 0, 0, &out);
+	tdx_module_call(TDG_VP_VEINFO_GET, 0, 0, 0, 0, &out);
 
 	/* Transfer the output parameters */
 	ve->exit_reason = out.rcx;
@@ -774,7 +774,7 @@ void __init tdx_early_init(void)
 	cc_set_mask(cc_mask);
 
 	/* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
-	tdx_module_call(TDX_WR, 0, TDCS_NOTIFY_ENABLES, 0, -1ULL, NULL);
+	tdx_module_call(TDG_VM_WR, 0, TDCS_NOTIFY_ENABLES, 0, -1ULL, NULL);
 
 	/*
 	 * All bits above GPA width are reserved and kernel treats shared bit
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 7513b3bb69b7..78f109446da6 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -11,11 +11,11 @@
 #define TDX_IDENT		"IntelTDX    "
 
 /* TDX module Call Leaf IDs */
-#define TDX_GET_INFO			1
-#define TDX_GET_VEINFO			3
-#define TDX_GET_REPORT			4
-#define TDX_ACCEPT_PAGE			6
-#define TDX_WR				8
+#define TDG_VP_INFO			1
+#define TDG_VP_VEINFO_GET		3
+#define TDG_MR_REPORT			4
+#define TDG_MEM_PAGE_ACCEPT		6
+#define TDG_VM_WR			8
 
 /* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
 #define TDCS_NOTIFY_ENABLES		0x9100000000000010
-- 
2.41.0
Re: [PATCH v3 03/12] x86/tdx: Make macros of TDCALLs consistent with the spec
Posted by Sathyanarayanan Kuppuswamy 2 years, 6 months ago

On 7/26/23 4:25 AM, Kai Huang wrote:
> The TDX spec names all TDCALLs with prefix "TDG".  Currently, the kernel
> doesn't follow such convention for the macros of those TDCALLs but uses
> prefix "TDX_" for all of them.  Although it's arguable whether the TDX
> spec names those TDCALLs properly, it's better for the kernel to follow
> the spec when naming those macros.
> 
> Change all macros of TDCALLs to make them consistent with the spec.  As
> a bonus, they get distinguished easily from the host-side SEAMCALLs,
> which all have prefix "TDH".
> 
> No functional change intended.
> 

When upstreaming the TDX guest patches, there was a discussion about using
TDG vs TDX. Final agreement is to use TDX_ prefix. I think it makes sense
to align with the spec, but it is up to the maintainer.

What about the function name prefix? Are you planning to change them to tdg_*?

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
> 
> v2 -> v3:
>  - No change.
> 
> v1 -> v2:
>  - Rebase to 6.5-rc2.
> 
> ---
>  arch/x86/coco/tdx/tdx-shared.c    |  4 ++--
>  arch/x86/coco/tdx/tdx.c           |  8 ++++----
>  arch/x86/include/asm/shared/tdx.h | 10 +++++-----
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx-shared.c b/arch/x86/coco/tdx/tdx-shared.c
> index ef20ddc37b58..f10cd3e4a04e 100644
> --- a/arch/x86/coco/tdx/tdx-shared.c
> +++ b/arch/x86/coco/tdx/tdx-shared.c
> @@ -35,7 +35,7 @@ static unsigned long try_accept_one(phys_addr_t start, unsigned long len,
>  	}
>  
>  	tdcall_rcx = start | page_size;
> -	if (__tdx_module_call(TDX_ACCEPT_PAGE, tdcall_rcx, 0, 0, 0, NULL))
> +	if (__tdx_module_call(TDG_MEM_PAGE_ACCEPT, tdcall_rcx, 0, 0, 0, NULL))
>  		return 0;
>  
>  	return accept_size;
> @@ -45,7 +45,7 @@ bool tdx_accept_memory(phys_addr_t start, phys_addr_t end)
>  {
>  	/*
>  	 * For shared->private conversion, accept the page using
> -	 * TDX_ACCEPT_PAGE TDX module call.
> +	 * TDG_MEM_PAGE_ACCEPT TDX module call.
>  	 */
>  	while (start < end) {
>  		unsigned long len = end - start;
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..05785df66b1c 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -91,7 +91,7 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>  {
>  	u64 ret;
>  
> -	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> +	ret = __tdx_module_call(TDG_MR_REPORT, virt_to_phys(tdreport),
>  				virt_to_phys(reportdata), TDREPORT_SUBTYPE_0,
>  				0, NULL);
>  	if (ret) {
> @@ -152,7 +152,7 @@ static void tdx_parse_tdinfo(u64 *cc_mask)
>  	 * Guest-Host-Communication Interface (GHCI), section 2.4.2 TDCALL
>  	 * [TDG.VP.INFO].
>  	 */
> -	tdx_module_call(TDX_GET_INFO, 0, 0, 0, 0, &out);
> +	tdx_module_call(TDG_VP_INFO, 0, 0, 0, 0, &out);
>  
>  	/*
>  	 * The highest bit of a guest physical address is the "sharing" bit.
> @@ -594,7 +594,7 @@ void tdx_get_ve_info(struct ve_info *ve)
>  	 * Note, the TDX module treats virtual NMIs as inhibited if the #VE
>  	 * valid flag is set. It means that NMI=>#VE will not result in a #DF.
>  	 */
> -	tdx_module_call(TDX_GET_VEINFO, 0, 0, 0, 0, &out);
> +	tdx_module_call(TDG_VP_VEINFO_GET, 0, 0, 0, 0, &out);
>  
>  	/* Transfer the output parameters */
>  	ve->exit_reason = out.rcx;
> @@ -774,7 +774,7 @@ void __init tdx_early_init(void)
>  	cc_set_mask(cc_mask);
>  
>  	/* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
> -	tdx_module_call(TDX_WR, 0, TDCS_NOTIFY_ENABLES, 0, -1ULL, NULL);
> +	tdx_module_call(TDG_VM_WR, 0, TDCS_NOTIFY_ENABLES, 0, -1ULL, NULL);
>  
>  	/*
>  	 * All bits above GPA width are reserved and kernel treats shared bit
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 7513b3bb69b7..78f109446da6 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -11,11 +11,11 @@
>  #define TDX_IDENT		"IntelTDX    "
>  
>  /* TDX module Call Leaf IDs */
> -#define TDX_GET_INFO			1
> -#define TDX_GET_VEINFO			3
> -#define TDX_GET_REPORT			4
> -#define TDX_ACCEPT_PAGE			6
> -#define TDX_WR				8
> +#define TDG_VP_INFO			1
> +#define TDG_VP_VEINFO_GET		3
> +#define TDG_MR_REPORT			4
> +#define TDG_MEM_PAGE_ACCEPT		6
> +#define TDG_VM_WR			8
>  
>  /* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
>  #define TDCS_NOTIFY_ENABLES		0x9100000000000010

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Re: [PATCH v3 03/12] x86/tdx: Make macros of TDCALLs consistent with the spec
Posted by Huang, Kai 2 years, 6 months ago
On Thu, 2023-07-27 at 18:54 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 7/26/23 4:25 AM, Kai Huang wrote:
> > The TDX spec names all TDCALLs with prefix "TDG".  Currently, the kernel
> > doesn't follow such convention for the macros of those TDCALLs but uses
> > prefix "TDX_" for all of them.  Although it's arguable whether the TDX
> > spec names those TDCALLs properly, it's better for the kernel to follow
> > the spec when naming those macros.
> > 
> > Change all macros of TDCALLs to make them consistent with the spec.  As
> > a bonus, they get distinguished easily from the host-side SEAMCALLs,
> > which all have prefix "TDH".
> > 
> > No functional change intended.
> > 
> 
> When upstreaming the TDX guest patches, there was a discussion about using
> TDG vs TDX. Final agreement is to use TDX_ prefix. I think it makes sense
> to align with the spec, but it is up to the maintainer.
> 
> What about the function name prefix? Are you planning to change them to tdg_*?

I changed to __tdcall() in the next patch.
 
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Thanks.

Re: [PATCH v3 03/12] x86/tdx: Make macros of TDCALLs consistent with the spec
Posted by kirill.shutemov@linux.intel.com 2 years, 6 months ago
On Wed, Jul 26, 2023 at 11:25:05PM +1200, Kai Huang wrote:
> The TDX spec names all TDCALLs with prefix "TDG".  Currently, the kernel
> doesn't follow such convention for the macros of those TDCALLs but uses
> prefix "TDX_" for all of them.  Although it's arguable whether the TDX
> spec names those TDCALLs properly, it's better for the kernel to follow
> the spec when naming those macros.
> 
> Change all macros of TDCALLs to make them consistent with the spec.  As
> a bonus, they get distinguished easily from the host-side SEAMCALLs,
> which all have prefix "TDH".
> 
> No functional change intended.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>

I remember we had few back-and-forth on naming this defines. I think
matching names to the spec is helpful for future readers:

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov