Re: [PATCH v3 3/4] RAS/AMD/ATL: Validate address map when information is gathered
From: Yazen Ghannam
Date: Fri Apr 19 2024 - 12:21:06 EST
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@xxxxxxx>
> ---
> 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;