Re: [RFC PATCH 1/5] x86/kexec: Do unconditional WBINVD for bare-metal in stop_this_cpu()

From: Huang, Kai
Date: Fri Mar 14 2025 - 06:05:30 EST


On Thu, 2025-03-13 at 18:40 +0000, Edgecombe, Rick P wrote:
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index 9c75d701011f..8475d9d2d8c4 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -819,18 +819,19 @@ void __noreturn stop_this_cpu(void *dummy)
> >    mcheck_cpu_clear(c);
> >  
> >    /*
> > - * Use wbinvd on processors that support SME. This provides support
> > - * for performing a successful kexec when going from SME inactive
> > - * to SME active (or vice-versa). The cache must be cleared so that
> > - * if there are entries with the same physical address, both with and
> > - * without the encryption bit, they don't race each other when flushed
> > - * and potentially end up with the wrong entry being committed to
> > - * memory.
> > + * Use wbinvd to support kexec for both SME (from inactive to active
> > + * or vice-versa) and TDX.  The cache must be cleared so that if there
> > + * are entries with the same physical address, both with and without
> > + * the encryption bit(s), they don't race each other when flushed and
> > + * potentially end up with the wrong entry being committed to memory.
> >    *
> > - * Test the CPUID bit directly because the machine might've cleared
> > - * X86_FEATURE_SME due to cmdline options.
> > + * stop_this_cpu() isn't a fast path, just do unconditional WBINVD for
> > + * bare-metal to cover both SME and TDX.  Do not do WBINVD in a guest
> > + * since performing one will result in an exception (#VE or #VC) for
> > + * TDX or SEV-ES/SEV-SNP guests which the guest may not be able to
> > + * handle (e.g., TDX guest panics if it sees #VE).
> >    */
> > - if (c->extended_cpuid_level >= 0x8000001f && (cpuid_eax(0x8000001f) & BIT(0)))
> > + if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))
> >    wbinvd();
>
> I see that this already has Tom's RB, but I'm not sure how this works for AMD.
> The original SME patch tried to avoid writing to memory by putting the wbinvd
> immediately before the halt, but today it is further away. Below this hunk there
> are more instructions that could dirty memory before the halt.  Ohh... it's new.
> 9 months ago 26ba7353caaa ("x86/smp: Add smp_ops.stop_this_cpu() callback") adds
> a function call that would touch the stack. I think it's wrong? And probably
> introduced after this patch was originally written.

(I'll reply others separately since today I am a little bit sick.)

That callback is for TDX guset (which doesn't invoke WBINVD during kexec). It
doesn't impact SME host.

I kinda agree maybe the code here can be improved, e.g., the code to call
smp_ops.stop_this_cpu() perhaps should be called before the WBINVD.

>
> Then the cpuid_eax() could be non-inlined, but probably not. But the
> boot_cpu_has() added in this patch could call out to kasan and dirty the stack.

Could you elaborate this since I don't see how kasan is involved?

>
> So I think the existing SME case might be theoretically incorrect, and if so
> this makes things very slightly worse.

As explained above the function call is empty for SME host.