diff mbox series

[v7,16/20] x86/virt/tdx: Configure TDX module with TDMRs and global KeyID

Message ID 344234642a5eb9dc1aa34410f641f596ec428ea5.1668988357.git.kai.huang@intel.com
State New
Headers
Series TDX host kernel support |

Commit Message

Kai Huang Nov. 21, 2022, 12:26 a.m. UTC 
  After the TDX-usable memory regions are constructed in an array of TDMRs
and the global KeyID is reserved, configure them to the TDX module using
TDH.SYS.CONFIG SEAMCALL.  TDH.SYS.CONFIG can only be called once and can
be done on any logical cpu.

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/virt/vmx/tdx/tdx.c | 37 +++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h |  2 ++
 2 files changed, 39 insertions(+)

Comments

Dave Hansen Nov. 23, 2022, 11:56 p.m. UTC | #1 
On 11/20/22 16:26, Kai Huang wrote:
> After the TDX-usable memory regions are constructed in an array of TDMRs
> and the global KeyID is reserved, configure them to the TDX module using
> TDH.SYS.CONFIG SEAMCALL.  TDH.SYS.CONFIG can only be called once and can
> be done on any logical cpu.
> 
> Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 37 +++++++++++++++++++++++++++++++++++++
>  arch/x86/virt/vmx/tdx/tdx.h |  2 ++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index e2cbeeb7f0dc..3a032930e58a 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -979,6 +979,37 @@ static int construct_tdmrs(struct tdmr_info *tdmr_array, int *tdmr_num)
>  	return ret;
>  }
>  
> +static int config_tdx_module(struct tdmr_info *tdmr_array, int tdmr_num,
> +			     u64 global_keyid)
> +{
> +	u64 *tdmr_pa_array;
> +	int i, array_sz;
> +	u64 ret;
> +
> +	/*
> +	 * TDMR_INFO entries are configured to the TDX module via an
> +	 * array of the physical address of each TDMR_INFO.  TDX module
> +	 * requires the array itself to be 512-byte aligned.  Round up
> +	 * the array size to 512-byte aligned so the buffer allocated
> +	 * by kzalloc() will meet the alignment requirement.
> +	 */

Aagh.  Return of (a different) 512-byte aligned structure.

> +	array_sz = ALIGN(tdmr_num * sizeof(u64), TDMR_INFO_PA_ARRAY_ALIGNMENT);
> +	tdmr_pa_array = kzalloc(array_sz, GFP_KERNEL);

Just to be clear, all that chatter about alignment is because the
*START* of the array has to be aligned.  Right?  I see alignment for
'array_sz', but that's not the start of the array.

tdmr_pa_array is the start of the array.  Where is *THAT* aligned?

How does rounding up the size make kzalloc() magically know how to align
the *START* of the allocation?

Because I'm actually questioning my own sanity at this point, I went and
double-checked the docs (Documentation/core-api/memory-allocation.rst):

> The address of a chunk allocated with `kmalloc` is aligned to at least
> ARCH_KMALLOC_MINALIGN bytes.  For sizes which are a power of two, the
> alignment is also guaranteed to be at least the respective size.

Hint #1: ARCH_KMALLOC_MINALIGN is way less than 512.
Hint #2: tdmr_num is not guaranteed to be a power of two.
Hint #3: Comments don't actually affect the allocation

<snip>
Kai Huang Nov. 25, 2022, 12:59 a.m. UTC | #2 
On Wed, 2022-11-23 at 15:56 -0800, Dave Hansen wrote:
> On 11/20/22 16:26, Kai Huang wrote:
> > After the TDX-usable memory regions are constructed in an array of TDMRs
> > and the global KeyID is reserved, configure them to the TDX module using
> > TDH.SYS.CONFIG SEAMCALL.  TDH.SYS.CONFIG can only be called once and can
> > be done on any logical cpu.
> > 
> > Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> >  arch/x86/virt/vmx/tdx/tdx.c | 37 +++++++++++++++++++++++++++++++++++++
> >  arch/x86/virt/vmx/tdx/tdx.h |  2 ++
> >  2 files changed, 39 insertions(+)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index e2cbeeb7f0dc..3a032930e58a 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -979,6 +979,37 @@ static int construct_tdmrs(struct tdmr_info *tdmr_array, int *tdmr_num)
> >  	return ret;
> >  }
> >  
> > +static int config_tdx_module(struct tdmr_info *tdmr_array, int tdmr_num,
> > +			     u64 global_keyid)
> > +{
> > +	u64 *tdmr_pa_array;
> > +	int i, array_sz;
> > +	u64 ret;
> > +
> > +	/*
> > +	 * TDMR_INFO entries are configured to the TDX module via an
> > +	 * array of the physical address of each TDMR_INFO.  TDX module
> > +	 * requires the array itself to be 512-byte aligned.  Round up
> > +	 * the array size to 512-byte aligned so the buffer allocated
> > +	 * by kzalloc() will meet the alignment requirement.
> > +	 */
> 
> Aagh.  Return of (a different) 512-byte aligned structure.
> 
> > +	array_sz = ALIGN(tdmr_num * sizeof(u64), TDMR_INFO_PA_ARRAY_ALIGNMENT);
> > +	tdmr_pa_array = kzalloc(array_sz, GFP_KERNEL);
> 
> Just to be clear, all that chatter about alignment is because the
> *START* of the array has to be aligned.  Right?  
> 

Correct.

> I see alignment for
> 'array_sz', but that's not the start of the array.
> 
> tdmr_pa_array is the start of the array.  Where is *THAT* aligned?

The alignment is assumed to be guaranteed based on the Documentation you quoted
below.

> 
> How does rounding up the size make kzalloc() magically know how to align
> the *START* of the allocation?
> 
> Because I'm actually questioning my own sanity at this point, I went and
> double-checked the docs (Documentation/core-api/memory-allocation.rst):
> 
> > The address of a chunk allocated with `kmalloc` is aligned to at least
> > ARCH_KMALLOC_MINALIGN bytes.  For sizes which are a power of two, the
> > alignment is also guaranteed to be at least the respective size.
> 
> Hint #1: ARCH_KMALLOC_MINALIGN is way less than 512.
> Hint #2: tdmr_num is not guaranteed to be a power of two.

	tdmr_num * sizeof(u64) is not guaranteed.

> Hint #3: Comments don't actually affect the allocation

Sorry I don't understand the Hint #3.

Perhaps I should just allocate one page so it must be 512-byte aligned?

	/*
	 * TDMR_INFO entries are configured to the TDX module  via an array
	 * of physical address of each TDMR_INFO.  The TDX module requires
	 * the array itself to be 512-byte aligned.  Just allocate a page
	 * to use it as the array so the alignment can be guaranteed.  The
	 * page will be freed after TDH.SYS.CONFIG anyway.
	 */
Dave Hansen Nov. 25, 2022, 1:18 a.m. UTC | #3 
On 11/24/22 16:59, Huang, Kai wrote:
> On Wed, 2022-11-23 at 15:56 -0800, Dave Hansen wrote:
>> On 11/20/22 16:26, Kai Huang wrote:
>>> +   array_sz = ALIGN(tdmr_num * sizeof(u64), TDMR_INFO_PA_ARRAY_ALIGNMENT);
>>> +   tdmr_pa_array = kzalloc(array_sz, GFP_KERNEL);
>>
>> Just to be clear, all that chatter about alignment is because the
>> *START* of the array has to be aligned.  Right?
> 
> Correct.
> 
>> I see alignment for
>> 'array_sz', but that's not the start of the array.
>>
>> tdmr_pa_array is the start of the array.  Where is *THAT* aligned?
> 
> The alignment is assumed to be guaranteed based on the Documentation you quoted
> below.

I'm feeling kinda dense today, being Thanksgiving and all.  Could you
please walk me through, step-by-step how you get kzalloc() to give you
an allocation where the start address is 512-byte aligned?

...
> Perhaps I should just allocate one page so it must be 512-byte aligned?
> 
>         /*
>          * TDMR_INFO entries are configured to the TDX module  via an array
>          * of physical address of each TDMR_INFO.  The TDX module requires
>          * the array itself to be 512-byte aligned.  Just allocate a page
>          * to use it as the array so the alignment can be guaranteed.  The
>          * page will be freed after TDH.SYS.CONFIG anyway.
>          */

Kai, I just plain can't believe at this point that comments like this
are still being written.  I _thought_ I was very clear before that if
you have a constant, say:

#define FOO_ALIGN 512

and you want to align foo, you can just do:

	foo = ALIGN(foo, FOO_ALIGN);

You don't need to mention the 512-byte alignment again.  The #define is
good enough.
Kai Huang Nov. 25, 2022, 1:44 a.m. UTC | #4 
On Thu, 2022-11-24 at 17:18 -0800, Dave Hansen wrote:
> On 11/24/22 16:59, Huang, Kai wrote:
> > On Wed, 2022-11-23 at 15:56 -0800, Dave Hansen wrote:
> > > On 11/20/22 16:26, Kai Huang wrote:
> > > > +   array_sz = ALIGN(tdmr_num * sizeof(u64), TDMR_INFO_PA_ARRAY_ALIGNMENT);
> > > > +   tdmr_pa_array = kzalloc(array_sz, GFP_KERNEL);
> > > 
> > > Just to be clear, all that chatter about alignment is because the
> > > *START* of the array has to be aligned.  Right?
> > 
> > Correct.
> > 
> > > I see alignment for
> > > 'array_sz', but that's not the start of the array.
> > > 
> > > tdmr_pa_array is the start of the array.  Where is *THAT* aligned?
> > 
> > The alignment is assumed to be guaranteed based on the Documentation you quoted
> > below.
> 
> I'm feeling kinda dense today, being Thanksgiving and all.  Could you
> please walk me through, step-by-step how you get kzalloc() to give you
> an allocation where the start address is 512-byte aligned?

Sorry I am not good at math.  I forgot although 512 is power of two, n x 512 may
not be power of two.

The code works because in practice tdmr_num is never larger than 64 so tdmr_num
* sizeof(64) cannot be larger than 512.

So if want to consider array size being larger than 4K, we should use
alloc_pages_exact() to allocate?

> 
> ...
> > Perhaps I should just allocate one page so it must be 512-byte aligned?
> > 
> >         /*
> >          * TDMR_INFO entries are configured to the TDX module  via an array
> >          * of physical address of each TDMR_INFO.  The TDX module requires
> >          * the array itself to be 512-byte aligned.  Just allocate a page
> >          * to use it as the array so the alignment can be guaranteed.  The
> >          * page will be freed after TDH.SYS.CONFIG anyway.
> >          */
> 
> Kai, I just plain can't believe at this point that comments like this
> are still being written.  I _thought_ I was very clear before that if
> you have a constant, say:
> 
> #define FOO_ALIGN 512
> 
> and you want to align foo, you can just do:
> 
> 	foo = ALIGN(foo, FOO_ALIGN);
> 
> You don't need to mention the 512-byte alignment again.  The #define is
> good enough.
> 
> 

My fault.  I thought by changing to allocate a page we don't need to do 'foo =
ALIGN(foo, FOO_ALIGN)' so I thought the comment could be useful.

Thanks for responding at Thanksgiving day.
diff mbox series

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index e2cbeeb7f0dc..3a032930e58a 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -979,6 +979,37 @@  static int construct_tdmrs(struct tdmr_info *tdmr_array, int *tdmr_num)
 	return ret;
 }
 
+static int config_tdx_module(struct tdmr_info *tdmr_array, int tdmr_num,
+			     u64 global_keyid)
+{
+	u64 *tdmr_pa_array;
+	int i, array_sz;
+	u64 ret;
+
+	/*
+	 * TDMR_INFO entries are configured to the TDX module via an
+	 * array of the physical address of each TDMR_INFO.  TDX module
+	 * requires the array itself to be 512-byte aligned.  Round up
+	 * the array size to 512-byte aligned so the buffer allocated
+	 * by kzalloc() will meet the alignment requirement.
+	 */
+	array_sz = ALIGN(tdmr_num * sizeof(u64), TDMR_INFO_PA_ARRAY_ALIGNMENT);
+	tdmr_pa_array = kzalloc(array_sz, GFP_KERNEL);
+	if (!tdmr_pa_array)
+		return -ENOMEM;
+
+	for (i = 0; i < tdmr_num; i++)
+		tdmr_pa_array[i] = __pa(tdmr_array_entry(tdmr_array, i));
+
+	ret = seamcall(TDH_SYS_CONFIG, __pa(tdmr_pa_array), tdmr_num,
+				global_keyid, 0, NULL, NULL);
+
+	/* Free the array as it is not required anymore. */
+	kfree(tdmr_pa_array);
+
+	return ret;
+}
+
 /*
  * Detect and initialize the TDX module.
  *
@@ -1062,11 +1093,17 @@  static int init_tdx_module(void)
 	 */
 	tdx_global_keyid = tdx_keyid_start;
 
+	/* Pass the TDMRs and the global KeyID to the TDX module */
+	ret = config_tdx_module(tdmr_array, tdmr_num, tdx_global_keyid);
+	if (ret)
+		goto out_free_pamts;
+
 	/*
 	 * Return -EINVAL until all steps of TDX module initialization
 	 * process are done.
 	 */
 	ret = -EINVAL;
+out_free_pamts:
 	if (ret)
 		tdmrs_free_pamt_all(tdmr_array, tdmr_num);
 	else
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index a737f2b51474..c26bab2555ca 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -19,6 +19,7 @@ 
 #define TDH_SYS_INIT		33
 #define TDH_SYS_LP_INIT		35
 #define TDH_SYS_LP_SHUTDOWN	44
+#define TDH_SYS_CONFIG		45
 
 struct cmr_info {
 	u64	base;
@@ -86,6 +87,7 @@  struct tdmr_reserved_area {
 } __packed;
 
 #define TDMR_INFO_ALIGNMENT	512
+#define TDMR_INFO_PA_ARRAY_ALIGNMENT	512
 
 struct tdmr_info {
 	u64 base;