[PATCH 0/2] MI300A ATL and FMPM fixes

[PATCH 0/2] MI300A ATL and FMPM fixes

[Yazen Ghannam]

Hi all,

This set addresses two issues in the current MI300A solution.

Patch 1:
The row retirement flow is updated to iterate over an additional bit.
Without this, only half of the affected addresses are calculated.

Patch 2:
The record keeping flow is updated to match only on unique address bits
of interest. Without this, multiple records can be created for the same
event. For example, for row retirement, there will be a record that
includes column bit 0, another record for column bit 1, etc., even
though these are part of the same row retirement event.

Thanks,
Yazen
 

---
Yazen Ghannam (2):
      RAS/AMD/ATL: Include row[13] bit in row retirement
      RAS/AMD/{ATL,FMPM}: Get masked address

 drivers/ras/amd/atl/umc.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/ras/amd/fmpm.c    |  2 +-
 include/linux/ras.h       |  2 ++
 3 files changed, 35 insertions(+), 2 deletions(-)
---
base-commit: ff719e243db5de2ca595bf4e2ab6ad530a08f378
change-id: 20250308-fix-fmpm-extra-records-de44d3956d9d

[PATCH 1/2] RAS/AMD/ATL: Include row[13] bit in row retirement

[Yazen Ghannam]

Based on feedback from hardware folks, row[13] is part of the variable
bits within a physical row (along with all column bits).

Only half the physical addresses affected by a row are calculated if
this bit is not included.

Add the row[13] bit to the row retirement flow.

Fixes: 3b566b30b414 ("RAS/AMD/ATL: Add MI300 row retirement support")
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/ras/amd/atl/umc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
index dc8aa12f63c8..cb8ace3d4e42 100644
--- a/drivers/ras/amd/atl/umc.c
+++ b/drivers/ras/amd/atl/umc.c
@@ -320,7 +320,7 @@ static unsigned long convert_dram_to_norm_addr_mi300(unsigned long addr)
  * See amd_atl::convert_dram_to_norm_addr_mi300() for MI300 address formats.
  */
 #define MI300_NUM_COL		BIT(HWEIGHT(MI300_UMC_MCA_COL))
-static void retire_row_mi300(struct atl_err *a_err)
+static void _retire_row_mi300(struct atl_err *a_err)
 {
 	unsigned long addr;
 	struct page *p;
@@ -351,6 +351,23 @@ static void retire_row_mi300(struct atl_err *a_err)
 	}
 }
 
+/*
+ * In addition to the column bits, the row[13] bit should also be included when
+ * calculating addresses affected by a physical row.
+ *
+ * Instead of running through another loop over a single bit, just run through
+ * the column bits twice and flip the row[13] bit in-between.
+ *
+ * See MI300_UMC_MCA_ROW for the row bits in MCA_ADDR_UMC value.
+ */
+#define MI300_UMC_MCA_ROW13	BIT(23)
+static void retire_row_mi300(struct atl_err *a_err)
+{
+	_retire_row_mi300(a_err);
+	a_err->addr ^= MI300_UMC_MCA_ROW13;
+	_retire_row_mi300(a_err);
+}
+
 void amd_retire_dram_row(struct atl_err *a_err)
 {
 	if (df_cfg.rev == DF4p5 && df_cfg.flags.heterogeneous)

-- 
2.49.0

[PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Yazen Ghannam]

Some operations require checking, or ignoring, specific bits in an
address value. For example, this can be comparing address values to
identify unique structures.

Currently, the full address value is compared when filtering for
duplicates. This results in over counting and creation of extra records.
This gives the impression that more unique events occurred than did in
reality.

Introduce a helper to return a masked address. Do this in the AMD
Address Translation Library, since the bits depend on particular
structures and are implementation-specific.

Start with the case for physical rows on MI300.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/ras/amd/atl/umc.c | 14 ++++++++++++++
 drivers/ras/amd/fmpm.c    |  2 +-
 include/linux/ras.h       |  2 ++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
index cb8ace3d4e42..0b6e5305ea83 100644
--- a/drivers/ras/amd/atl/umc.c
+++ b/drivers/ras/amd/atl/umc.c
@@ -375,6 +375,20 @@ void amd_retire_dram_row(struct atl_err *a_err)
 }
 EXPORT_SYMBOL_GPL(amd_retire_dram_row);
 
