Re: [PATCH v6 01/21] x86/tdx: Use enum to define page level of TDX supported page sizes

[Huang, Kai]

On Thu, 2022-10-27 at 08:27 -0700, Dave Hansen wrote:
> On 10/26/22 16:16, Kai Huang wrote:
> > +/*
> > + * Get the TDX page level based on the kernel page level.  The caller
> > + * to make sure only pass 4K/2M/1G kernel page level.
> > + */
> > +static inline enum tdx_pg_level to_tdx_pg_level(enum pg_level pglvl)
> > +{
> > +	switch (pglvl) {
> > +	case PG_LEVEL_4K:
> > +		return TDX_PG_LEVEL_4K;
> > +	case PG_LEVEL_2M:
> > +		return TDX_PG_LEVEL_2M;
> > +	case PG_LEVEL_1G:
> > +		return TDX_PG_LEVEL_1G;
> > +	default:
> > +		WARN_ON_ONCE(1);
> > +	}
> > +	return TDX_PG_LEVEL_NUM;
> > +}
> 
> Is TDX_PG_LEVEL_NUM part of the ABI?  Or, is this going to accidentally
> pass a whacky value to the SEAM module?

The intention is TDX_PG_LEVEL_NUM is not part of ABI, but looks I was wrong. 
KVM secure EPT can accept larger page level of 1G as page table.

> This needs something like this at the call-site:
> 
> 	page_size = to_tdx_pg_level(pg_level);
> 	if (page_size >= TDX_PG_LEVEL_NUM)
> 		return false;

Yes.  Thanks for the time to review.  It's bad, and should go away.

This reminds me I have mixed two tings together: 1) leaf page sizes (4K/2M/1G);
2) page table levels, which can have larger level than 1G.

In fact, the TDX module spec has a separate definition for the leaf page sizes:

	Table 20.10: Page Size Definition

	PS_1G	1G	2
	PS_2M	2M	1
	PS_4K	4K	0

While TDX guest and TDX host code only needs leaf page sizes, KVM needs all the
page table levels, so it's not necessarily to provide a common helper to get TDX
page level from kernel page level.

As Isaku also replied, I'll remove the helper.

Hi Kirill,

You expressed perhaps we can use macro definitions instead of the enum type. 
Does below look good to you?

--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -671,13 +671,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 020c81a7c729..74845d014d1c 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -20,6 +20,18 @@
 
 #ifndef __ASSEMBLY__
 
+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Please refer to the TDX module 1.0 spec 20.4.1 Physical Page Size.
+ *
+ * Those values are part of the TDX module ABI (except TDX_PS_NUM).
+ */
+#define TDX_PS_4K      0
+#define TDX_PS_2M      1
+#define TDX_PS_1G      2
+#define TDX_PS_NUM     3
+

Btw, TDX host patch will use them in below way (please refer to patch 14:
x86/virt/tdx: Allocate and set up PAMTs for TDMRs):

	unsigned long pamt_size[TDX_PS_NUM];

	/*
	 * Calculate the PAMT size for each TDX supported page size
	 * and the total PAMT size.  TDX_PS_* are contiguous from 0 to 3.
	 */
	for (pgsz = TDX_PS_4K; pgsz < TDX_PS_NUM; pgsz++) {
		pamt_size[pgsz] = tdmr_get_pamt_sz(tdmr, pgsz);
		...
	}