Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library
From: Yazen Ghannam
Date: Tue Dec 12 2023 - 09:33:55 EST
On 12/12/2023 8:29 AM, Borislav Petkov wrote:
On Sun, Dec 10, 2023 at 01:49:30PM -0600, Yazen Ghannam wrote:
diff --git a/drivers/ras/amd/atl/dehash.c b/drivers/ras/amd/atl/dehash.c
new file mode 100644
index 000000000000..84fe9793694e
--- /dev/null
+++ b/drivers/ras/amd/atl/dehash.c
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AMD Address Translation Library
+ *
+ * dehash.c : Functions to account for hashing bits
+ *
+ * Copyright (c) 2023, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Author: Yazen Ghannam <Yazen.Ghannam@xxxxxxx>
+ */
+
+#include "internal.h"
+
+static inline bool assert_intlv_bit(struct addr_ctx *ctx, u8 bit1, u8 bit2)
+{
+ if (ctx->map.intlv_bit_pos == bit1 || ctx->map.intlv_bit_pos == bit2)
+ return false;
+
+ warn_on_assert("%s: Invalid interleave bit: %u",
+ __func__, ctx->map.intlv_bit_pos);
+
+ return true;
+}
+
+static inline bool assert_num_intlv_dies(struct addr_ctx *ctx, u8 num_intlv_dies)
+{
+ if (ctx->map.num_intlv_dies <= num_intlv_dies)
+ return false;
+
+ warn_on_assert("%s: Invalid number of interleave dies: %u",
+ __func__, ctx->map.num_intlv_dies);
+
+ return true;
+}
+
+static inline bool assert_num_intlv_sockets(struct addr_ctx *ctx, u8 num_intlv_sockets)
+{
+ if (ctx->map.num_intlv_sockets <= num_intlv_sockets)
+ return false;
+
+ warn_on_assert("%s: Invalid number of interleave sockets: %u",
+ __func__, ctx->map.num_intlv_sockets);
+
+ return true;
+}
+
+static int df2_dehash_addr(struct addr_ctx *ctx)
+{
+ u8 hashed_bit, intlv_bit, intlv_bit_pos;
+
+ /* Assert that interleave bit is 8 or 9. */
+ if (assert_intlv_bit(ctx, 8, 9))
+ return -EINVAL;
You don't need those homegrown assertions. Instead, you do this:
diff --git a/drivers/ras/amd/atl/dehash.c b/drivers/ras/amd/atl/dehash.c
index 84fe9793694e..11634001702e 100644
--- a/drivers/ras/amd/atl/dehash.c
+++ b/drivers/ras/amd/atl/dehash.c
@@ -47,10 +47,12 @@ static inline bool assert_num_intlv_sockets(struct addr_ctx *ctx, u8 num_intlv_s
static int df2_dehash_addr(struct addr_ctx *ctx)
{
- u8 hashed_bit, intlv_bit, intlv_bit_pos;
+ u8 hashed_bit, intlv_bit;
+ u8 intlv_bit_pos = ctx->map.intlv_bit_pos;
/* Assert that interleave bit is 8 or 9. */
- if (assert_intlv_bit(ctx, 8, 9))
+ if (WARN(intlv_bit_pos != 8 && intlv_bit_pos != 9,
+ "Invalid interleave bit: %u\n", intlv_bit_pos))
return -EINVAL;
/* Assert that die and socket interleaving are disabled. */
@@ -60,7 +62,6 @@ static int df2_dehash_addr(struct addr_ctx *ctx)
if (assert_num_intlv_sockets(ctx, 1))
return -EINVAL;
- intlv_bit_pos = ctx->map.intlv_bit_pos;
intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
hashed_bit = intlv_bit;
and so on for the other two.
That's mostly how it was in the previous revision. Should I go back to
that then?
+ /* Assert that die and socket interleaving are disabled. */
+ if (assert_num_intlv_dies(ctx, 1))
+ return -EINVAL;
+
+ if (assert_num_intlv_sockets(ctx, 1))
+ return -EINVAL;
+
+ intlv_bit_pos = ctx->map.intlv_bit_pos;
+ intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
Can we keep it simple please?
intlv_bit = !!(BIT_ULL(intlv_bit_pos) & ctx->ret_addr);
That atl_get_bit() is not necessary.
Okay, will change.
+ hashed_bit = intlv_bit;
+ hashed_bit ^= FIELD_GET(BIT_ULL(12), ctx->ret_addr);
+ hashed_bit ^= FIELD_GET(BIT_ULL(18), ctx->ret_addr);
+ hashed_bit ^= FIELD_GET(BIT_ULL(21), ctx->ret_addr);
+ hashed_bit ^= FIELD_GET(BIT_ULL(30), ctx->ret_addr);
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
+
+ return 0;
+}
<---
Ack
+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;
+
+ /* Assert that interleave bit is 8 or 9. */
+ if (assert_intlv_bit(ctx, 8, 9))
+ return -EINVAL;
+
+ /* Assert that die and socket interleaving are disabled. */
+ if (assert_num_intlv_dies(ctx, 1))
+ return -EINVAL;
+
+ if (assert_num_intlv_sockets(ctx, 1))
+ return -EINVAL;
Those assertions keep repeating. Extract them into a separate function
which you call from every *dehash_addr function?
Okay, can do. This can drop the assert_*() helpers like the comments above.
+ 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);
I believe without the tabs looks good too:
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);
Okay.
+ intlv_bit_pos = ctx->map.intlv_bit_pos;
+ intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
+
+ hashed_bit = intlv_bit;
+ hashed_bit ^= FIELD_GET(BIT_ULL(14), ctx->ret_addr);
+ hashed_bit ^= FIELD_GET(BIT_ULL(18), ctx->ret_addr) & hash_ctl_64k;
+ hashed_bit ^= FIELD_GET(BIT_ULL(23), ctx->ret_addr) & hash_ctl_2M;
+ hashed_bit ^= FIELD_GET(BIT_ULL(32), ctx->ret_addr) & hash_ctl_1G;
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
+
+ /* Calculation complete for 2 channels. Continue for 4 and 8 channels. */
+ if (ctx->map.intlv_mode == DF3_COD4_2CHAN_HASH)
+ return 0;
+
+ intlv_bit = FIELD_GET(BIT_ULL(12), ctx->ret_addr);
+
+ hashed_bit = intlv_bit;
+ hashed_bit ^= FIELD_GET(BIT_ULL(16), ctx->ret_addr) & hash_ctl_64k;
+ hashed_bit ^= FIELD_GET(BIT_ULL(21), ctx->ret_addr) & hash_ctl_2M;
+ hashed_bit ^= FIELD_GET(BIT_ULL(30), ctx->ret_addr) & hash_ctl_1G;
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(12);
+
+ /* Calculation complete for 4 channels. Continue for 8 channels. */
+ if (ctx->map.intlv_mode == DF3_COD2_4CHAN_HASH)
+ return 0;
+
+ intlv_bit = FIELD_GET(BIT_ULL(13), ctx->ret_addr);
+
+ hashed_bit = intlv_bit;
+ hashed_bit ^= FIELD_GET(BIT_ULL(17), ctx->ret_addr) & hash_ctl_64k;
+ hashed_bit ^= FIELD_GET(BIT_ULL(22), ctx->ret_addr) & hash_ctl_2M;
+ hashed_bit ^= FIELD_GET(BIT_ULL(31), ctx->ret_addr) & hash_ctl_1G;
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(13);
+
+ return 0;
+}
Also, same comments about this function as for df2_dehash_addr(). Below
too.
Okay.
+
+static int df3_6chan_dehash_addr(struct addr_ctx *ctx)
+{
+ u8 intlv_bit_pos = ctx->map.intlv_bit_pos;
+ u8 hashed_bit, intlv_bit, num_intlv_bits;
+ bool hash_ctl_2M, hash_ctl_1G;
+
+ if (ctx->map.intlv_mode != DF3_6CHAN) {
+ warn_on_bad_intlv_mode(ctx);
+ return -EINVAL;
+ }
+
+ num_intlv_bits = ilog2(ctx->map.num_intlv_chan) + 1;
+
+ hash_ctl_2M = FIELD_GET(DF3_HASH_CTL_2M, ctx->map.ctl);
+ hash_ctl_1G = FIELD_GET(DF3_HASH_CTL_1G, ctx->map.ctl);
+
+ intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
+
+ hashed_bit = intlv_bit;
+ hashed_bit ^= atl_get_bit((intlv_bit_pos + num_intlv_bits), ctx->ret_addr);
+ hashed_bit ^= FIELD_GET(BIT_ULL(23), ctx->ret_addr) & hash_ctl_2M;
+ hashed_bit ^= FIELD_GET(BIT_ULL(32), ctx->ret_addr) & hash_ctl_1G;
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
+
+ intlv_bit_pos++;
+ intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
+
+ hashed_bit = intlv_bit;
+ hashed_bit ^= FIELD_GET(BIT_ULL(21), ctx->ret_addr) & hash_ctl_2M;
+ hashed_bit ^= FIELD_GET(BIT_ULL(30), ctx->ret_addr) & hash_ctl_1G;
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
+
+ intlv_bit_pos++;
+ intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
+
+ hashed_bit = intlv_bit;
+ hashed_bit ^= FIELD_GET(BIT_ULL(22), ctx->ret_addr) & hash_ctl_2M;
+ hashed_bit ^= FIELD_GET(BIT_ULL(31), ctx->ret_addr) & hash_ctl_1G;
+
+ if (hashed_bit != intlv_bit)
+ ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
+
+ return 0;
+}
...
Thanks,
Yazen