+u64 amd_atl_masked_addr(u64 addr)
+{
+	/*
+	 * Row retirement is done on MI300 systems, and some bits are 'don't care'
+	 * for comparing addresses with unique physical rows.
+	 * This includes all column bits and the row[13] bit.
+	 */
+	if (df_cfg.rev == DF4p5 && df_cfg.flags.heterogeneous)
+		return addr & ~(MI300_UMC_MCA_ROW13 | MI300_UMC_MCA_COL);
+
+	return addr;
+}
+EXPORT_SYMBOL_GPL(amd_atl_masked_addr);
+
 static unsigned long get_addr(unsigned long addr)
 {
 	if (df_cfg.rev == DF4p5 && df_cfg.flags.heterogeneous)
diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 90de737fbc90..2ca789d7c8ec 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -258,7 +258,7 @@ static bool fpds_equal(struct cper_fru_poison_desc *old, struct cper_fru_poison_
 	 *
 	 * Also, order the checks from most->least likely to fail to shortcut the code.
 	 */
-	if (old->addr != new->addr)
+	if (amd_atl_masked_addr(old->addr) != amd_atl_masked_addr(new->addr))
 		return false;
 
 	if (old->hw_id != new->hw_id)
diff --git a/include/linux/ras.h b/include/linux/ras.h
index a64182bc72ad..e822275bed6a 100644
--- a/include/linux/ras.h
+++ b/include/linux/ras.h
@@ -46,9 +46,11 @@ struct atl_err {
 void amd_atl_register_decoder(unsigned long (*f)(struct atl_err *));
 void amd_atl_unregister_decoder(void);
 void amd_retire_dram_row(struct atl_err *err);
+u64 amd_atl_masked_addr(u64 addr);
 unsigned long amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err);
 #else
 static inline void amd_retire_dram_row(struct atl_err *err) { }
+static inline u64 amd_atl_masked_addr(u64 addr) { return addr; }
 static inline unsigned long
 amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err) { return -EINVAL; }
 #endif /* CONFIG_AMD_ATL */

-- 
2.49.0

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Borislav Petkov]

On Tue, Apr 01, 2025 at 08:49:01PM +0000, Yazen Ghannam wrote:
> +u64 amd_atl_masked_addr(u64 addr)
> +{
> +	/*
> +	 * Row retirement is done on MI300 systems, and some bits are 'don't care'
> +	 * for comparing addresses with unique physical rows.
> +	 * This includes all column bits and the row[13] bit.
> +	 */
> +	if (df_cfg.rev == DF4p5 && df_cfg.flags.heterogeneous)
> +		return addr & ~(MI300_UMC_MCA_ROW13 | MI300_UMC_MCA_COL);
> +
> +	return addr;
> +}
> +EXPORT_SYMBOL_GPL(amd_atl_masked_addr);

Uff, an exported silly helper.

Make that static inline and stick into a header so that compiler can inline
and optimize.

And that header should be drivers/ras/amd/atl/internal.h

Also, the helper should be called "atl_mask_address()" or so as all our
functions have verbs in imperative tone.

> diff --git a/include/linux/ras.h b/include/linux/ras.h
> index a64182bc72ad..e822275bed6a 100644
> --- a/include/linux/ras.h
> +++ b/include/linux/ras.h

Internal helpers go into the internal header, not here.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Yazen Ghannam]

On Mon, Apr 07, 2025 at 03:24:15PM +0200, Borislav Petkov wrote:
> On Tue, Apr 01, 2025 at 08:49:01PM +0000, Yazen Ghannam wrote:
> > +u64 amd_atl_masked_addr(u64 addr)
> > +{
> > +	/*
> > +	 * Row retirement is done on MI300 systems, and some bits are 'don't care'
> > +	 * for comparing addresses with unique physical rows.
> > +	 * This includes all column bits and the row[13] bit.
> > +	 */
> > +	if (df_cfg.rev == DF4p5 && df_cfg.flags.heterogeneous)
> > +		return addr & ~(MI300_UMC_MCA_ROW13 | MI300_UMC_MCA_COL);
> > +
> > +	return addr;
> > +}
> > +EXPORT_SYMBOL_GPL(amd_atl_masked_addr);
> 
> Uff, an exported silly helper.
> 
> Make that static inline and stick into a header so that compiler can inline
> and optimize.
> 
> And that header should be drivers/ras/amd/atl/internal.h
> 
> Also, the helper should be called "atl_mask_address()" or so as all our
> functions have verbs in imperative tone.
> 
> > diff --git a/include/linux/ras.h b/include/linux/ras.h
> > index a64182bc72ad..e822275bed6a 100644
> > --- a/include/linux/ras.h
> > +++ b/include/linux/ras.h
> 
> Internal helpers go into the internal header, not here.
> 
> Thx.
> 

