| Message ID | 20240327185222.98998-4-john.allen@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RAS: ATL: DF 4.5 NP2 Denormalization | expand |
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@amd.com> > --- > 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
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, + 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; + + 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; }
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. Signed-off-by: John Allen <john.allen@amd.com> --- 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(-)