Received-SPF: pass (google.com: domain of linux-kernel+bounces-150619-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <354cce06-b1b9-4523-9b71-d0576eb3a01f@amd.com>
Date: Thu, 18 Apr 2024 14:16:06 -0400
User-Agent: Mozilla Thunderbird
Cc: yazen.ghannam@amd.com, x86@kernel.org, linux-kernel@vger.kernel.org,
 linux-edac@vger.kernel.org, patches@lists.linux.dev
Subject: Re: [PATCH] x86/mce: Implement recovery for errors in TDX/SEAM
 non-root mode
Content-Language: en-US
To: Tony Luck <tony.luck@intel.com>, Borislav Petkov <bp@alien8.de>
References: <20240408180944.44638-1-tony.luck@intel.com>
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20240408180944.44638-1-tony.luck@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?cHlrQ01PY0NsYi9pNEwwNmkrb0YxS1RQdGd6d3ZOZm5DTEJUSVZtcnJaUDMx?=
 =?utf-8?B?Rm1xd0l3dG43WUZLUFhsek9LRlBtMFkwRTg5Nkp2ZFc0cEwxTEtQbHR0T3dV?=
 =?utf-8?B?akZ0SGRxcngwWENxNWtSV1c5TUVGb1RDQ0V5MzdUUUZPYU9OSXJjbjhLeDN4?=
 =?utf-8?B?SW9OSW50b0J0cGN6TlVrMVVFcDZkTTh1WmdERjZMK3BUTFBCR3orYXNLTWh2?=
 =?utf-8?B?blQzMTNWQWRNT0FiQVFjV2hQZFBrOElZR1dMcWw3SnAwaWNjTUhaYlJTM3Ni?=
 =?utf-8?B?SFoySFdGY1lCd3FxUDFmaGh5NUFTUDdzRWxsc0RPT281dU0waklpc05hVEJV?=
 =?utf-8?B?eHdoajlwVEZTdXRkWnFwcGtTb2t2TXJKdXJSMzJMSmM0b3RwZVVuQlFkeWhF?=
 =?utf-8?B?MEpYcUlTYU1kNy9VdXRjM0c0ZTJibDBxYnhqSFo0UEptM21DR3Nha29WeHl6?=
 =?utf-8?B?bC9qY2p6YlpLTVA5andtU0ROb2J1WXhIWnNZemJPbERnUWx5MGtYcXdVSDhq?=
 =?utf-8?B?d1VXRkFFOWJrczZMeFhTS096dWU3cHdpVTdHNU02czNSR2V6TjB6VmJ4Rmho?=
 =?utf-8?B?VDZyWktMb3dJdTd1Z2VSdU1tVUVUY1V1Z1lDT3FWWTJmai9MazhqNEZ1d2I3?=
 =?utf-8?B?eGxjcVc5YzYzbnFaRE1xUEJyV1QwVm9RclpJYnVmMHNCdVh4Q0swTEJFVGk1?=
 =?utf-8?B?ZmN4QVRGL3JCZGlrMldheUY5SXV0c3hzeC9vd1lESnIxbG41Zk94MFY1Nits?=
 =?utf-8?B?dWQ2Rmp3ZFhJMkN0QTVPQ244MEh2Qk9CYlhtK0FNVFFxNkl6VDRMOFNYMTdI?=
 =?utf-8?B?YTRjZ29jYVZwS1ZVQkpqMTFOUFFqcXJJdmp6VnM3UWJ6T3RmNjhJMnpud0xj?=
 =?utf-8?B?TkxvR05OQTRqZThYK0JhOUgwSFZadGFrM3kxaXhpcWhMeUdNNW4xYmg2Q2F0?=
 =?utf-8?B?R3UwM3lmbXZpTnRDNkI2N0dBUFRZYllnR1QwUGpsRDN5cUw0Z3BhaW1iY0s0?=
 =?utf-8?B?WFliekNmc1pSK1RzRkRIWFZkS2ZESHBuTXZ0TmRjaVpyVTNYVERUSVZLMnlD?=
 =?utf-8?B?Vnk0MmEvYk05MUVNeVMzRCt6Yys5b1NSRTNMRlpFOVlWaFUvNElTVHlUN2RJ?=
 =?utf-8?B?UVE4dGJ2RXdZU25iSWdMdTE4TVNQVzBOQ0JjeHBSNFhYcVlpSFhHTnNtNmlU?=
 =?utf-8?B?dVRsbDhlb1ZEQU9hQmRVZEVQQmYveDJmQyt1MmhoRmhNNTFhOHpMbWhEdnhT?=
 =?utf-8?B?N3M0TEZqM01wempKWVh3MnVoa1ZNQm12b2Z6L0NGR2NzeGMvUXhaQjBEdmt6?=
 =?utf-8?B?enlwcFNPNjY5QWZObGFTdE9Iekszd0ZIZjVNZTIzZW1LeHg0V3FKdHBCVmti?=
 =?utf-8?B?RWpHa2IySDI0QitNeFpyeUxGeWpmS2JlTTZqTG9ZZ3lDd1R6QW13Rm1SRTkx?=
 =?utf-8?B?MTh1VlIvRDZuOUxEWS9HZ3BMeUxTcWJUazUxK0lsS2w4dk5QUU9jaDk5QmZF?=
 =?utf-8?B?aUZHOW5jMERYQzhsUjkyZnJNN0hjNVIxRjMrblpRTks1bXVPekZxdlo1ZUx1?=
 =?utf-8?B?WThyRzZXbFVFWGFxbmtZbzAyN3NIY0ZIV3RhOW4yZnQ2clBTMDRaM2N5RXJK?=
 =?utf-8?B?b3dCZGhwNVp1TTBkVCt2Zjc4M1pBc3RRTHlJbk1PNjdoNWw4ZFRJZ1lEUkZS?=
 =?utf-8?B?RkZGMWVpSnV6dTBvYm8zYkppdk1lSWFWemhrTFE4amZmRndEY2tGdTlhaFl1?=
 =?utf-8?B?eEFmei9rN2tldlg1Z1FudkxSRGQ4SGlVMTFhemNlR3EzM0RpWk1kYzNrem90?=
 =?utf-8?B?c3NuZ1FiVVRFTE4vSWNrMDd6Yk1nMFB1YkxwcDJhNUtzT1BqM0g1ZFpLMEo3?=
 =?utf-8?B?cjlUbnhoNmo5TksvWmZrdXI5QnllMGVFWGhud3dOeFlyZXZRWjhTUmNSeklH?=
 =?utf-8?B?emNXL09LOS9KSWxRRjRKZ2dDaWtCa0pXWkFtejBVa0pxTFIzajBmaERkZ1oy?=
 =?utf-8?B?ZzZ1dWZVVEVzOHkyL1M5NTU4V3I5WkZMQ1ZHVUJ4cnFWa3FSMUhacmlUa3hn?=
 =?utf-8?B?bHVPRElmdFhYN25JUEFpNG95TC82MWVEbXBUS25FMnhJZVNEeDdXVjRpdTBn?=
 =?utf-8?Q?YMneWzliW5/palnrcmHNUwHgt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 62b138a4-42fa-4423-9f75-08dc5fd39efd
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 18:16:09.1836
 (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: NR8pKcnl0m02uPMGgcfbhLcO5QtYGEXZKXi2aX+pNpB1bQGw/T2FhiQx3Bz9VOla0AWbNc+4CaEepJF5VAsjPQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB8642

On 4/8/2024 2:09 PM, Tony Luck wrote:
> Machine check SMIs (MSMI) signaled during SEAM operation (typically
> inside TDX guests), on a system with Intel eMCA enabled, might eventually
> be reported to the kernel #MC handler with the saved RIP on the stack
> pointing to the instruction in kernel code after the SEAMCALL instruction
> that entered the SEAM operation. Linux currently says that is a fatal
> error and shuts down.
> 
> There is a new bit in IA32_MCG_STATUS that, when set to 1, indicates
> that the machine check didn't originally occur at that saved RIP, but
> during SEAM non-root operation.
> 
> Add new entries to the severity table to detect this for both data load
> and instruction fetch that set the severity to "AR" (action required).
> 
> Increase the width of the mcgmask/mcgres fields in "struct severity"
> from unsigned char to unsigned short since the new bit is in position 12.
> 
> Action required for these errors is just mark the page as poisoned and
> return from the machine check handler.
> 
> Backport note. Little value in backporting this patch to stable or LTS
> kernels as this is only relevant with support for TDX, which I assume
> won't be backported. But for anyone taking this to v6.1 or older, you
> also need commit a51cbd0d86d3 ("x86/mce: Use severity table to handle
> uncorrected errors in kernel")
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> 
> ---
> The SEAM_NR bit in IA32_MCG_STATUS hasn't yet made it into the Intel
> Software Developers' Manual. But it is described in section 16.5.2
> of "Intel(R) Trust Domain Extensions (Intel(R) TDX) Module Base
> Architecture Specification" downloadable from:
> https://cdrdv2.intel.com/v1/dl/getContent/733575
> ---
>  arch/x86/include/asm/mce.h         |  2 ++
>  arch/x86/kernel/cpu/mce/core.c     | 18 ++++++++++++++++++
>  arch/x86/kernel/cpu/mce/severity.c | 16 ++++++++++++++--
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index de3118305838..dfd2e9699bd7 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -13,6 +13,7 @@
>  #define MCG_CTL_P		BIT_ULL(8)   /* MCG_CTL register available */
>  #define MCG_EXT_P		BIT_ULL(9)   /* Extended registers available */
>  #define MCG_CMCI_P		BIT_ULL(10)  /* CMCI supported */
> +#define MCG_SEAM_NR		BIT_ULL(12)  /* MCG_STATUS_SEAM_NR supported */
>  #define MCG_EXT_CNT_MASK	0xff0000     /* Number of Extended registers */
>  #define MCG_EXT_CNT_SHIFT	16
>  #define MCG_EXT_CNT(c)		(((c) & MCG_EXT_CNT_MASK) >> MCG_EXT_CNT_SHIFT)
> @@ -25,6 +26,7 @@
>  #define MCG_STATUS_EIPV		BIT_ULL(1)   /* ip points to correct instruction */
>  #define MCG_STATUS_MCIP		BIT_ULL(2)   /* machine check in progress */
>  #define MCG_STATUS_LMCES	BIT_ULL(3)   /* LMCE signaled */
> +#define MCG_STATUS_SEAM_NR	BIT_ULL(12)  /* Machine check inside SEAM non-root mode */
>  
>  /* MCG_EXT_CTL register defines */
>  #define MCG_EXT_CTL_LMCE_EN	BIT_ULL(0) /* Enable LMCE */
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 84d41be6d06b..771a9f183260 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1593,6 +1593,24 @@ noinstr void do_machine_check(struct pt_regs *regs)
>  		else
>  			queue_task_work(&m, msg, kill_me_maybe);
>  
> +	} else if (m.mcgstatus & MCG_STATUS_SEAM_NR) {

MCG_CAP[12] (MCG_SEAM_NR) should be checked first, correct? This could be a
new mce_vendor_flags field set during MCA init.

> +		/*
> +		 * Saved RIP on stack makes it look like the machine check
> +		 * was taken in the kernel on the instruction following
> +		 * the entry to SEAM mode. But MCG_STATUS_SEAM_NR indicates
> +		 * that the machine check was taken inside SEAM non-root
> +		 * mode.  CPU core has already marked that guest as dead.
> +		 * It is OK for the kernel to resume execution at the
> +		 * apparent point of the machine check as the fault did
> +		 * not occur there. Mark the page as poisoned so it won't
> +		 * be added to free list when the guest is terminated.
> +		 */
> +		if (mce_usable_address(&m)) {
> +			struct page *p = pfn_to_online_page(m.addr >> PAGE_SHIFT);
> +
> +			if (p)
> +				SetPageHWPoison(p);
> +		}

I think this is okay, and it could even be more generalized as a "page
offline" action.

Here's some WIP for a generic MCE "action table":
https://github.com/AMDESE/linux/commit/cf0b8a97240ab

This is based on the short discussion here:
https://lore.kernel.org/linux-edac/ZD7gPkfWQeEeEfBe@agluck-desk3.sc.intel.com/

Basically, all the status bits would be checked in mce_severity() and the
appropriate action is set to be done later.

This would be future work, of course. What do you think?

Thanks,
Yazen