Re: [PATCH 1/2] x86/mce: Fix missing address mask in recovery for errors in TDX/SEAM non-root mode

[Adrian Hunter <adrian.hunter@intel.com>]

On 19/06/2025 14:57, Adrian Hunter wrote:
> On 18/06/2025 17:55, Dave Hansen wrote:
>> On 6/18/25 05:08, Adrian Hunter wrote:
>>> --- a/arch/x86/kernel/cpu/mce/core.c
>>> +++ b/arch/x86/kernel/cpu/mce/core.c
>>> @@ -1665,7 +1665,8 @@ noinstr void do_machine_check(struct pt_regs *regs)
>>>  		 * 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);
>>> +			unsigned long pfn = (m->addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
>>> +			struct page *p = pfn_to_online_page(pfn);
>>
>> If ->addr isn't really an address that software can do much with,
>> shouldn't we mask MCI_ADDR_PHYSADDR off up front, like in mce_read_aux()?
> 
> Would that mean no one would know if the mce addr had KeyID bits or not?

Current design, to keep the bits in mce addr, is from Tony's patch:

	x86/mce: Mask out non-address bits from machine check bank
	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a01ec97dc066009dd89e43bfcf55644f2dd6d19

Assuming that is not altered, a tidy-up is still possible like:

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 6c77c03139f7..b469b7a7ecfa 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -386,4 +386,14 @@ static inline void mce_amd_feature_init(struct cpuinfo_x86 *c)		{ }
 
 unsigned long copy_mc_fragile_handle_tail(char *to, char *from, unsigned len);
 
+static inline unsigned long mce_addr_to_phys(u64 mce_addr)
+{
+	return mce_addr & MCI_ADDR_PHYSADDR;
+}
+
+static inline unsigned long mce_addr_to_pfn(u64 mce_addr)
+{
+	return mce_addr_to_phys(mce_addr) >> PAGE_SHIFT;
+}
+
 #endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 76c4634c6a5f..e9e8c377790f 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -642,7 +642,7 @@ static int uc_decode_notifier(struct notifier_block *nb, unsigned long val,
 	    mce->severity != MCE_DEFERRED_SEVERITY)
 		return NOTIFY_DONE;
 
-	pfn = (mce->addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
+	pfn = mce_addr_to_pfn(mce->addr);
 	if (!memory_failure(pfn, 0)) {
 		set_mce_nospec(pfn);
 		mce->kflags |= MCE_HANDLED_UC;
@@ -1412,7 +1412,7 @@ static void kill_me_maybe(struct callback_head *cb)
 	if (!p->mce_ripv)
 		flags |= MF_MUST_KILL;
 
-	pfn = (p->mce_addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
+	pfn = mce_addr_to_pfn(p->mce_addr);
 	ret = memory_failure(pfn, flags);
 	if (!ret) {
 		set_mce_nospec(pfn);
@@ -1441,7 +1441,7 @@ static void kill_me_never(struct callback_head *cb)
 
 	p->mce_count = 0;
 	pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
-	pfn = (p->mce_addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
+	pfn = mce_addr_to_pfn(p->mce_addr);
 	if (!memory_failure(pfn, 0))
 		set_mce_nospec(pfn);
 }
@@ -1665,7 +1665,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		 * be added to free list when the guest is terminated.
 		 */
 		if (mce_usable_address(m)) {
-			unsigned long pfn = (m->addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
+			unsigned long pfn = mce_addr_to_pfn(m->addr);
 			struct page *p = pfn_to_online_page(pfn);
 
 			if (p)
diff --git a/drivers/cxl/core/mce.c b/drivers/cxl/core/mce.c
index ff8d078c6ca1..f3c4d6a5f159 100644
--- a/drivers/cxl/core/mce.c
+++ b/drivers/cxl/core/mce.c
@@ -24,7 +24,7 @@ static int cxl_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!endpoint)
 		return NOTIFY_DONE;
 
-	spa = mce->addr & MCI_ADDR_PHYSADDR;
+	spa = mce_addr_to_phys(mce->addr);
 
 	pfn = spa >> PAGE_SHIFT;
 	if (!pfn_valid(pfn))
diff --git a/drivers/edac/skx_common.c b/drivers/edac/skx_common.c
index c9ade45c1a99..83fcec743ea7 100644
--- a/drivers/edac/skx_common.c
+++ b/drivers/edac/skx_common.c
@@ -732,7 +732,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 & MCI_ADDR_PHYSADDR;
+	res.addr = mce_addr_to_phys(mce->addr);
 	if (!pfn_to_online_page(res.addr >> PAGE_SHIFT) && !arch_is_platform_page(res.addr)) {
 		pr_err("Invalid address 0x%llx in IA32_MC%d_ADDR\n", mce->addr, mce->bank);
 		return NOTIFY_DONE;