Re: [PATCH v6 18/21] x86/virt/tdx: Configure global KeyID on all packages

[Andi Kleen]

On 10/26/2022 4:16 PM, Kai Huang wrote:
> After the array of TDMRs and the global KeyID are configured to the TDX
> module, use TDH.SYS.KEY.CONFIG to configure the key of the global KeyID
> on all packages.
>
> TDH.SYS.KEY.CONFIG must be done on one (any) cpu for each package.  And
> it cannot run concurrently on different CPUs.  Implement a helper to
> run SEAMCALL on one cpu for each package one by one, and use it to
> configure the global KeyID on all packages.
>
> Intel hardware doesn't guarantee cache coherency across different
> KeyIDs.  The kernel needs to flush PAMT's dirty cachelines (associated
> with KeyID 0) before the TDX module uses the global KeyID to access the
> PAMT.  Following the TDX module specification, flush cache before
> configuring the global KeyID on all packages.
>
> Given the PAMT size can be large (~1/256th of system RAM), just use
> WBINVD on all CPUs to flush.
>
> Reviewed-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> ---
>   arch/x86/virt/vmx/tdx/tdx.c | 83 ++++++++++++++++++++++++++++++++++++-
>   arch/x86/virt/vmx/tdx/tdx.h |  1 +
>   2 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index fdfce715dda6..9cfb01e7666a 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -354,6 +354,46 @@ static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
>   	on_each_cpu(seamcall_smp_call_function, sc, true);
>   }
>   
> +/*
> + * Call one SEAMCALL on one (any) cpu for each physical package in
> + * serialized way.  Return immediately in case of any error if
> + * SEAMCALL fails on any cpu.


It's not clear what are you serializing against (against itself or other 
calls of this functions)

I assume its because the TDX module errors out for parallel calls 
instead of waiting.

The code seems to only do itself, so where is the check against others? 
I assume in the callers but that would need to be explained. Also could 
it need serialization against other kinds of seam calls?

Perhaps it might be more efficient to just broad cast and handle a retry 
with some synchronization in the low level code.

That likely would cause less review thrash than just reimplementing a 
common function like this here.


-Andi