Received-SPF: pass (google.com: domain of linux-kernel+bounces-140609-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <3ad70c7a-a2d9-c7d9-4921-13000240c086@amd.com>
Date: Thu, 11 Apr 2024 09:13:35 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v3 1/5] x86/kexec: do unconditional WBINVD for bare-metal
 in stop_this_cpu()
Content-Language: en-US
To: "Huang, Kai" <kai.huang@intel.com>, linux-kernel@vger.kernel.org
Cc: x86@kernel.org, dave.hansen@intel.com, bp@alien8.de,
 kirill.shutemov@linux.intel.com, tglx@linutronix.de, mingo@redhat.com,
 hpa@zytor.com, luto@kernel.org, peterz@infradead.org,
 rick.p.edgecombe@intel.com, ashish.kalra@amd.com, chao.gao@intel.com,
 bhe@redhat.com, nik.borisov@suse.com, pbonzini@redhat.com, seanjc@google.com
References: <cover.1712493366.git.kai.huang@intel.com>
 <33b985a8f4346f4bcf0944eaf37193a906b11af3.1712493366.git.kai.huang@intel.com>
 <e94d208e-964f-cfc0-60a4-fe70db52bec1@amd.com>
 <cbf171e1-a518-429b-fd07-3526a0b252bf@amd.com>
 <e24e0de1-d984-474a-8b79-ea0934ac668e@intel.com>
From: Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <e24e0de1-d984-474a-8b79-ea0934ac668e@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?U01NazAxUDhTa21aRkZjMEZ0emVrOXF4aDFkdnNhalFqYklJOWtLckRIT1VU?=
 =?utf-8?B?eGI1MnVWNTlYTnJ6a0IrNTgyNnArRlZIL0hNTkg2cDVvaTFmeWpZWnVObERo?=
 =?utf-8?B?YTVWSHY3NlNlbGQrREVRVXAxbXM3S21MbExMU0E4MU50eHVrc0lNQ3FHaUw0?=
 =?utf-8?B?aEZDaWxTNkRrVytmeVJiSUNuVjcyOER1WDdnOCs4ZGVCUVBWMW4ybURXUERz?=
 =?utf-8?B?cFVYdGxRRWozV0pLSGJqdVNDV24rd3pFN2lUVWhyQW4wQkxFSnMyS2w4dUV1?=
 =?utf-8?B?amFFL0lsd2hGUHZuV0dXNVZRM3cybnRpZUFzWWFPMVZHc0Z1OWNIbEFjMkZI?=
 =?utf-8?B?aUJ4dUFaN0l6V0VYcVlSamRoZlZnVlhtMFc5MkZ2WnpLM1AwN3h0WGdYY0x3?=
 =?utf-8?B?Ullic1o4WEl1R29nMUgxSGpubWQ2bTJvNVVMVVRBM1pIUGZBU1RMUjkzZE5i?=
 =?utf-8?B?bGRrYWpwNXBnalMxcmlmOXV0ZnZyY2JSb3VpM3ZOUzU3Mzdib2x3MnZDR0tN?=
 =?utf-8?B?cHpUM1QyUnZMbGE2RWM4bkVLN0RvZWtDYTRaZWdwWGtudUh1b3pua29NNmxo?=
 =?utf-8?B?ZlhkSXFKS3BxSVNhYTRwbWcvZlNsZUM5SC9XZ3hwOVdrODd5MmNBbEk5aEtl?=
 =?utf-8?B?N2FUQVJOVlNEV1dxUjFta3FHWlFyeXdhLzZVVDlQSjRqdVNRM29ZcUZYOHds?=
 =?utf-8?B?bkZuTEROMDVuMWJyRnRxc2ZSK1FhaVVuS3JXTGdxbVdiV1pFYWMzZlc0NDhn?=
 =?utf-8?B?SVZOUWJmNXNTZkRQVStPQWlmMCttWXkxSTAwM25uSExwdmFIdStlNnBMT3lW?=
 =?utf-8?B?WXN0U2NTSmVFQzJHMitFTWxZVlo3RHFWV1JtdGUxalZXRlFENDdsVU9EN3Bj?=
 =?utf-8?B?NTVhQ1JUMWFudlhaRjQvSUpvU0hRNC8reWwvMWFwcGxaM2xuUzQwVUZTb2xD?=
 =?utf-8?B?SzRHMWlMVThEc1FzWUZpMEoxVGo5bitlL2pBL0c2cTE0ZVNqN3doWHRzL2Z6?=
 =?utf-8?B?cys2MEkwQ2QrcWFjbWFPZHVWOG5LcjdpVnZaK21CRUNheVA2OGNjdXVrMlpF?=
 =?utf-8?B?UmVsdklZUkE3ZmhCbkN2Zk1jamZPQ044L3FXSFFtOEVzb1E5VnRSR1F0NjNU?=
 =?utf-8?B?MVZ6bzZKaG4zemhMc1Q5VjVFMm9TN2tMWVV1ekxIUHJLNllXRTRuRXJsRm9i?=
 =?utf-8?B?VWR0dlpRWjhkbWJpNDBwS1pvQjRiRi9xSjU3S0NlMjVJY3gzeXE1Smh4ak1s?=
 =?utf-8?B?MlF1SWgzMUlRbmw0Zk1XMGxIcVBaTzBJUS9qN1ExNjdUbGlNVU5ob01tN2VP?=
 =?utf-8?B?Ly9WVCtXZkJWcmNDYXVjWUJJR1dNRC9ZZFZJTS9iaXlGUzVJNXNBWCs5a2pF?=
 =?utf-8?B?ZDJEYzRJWlBXZnc5Wm92RWhsSjliMHpLbU04d0hrU29qdFVQQnc3Q2o3YzRF?=
 =?utf-8?B?VkEzNnNuS2ovUFlLUGp2QWw0bVhCd0w5Y2hoUDBCY2FJcnRZOWMvVHBEQkVJ?=
 =?utf-8?B?TWdFT0hVTjZYU055SXZRQjF5UUwxaWJydE1zWDgwTDR4TTdheVFJK3U5TWgv?=
 =?utf-8?B?Y3ZLMVd4OHhyZEQ2WitxR1pma2kyS3NzL3VZQ1NNOVhCYzQxcEdRL2pjb1JD?=
 =?utf-8?B?Ym9hM3RPM1Fib1VnMHNOUDRzeW5XamVEYXAzLzBKek9ob2VyaFlCaFFGY1lh?=
 =?utf-8?B?YUdBTncvdm82T0E5aC9XL3hXRkh0SDNVY2FmbHNJZVpIZmZZTmdCeU8xM0R2?=
 =?utf-8?B?MW11NXp5TzFhWU81cFZFbktTYm1paVVmd25VR3ZqUTVQdzBCY0NWVEVHeGZP?=
 =?utf-8?B?eHVrbEx6cnQzbnZ5d1RuSVFnZmh5ak5RNXVYdlhTK3BRT1ppT1QvS0R6MFJC?=
 =?utf-8?B?WkF3N2VPNnc3Mk1wblgzd2JuQUlZaDlmVm5hbStoNkYyb05hQXl1YUk3b2lm?=
 =?utf-8?B?NXJaKzg2T0VTQ3lVcklFbEUwUXdDaWxJMHp6QWxRNjhXVE0wTy9jMFY4THlH?=
 =?utf-8?B?R2NCb1kxZ2dJNTZKTFZQZS9oOTR6QmtZNFBFRjBKYjBlZ21uQTFYZFhxZ2pB?=
 =?utf-8?B?Z1gyVEhETDA4UW5VL3VHU1d5ZEdwYjFkeTM3aGNub0NrWENtSnlId2JGYTdS?=
 =?utf-8?Q?Et45ojgiGoHiUe9c7xh/IfhKS?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a794f30c-fad9-4c72-1b47-08dc5a319585
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2024 14:13:38.9643
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3ETRZJJB/awVlc6ub9WEKpb+IwfCICT27d3nSel0pvTlW8dxz4RVqBr97zHx6Bu3XHuXhabX6sT8elb5M1VHBg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB7719

On 4/10/24 17:26, Huang, Kai wrote:
> On 11/04/2024 4:14 am, Tom Lendacky wrote:
>> On 4/10/24 11:08, Tom Lendacky wrote:
>>> On 4/7/24 07:44, Kai Huang wrote:
>>>
>>>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>>>> index b8441147eb5e..5ba8a9c1e47a 100644
>>>> --- a/arch/x86/kernel/process.c
>>>> +++ b/arch/x86/kernel/process.c
>>>> @@ -813,18 +813,16 @@ 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.
>>>> +     * The kernel could leave caches in incoherent state on SME/TDX
>>>> +     * capable platforms.  Flush cache to avoid silent memory
>>>> +     * corruption for these platforms.
>>>>        *
>>>> -     * Test the CPUID bit directly because the machine might've 
>>>> cleared
>>>> -     * X86_FEATURE_SME due to cmdline options.
>>>> +     * stop_this_cpu() is not a fast path, just do unconditional
>>>> +     * WBINVD for simplicity.  But only do WBINVD for bare-metal
>>>> +     * as TDX guests and SEV-ES/SEV-SNP guests will get unexpected
>>>> +     * (and unnecessary) #VE and may unable to handle.
>>>
>>> In addition to Kirill's comment on #VE...
>>>
>>> This last part of the comment reads a bit odd since you say 
>>> unconditional and then say only do WBINVD for bare-metal. Maybe 
>>> something like this makes it a bit clearer?:
>>>
>>> For TDX and SEV-ES/SEV-SNP guests, a WBINVD may cause an exception 
>>> (#VE or #VC). However, all exception handling has been torn down at 
>>> this point, so this would cause the guest to crash. Since memory 
>>> within these types of guests is coherent only issue the WBINVD on 
>>> bare-metal.
>>
>> Hmmm... actually it was the other WBINVD in patch #2 that caused the 
>> crash, so what I wrote above isn't accurate. You might want to re-word 
>> as appropriate.
> 
> Yeah that's why I used "may unable to handle" in the comment, as I 
> thought there's no need to be that specific?

Yes, makes sense.

> 
> I tend not to mention "memory within these types of guests is coherent". 
>   I mean the current upstream kernel _ONLY_ does WBINVD for SME, that 
> means for all non-SME environment there's no concern to do WBINVD here.
> 
> Here we only extend to do WBINVD on more environments, so as long as 
> there's no harm to do WBINVD for them it's OK.
> 
> How about below?
> 
>      /*
>       * The kernel could leave caches in incoherent state on SME/TDX
>       * capable platforms.  Flush cache to avoid silent memory
>       * corruption for these platforms.
>       *
>       * For TDX and SEV-ES/SEV-SNP guests, a WBINVD causes an
>       * exception (#VE or #VC), and the kernel may not be able
>       * to handle such exception (e.g., TDX guest panics if it
>       * sees #VE).  Since stop_this_cpu() isn't a fast path, just
>       * issue the WBINVD on bare-metal instead of sprinkling
>       * around vendor-specific checks.
>       */

I think that's ok, but maybe just adding that the WBINVD is not 
necessary for TDX and SEV-ES/SEV-SNP would make it clearer. Just my 
opinion, though.

Thanks,
Tom

>>
>> Thanks,
>> Tom
>>
>>>
>>> And you can expand the comment block out to at least 80 characters to 
>>> make it more compact.
> 
> OK I can do.  I guess I have to change my vim setting to do so, though :-)