Re: [PATCH v2 3/4] RAS/AMD/ATL: Validate address map when information is gathered
From: Yazen Ghannam
Date: Wed Apr 03 2024 - 10:26:55 EST
On 3/27/24 14:52, John Allen wrote:
The address map will not change during translation so all checks to
validate the map can be moved to a single function that checks the map
at the time the information is gathered.
There needs to be an command in the message (imperative mood).
"Validate maps earlier..." or similar.
Signed-off-by: John Allen <john.allen@xxxxxxx>
---
v2:
- New in v2.
---
drivers/ras/amd/atl/dehash.c | 43 --------------
drivers/ras/amd/atl/map.c | 105 +++++++++++++++++++++++++++++++++++
2 files changed, 105 insertions(+), 43 deletions(-)
diff --git a/drivers/ras/amd/atl/dehash.c b/drivers/ras/amd/atl/dehash.c
index 4ea46262c4f5..d4ee7ecabaee 100644
--- a/drivers/ras/amd/atl/dehash.c
+++ b/drivers/ras/amd/atl/dehash.c
@@ -12,41 +12,10 @@
#include "internal.h"
-/*
- * Verify the interleave bits are correct in the different interleaving
- * settings.
- *
- * If @num_intlv_dies and/or @num_intlv_sockets are 1, it means the
- * respective interleaving is disabled.
- */
-static inline bool map_bits_valid(struct addr_ctx *ctx, u8 bit1, u8 bit2,
- u8 num_intlv_dies, u8 num_intlv_sockets)
-{
- if (!(ctx->map.intlv_bit_pos == bit1 || ctx->map.intlv_bit_pos == bit2)) {
- pr_debug("Invalid interleave bit: %u", ctx->map.intlv_bit_pos);
- return false;
- }
-
- if (ctx->map.num_intlv_dies > num_intlv_dies) {
- pr_debug("Invalid number of interleave dies: %u", ctx->map.num_intlv_dies);
- return false;
- }
-
- if (ctx->map.num_intlv_sockets > num_intlv_sockets) {
- pr_debug("Invalid number of interleave sockets: %u", ctx->map.num_intlv_sockets);
- return false;
- }
-
- return true;
-}
-
static int df2_dehash_addr(struct addr_ctx *ctx)
{
u8 hashed_bit, intlv_bit, intlv_bit_pos;
- if (!map_bits_valid(ctx, 8, 9, 1, 1))
- return -EINVAL;
-
intlv_bit_pos = ctx->map.intlv_bit_pos;
intlv_bit = !!(BIT_ULL(intlv_bit_pos) & ctx->ret_addr);
@@ -67,9 +36,6 @@ static int df3_dehash_addr(struct addr_ctx *ctx)
bool hash_ctl_64k, hash_ctl_2M, hash_ctl_1G;
u8 hashed_bit, intlv_bit, intlv_bit_pos;
- if (!map_bits_valid(ctx, 8, 9, 1, 1))
- return -EINVAL;
-
hash_ctl_64k = FIELD_GET(DF3_HASH_CTL_64K, ctx->map.ctl);
hash_ctl_2M = FIELD_GET(DF3_HASH_CTL_2M, ctx->map.ctl);
hash_ctl_1G = FIELD_GET(DF3_HASH_CTL_1G, ctx->map.ctl);
@@ -171,9 +137,6 @@ static int df4_dehash_addr(struct addr_ctx *ctx)
bool hash_ctl_64k, hash_ctl_2M, hash_ctl_1G;
u8 hashed_bit, intlv_bit;
- if (!map_bits_valid(ctx, 8, 8, 1, 2))
- return -EINVAL;
-
hash_ctl_64k = FIELD_GET(DF4_HASH_CTL_64K, ctx->map.ctl);
hash_ctl_2M = FIELD_GET(DF4_HASH_CTL_2M, ctx->map.ctl);
hash_ctl_1G = FIELD_GET(DF4_HASH_CTL_1G, ctx->map.ctl);
@@ -247,9 +210,6 @@ static int df4p5_dehash_addr(struct addr_ctx *ctx)
u8 hashed_bit, intlv_bit;
u64 rehash_vector;
- if (!map_bits_valid(ctx, 8, 8, 1, 2))
- return -EINVAL;
-
hash_ctl_64k = FIELD_GET(DF4_HASH_CTL_64K, ctx->map.ctl);
hash_ctl_2M = FIELD_GET(DF4_HASH_CTL_2M, ctx->map.ctl);
hash_ctl_1G = FIELD_GET(DF4_HASH_CTL_1G, ctx->map.ctl);
@@ -360,9 +320,6 @@ static int mi300_dehash_addr(struct addr_ctx *ctx)
bool hashed_bit, intlv_bit, test_bit;
u8 num_intlv_bits, base_bit, i;
- if (!map_bits_valid(ctx, 8, 8, 4, 1))
- return -EINVAL;
-
hash_ctl_4k = FIELD_GET(DF4p5_HASH_CTL_4K, ctx->map.ctl);
hash_ctl_64k = FIELD_GET(DF4_HASH_CTL_64K, ctx->map.ctl);
hash_ctl_2M = FIELD_GET(DF4_HASH_CTL_2M, ctx->map.ctl);
diff --git a/drivers/ras/amd/atl/map.c b/drivers/ras/amd/atl/map.c
index 8b908e8d7495..c7772733a363 100644
--- a/drivers/ras/amd/atl/map.c
+++ b/drivers/ras/amd/atl/map.c
@@ -642,6 +642,107 @@ static int get_global_map_data(struct addr_ctx *ctx)
return 0;
}
+/*
+ * Verify the interleave bits are correct in the different interleaving
+ * settings.
+ *
+ * If @num_intlv_dies and/or @num_intlv_sockets are 1, it means the
+ * respective interleaving is disabled.
+ */
+inline bool map_bits_valid(struct addr_ctx *ctx, u8 bit1, u8 bit2,
Missing "static" keyword.
+ u8 num_intlv_dies, u8 num_intlv_sockets)
+{
+ if (!(ctx->map.intlv_bit_pos == bit1 || ctx->map.intlv_bit_pos == bit2)) {
+ pr_debug("Invalid interleave bit: %u", ctx->map.intlv_bit_pos);
+ return false;
+ }
+
+ if (ctx->map.num_intlv_dies > num_intlv_dies) {
+ pr_debug("Invalid number of interleave dies: %u", ctx->map.num_intlv_dies);
+ return false;
+ }
+
+ if (ctx->map.num_intlv_sockets > num_intlv_sockets) {
+ pr_debug("Invalid number of interleave sockets: %u", ctx->map.num_intlv_sockets);
+ return false;
+ }
+
+ return true;
+}
+
+static int validate_address_map(struct addr_ctx *ctx)
+{
+ switch (ctx->map.intlv_mode) {
+ case DF2_2CHAN_HASH:
+ if (!map_bits_valid(ctx, 8, 9, 1, 1))
+ goto out;
+ break;
+
+ case DF3_COD4_2CHAN_HASH:
+ case DF3_COD2_4CHAN_HASH:
+ case DF3_COD1_8CHAN_HASH:
+ if (!map_bits_valid(ctx, 8, 9, 1, 1))
+ goto out;
+ break;
+
+ case DF4_NPS4_2CHAN_HASH:
+ case DF4_NPS2_4CHAN_HASH:
+ case DF4_NPS1_8CHAN_HASH:
+ if (!map_bits_valid(ctx, 8, 8, 1, 2))
+ goto out;
+ break;
+
+ case DF4p5_NPS4_2CHAN_1K_HASH:
+ case DF4p5_NPS4_2CHAN_2K_HASH:
+ case DF4p5_NPS2_4CHAN_1K_HASH:
+ case DF4p5_NPS2_4CHAN_2K_HASH:
+ case DF4p5_NPS1_8CHAN_1K_HASH:
+ case DF4p5_NPS1_8CHAN_2K_HASH:
+ case DF4p5_NPS1_16CHAN_1K_HASH:
+ case DF4p5_NPS1_16CHAN_2K_HASH:
+ if (!map_bits_valid(ctx, 8, 8, 1, 2))
+ goto out;
+ break;
+
+ case DF4p5_NPS4_3CHAN_1K_HASH:
+ case DF4p5_NPS4_3CHAN_2K_HASH:
+ case DF4p5_NPS2_5CHAN_1K_HASH:
+ case DF4p5_NPS2_5CHAN_2K_HASH:
+ case DF4p5_NPS2_6CHAN_1K_HASH:
+ case DF4p5_NPS2_6CHAN_2K_HASH:
+ case DF4p5_NPS1_10CHAN_1K_HASH:
+ case DF4p5_NPS1_10CHAN_2K_HASH:
+ case DF4p5_NPS1_12CHAN_1K_HASH:
+ case DF4p5_NPS1_12CHAN_2K_HASH:
+ if (ctx->map.num_intlv_sockets != 1 || !map_bits_valid(ctx, 8, 0, 1, 1))
+ goto out;
+ break;
+
+ case DF4p5_NPS0_24CHAN_1K_HASH:
+ case DF4p5_NPS0_24CHAN_2K_HASH:
+ if (ctx->map.num_intlv_sockets < 2 || !map_bits_valid(ctx, 8, 0, 1, 2))
+ goto out;
+ break;
Please move the new DF4p5 np2 cases to the next patch. This patch can
then just be rearranging the existing code. And the next patch can add
new code.
+
+ case MI3_HASH_8CHAN:
+ case MI3_HASH_16CHAN:
+ case MI3_HASH_32CHAN:
+ if (!map_bits_valid(ctx, 8, 8, 4, 1))
+ goto out;
+ break;
+
+ default:
+ atl_debug_on_bad_intlv_mode(ctx);
+ return -EINVAL;
+ }
+
+ return 0;
+
+out:
+ atl_debug(ctx, "Inconsistent address map");
+ return -EINVAL;
+}
+
static void dump_address_map(struct dram_addr_map *map)
{
u8 i;
@@ -678,5 +779,9 @@ int get_address_map(struct addr_ctx *ctx)
dump_address_map(&ctx->map);
+ ret = validate_address_map(ctx);
+ if (ret)
+ return ret;
+
return ret;
}
Thanks,
Yazen