On Fri, 2023-09-15 at 11:42 +0000, Huang, Kai wrote: > On Thu, 2023-09-14 at 21:36 +0000, Edgecombe, Rick P wrote: > > On Sat, 2023-08-26 at 00:14 +1200, Kai Huang wrote: > > > diff --git a/arch/x86/kernel/machine_kexec_64.c > > > b/arch/x86/kernel/machine_kexec_64.c > > > index 1a3e2c05a8a5..03d9689ef808 100644 > > > --- a/arch/x86/kernel/machine_kexec_64.c > > > +++ b/arch/x86/kernel/machine_kexec_64.c > > > @@ -28,6 +28,7 @@ > > > #include <asm/setup.h> > > > #include <asm/set_memory.h> > > > #include <asm/cpu.h> > > > +#include <asm/tdx.h> > > > > > > #ifdef CONFIG_ACPI > > > /* > > > @@ -301,6 +302,14 @@ void machine_kexec(struct kimage *image) > > > void *control_page; > > > int save_ftrace_enabled; > > > > > > + /* > > > + * For platforms with TDX "partial write machine check" > > > erratum, > > > + * all TDX private pages need to be converted back to > > > normal > > > + * before booting to the new kernel, otherwise the new > > > kernel > > > + * may get unexpected machine check. > > > + */ > > > + tdx_reset_memory(); > > > + > > > #ifdef CONFIG_KEXEC_JUMP > > > if (image->preserve_context) > > > save_processor_state(); > > > > Without a ton of knowledge on TDX arch stuff, I'm mostly looked at > > the > > kexec flow with respect to anything that might be tinkering with > > the > > PAMT. Everything there looked good to me. > > > > But I'm wondering if you want to skip the tdx_reset_memory() in the > > KEXEC_JUMP/preserve_context case. Somehow (I'm not clear on all the > > details), kexec can be configured to have the new kernel jump back > > to > > the old kernel and resume execution as if nothing happened. Then I > > think you would want to keep the TDX data around. Does that make > > any > > sense? > > > > Good point. Thanks! > > Based on my understanding, it should be OK to skip tdx_reset_memory() > (or better > to) when preserve_context is on. The second kernel shouldn't touch > first > kernel's memory anyway otherwise it may corrupt the first kernel > state (if it > does this maliciously or accidentally, then the first kernel isn't > guaranteed to > work anyway). I think it may read the memory, is it ok? > > In fact, if we do tdx_reset_memory() when preserve_memory is on, we > will need to > do additional things to mark TDX as dead otherwise after jumping back > other > kernel code will still believe TDX is alive and continue to use TDX. > > I'll do this if I don't hear objection from other people. > > Something like below? > > diff --git a/arch/x86/kernel/machine_kexec_64.c > b/arch/x86/kernel/machine_kexec_64.c > index 03d9689ef808..73ed01360408 100644 > --- a/arch/x86/kernel/machine_kexec_64.c > +++ b/arch/x86/kernel/machine_kexec_64.c > @@ -307,12 +307,18 @@ void machine_kexec(struct kimage *image) > * all TDX private pages need to be converted back to normal > * before booting to the new kernel, otherwise the new kernel > * may get unexpected machine check. > + * > + * But skip this when preserve_context is on. The second > kernel > + * shouldn't touch the first kernel's memory anyway. > Skipping > + * this also avoids killing TDX in the first kernel, which > would > + * require more complicated handling. > */ > - tdx_reset_memory(); > - > #ifdef CONFIG_KEXEC_JUMP > if (image->preserve_context) > save_processor_state(); > + else > +#else > + tdx_reset_memory(); > #endif > > Not the most beautiful ifdeffery, I'd just duplicate the tdx_reset_memory() call. But not a strong opinion.