Okay, will do.

Thanks,
Yazen

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Borislav Petkov]

On Mon, Apr 07, 2025 at 11:16:57AM -0400, Yazen Ghannam wrote:
> Okay, will do.

Yah, except that df_cfg crap needs an export now:

ERROR: modpost: "df_cfg" [drivers/ras/amd/fmpm.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:147: Module.symvers] Error 1
make[1]: *** [/mnt/kernel/kernel/linux/Makefile:1956: modpost] Error 2
make: *** [Makefile:248: __sub-make] Error 2

Looking at the call chain how we land in that fpds_equal() in fmpm, can we
read out from the error records themselves that those are from MI300 and not
need df_cfg at all?

There's a struct mce here:

static void update_fru_record(struct fru_rec *rec, struct mce *m)

which has CPUID for example.

Otherwise we'll have to go back to your original thing if we're going to have
to export *something*...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Yazen Ghannam]

On Tue, Apr 08, 2025 at 12:14:15PM +0200, Borislav Petkov wrote:
> On Mon, Apr 07, 2025 at 11:16:57AM -0400, Yazen Ghannam wrote:
> > Okay, will do.
> 
> Yah, except that df_cfg crap needs an export now:
> 
> ERROR: modpost: "df_cfg" [drivers/ras/amd/fmpm.ko] undefined!
> make[2]: *** [scripts/Makefile.modpost:147: Module.symvers] Error 1
> make[1]: *** [/mnt/kernel/kernel/linux/Makefile:1956: modpost] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
> 

*sigh*, sorry for the noise. I did a 'make -f' build rather than a full
build.

> Looking at the call chain how we land in that fpds_equal() in fmpm, can we
> read out from the error records themselves that those are from MI300 and not
> need df_cfg at all?
> 
> There's a struct mce here:
> 
> static void update_fru_record(struct fru_rec *rec, struct mce *m)
> 
> which has CPUID for example.
> 
> Otherwise we'll have to go back to your original thing if we're going to have
> to export *something*...
> 

At the moment, FMPM only loads on MI300A. We can just have a local
function to mask the addresses. I was thinking we can have function
pointers to make things generic. But maybe we keep it simple until
really necessary by just using the MI300 version by default.

Please see patch below.

Thanks,
Yazen

----------------

From 7f6e2196cb54bb2134e5c1854e5a54530eec77a1 Mon Sep 17 00:00:00 2001
From: Yazen Ghannam <yazen.ghannam@amd.com>
Date: Thu, 27 Feb 2025 19:31:32 +0000
Subject: [PATCH] RAS/AMD/FMPM: Get masked address

Some operations require checking, or ignoring, specific bits in an
address value. For example, this can be comparing address values to
identify unique structures.

Currently, the full address value is compared when filtering for
duplicates. This results in over counting and creation of extra records.
This gives the impression that more unique events occurred than did in
reality.

Introduce a helper to return a masked address. Start with the case for
physical rows on MI300.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/ras/amd/atl/internal.h |  3 +++
 drivers/ras/amd/atl/umc.c      |  2 --
 drivers/ras/amd/fmpm.c         | 14 +++++++++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
index f9be26d25348..d096b58cd0ae 100644
--- a/drivers/ras/amd/atl/internal.h
+++ b/drivers/ras/amd/atl/internal.h
@@ -362,4 +362,7 @@ static inline void atl_debug_on_bad_intlv_mode(struct addr_ctx *ctx)
 	atl_debug(ctx, "Unrecognized interleave mode: %u", ctx->map.intlv_mode);
 }
 
