Messages in this thread Patch in this message | | | Date | Fri, 19 Apr 2024 12:20:53 -0400 | Subject | Re: [PATCH v3 3/4] RAS/AMD/ATL: Validate address map when information is gathered | From | Yazen Ghannam <> |
| |
On 4/18/24 12:14, John Allen wrote: > Validate address maps at the time the information is gathered as the > address map will not change during translation. > > Signed-off-by: John Allen <john.allen@amd.com> > --- > v2: > - New in v2. > v3: > - Move DF 4.5 np2 validation to patch 4. > - Update commit description. > --- > drivers/ras/amd/atl/dehash.c | 43 ------------------ > drivers/ras/amd/atl/map.c | 85 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 85 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..94c5f665d5bc 100644 > --- a/drivers/ras/amd/atl/map.c > +++ b/drivers/ras/amd/atl/map.c > @@ -642,6 +642,87 @@ 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. > + */ > +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 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; > +
This case ^^^ is the same as below and can fall through.
> + 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; > +
Same here.
> + 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 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;
This catches valid modes that don't have validation checks. We should drop this. Please see diff below.
> + } > + > + 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 +759,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/map.c b/drivers/ras/amd/atl/map.c index 07de50017315..75be1434505b 100644 --- a/drivers/ras/amd/atl/map.c +++ b/drivers/ras/amd/atl/map.c @@ -731,9 +731,9 @@ static int validate_address_map(struct addr_ctx *ctx) goto out; break;
+ /* Nothing to do for modes that don't need special validation checks. */ default: - atl_debug_on_bad_intlv_mode(ctx); - return -EINVAL; + break; }
return 0;
| |