On 11/20/22 16:26, Kai Huang wrote:
> TDX supports shutting down the TDX module at any time during its
> lifetime. After the module is shut down, no further TDX module SEAMCALL
> leaf functions can be made to the module on any logical cpu.
>
> Shut down the TDX module in case of any error during the initialization
> process. It's pointless to leave the TDX module in some middle state.
>
> Shutting down the TDX module requires calling TDH.SYS.LP.SHUTDOWN on all
> BIOS-enabled CPUs, and the SEMACALL can run concurrently on different
> CPUs. Implement a mechanism to run SEAMCALL concurrently on all online
> CPUs and use it to shut down the module. Later logical-cpu scope module
> initialization will use it too.
To me, this starts to veer way too far into internal implementation details.
Issue the TDH.SYS.LP.SHUTDOWN SEAMCALL on all BIOS-enabled CPUs
to shut down the TDX module.
This is also the point where you should talk about the new
infrastructure. Why do you need a new 'struct seamcall_something'?
What makes it special?
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index b06c1a2bc9cb..5db1a05cb4bd 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -13,6 +13,8 @@
> #include <linux/mutex.h>
> #include <linux/cpu.h>
> #include <linux/cpumask.h>
> +#include <linux/smp.h>
> +#include <linux/atomic.h>
> #include <asm/msr-index.h>
> #include <asm/msr.h>
> #include <asm/apic.h>
> @@ -124,15 +126,27 @@ bool platform_tdx_enabled(void)
> return !!tdx_keyid_num;
> }
>
> +/*
> + * Data structure to make SEAMCALL on multiple CPUs concurrently.
> + * @err is set to -EFAULT when SEAMCALL fails on any cpu.
> + */
> +struct seamcall_ctx {
> + u64 fn;
> + u64 rcx;
> + u64 rdx;
> + u64 r8;
> + u64 r9;
> + atomic_t err;
> +};
> +
> /*
> * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
> * to kernel error code. @seamcall_ret and @out contain the SEAMCALL
> * leaf function return code and the additional output respectively if
> * not NULL.
> */
> -static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> - u64 *seamcall_ret,
> - struct tdx_module_output *out)
> +static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + u64 *seamcall_ret, struct tdx_module_output *out)
> {
> u64 sret;
>
> @@ -166,6 +180,25 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> }
> }
>
> +static void seamcall_smp_call_function(void *data)
> +{
> + struct seamcall_ctx *sc = data;
> + int ret;
> +
> + ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9, NULL, NULL);
> + if (ret)
> + atomic_set(&sc->err, -EFAULT);
> +}
The atomic_t is kinda silly. I guess it's not *that* wasteful though.
I think it would have actually been a lot more clear if instead of
containing an errno it was a *count* of the number of encountered errors.
An "atomic_set()" where everyone is overwriting each other is a bit
counterintuitive. It's OK here, of course, but it still looks goofy.
If this were:
atomic_inc(&sc->nr_errors);
it would be a lot more clear that *anyone* can increment and that it
truly is shared.
> +/*
> + * Call the SEAMCALL on all online CPUs concurrently. Caller to check
> + * @sc->err to determine whether any SEAMCALL failed on any cpu.
> + */
> +static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
> +{
> + on_each_cpu(seamcall_smp_call_function, sc, true);
> +}
> +
> /*
> * Detect and initialize the TDX module.
> *
> @@ -181,7 +214,9 @@ static int init_tdx_module(void)
>
> static void shutdown_tdx_module(void)
> {
> - /* TODO: Shut down the TDX module */
> + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN };
> +
> + seamcall_on_each_cpu(&sc);
> }
The seamcall_on_each_cpu() function is silly as-is. Either collapse the
functions or note in the changelog why this is not as silly as it looks.
> static int __tdx_enable(void)
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 92a8de957dc7..215cc1065d78 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -12,6 +12,11 @@
> /* MSR to report KeyID partitioning between MKTME and TDX */
> #define MSR_IA32_MKTME_KEYID_PARTITIONING 0x00000087
>
> +/*
> + * TDX module SEAMCALL leaf functions
> + */
> +#define TDH_SYS_LP_SHUTDOWN 44
> +
> /*
> * Do not put any hardware-defined TDX structure representations below
> * this comment!