+#define MI300_UMC_MCA_COL	GENMASK(5, 1)
+#define MI300_UMC_MCA_ROW13	BIT(23)
+
 #endif /* __AMD_ATL_INTERNAL_H__ */
diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
index cb8ace3d4e42..6e072b7667e9 100644
--- a/drivers/ras/amd/atl/umc.c
+++ b/drivers/ras/amd/atl/umc.c
@@ -229,7 +229,6 @@ int get_umc_info_mi300(void)
  * Additionally, the PC and Bank bits may be hashed. This must be accounted for before
  * reconstructing the normalized address.
  */
-#define MI300_UMC_MCA_COL	GENMASK(5, 1)
 #define MI300_UMC_MCA_BANK	GENMASK(9, 6)
 #define MI300_UMC_MCA_ROW	GENMASK(24, 10)
 #define MI300_UMC_MCA_PC	BIT(25)
@@ -360,7 +359,6 @@ static void _retire_row_mi300(struct atl_err *a_err)
  *
  * See MI300_UMC_MCA_ROW for the row bits in MCA_ADDR_UMC value.
  */
-#define MI300_UMC_MCA_ROW13	BIT(23)
 static void retire_row_mi300(struct atl_err *a_err)
 {
 	_retire_row_mi300(a_err);
diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 90de737fbc90..6119bd0d6ae1 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -167,6 +167,18 @@ static unsigned int spa_nr_entries;
  */
 static DEFINE_MUTEX(fmpm_update_mutex);
 
+/*
+ * Row retirement is done on MI300 systems, and some bits are 'don't care'
+ * for comparing addresses with unique physical rows.
+ * This includes all column bits and the row[13] bit.
+ */
+static u64 mi300_mask_address(u64 addr)
+{
+	return addr & ~(MI300_UMC_MCA_ROW13 | MI300_UMC_MCA_COL);
+}
+
+static u64 (*fmpm_mask_address)(u64 addr) = mi300_mask_address;
+
 #define for_each_fru(i, rec) \
 	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
 
@@ -258,7 +270,7 @@ static bool fpds_equal(struct cper_fru_poison_desc *old, struct cper_fru_poison_
 	 *
 	 * Also, order the checks from most->least likely to fail to shortcut the code.
 	 */
-	if (old->addr != new->addr)
+	if (fmpm_mask_address(old->addr) != fmpm_mask_address(new->addr))
 		return false;
 
 	if (old->hw_id != new->hw_id)
-- 
2.49.0

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Borislav Petkov]

On Tue, Apr 08, 2025 at 11:52:42AM -0400, Yazen Ghannam wrote:
> At the moment, FMPM only loads on MI300A. We can just have a local
> function to mask the addresses. I was thinking we can have function
> pointers to make things generic. But maybe we keep it simple until
> really necessary by just using the MI300 version by default.

Now you're talking! :-P

> Please see patch below.

... which I simplified even more.

I'm thinking whoever is going to test fmpm on something else besides MI300A,
they will have to extend this address masking thing and then we can cross that
bridge when we get to it.

So this is keeping it simple for now.

Ack?

---

commit 58029c39cdc54ac4f4dc40b4a9c05eed9f9b808a (HEAD -> refs/heads/edac-urgent)
Author: Yazen Ghannam <yazen.ghannam@amd.com>
Date:   Thu Feb 27 19:31:32 2025 +0000

    RAS/AMD/FMPM: Get masked address
    
    Some operations require checking, or ignoring, specific bits in an address
    value. For example, this can be comparing address values to identify unique
    structures.
    
    Currently, the full address value is compared when filtering for duplicates.
    This results in over counting and creation of extra records.  This gives the
    impression that more unique events occurred than did in reality.
    
    Mask the address for physical rows on MI300.
    
      [ bp: Simplify. ]
    
    Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
    Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
    Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
    Cc: stable@vger.kernel.org

diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
index f9be26d25348..d096b58cd0ae 100644
--- a/drivers/ras/amd/atl/internal.h
+++ b/drivers/ras/amd/atl/internal.h
@@ -362,4 +362,7 @@ static inline void atl_debug_on_bad_intlv_mode(struct addr_ctx *ctx)
 	atl_debug(ctx, "Unrecognized interleave mode: %u", ctx->map.intlv_mode);
 }
 
