Re: [PATCH 1/4] x86/coco: Add a new CC attribute to unify cache flush during kexec

[Huang, Kai]

On Tue, 2024-02-20 at 16:30 -0600, Tom Lendacky wrote:
> On 2/20/24 14:07, Huang, Kai wrote:
> > On Tue, 2024-02-20 at 08:47 -0600, Tom Lendacky wrote:
> > > On 2/20/24 08:28, Borislav Petkov wrote:
> > > > On Mon, Feb 19, 2024 at 04:09:47PM -0600, Tom Lendacky wrote:
> > > > > That's why the '!(sev_status & MSR_AMD64_SEV_ENABLED)' works here.
> > > > 
> > > > I would've never figured that out just from staring at the test. :-\
> > > > 
> > > > > Basically, if you are bare-metal, it will return true. And it will only
> > > > > return true for machines that support SME and have the
> > > > > MSR_AMD64_SYSCFG_MEM_ENCRYPT bit set in SYS_CFG MSR because of where the
> > > > > 'cc_vendor = CC_VENDOR_AMD' assignment is. However, if you move the
> > > > > 'cc_vendor = CC_VENDOR_AMD' to before the if statement, then you will have
> > > > > the WBINVD called for any machine that supports SME, even if SME is not
> > > > > possible because the proper bit in the SYS_CFG MSR hasn't been set.
> > > > > 
> > > > > I know what I'm trying to say, let me know if it is making sense...
> > > > 
> > > > Yah, thanks for taking the time to explain.
> > > > 
> > > > Here's an even more radical idea:
> > > > 
> > > > Why not do WBINVD *unconditionally* on the CPU down path?
> > > > 
> > > > - it is the opposite of a fast path, i.e., no one cares
> > > > 
> > > > - it'll take care of every possible configuration without ugly logic
> > > > 
> > > > - it wouldn't hurt to have the caches nice and coherent before going
> > > >     down
> > > > 
> > > > Hmmm.
> > > 
> > > That's what I initially did, but errors were reported, see commit:
> > >     f23d74f6c66c ("x86/mm: Rework wbinvd, hlt operation in stop_this_cpu()")
> > 
> > This changelog only mentions "Some issues".  Do you know exactly what kind
> > issues did you see?  Are these issues only appeared on SME enabled system or
> > other non-SME-capable systems too?
> 
> I believe the issues were that different Intel systems would hang or reset 
> and it was bisected to that commit that added the WBINVD. It was a while 
> ago, but I remember that they were similar to what the 1f5e7eb7868e commit 
> ended up fixing, which was debugged because sometimes the WBINVD was still 
> occasionally issued resulting in the following patch
> 
>    9b040453d444 ("x86/smp: Dont access non-existing CPUID leaf")
> 
> It just means that if we go to an unconditional WBINVD, then we need to be 
> careful.
> 
> 

Thanks Tom for the info.  That helps a lot.

Hi Boris, Dave,

I think I still prefer to keeping the existing SME kexec behaviour, that is, to
have the new CC_ATTR_HOST_MEM_INCOHERENT attribute, because in this way there
will be no risk.

However based on the information above I believe the risk is small if we switch
to unconditional WBINVD, in which way we don't need the new attribute and
there's also no new code needed for TDX to do cache flush.

Btw, I want to point out stop_this_cpu() is not the only place that needs to do
WBINVD for SME/TDX, the relocate_kernel() assembly also needs to:

        image->start = relocate_kernel((unsigned long)image->head,
                                     (unsigned long)page_list,
                                     image->start,
                                     image->preserve_context,
                                     cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT));

The last function argument cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) is for SME.
The relocate_kernel() assembly checks the last argument and does WBINVD if it is
true.  If we go with unconditional WBINVD, I think we can just change the
assembly to do unconditional WBINVD and remove the last function parameter.

Please let me know your preference?