Re: [PATCH v2 2/5] x86/kexec: do unconditional WBINVD in relocate_kernel()

["Huang, Kai" <kai.huang@intel.com>]

On 20/03/2024 1:19 pm, Kirill A. Shutemov wrote:
> On Wed, Mar 20, 2024 at 10:20:50AM +1300, Huang, Kai wrote:
>>
>>
>> On 20/03/2024 3:38 am, Tom Lendacky wrote:
>>> On 3/19/24 06:13, Kirill A. Shutemov wrote:
>>>> On Tue, Mar 19, 2024 at 01:48:45AM +0000, Kai Huang wrote:
>>>>> Both SME and TDX can leave caches in incoherent state due to memory
>>>>> encryption.  During kexec, the caches must be flushed before jumping to
>>>>> the second kernel to avoid silent memory corruption to the
>>>>> second kernel.
>>>>>
>>>>> During kexec, the WBINVD in stop_this_cpu() flushes caches for all
>>>>> remote cpus when they are being stopped.  For SME, the WBINVD in
>>>>> relocate_kernel() flushes the cache for the last running cpu (which is
>>>>> executing the kexec).
>>>>>
>>>>> Similarly, for TDX after stopping all remote cpus with cache flushed, to
>>>>> support kexec, the kernel needs to flush cache for the last running cpu.
>>>>>
>>>>> Make the WBINVD in the relocate_kernel() unconditional to cover both SME
>>>>> and TDX.
>>>>
>>>> Nope. It breaks TDX guest. WBINVD triggers #VE for TDX guests.
>>>
>>> Ditto for SEV-ES/SEV-SNP, a #VC is generated and crashes the guest.
>>>
>>
>> Oh I forgot these.
>>
>> Hi Kirill,
>>
>> Then I think patch 1 will also break TDX guest after your series to enable
>> multiple cpus for the second kernel after kexec()?
> 
> Well, not exactly.
> 
> My patchset overrides stop_this_cpu() with own implementation for MADT
> wakeup method that doesn't have WBINVD. So the patch doesn't break
> anything, 

Well, your callback actually only gets called _after_ this WBINVD, so...

I guess I should have that checked by myself. :-)

but if in the future TDX (or SEV) host would use MADT wake up
> method instead of IPI we will get back to the problem with missing
> WBINVD.
> 
> I don't know if we care. There's no reason for host to use MADT wake up
> method.
> 

I don't think MADT wake up will be used at any native environment.

Anyway, regardless whether patch 1 will break TDX/SEV-ES/SEV-SNP guests, 
I think to resolve this, we can simply adjust our mindset from ...

	"do unconditional WBINVD"

to ...

	"do unconditional WBINVD when it can be done safely"

For now, AFAICT, only TDX guests and SEV-ES/SEV-SNP guests are such guests.

And they all report the CC_ATTR_GUEST_MEM_ENCRYPT flag as true, so we 
can change to only do WBINVD when the kernel sees that flag.

	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
		native_wbinvd();

Alternatively, we can have a dedicated X86_FEATURE_NO_WBINVD and get it 
set for TDX/SEV-ES/SEV-SNP guests (and any guests if this is true), and do:

	if (!boot_cpu_has(X86_FEATURE_NO_WBINVD))
		native_wbinvd();

It seems the first one is too generic (for any CoCo VMs), and the second 
one is better.

Any comments?

Hi Boris/Dave,

Do you have any comments?