+#define MI300_UMC_MCA_COL	GENMASK(5, 1)
+#define MI300_UMC_MCA_ROW13	BIT(23)
+
 #endif /* __AMD_ATL_INTERNAL_H__ */
diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
index cb8ace3d4e42..6e072b7667e9 100644
--- a/drivers/ras/amd/atl/umc.c
+++ b/drivers/ras/amd/atl/umc.c
@@ -229,7 +229,6 @@ int get_umc_info_mi300(void)
  * Additionally, the PC and Bank bits may be hashed. This must be accounted for before
  * reconstructing the normalized address.
  */
-#define MI300_UMC_MCA_COL	GENMASK(5, 1)
 #define MI300_UMC_MCA_BANK	GENMASK(9, 6)
 #define MI300_UMC_MCA_ROW	GENMASK(24, 10)
 #define MI300_UMC_MCA_PC	BIT(25)
@@ -360,7 +359,6 @@ static void _retire_row_mi300(struct atl_err *a_err)
  *
  * See MI300_UMC_MCA_ROW for the row bits in MCA_ADDR_UMC value.
  */
-#define MI300_UMC_MCA_ROW13	BIT(23)
 static void retire_row_mi300(struct atl_err *a_err)
 {
 	_retire_row_mi300(a_err);
diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 90de737fbc90..8877c6ff64c4 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -250,6 +250,13 @@ static bool rec_has_valid_entries(struct fru_rec *rec)
 	return true;
 }
 
+/*
+ * Row retirement is done on MI300 systems, and some bits are 'don't
+ * care' for comparing addresses with unique physical rows.  This
+ * includes all column bits and the row[13] bit.
+ */
+#define MASK_ADDR(addr)	((addr) & ~(MI300_UMC_MCA_ROW13 | MI300_UMC_MCA_COL))
+
 static bool fpds_equal(struct cper_fru_poison_desc *old, struct cper_fru_poison_desc *new)
 {
 	/*
@@ -258,7 +265,7 @@ static bool fpds_equal(struct cper_fru_poison_desc *old, struct cper_fru_poison_
 	 *
 	 * Also, order the checks from most->least likely to fail to shortcut the code.
 	 */
-	if (old->addr != new->addr)
+	if (MASK_ADDR(old->addr) != MASK_ADDR(new->addr))
 		return false;
 
 	if (old->hw_id != new->hw_id)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Yazen Ghannam]

On Tue, Apr 08, 2025 at 07:33:33PM +0200, Borislav Petkov wrote:
> On Tue, Apr 08, 2025 at 11:52:42AM -0400, Yazen Ghannam wrote:
> > At the moment, FMPM only loads on MI300A. We can just have a local
> > function to mask the addresses. I was thinking we can have function
> > pointers to make things generic. But maybe we keep it simple until
> > really necessary by just using the MI300 version by default.
> 
> Now you're talking! :-P
> 
> > Please see patch below.
> 
> ... which I simplified even more.
> 
> I'm thinking whoever is going to test fmpm on something else besides MI300A,
> they will have to extend this address masking thing and then we can cross that
> bridge when we get to it.
> 
> So this is keeping it simple for now.
> 
> Ack?

Yes, looks good to me.

Thanks!

-Yazen

Re: [PATCH 2/2] RAS/AMD/{ATL,FMPM}: Get masked address

[Borislav Petkov]

On Tue, Apr 08, 2025 at 04:10:55PM -0400, Yazen Ghannam wrote:
> On Tue, Apr 08, 2025 at 07:33:33PM +0200, Borislav Petkov wrote:
> > On Tue, Apr 08, 2025 at 11:52:42AM -0400, Yazen Ghannam wrote:
> > > At the moment, FMPM only loads on MI300A. We can just have a local
> > > function to mask the addresses. I was thinking we can have function
> > > pointers to make things generic. But maybe we keep it simple until
> > > really necessary by just using the MI300 version by default.
> > 
> > Now you're talking! :-P
> > 
> > > Please see patch below.
> > 
> > ... which I simplified even more.
> > 
> > I'm thinking whoever is going to test fmpm on something else besides MI300A,
> > they will have to extend this address masking thing and then we can cross that
> > bridge when we get to it.
> > 
> > So this is keeping it simple for now.
> > 
> > Ack?
> 
> Yes, looks good to me.

Queued, thanks.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette