On Fri, 2023-01-06 at 14:21 -0800, Dave Hansen wrote:
> On 12/8/22 22:52, Kai Huang wrote:
> > After a list of "TD Memory Regions" (TDMRs) has been constructed to
> > cover all TDX-usable memory regions, the next step is to pick up a TDX
> > private KeyID as the "global KeyID" (which protects, i.e. TDX module's
> > metadata), and configure it to the TDX module along with the TDMRs.
>
> For whatever reason, whenever I see "i.e." in a changelog, it's usually
> going off the rails. This is no exception. Let's also get rid of the
> passive voice:
>
> The next step After constructing a list of "TD Memory Regions"
> (TDMRs) to cover all TDX-usable memory regions is to designate a
> TDX private KeyID as the "global KeyID". This KeyID is used by
> the TDX module for mapping things like the PAMT and other TDX
> metadata. This KeyID is passed to the TDX module at the same
> time as the TDMRs.
Thanks. Will use.
>
> > To keep things simple, just use the first TDX KeyID as the global KeyID.
>
>
>
>
> > ---
> > arch/x86/virt/vmx/tdx/tdx.c | 41 +++++++++++++++++++++++++++++++++++--
> > arch/x86/virt/vmx/tdx/tdx.h | 2 ++
> > 2 files changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 620b35e2a61b..ab961443fed5 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -916,6 +916,36 @@ static int construct_tdmrs(struct list_head *tmb_list,
> > return ret;
> > }
> >
> > +static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
> > +{
> > + u64 *tdmr_pa_array, *p;
> > + size_t array_sz;
> > + int i, ret;
> > +
> > + /*
> > + * TDMRs are passed to the TDX module via an array of physical
> > + * addresses of each TDMR. The array itself has alignment
> > + * requirement.
> > + */
> > + array_sz = tdmr_list->nr_tdmrs * sizeof(u64) +
> > + TDMR_INFO_PA_ARRAY_ALIGNMENT - 1;
>
> One other way of doing this which might be a wee bit less messy:
>
> array_sz = roundup_pow_of_two(array_sz);
> if (array_sz < TDMR_INFO_PA_ARRAY_ALIGNMENT)
> array_sz = TDMR_INFO_PA_ARRAY_ALIGNMENT;
>
> Since that keeps 'array_sz' at a power-of-two, then kzalloc() will give
> you all the alignment you need, except if the array is too small, in
> which case you can just bloat it to the alignment requirement.
>
> This would get rid of the PTR_ALIGN() below too.
>
> Your choice. What you have works too.
Your code can also get rid of the additional 'p' local variable. As you said it
is simpler. I'll use your code. Thanks!
>
> > + p = kzalloc(array_sz, GFP_KERNEL);
> > + if (!p)
> > + return -ENOMEM;
> > +
> > + tdmr_pa_array = PTR_ALIGN(p, TDMR_INFO_PA_ARRAY_ALIGNMENT);
> > + for (i = 0; i < tdmr_list->nr_tdmrs; i++)
> > + tdmr_pa_array[i] = __pa(tdmr_entry(tdmr_list, i));
> > +
> > + ret = seamcall(TDH_SYS_CONFIG, __pa(tdmr_pa_array), tdmr_list->nr_tdmrs,
> > + global_keyid, 0, NULL, NULL);
> > +
> > + /* Free the array as it is not required anymore. */
> > + kfree(p);
> > +
> > + return ret;
> > +}
> > +
> > static int init_tdx_module(void)
> > {
> > /*
> > @@ -960,17 +990,24 @@ static int init_tdx_module(void)
> > if (ret)
> > goto out_free_tdmrs;
> >
> > + /*
> > + * Use the first private KeyID as the global KeyID, and pass
> > + * it along with the TDMRs to the TDX module.
> > + */
> > + ret = config_tdx_module(&tdmr_list, tdx_keyid_start);
> > + if (ret)
> > + goto out_free_pamts;
>
> This is "consuming" tdx_keyid_start. Does it need to get incremented
> since the first guest can't use this KeyID now?
It depends on how we treat 'tdx_keyid_start'. If it means the first _usable_
KeyID for KVM, then we should increase it; but if it only used for the hardware-
enabled TDX KeyID range, then we don't need to increase it.
Currently it is marked as __ro_after_init so my intention is the latter (also in
the spirit of keeping this series minimal).
Eventually we will need to have functions to allocate/free TDX KeyIDs anyway for
KVM, but in that we can just treat 'tdx_keyid_start + 1' as the first usable
KeyID.
[snip]
