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.0On 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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.