| Message ID | 3f19ac995d184e52107e7117a82376cb7ecb35e7.1649219184.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host kernel support | expand |
On 4/5/22 9:49 PM, Kai Huang wrote: > TDX supports shutting down the TDX module at any time during its > lifetime. After TDX module is shut down, no further SEAMCALL can be > made on any logical cpu. > > Shut down the TDX module in case of any error happened 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 May be adding specification reference will help. > BIOS-enabled cpus, and the SEMACALL can run concurrently on different > cpus. Implement a mechanism to run SEAMCALL concurrently on all online From TDX Module spec, sec 13.4.1 titled "Shutdown Initiated by the Host VMM (as Part of Module Update)", TDH.SYS.LP.SHUTDOWN is designed to set state variables to block all SEAMCALLs on the current LP and all SEAMCALL leaf functions except TDH.SYS.LP.SHUTDOWN on the other LPs. As per above spec reference, executing TDH.SYS.LP.SHUTDOWN in one LP prevent all SEAMCALL leaf function on all other LPs. If so, why execute it on all CPUs? > cpus. Logical-cpu scope initialization will use it too. Concurrent SEAMCALL support seem to be useful for other SEAMCALL types as well. If you agree, I think it would be better if you move it out to a separate common patch. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++++++++++++++++++- > arch/x86/virt/vmx/tdx/tdx.h | 5 +++++ > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 674867bccc14..faf8355965a5 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -11,6 +11,8 @@ > #include <linux/cpumask.h> > #include <linux/mutex.h> > #include <linux/cpu.h> > +#include <linux/smp.h> > +#include <linux/atomic.h> > #include <asm/msr-index.h> > #include <asm/msr.h> > #include <asm/cpufeature.h> > @@ -328,6 +330,39 @@ static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > return 0; > } > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ > +struct seamcall_ctx { > + u64 fn; > + u64 rcx; > + u64 rdx; > + u64 r8; > + u64 r9; > + atomic_t err; > + u64 seamcall_ret; > + struct tdx_module_output out; > +}; > + > +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, > + &sc->seamcall_ret, &sc->out); > + if (ret) > + atomic_set(&sc->err, ret); > +} > + > +/* > + * Call the SEAMCALL on all online cpus concurrently. > + * Return error if SEAMCALL fails on any cpu. > + */ > +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) > +{ > + on_each_cpu(seamcall_smp_call_function, sc, true); > + return atomic_read(&sc->err); > +} > + > static inline bool p_seamldr_ready(void) > { > return !!p_seamldr_info.p_seamldr_ready; > @@ -437,7 +472,10 @@ 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); May be check the error and WARN_ON on failure? > + > tdx_module_status = TDX_MODULE_SHUTDOWN; > } > > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index 6990c93198b3..dcc1f6dfe378 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -35,6 +35,11 @@ struct p_seamldr_info { > #define P_SEAMLDR_SEAMCALL_BASE BIT_ULL(63) > #define P_SEAMCALL_SEAMLDR_INFO (P_SEAMLDR_SEAMCALL_BASE | 0x0) > > +/* > + * TDX module SEAMCALL leaf functions > + */ > +#define TDH_SYS_LP_SHUTDOWN 44 > + > struct tdx_module_output; > u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > struct tdx_module_output *out);
On Sat, 2022-04-23 at 08:39 -0700, Sathyanarayanan Kuppuswamy wrote: > > On 4/5/22 9:49 PM, Kai Huang wrote: > > TDX supports shutting down the TDX module at any time during its > > lifetime. After TDX module is shut down, no further SEAMCALL can be > > made on any logical cpu. > > > > Shut down the TDX module in case of any error happened 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 > > May be adding specification reference will help. How about adding the reference to the code comment? Here we just need some fact description. Adding reference to the code comment also allows people to find the relative part in the spec easily when they are looking at the actual code (i.e. after the code is merged to upstream). Otherwise people needs to do a git blame and find the exact commit message for that. > > > BIOS-enabled cpus, and the SEMACALL can run concurrently on different > > cpus. Implement a mechanism to run SEAMCALL concurrently on all online > > From TDX Module spec, sec 13.4.1 titled "Shutdown Initiated by the Host > VMM (as Part of Module Update)", > > TDH.SYS.LP.SHUTDOWN is designed to set state variables to block all > SEAMCALLs on the current LP and all SEAMCALL leaf functions except > TDH.SYS.LP.SHUTDOWN on the other LPs. > > As per above spec reference, executing TDH.SYS.LP.SHUTDOWN in > one LP prevent all SEAMCALL leaf function on all other LPs. If so, > why execute it on all CPUs? Prevent all SEAMCALLs on other LPs except TDH.SYS.LP.SHUTDOWN. The spec defnies shutting down the TDX module as running this SEAMCALl on all LPs, so why just run on a single cpu? What's the benefit? Also, the spec also mentions for runtime update, "SEAMLDR can check that TDH.SYS.SHUTDOWN has been executed on all LPs". Runtime update isn't supported in this series, but it can leverage the existing code if we run SEAMCALL on all LPs to shutdown the module as spec suggested. Why just run on a single cpu? > > > cpus. Logical-cpu scope initialization will use it too. > > Concurrent SEAMCALL support seem to be useful for other SEAMCALL > types as well. If you agree, I think it would be better if you move > it out to a separate common patch. There are couple of problems of doing that: - All the functions are static in this tdx.c. Introducing them separately in dedicated patch would result in compile warning about those static functions are not used. - I have received comments from others I can add those functions when they are firstly used. Given those functions is not large, so I prefer this way too. > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++++++++++++++++++- > > arch/x86/virt/vmx/tdx/tdx.h | 5 +++++ > > 2 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > > index 674867bccc14..faf8355965a5 100644 > > --- a/arch/x86/virt/vmx/tdx/tdx.c > > +++ b/arch/x86/virt/vmx/tdx/tdx.c > > @@ -11,6 +11,8 @@ > > #include <linux/cpumask.h> > > #include <linux/mutex.h> > > #include <linux/cpu.h> > > +#include <linux/smp.h> > > +#include <linux/atomic.h> > > #include <asm/msr-index.h> > > #include <asm/msr.h> > > #include <asm/cpufeature.h> > > @@ -328,6 +330,39 @@ static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > > return 0; > > } > > > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ > > +struct seamcall_ctx { > > + u64 fn; > > + u64 rcx; > > + u64 rdx; > > + u64 r8; > > + u64 r9; > > + atomic_t err; > > + u64 seamcall_ret; > > + struct tdx_module_output out; > > +}; > > + > > +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, > > + &sc->seamcall_ret, &sc->out); > > + if (ret) > > + atomic_set(&sc->err, ret); > > +} > > + > > +/* > > + * Call the SEAMCALL on all online cpus concurrently. > > + * Return error if SEAMCALL fails on any cpu. > > + */ > > +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) > > +{ > > + on_each_cpu(seamcall_smp_call_function, sc, true); > > + return atomic_read(&sc->err); > > +} > > + > > static inline bool p_seamldr_ready(void) > > { > > return !!p_seamldr_info.p_seamldr_ready; > > @@ -437,7 +472,10 @@ 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); > > May be check the error and WARN_ON on failure? When SEAMCALL fails, the error code will be printed out actually (please see previous patch), so I thought there's no need to WARN_ON() here (and some other similar places). I am not sure the additional WARN_ON() will do any help?
On 4/25/22 4:41 PM, Kai Huang wrote: > On Sat, 2022-04-23 at 08:39 -0700, Sathyanarayanan Kuppuswamy wrote: >> >> On 4/5/22 9:49 PM, Kai Huang wrote: >>> TDX supports shutting down the TDX module at any time during its >>> lifetime. After TDX module is shut down, no further SEAMCALL can be >>> made on any logical cpu. >>> >>> Shut down the TDX module in case of any error happened 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 >> >> May be adding specification reference will help. > > How about adding the reference to the code comment? Here we just need some fact > description. Adding reference to the code comment also allows people to find > the relative part in the spec easily when they are looking at the actual code > (i.e. after the code is merged to upstream). Otherwise people needs to do a git > blame and find the exact commit message for that. If it is not a hassle, you can add references both in code and at the end of the commit log. Adding two more lines to the commit log should not be difficult. I think it is fine either way. Your choice. > >> >>> BIOS-enabled cpus, and the SEMACALL can run concurrently on different >>> cpus. Implement a mechanism to run SEAMCALL concurrently on all online >> >> From TDX Module spec, sec 13.4.1 titled "Shutdown Initiated by the Host >> VMM (as Part of Module Update)", >> >> TDH.SYS.LP.SHUTDOWN is designed to set state variables to block all >> SEAMCALLs on the current LP and all SEAMCALL leaf functions except >> TDH.SYS.LP.SHUTDOWN on the other LPs. >> >> As per above spec reference, executing TDH.SYS.LP.SHUTDOWN in >> one LP prevent all SEAMCALL leaf function on all other LPs. If so, >> why execute it on all CPUs? > > Prevent all SEAMCALLs on other LPs except TDH.SYS.LP.SHUTDOWN. The spec defnies > shutting down the TDX module as running this SEAMCALl on all LPs, so why just > run on a single cpu? What's the benefit? If executing it in one LP prevents SEAMCALLs on all other LPs, I am trying to understand why spec recommends running it in all LPs? But the following explanation answers my query. I recommend making a note about it in commit log or comments. > > Also, the spec also mentions for runtime update, "SEAMLDR can check that > TDH.SYS.SHUTDOWN has been executed on all LPs". Runtime update isn't supported > in this series, but it can leverage the existing code if we run SEAMCALL on all > LPs to shutdown the module as spec suggested. Why just run on a single cpu? > >> >>> cpus. Logical-cpu scope initialization will use it too. >> >> Concurrent SEAMCALL support seem to be useful for other SEAMCALL >> types as well. If you agree, I think it would be better if you move >> it out to a separate common patch. > > There are couple of problems of doing that: > > - All the functions are static in this tdx.c. Introducing them separately in > dedicated patch would result in compile warning about those static functions are > not used. > - I have received comments from others I can add those functions when they are > firstly used. Given those functions is not large, so I prefer this way too. Ok > >> >>> >>> Signed-off-by: Kai Huang <kai.huang@intel.com> >>> --- >>> arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++++++++++++++++++- >>> arch/x86/virt/vmx/tdx/tdx.h | 5 +++++ >>> 2 files changed, 44 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c >>> index 674867bccc14..faf8355965a5 100644 >>> --- a/arch/x86/virt/vmx/tdx/tdx.c >>> +++ b/arch/x86/virt/vmx/tdx/tdx.c >>> @@ -11,6 +11,8 @@ >>> #include <linux/cpumask.h> >>> #include <linux/mutex.h> >>> #include <linux/cpu.h> >>> +#include <linux/smp.h> >>> +#include <linux/atomic.h> >>> #include <asm/msr-index.h> >>> #include <asm/msr.h> >>> #include <asm/cpufeature.h> >>> @@ -328,6 +330,39 @@ static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, >>> return 0; >>> } >>> >>> +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ >>> +struct seamcall_ctx { >>> + u64 fn; >>> + u64 rcx; >>> + u64 rdx; >>> + u64 r8; >>> + u64 r9; >>> + atomic_t err; >>> + u64 seamcall_ret; >>> + struct tdx_module_output out; >>> +}; >>> + >>> +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, >>> + &sc->seamcall_ret, &sc->out); >>> + if (ret) >>> + atomic_set(&sc->err, ret); >>> +} >>> + >>> +/* >>> + * Call the SEAMCALL on all online cpus concurrently. >>> + * Return error if SEAMCALL fails on any cpu. >>> + */ >>> +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) >>> +{ >>> + on_each_cpu(seamcall_smp_call_function, sc, true); >>> + return atomic_read(&sc->err); >>> +} >>> + >>> static inline bool p_seamldr_ready(void) >>> { >>> return !!p_seamldr_info.p_seamldr_ready; >>> @@ -437,7 +472,10 @@ 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); >> >> May be check the error and WARN_ON on failure? > > When SEAMCALL fails, the error code will be printed out actually (please see > previous patch), so I thought there's no need to WARN_ON() here (and some other > similar places). I am not sure the additional WARN_ON() will do any help? OK. I missed that part. >
> > > > Prevent all SEAMCALLs on other LPs except TDH.SYS.LP.SHUTDOWN. The spec defnies > > shutting down the TDX module as running this SEAMCALl on all LPs, so why just > > run on a single cpu? What's the benefit? > > If executing it in one LP prevents SEAMCALLs on all other LPs, I am > trying to understand why spec recommends running it in all LPs? Please see 3.1.2 Intel TDX Module Shutdown and Update The "shutdown" case requires "Execute On" on "Each LP". Also, TDH.SYS.LP.SHUTDOWN describe this is shutdown on *current* LP. > > But the following explanation answers my query. I recommend making a > note about it in commit log or comments. Is above enough to address your question?
On 4/5/22 21:49, Kai Huang wrote: > TDX supports shutting down the TDX module at any time during its > lifetime. After TDX module is shut down, no further SEAMCALL can be > made on any logical cpu. Is this strictly true? I thought SEAMCALLs were used for the P-SEAMLDR too. > Shut down the TDX module in case of any error happened 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. Logical-cpu scope initialization will use it too. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++++++++++++++++++- > arch/x86/virt/vmx/tdx/tdx.h | 5 +++++ > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 674867bccc14..faf8355965a5 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -11,6 +11,8 @@ > #include <linux/cpumask.h> > #include <linux/mutex.h> > #include <linux/cpu.h> > +#include <linux/smp.h> > +#include <linux/atomic.h> > #include <asm/msr-index.h> > #include <asm/msr.h> > #include <asm/cpufeature.h> > @@ -328,6 +330,39 @@ static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > return 0; > } > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ > +struct seamcall_ctx { > + u64 fn; > + u64 rcx; > + u64 rdx; > + u64 r8; > + u64 r9; > + atomic_t err; > + u64 seamcall_ret; > + struct tdx_module_output out; > +}; > + > +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, > + &sc->seamcall_ret, &sc->out); > + if (ret) > + atomic_set(&sc->err, ret); > +} > + > +/* > + * Call the SEAMCALL on all online cpus concurrently. > + * Return error if SEAMCALL fails on any cpu. > + */ > +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) > +{ > + on_each_cpu(seamcall_smp_call_function, sc, true); > + return atomic_read(&sc->err); > +} Why bother returning something that's not read? > static inline bool p_seamldr_ready(void) > { > return !!p_seamldr_info.p_seamldr_ready; > @@ -437,7 +472,10 @@ 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); > + > tdx_module_status = TDX_MODULE_SHUTDOWN; > } > > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index 6990c93198b3..dcc1f6dfe378 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -35,6 +35,11 @@ struct p_seamldr_info { > #define P_SEAMLDR_SEAMCALL_BASE BIT_ULL(63) > #define P_SEAMCALL_SEAMLDR_INFO (P_SEAMLDR_SEAMCALL_BASE | 0x0) > > +/* > + * TDX module SEAMCALL leaf functions > + */ > +#define TDH_SYS_LP_SHUTDOWN 44 > + > struct tdx_module_output; > u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > struct tdx_module_output *out);
On Tue, 2022-04-26 at 13:59 -0700, Dave Hansen wrote: > On 4/5/22 21:49, Kai Huang wrote: > > TDX supports shutting down the TDX module at any time during its > > lifetime. After TDX module is shut down, no further SEAMCALL can be > > made on any logical cpu. > > Is this strictly true? > > I thought SEAMCALLs were used for the P-SEAMLDR too. Sorry will change to no TDX module SEAMCALL can be made on any logical cpu. [...] > > > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ > > +struct seamcall_ctx { > > + u64 fn; > > + u64 rcx; > > + u64 rdx; > > + u64 r8; > > + u64 r9; > > + atomic_t err; > > + u64 seamcall_ret; > > + struct tdx_module_output out; > > +}; > > + > > +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, > > + &sc->seamcall_ret, &sc->out); > > + if (ret) > > + atomic_set(&sc->err, ret); > > +} > > + > > +/* > > + * Call the SEAMCALL on all online cpus concurrently. > > + * Return error if SEAMCALL fails on any cpu. > > + */ > > +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) > > +{ > > + on_each_cpu(seamcall_smp_call_function, sc, true); > > + return atomic_read(&sc->err); > > +} > > Why bother returning something that's not read? It's not needed. I'll make it void. Caller can check seamcall_ctx::err directly if they want to know whether any error happened.
On Tue, Apr 26, 2022 at 5:06 PM Kai Huang <kai.huang@intel.com> wrote: > > On Tue, 2022-04-26 at 13:59 -0700, Dave Hansen wrote: > > On 4/5/22 21:49, Kai Huang wrote: > > > TDX supports shutting down the TDX module at any time during its > > > lifetime. After TDX module is shut down, no further SEAMCALL can be > > > made on any logical cpu. > > > > Is this strictly true? > > > > I thought SEAMCALLs were used for the P-SEAMLDR too. > > Sorry will change to no TDX module SEAMCALL can be made on any logical cpu. > > [...] > > > > > > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ > > > +struct seamcall_ctx { > > > + u64 fn; > > > + u64 rcx; > > > + u64 rdx; > > > + u64 r8; > > > + u64 r9; > > > + atomic_t err; > > > + u64 seamcall_ret; > > > + struct tdx_module_output out; > > > +}; > > > + > > > +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, > > > + &sc->seamcall_ret, &sc->out); Are the seamcall_ret and out fields in seamcall_ctx going to be used? Right now it looks like no one is going to read them. If they are going to be used then this is going to cause a race since the different CPUs are going to write concurrently to the same address inside seamcall(). We should either use local memory and write using atomic_set like the case for the err field or hard code NULL at the call site if they are not going to be used. > > > + if (ret) > > > + atomic_set(&sc->err, ret); > > > +} > > > + > > > +/* > > > + * Call the SEAMCALL on all online cpus concurrently. > > > + * Return error if SEAMCALL fails on any cpu. > > > + */ > > > +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) > > > +{ > > > + on_each_cpu(seamcall_smp_call_function, sc, true); > > > + return atomic_read(&sc->err); > > > +} > > > > Why bother returning something that's not read? > > It's not needed. I'll make it void. > > Caller can check seamcall_ctx::err directly if they want to know whether any > error happened. > > > > -- > Thanks, > -Kai > > Sagi
On Wed, 2022-05-18 at 09:19 -0700, Sagi Shahar wrote: > On Tue, Apr 26, 2022 at 5:06 PM Kai Huang <kai.huang@intel.com> wrote: > > > > On Tue, 2022-04-26 at 13:59 -0700, Dave Hansen wrote: > > > On 4/5/22 21:49, Kai Huang wrote: > > > > TDX supports shutting down the TDX module at any time during its > > > > lifetime. After TDX module is shut down, no further SEAMCALL can be > > > > made on any logical cpu. > > > > > > Is this strictly true? > > > > > > I thought SEAMCALLs were used for the P-SEAMLDR too. > > > > Sorry will change to no TDX module SEAMCALL can be made on any logical cpu. > > > > [...] > > > > > > > > > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ > > > > +struct seamcall_ctx { > > > > + u64 fn; > > > > + u64 rcx; > > > > + u64 rdx; > > > > + u64 r8; > > > > + u64 r9; > > > > + atomic_t err; > > > > + u64 seamcall_ret; > > > > + struct tdx_module_output out; > > > > +}; > > > > + > > > > +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, > > > > + &sc->seamcall_ret, &sc->out); > > Are the seamcall_ret and out fields in seamcall_ctx going to be used? > Right now it looks like no one is going to read them. > If they are going to be used then this is going to cause a race since > the different CPUs are going to write concurrently to the same address > inside seamcall(). > We should either use local memory and write using atomic_set like the > case for the err field or hard code NULL at the call site if they are > not going to be used. > > > > Thanks for catching this. Both 'seamcall_ret' and 'out' are actually not used in this series, but this needs to be improved for sure. I think I can just remove them from the 'seamcall_ctx' for now, since they are not used at all.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 674867bccc14..faf8355965a5 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -11,6 +11,8 @@ #include <linux/cpumask.h> #include <linux/mutex.h> #include <linux/cpu.h> +#include <linux/smp.h> +#include <linux/atomic.h> #include <asm/msr-index.h> #include <asm/msr.h> #include <asm/cpufeature.h> @@ -328,6 +330,39 @@ static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, return 0; } +/* Data structure to make SEAMCALL on multiple CPUs concurrently */ +struct seamcall_ctx { + u64 fn; + u64 rcx; + u64 rdx; + u64 r8; + u64 r9; + atomic_t err; + u64 seamcall_ret; + struct tdx_module_output out; +}; + +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, + &sc->seamcall_ret, &sc->out); + if (ret) + atomic_set(&sc->err, ret); +} + +/* + * Call the SEAMCALL on all online cpus concurrently. + * Return error if SEAMCALL fails on any cpu. + */ +static int seamcall_on_each_cpu(struct seamcall_ctx *sc) +{ + on_each_cpu(seamcall_smp_call_function, sc, true); + return atomic_read(&sc->err); +} + static inline bool p_seamldr_ready(void) { return !!p_seamldr_info.p_seamldr_ready; @@ -437,7 +472,10 @@ 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); + tdx_module_status = TDX_MODULE_SHUTDOWN; } diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h index 6990c93198b3..dcc1f6dfe378 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -35,6 +35,11 @@ struct p_seamldr_info { #define P_SEAMLDR_SEAMCALL_BASE BIT_ULL(63) #define P_SEAMCALL_SEAMLDR_INFO (P_SEAMLDR_SEAMCALL_BASE | 0x0) +/* + * TDX module SEAMCALL leaf functions + */ +#define TDH_SYS_LP_SHUTDOWN 44 + struct tdx_module_output; u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, struct tdx_module_output *out);
TDX supports shutting down the TDX module at any time during its lifetime. After TDX module is shut down, no further SEAMCALL can be made on any logical cpu. Shut down the TDX module in case of any error happened 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. Logical-cpu scope initialization will use it too. Signed-off-by: Kai Huang <kai.huang@intel.com> --- arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++++++++++++++++++- arch/x86/virt/vmx/tdx/tdx.h | 5 +++++ 2 files changed, 44 insertions(+), 1 deletion(-)