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

[Huang, Kai]

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.