[PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros

Kai Huang posted 20 patches 2 years, 3 months ago
There is a newer version of this series
[PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Kai Huang 2 years, 3 months ago
TDX supports 4K, 2M and 1G page sizes.  The corresponding values are
defined by the TDX module spec and used as TDX module ABI.  Currently,
they are used in try_accept_one() when the TDX guest tries to accept a
page.  However currently try_accept_one() uses hard-coded magic values.

Define TDX supported page sizes as macros and get rid of the hard-coded
values in try_accept_one().  TDX host support will need to use them too.

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v6 -> v7:

 - Removed the helper to convert kernel page level to TDX page level.
 - Changed to use macro to define TDX supported page sizes.

---
 arch/x86/coco/tdx/tdx.c    | 6 +++---
 arch/x86/include/asm/tdx.h | 9 +++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index cfd4c95b9f04..7fa7fb54f438 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -722,13 +722,13 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
 	 */
 	switch (pg_level) {
 	case PG_LEVEL_4K:
-		page_size = 0;
+		page_size = TDX_PS_4K;
 		break;
 	case PG_LEVEL_2M:
-		page_size = 1;
+		page_size = TDX_PS_2M;
 		break;
 	case PG_LEVEL_1G:
-		page_size = 2;
+		page_size = TDX_PS_1G;
 		break;
 	default:
 		return false;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 28d889c9aa16..e9a3f4a6fba1 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -20,6 +20,15 @@
 
 #ifndef __ASSEMBLY__
 
+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Those values are part of the TDX module ABI.  Do not change them.
+ */
+#define TDX_PS_4K	0
+#define TDX_PS_2M	1
+#define TDX_PS_1G	2
+
 /*
  * Used to gather the output registers values of the TDCALL and SEAMCALL
  * instructions when requesting services from the TDX module.
-- 
2.38.1
Re: [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Dave Hansen 2 years, 3 months ago
On 11/20/22 16:26, Kai Huang wrote:
> +/*
> + * TDX supported page sizes (4K/2M/1G).
> + *
> + * Those values are part of the TDX module ABI.  Do not change them.
> + */
> +#define TDX_PS_4K	0
> +#define TDX_PS_2M	1
> +#define TDX_PS_1G	2

That comment can just be:

/* TDX supported page sizes from the TDX module ABI. */

I think folks understand that the kernel can't willy nilly change ABI
values.
Re: [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Huang, Kai 2 years, 3 months ago
On Mon, 2022-11-21 at 15:48 -0800, Hansen, Dave wrote:
> On 11/20/22 16:26, Kai Huang wrote:
> > +/*
> > + * TDX supported page sizes (4K/2M/1G).
> > + *
> > + * Those values are part of the TDX module ABI.  Do not change them.
> > + */
> > +#define TDX_PS_4K	0
> > +#define TDX_PS_2M	1
> > +#define TDX_PS_1G	2
> 
> That comment can just be:
> 
> /* TDX supported page sizes from the TDX module ABI. */
> 
> I think folks understand that the kernel can't willy nilly change ABI
> values.

Thanks.  Will do.
Re: [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Sathyanarayanan Kuppuswamy 2 years, 3 months ago

On 11/20/22 4:26 PM, Kai Huang wrote:
> +/*
> + * TDX supported page sizes (4K/2M/1G).
> + *
> + * Those values are part of the TDX module ABI.  Do not change them.

It would be better if you include specification version and section
title.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Re: [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Dave Hansen 2 years, 3 months ago
On 11/20/22 18:52, Sathyanarayanan Kuppuswamy wrote:
> On 11/20/22 4:26 PM, Kai Huang wrote:
>> +/*
>> + * TDX supported page sizes (4K/2M/1G).
>> + *
>> + * Those values are part of the TDX module ABI.  Do not change them.
> It would be better if you include specification version and section
> title.

I actually think TDX code, in general, spends way too much time quoting
and referring to the spec.

Also, why quote the version?  Do we quote the SDM version when we add
new SDM-defined architecture?

It's just busywork that bloats the kernel and adds noise.  Please focus
on adding value to the comments that came from your brain and not just
pasting boilerplate gunk over and over.
Re: [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Huang, Kai 2 years, 3 months ago
On Sun, 2022-11-20 at 18:52 -0800, Sathyanarayanan Kuppuswamy wrote:
> 
> On 11/20/22 4:26 PM, Kai Huang wrote:
> > +/*
> > + * TDX supported page sizes (4K/2M/1G).
> > + *
> > + * Those values are part of the TDX module ABI.  Do not change them.
> 
> It would be better if you include specification version and section
> title.
> 

Such as below?

"Those values are part of the TDX module ABI (section "Physical Page Size", TDX
module 1.0 spec).  Do not change them."

Btw, Dave mentioned we should not put the "section numbers" to the comment:

https://lore.kernel.org/lkml/2a1886e7-fa5d-99e2-b1da-55ed7c0d024b@intel.com/

I was trying to follow.
Re: [PATCH v7 01/20] x86/tdx: Define TDX supported page sizes as macros
Posted by Sathyanarayanan Kuppuswamy 2 years, 3 months ago

On 11/21/22 1:15 AM, Huang, Kai wrote:
> On Sun, 2022-11-20 at 18:52 -0800, Sathyanarayanan Kuppuswamy wrote:
>>
>> On 11/20/22 4:26 PM, Kai Huang wrote:
>>> +/*
>>> + * TDX supported page sizes (4K/2M/1G).
>>> + *
>>> + * Those values are part of the TDX module ABI.  Do not change them.
>>
>> It would be better if you include specification version and section
>> title.
>>
> 
> Such as below?
> 
> "Those values are part of the TDX module ABI (section "Physical Page Size", TDX
> module 1.0 spec).  Do not change them."

Yes.

> 
> Btw, Dave mentioned we should not put the "section numbers" to the comment:
> 
> https://lore.kernel.org/lkml/2a1886e7-fa5d-99e2-b1da-55ed7c0d024b@intel.com/
> 
> I was trying to follow.

Yes. That's why suggested to put section title.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer