Re: [PATCH] x86/mce: Mask out non-address bits from machine check bank

[Yazen Ghannam]

On Tue, Jan 03, 2023 at 02:34:16PM -0800, Tony Luck wrote:
> Systems that support various memory encryption schemes (MKTME, TDX, SEV)
> use high order physical address bits to indicate which key should be
> used for a specific memory location.
> 
> When a memory error is reported, some systems may report those key
> bits in the IA32_MCi_ADDR machine check MSR. This is legitimate because
> the Intel SDM has a footnote for the contents of the address register
> that says: "Useful bits in this field depend on the address methodology
> in use when the register state is saved."
> 
> Note: I don't know if any AMD systems include key bits in the reported
> address, if they do, they also need this fix. If not, it is harmless.
>

The following note is in the description of the MCA_ADDR[ErrorAddr] field in
the AMD Processor Programming Reference.

  For physical addresses, the most significant bit is given by
  Core::X86::Cpuid::LongModeInfo[PhysAddrSize].

And I see that x86_phys_bits does get fixed up in early_detect_mem_encrypt().
I'm not sure if key bits are included in the reported address, or if the HW
automatically masks them off. But in any case, I think this patch is valid as
you stated above.

> Add a new #define MCI_ADDR_PHYSADDR for the mask of valid physical
> address bits within the machine check bank address register. Use this
> mask for recoverable machine check handling and in the EDAC driver to
> ignore any key bits that may be present.
> 
> [Credit: fix is based on those proposed by Fan Du and Isaku Yamahata]
> 
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> Reported-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> Reported-by: Fan Du <fan.du@xxxxxxxxx>
> ---
>  arch/x86/include/asm/mce.h     |  3 +++
>  arch/x86/kernel/cpu/mce/core.c | 14 +++++++++-----
>  drivers/edac/skx_common.c      |  2 +-
>  3 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 6e986088817d..a8eef87fb12a 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -88,6 +88,9 @@
>  #define  MCI_MISC_ADDR_MEM	3	/* memory address */
>  #define  MCI_MISC_ADDR_GENERIC	7	/* generic */
>  
> +/* MCi_ADDR register defines */
> +#define MCI_ADDR_PHYSADDR	GENMASK(boot_cpu_data.x86_phys_bits - 1, 0)

Should this use GENMASK_ULL in case we're running in 32-bit mode?

> +
>  /* CTL2 register defines */
>  #define MCI_CTL2_CMCI_EN		BIT_ULL(30)
>  #define MCI_CTL2_CMCI_THRESHOLD_MASK	0x7fffULL
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 2c8ec5c71712..949705bdb2f3 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -579,7 +579,7 @@ static int uc_decode_notifier(struct notifier_block *nb, unsigned long val,
>  	    mce->severity != MCE_DEFERRED_SEVERITY)
>  		return NOTIFY_DONE;
>  
> -	pfn = mce->addr >> PAGE_SHIFT;
> +	pfn = (mce->addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
>  	if (!memory_failure(pfn, 0)) {
>  		set_mce_nospec(pfn);
>  		mce->kflags |= MCE_HANDLED_UC;
> @@ -1308,6 +1308,7 @@ static void kill_me_maybe(struct callback_head *cb)
>  {
>  	struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
>  	int flags = MF_ACTION_REQUIRED;
> +	unsigned long pfn;
>  	int ret;
>  
>  	p->mce_count = 0;
> @@ -1316,9 +1317,10 @@ static void kill_me_maybe(struct callback_head *cb)
>  	if (!p->mce_ripv)
>  		flags |= MF_MUST_KILL;
>  
> -	ret = memory_failure(p->mce_addr >> PAGE_SHIFT, flags);
> +	pfn = (p->mce_addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
> +	ret = memory_failure(pfn, flags);
>  	if (!ret) {
> -		set_mce_nospec(p->mce_addr >> PAGE_SHIFT);
> +		set_mce_nospec(pfn);
>  		sync_core();
>  		return;
>  	}
> @@ -1340,11 +1342,13 @@ static void kill_me_maybe(struct callback_head *cb)
>  static void kill_me_never(struct callback_head *cb)
>  {
>  	struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
> +	unsigned long pfn;
>  
>  	p->mce_count = 0;
>  	pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
> -	if (!memory_failure(p->mce_addr >> PAGE_SHIFT, 0))
> -		set_mce_nospec(p->mce_addr >> PAGE_SHIFT);
> +	pfn = (p->mce_addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
> +	if (!memory_failure(pfn, 0))
> +		set_mce_nospec(pfn);
>  }
>  
>  static void queue_task_work(struct mce *m, char *msg, void (*func)(struct callback_head *))
> diff --git a/drivers/edac/skx_common.c b/drivers/edac/skx_common.c
> index f0f8e98f6efb..806986f03177 100644
> --- a/drivers/edac/skx_common.c
> +++ b/drivers/edac/skx_common.c
> @@ -657,7 +657,7 @@ int skx_mce_check_error(struct notifier_block *nb, unsigned long val,
>  
>  	memset(&res, 0, sizeof(res));
>  	res.mce  = mce;
> -	res.addr = mce->addr;
> +	res.addr = mce->addr & MCI_ADDR_PHYSADDR;
>  
>  	/* Try driver decoder first */
>  	if (!(driver_decode && driver_decode(&res))) {
> -- 

The address decode in the AMD64 EDAC module operates on a non-physical
address, so this update is not needed there. The MCE recovery changes look good
to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>

Thanks,
Yazen