Messages in this thread | | | Date | Thu, 18 Apr 2024 14:16:06 -0400 | Subject | Re: [PATCH] x86/mce: Implement recovery for errors in TDX/SEAM non-root mode | From | Yazen Ghannam <> |
| |
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
| |