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

[Huang, Kai]

On Mon, 2024-02-19 at 16:09 -0600, Tom Lendacky wrote:
> On 2/19/24 14:32, Borislav Petkov wrote:
> > On Mon, Feb 19, 2024 at 01:45:37PM -0600, Tom Lendacky wrote:
> > > This change won't return the correct answer. The check needs to remain
> > > against the sev_status value.
> > 
> > Feel free to explain because this patch is confusing me.
> 
> In your previous email, you want to put the CC_ATTR_HOST_MEM_INCOHERENT 
> case statement with the CC_ATTR_MEM_ENCRYPT case which is returning 
> sme_me_mask. That will be zero/false if SME is not active, skipping the 
> WBINVD. But, in reality you still need to perform the WBINVD in case the 
> kexec target is doing mem_encrypt=on.
> 
> That's why the '!(sev_status & MSR_AMD64_SEV_ENABLED)' works here. 
> 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.

Hi Tom,

Thanks for clarifying.  However it seems to me that this is the behaviour in the
current upstream code.  The stop_this_cpu() checks CPUID directly w/o checking
the SYS_CFG MSR:

        if (c->extended_cpuid_level >= 0x8000001f && 
			(cpuid_eax(0x8000001f) & BIT(0)))                                           
                native_wbinvd();  

I believe the BIT(0) in CPUID, which is "Secure Memory Encryption Support", will
always be true regardless of whether the MSR_AMD64_SYSCFG_MEM_ENCRYPT is set in
SYS_CFG MSR?

If so, IIUC moving the 'cc_vendor = CC_VENDOR_AMD' to the place right before the
if statement as suggested by Boris seems just follows the current behaviour in
the upstream code.

Of course we need to always return true for CC_ATTR_HOST_MEM_INCOHERENT but not
querying sme_me_mask.

> 
> I know what I'm trying to say, let me know if it is making sense...
> 
> > 
> > > > So you can't put it before the if - just slap it in both branches. Geez!
> > > 
> > > I think that will still work because sme_me_mask and sev_status will both be
> > > 0 on bare-metal if 'msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT' doesn't evaluate to
> > > true. However, that will cause any platform that hasn't enabled memory
> > > encryption (see SYS_CFG MSR), to also perform the WBINVD.
> > 
> > If it keeps the code simpler I don't mind. That's so not a fast path.
> > 
> > > That won't work, because the current system may not have SME active. The
> > > cases that needs to be caught are kexec'ing from a mem_encrypt=off to a
> > > mem_encrypt=on or from a mem_encrypt=on to a mem_encrypt=off.
> > 
> > And I'm saying, we should keep it simple and simply WBINVD on SME
> > capable machines, regardless of the encryption setting.
> 
> In that case, CC_ATTR_HOST_MEM_INCOHERENT needs to be separate from 
> CC_ATTR_MEM_ENCRYPT as the original patch has it. The comment might make 
> more sense as:
> 
> 	 * CC_ATTR_HOST_MEM_INCOHERENT represents whether SME is possible
> 	 * on the platform, regardless of whether mem_encrypt=on has been
> 	 * used to make SME active.
> 
> Thanks,
> Tom

This looks good to me.  Thanks!