This patchset refactors the AMD MCA Address Translation code and adds
support for newer systems.
The reference code was recently refactored in preparation for updates
for future systems. These patches try to follow the reference code as
closely as possible. I also tried to address comments from previous
patchset reviews.
Patches 1-23 do the refactor without adding new system support. The goal
is to break down the translation algorithm into smaller chunks. Code
that changes between Data Fabric versions or interleaving modes is moved
to a set of function pointers. The intention is that new system support
can be added without any major refactor.
I tried to make a patch for each logical change. The top level function
was split first, then the next level of functions, etc. in a somewhat
breadth-first approach.
Patch 24 adds support for systems with Data Fabric version 3 (Rome and
later).
Each patch was build tested individually. The entire set was
functionally tested with the following modes.
Naples:
No interleaving
Channel interleaving
Die interleaving
Socket interleaving
Rome:
No interleaving
Nodes-per-Socket 0 (NPS0)
Nodes-per-Socket 1 (NPS1)
Nodes-per-Socket 2 (NPS2)
Nodes-per-Socket 4 (NPS4)
NPS2 w/o hashing
NPS4 w/o hashing
Note to the maintainers:
If there are no major issues, I'd like for this set to be applied,
please. Comments are still welcome, of course. There will be a future
set to add support for Family 19h Models 10h, etc., and I'd like to
address comments there.
Thanks,
Yazen
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Rebased on latest ras/edac-for-next.
* Dropped patches merged in v5.17 (Thanks Boris!)
* Dropped patches for CPU+GPU systems.
* Dropped patch that changed function parameters.
* Folded glossary patch into other patches.
* Left in pr_debug() statements that I found useful during development.
v2->v3:
* Drop "df_regs" use.
* Include patches needed for CPU+GPU systems.
* Set "df_ops" at module init based on family type.
v1->v2:
* Move address translation code to EDAC.
* Use function pointers to handle code differences between DF versions.
* Add glossary of acronyms.
Yazen Ghannam (24):
EDAC/amd64: Define Data Fabric operations
EDAC/amd64: Define functions for DramOffset
EDAC/amd64: Define function to read DRAM address map registers
EDAC/amd64: Define function to find interleaving mode
EDAC/amd64: Define function to denormalize address
EDAC/amd64: Define function to add DRAM base and hole
EDAC/amd64: Define function to dehash address
EDAC/amd64: Define function to check DRAM limit address
EDAC/amd64: Remove goto statements
EDAC/amd64: Define function to get Interleave Address Bit
EDAC/amd64: Skip denormalization if no interleaving
EDAC/amd64: Define function to get number of interleaved channels
EDAC/amd64: Define function to get number of interleaved dies
EDAC/amd64: Define function to get number of interleaved sockets
EDAC/amd64: Remove unnecessary assert
EDAC/amd64: Define function to make space for CS ID
EDAC/amd64: Define function to calculate CS ID
EDAC/amd64: Define function to insert CS ID into address
EDAC/amd64: Define function to get CS Fabric ID
EDAC/amd64: Define function to find shift and mask values
EDAC/amd64: Update CS ID calculation to match reference code
EDAC/amd64: Match hash function to reference code
EDAC/amd64: Define function to get interleave address select bit
EDAC/amd64: Add support for address translation on DF3 systems
drivers/edac/amd64_edac.c | 710 ++++++++++++++++++++++++++++++--------
1 file changed, 565 insertions(+), 145 deletions(-)
--
2.25.1
Move the dehashing code into a separate helper function. Define a
DF2-specific function for the current code. Specific helper functions
will be added for future DF versions.
The dehashing function used is based on the interleaving mode rather
than the Data Fabric version. So save the function pointer in the
context struct. The use of "df2" in the name of the current function is
only because the interleaving mode using it only appears on Data Fabric
2 systems.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Include pr_debug() on failure.
v2->v3:
* Was patch 12 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Add new function pointer in ctx struct.
drivers/edac/amd64_edac.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 350204eadb27..da2d0d9ce406 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1077,7 +1077,7 @@ struct addr_ctx {
u8 map_num;
u8 intlv_addr_bit;
u8 cs_id;
- bool hash_enabled;
+ int (*dehash_addr)(struct addr_ctx *ctx);
};
struct data_fabric_ops {
@@ -1090,13 +1090,29 @@ static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
return (ctx->reg_dram_offset & GENMASK_ULL(31, 20)) << 8;
}
+static int dehash_addr_df2(struct addr_ctx *ctx)
+{
+ u8 hashed_bit = (ctx->ret_addr >> 12) ^
+ (ctx->ret_addr >> 18) ^
+ (ctx->ret_addr >> 21) ^
+ (ctx->ret_addr >> 30) ^
+ ctx->cs_id;
+
+ hashed_bit &= BIT(0);
+
+ if (hashed_bit != ((ctx->ret_addr >> ctx->intlv_addr_bit) & BIT(0)))
+ ctx->ret_addr ^= BIT(ctx->intlv_addr_bit);
+
+ return 0;
+}
+
static int get_intlv_mode_df2(struct addr_ctx *ctx)
{
ctx->intlv_mode = (ctx->reg_base_addr >> 4) & 0xF;
if (ctx->intlv_mode == 8) {
ctx->intlv_mode = DF2_HASH_2CH;
- ctx->hash_enabled = true;
+ ctx->dehash_addr = dehash_addr_df2;
}
if (ctx->intlv_mode != NONE &&
@@ -1313,7 +1329,6 @@ static int add_base_and_hole(struct addr_ctx *ctx)
static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
{
u64 dram_limit_addr;
- u8 hashed_bit;
struct addr_ctx ctx;
@@ -1357,18 +1372,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
goto out_err;
}
- if (ctx.hash_enabled) {
- /* Save some parentheses and grab ls-bit at the end. */
- hashed_bit = (ctx.ret_addr >> 12) ^
- (ctx.ret_addr >> 18) ^
- (ctx.ret_addr >> 21) ^
- (ctx.ret_addr >> 30) ^
- ctx.cs_id;
-
- hashed_bit &= BIT(0);
-
- if (hashed_bit != ((ctx.ret_addr >> ctx.intlv_addr_bit) & BIT(0)))
- ctx.ret_addr ^= BIT(ctx.intlv_addr_bit);
+ if (ctx.dehash_addr && ctx.dehash_addr(&ctx)) {
+ pr_debug("Failed to dehash address");
+ goto out_err;
}
/* Is calculated system address is above DRAM limit address? */
--
2.25.1
Move code to find the interleave address bit into a separate helper
function.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 16 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index d568ad886d35..53d9c4b1c233 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1182,25 +1182,33 @@ static int get_dram_addr_map(struct addr_ctx *ctx)
return 0;
}
-static int denormalize_addr(struct addr_ctx *ctx)
+static int get_intlv_addr_bit(struct addr_ctx *ctx)
{
- u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
- u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
u8 intlv_addr_sel = (ctx->reg_base_addr >> 8) & 0x7;
- u8 num_intlv_bits, cs_mask = 0;
/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
if (intlv_addr_sel > 3) {
- pr_err("%s: Invalid interleave address select %d.\n",
- __func__, intlv_addr_sel);
+ pr_debug("Invalid interleave address select %d.\n", intlv_addr_sel);
return -EINVAL;
}
+ ctx->intlv_addr_bit = intlv_addr_sel + 8;
+
+ return 0;
+}
+
+static int denormalize_addr(struct addr_ctx *ctx)
+{
+ u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
+ u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
+ u8 num_intlv_bits, cs_mask = 0;
+
+ if (get_intlv_addr_bit(ctx))
+ return -EINVAL;
+
intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1;
intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3;
- ctx->intlv_addr_bit = intlv_addr_sel + 8;
-
/* Re-use intlv_num_chan by setting it equal to log2(#channels) */
switch (intlv_num_chan) {
case 0: intlv_num_chan = 0; break;
--
2.25.1
Move the DRAM limit check into a separate helper function.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Include pr_debug() on failure.
v2->v3:
* Was patch 13 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index da2d0d9ce406..139dca3a3ba4 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1326,10 +1326,20 @@ static int add_base_and_hole(struct addr_ctx *ctx)
return 0;
}
-static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
+static int addr_over_limit(struct addr_ctx *ctx)
{
- u64 dram_limit_addr;
+ u64 dram_limit_addr = ((ctx->reg_limit_addr & GENMASK_ULL(31, 12)) << 16)
+ | GENMASK_ULL(27, 0);
+
+ /* Is calculated system address above DRAM limit address? */
+ if (ctx->ret_addr > dram_limit_addr)
+ return -EINVAL;
+
+ return 0;
+}
+static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
+{
struct addr_ctx ctx;
if (!df_ops) {
@@ -1365,8 +1375,6 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
goto out_err;
}
- dram_limit_addr = ((ctx.reg_limit_addr & GENMASK_ULL(31, 12)) << 16) | GENMASK_ULL(27, 0);
-
if (add_base_and_hole(&ctx)) {
pr_debug("Failed to add DRAM base address and hole");
goto out_err;
@@ -1377,9 +1385,10 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
goto out_err;
}
- /* Is calculated system address is above DRAM limit address? */
- if (ctx.ret_addr > dram_limit_addr)
+ if (addr_over_limit(&ctx)) {
+ pr_debug("Calculated address is over limit");
goto out_err;
+ }
*sys_addr = ctx.ret_addr;
return 0;
--
2.25.1
Denormalization doesn't apply to the "no interleaving" mode, so return
early without error in this case.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 17 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 53d9c4b1c233..b75311acbe13 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1203,6 +1203,10 @@ static int denormalize_addr(struct addr_ctx *ctx)
u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
u8 num_intlv_bits, cs_mask = 0;
+ /* Return early if no interleaving. */
+ if (ctx->intlv_mode == NONE)
+ return 0;
+
if (get_intlv_addr_bit(ctx))
return -EINVAL;
--
2.25.1
Move parsing of the number of interleaved dies to a separate helper
function. This will be expanded for future DF versions. Also, drop an
unneeded assert to match the reference code.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Remove leading whitespace in function pointer.
v2->v3:
* Was patch 19 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Add new function to data_fabric_ops.
drivers/edac/amd64_edac.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 87439374a076..2973b7b5e8a2 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1077,6 +1077,7 @@ struct addr_ctx {
u8 map_num;
u8 intlv_addr_bit;
u8 intlv_num_chan;
+ u8 intlv_num_dies;
u8 cs_id;
int (*dehash_addr)(struct addr_ctx *ctx);
};
@@ -1084,6 +1085,7 @@ struct addr_ctx {
struct data_fabric_ops {
u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
int (*get_intlv_mode)(struct addr_ctx *ctx);
+ void (*get_intlv_num_dies)(struct addr_ctx *ctx);
};
static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
@@ -1124,9 +1126,15 @@ static int get_intlv_mode_df2(struct addr_ctx *ctx)
return 0;
}
+static void get_intlv_num_dies_df2(struct addr_ctx *ctx)
+{
+ ctx->intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3;
+}
+
struct data_fabric_ops df2_ops = {
.get_hi_addr_offset = get_hi_addr_offset_df2,
.get_intlv_mode = get_intlv_mode_df2,
+ .get_intlv_num_dies = get_intlv_num_dies_df2,
};
struct data_fabric_ops *df_ops;
@@ -1218,7 +1226,7 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
static int denormalize_addr(struct addr_ctx *ctx)
{
u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
- u8 intlv_num_dies, intlv_num_sockets;
+ u8 intlv_num_sockets;
u8 num_intlv_bits, cs_mask = 0;
/* Return early if no interleaving. */
@@ -1229,19 +1237,12 @@ static int denormalize_addr(struct addr_ctx *ctx)
return -EINVAL;
intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1;
- intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3;
get_intlv_num_chan(ctx);
+ df_ops->get_intlv_num_dies(ctx);
num_intlv_bits = ctx->intlv_num_chan;
-
- if (intlv_num_dies > 2) {
- pr_err("%s: Invalid number of interleaved nodes/dies %d.\n",
- __func__, intlv_num_dies);
- return -EINVAL;
- }
-
- num_intlv_bits += intlv_num_dies;
+ num_intlv_bits += ctx->intlv_num_dies;
/* Add a bit if sockets are interleaved. */
num_intlv_bits += intlv_num_sockets;
@@ -1279,13 +1280,13 @@ static int denormalize_addr(struct addr_ctx *ctx)
sock_id_bit = die_id_bit;
/* Read D18F1x208 (SystemFabricIdMask). */
- if (intlv_num_dies || intlv_num_sockets)
+ if (ctx->intlv_num_dies || intlv_num_sockets)
if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->tmp))
return -EINVAL;
/* If interleaved over more than 1 die. */
- if (intlv_num_dies) {
- sock_id_bit = die_id_bit + intlv_num_dies;
+ if (ctx->intlv_num_dies) {
+ sock_id_bit = die_id_bit + ctx->intlv_num_dies;
die_id_shift = (ctx->tmp >> 24) & 0xF;
die_id_mask = (ctx->tmp >> 8) & 0xFF;
--
2.25.1
...and just return error codes directly.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 14 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 139dca3a3ba4..d568ad886d35 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1357,44 +1357,41 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
if (remove_dram_offset(&ctx)) {
pr_debug("Failed to remove DRAM offset");
- goto out_err;
+ return -EINVAL;
}
if (get_dram_addr_map(&ctx)) {
pr_debug("Failed to get DRAM address map");
- goto out_err;
+ return -EINVAL;
}
if (df_ops->get_intlv_mode(&ctx)) {
pr_debug("Failed to get interleave mode");
- goto out_err;
+ return -EINVAL;
}
if (denormalize_addr(&ctx)) {
pr_debug("Failed to denormalize address");
- goto out_err;
+ return -EINVAL;
}
if (add_base_and_hole(&ctx)) {
pr_debug("Failed to add DRAM base address and hole");
- goto out_err;
+ return -EINVAL;
}
if (ctx.dehash_addr && ctx.dehash_addr(&ctx)) {
pr_debug("Failed to dehash address");
- goto out_err;
+ return -EINVAL;
}
if (addr_over_limit(&ctx)) {
pr_debug("Calculated address is over limit");
- goto out_err;
+ return -EINVAL;
}
*sys_addr = ctx.ret_addr;
return 0;
-
-out_err:
- return -EINVAL;
}
static int get_channel_from_ecc_syndrome(struct mem_ctl_info *, u16);
--
2.25.1
Move number of interleaved channel calculation to a separate helper
function. Drop unused cases.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 18 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 42 +++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index b75311acbe13..87439374a076 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1076,6 +1076,7 @@ struct addr_ctx {
u8 inst_id;
u8 map_num;
u8 intlv_addr_bit;
+ u8 intlv_num_chan;
u8 cs_id;
int (*dehash_addr)(struct addr_ctx *ctx);
};
@@ -1197,10 +1198,27 @@ static int get_intlv_addr_bit(struct addr_ctx *ctx)
return 0;
}
+static void get_intlv_num_chan(struct addr_ctx *ctx)
+{
+ /* Save the log2(# of channels). */
+ switch (ctx->intlv_mode) {
+ case NONE:
+ ctx->intlv_num_chan = 0;
+ break;
+ case NOHASH_2CH:
+ case DF2_HASH_2CH:
+ ctx->intlv_num_chan = 1;
+ break;
+ default:
+ /* Valid interleaving modes where checked earlier. */
+ break;
+ }
+}
+
static int denormalize_addr(struct addr_ctx *ctx)
{
u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
- u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
+ u8 intlv_num_dies, intlv_num_sockets;
u8 num_intlv_bits, cs_mask = 0;
/* Return early if no interleaving. */
@@ -1213,23 +1231,9 @@ static int denormalize_addr(struct addr_ctx *ctx)
intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1;
intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3;
- /* Re-use intlv_num_chan by setting it equal to log2(#channels) */
- switch (intlv_num_chan) {
- case 0: intlv_num_chan = 0; break;
- case 1: intlv_num_chan = 1; break;
- case 3: intlv_num_chan = 2; break;
- case 5: intlv_num_chan = 3; break;
- case 7: intlv_num_chan = 4; break;
-
- case 8: intlv_num_chan = 1;
- break;
- default:
- pr_err("%s: Invalid number of interleaved channels %d.\n",
- __func__, intlv_num_chan);
- return -EINVAL;
- }
+ get_intlv_num_chan(ctx);
- num_intlv_bits = intlv_num_chan;
+ num_intlv_bits = ctx->intlv_num_chan;
if (intlv_num_dies > 2) {
pr_err("%s: Invalid number of interleaved nodes/dies %d.\n",
@@ -1266,8 +1270,8 @@ static int denormalize_addr(struct addr_ctx *ctx)
die_id_bit = 0;
/* If interleaved over more than 1 channel: */
- if (intlv_num_chan) {
- die_id_bit = intlv_num_chan;
+ if (ctx->intlv_num_chan) {
+ die_id_bit = ctx->intlv_num_chan;
cs_mask = (1 << die_id_bit) - 1;
ctx->cs_id = cs_fabric_id & cs_mask;
}
--
2.25.1
Move parsing of the number of interleaved sockets to a separate helper
function. This will be expanded for future DF versions.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Remove leading whitespace in function pointer.
v2->v3:
* Was patch 20 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Add new function to data_fabric_ops.
drivers/edac/amd64_edac.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 2973b7b5e8a2..898f53eaaff3 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1078,6 +1078,7 @@ struct addr_ctx {
u8 intlv_addr_bit;
u8 intlv_num_chan;
u8 intlv_num_dies;
+ u8 intlv_num_sockets;
u8 cs_id;
int (*dehash_addr)(struct addr_ctx *ctx);
};
@@ -1086,6 +1087,7 @@ struct data_fabric_ops {
u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
int (*get_intlv_mode)(struct addr_ctx *ctx);
void (*get_intlv_num_dies)(struct addr_ctx *ctx);
+ void (*get_intlv_num_sockets)(struct addr_ctx *ctx);
};
static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
@@ -1131,10 +1133,16 @@ static void get_intlv_num_dies_df2(struct addr_ctx *ctx)
ctx->intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3;
}
+static void get_intlv_num_sockets_df2(struct addr_ctx *ctx)
+{
+ ctx->intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1;
+}
+
struct data_fabric_ops df2_ops = {
.get_hi_addr_offset = get_hi_addr_offset_df2,
.get_intlv_mode = get_intlv_mode_df2,
.get_intlv_num_dies = get_intlv_num_dies_df2,
+ .get_intlv_num_sockets = get_intlv_num_sockets_df2,
};
struct data_fabric_ops *df_ops;
@@ -1226,7 +1234,6 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
static int denormalize_addr(struct addr_ctx *ctx)
{
u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
- u8 intlv_num_sockets;
u8 num_intlv_bits, cs_mask = 0;
/* Return early if no interleaving. */
@@ -1236,16 +1243,13 @@ static int denormalize_addr(struct addr_ctx *ctx)
if (get_intlv_addr_bit(ctx))
return -EINVAL;
- intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1;
-
get_intlv_num_chan(ctx);
df_ops->get_intlv_num_dies(ctx);
+ df_ops->get_intlv_num_sockets(ctx);
num_intlv_bits = ctx->intlv_num_chan;
num_intlv_bits += ctx->intlv_num_dies;
-
- /* Add a bit if sockets are interleaved. */
- num_intlv_bits += intlv_num_sockets;
+ num_intlv_bits += ctx->intlv_num_sockets;
/* Assert num_intlv_bits <= 4 */
if (num_intlv_bits > 4) {
@@ -1280,7 +1284,7 @@ static int denormalize_addr(struct addr_ctx *ctx)
sock_id_bit = die_id_bit;
/* Read D18F1x208 (SystemFabricIdMask). */
- if (ctx->intlv_num_dies || intlv_num_sockets)
+ if (ctx->intlv_num_dies || ctx->intlv_num_sockets)
if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->tmp))
return -EINVAL;
@@ -1295,7 +1299,7 @@ static int denormalize_addr(struct addr_ctx *ctx)
}
/* If interleaved over more than 1 socket. */
- if (intlv_num_sockets) {
+ if (ctx->intlv_num_sockets) {
socket_id_shift = (ctx->tmp >> 28) & 0xF;
socket_id_mask = (ctx->tmp >> 16) & 0xFF;
--
2.25.1
Move adding of DRAM base and hole into a separate helper function.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Include pr_debug() on failure.
v2->v3:
* Was patch 11 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 43 +++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 9c61e3fa231a..350204eadb27 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1288,12 +1288,32 @@ static int denormalize_addr(struct addr_ctx *ctx)
return 0;
}
-static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
+static int add_base_and_hole(struct addr_ctx *ctx)
{
- u64 dram_base_addr, dram_limit_addr, dram_hole_base;
+ u64 dram_base_addr = (ctx->reg_base_addr & GENMASK_ULL(31, 12)) << 16;
+
+ /* Add dram base address */
+ ctx->ret_addr += dram_base_addr;
+
+ /* If legacy MMIO hole enabled */
+ if (ctx->reg_base_addr & BIT(1)) {
+ u32 dram_hole_base;
+
+ if (df_indirect_read_broadcast(ctx->nid, 0, 0x104, &dram_hole_base))
+ return -EINVAL;
+
+ dram_hole_base &= GENMASK(31, 24);
+ if (ctx->ret_addr >= dram_hole_base)
+ ctx->ret_addr += (BIT_ULL(32) - dram_hole_base);
+ }
+
+ return 0;
+}
+static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
+{
+ u64 dram_limit_addr;
u8 hashed_bit;
- u8 lgcy_mmio_hole_en;
struct addr_ctx ctx;
@@ -1330,22 +1350,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
goto out_err;
}
- lgcy_mmio_hole_en = ctx.reg_base_addr & BIT(1);
- dram_base_addr = (ctx.reg_base_addr & GENMASK_ULL(31, 12)) << 16;
-
dram_limit_addr = ((ctx.reg_limit_addr & GENMASK_ULL(31, 12)) << 16) | GENMASK_ULL(27, 0);
- /* Add dram base address */
- ctx.ret_addr += dram_base_addr;
-
- /* If legacy MMIO hole enabled */
- if (lgcy_mmio_hole_en) {
- if (df_indirect_read_broadcast(nid, 0, 0x104, &ctx.tmp))
- goto out_err;
-
- dram_hole_base = ctx.tmp & GENMASK(31, 24);
- if (ctx.ret_addr >= dram_hole_base)
- ctx.ret_addr += (BIT_ULL(32) - dram_hole_base);
+ if (add_base_and_hole(&ctx)) {
+ pr_debug("Failed to add DRAM base address and hole");
+ goto out_err;
}
if (ctx.hash_enabled) {
--
2.25.1
It was removed in the reference code, so remove it here.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 21 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 898f53eaaff3..c3342f0bec45 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1251,13 +1251,6 @@ static int denormalize_addr(struct addr_ctx *ctx)
num_intlv_bits += ctx->intlv_num_dies;
num_intlv_bits += ctx->intlv_num_sockets;
- /* Assert num_intlv_bits <= 4 */
- if (num_intlv_bits > 4) {
- pr_err("%s: Invalid interleave bits %d.\n",
- __func__, num_intlv_bits);
- return -EINVAL;
- }
-
if (num_intlv_bits > 0) {
u64 temp_addr_x, temp_addr_i, temp_addr_y;
u8 die_id_bit, sock_id_bit, cs_fabric_id;
--
2.25.1
Move the code that inserts the CS ID into the address into a separate
helper function.
Save the function pointer in the context struct. It will be set based on
interleaving mode rather than Data Fabric version. This will be expanded
for new interleaving modes added in future Data Fabric versions.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 24 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Added function pointer to ctx struct.
drivers/edac/amd64_edac.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index b2bcd8ea1a06..e3db4e98fe58 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1083,6 +1083,7 @@ struct addr_ctx {
u8 cs_id;
int (*dehash_addr)(struct addr_ctx *ctx);
void (*make_space_for_cs_id)(struct addr_ctx *ctx);
+ void (*insert_cs_id)(struct addr_ctx *ctx);
};
struct data_fabric_ops {
@@ -1115,6 +1116,11 @@ static void make_space_for_cs_id_simple(struct addr_ctx *ctx)
expand_bits(ctx->intlv_addr_bit, num_intlv_bits, &ctx->ret_addr);
}
+static void insert_cs_id_simple(struct addr_ctx *ctx)
+{
+ ctx->ret_addr |= (ctx->cs_id << ctx->intlv_addr_bit);
+}
+
static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
{
return (ctx->reg_dram_offset & GENMASK_ULL(31, 20)) << 8;
@@ -1146,6 +1152,7 @@ static int get_intlv_mode_df2(struct addr_ctx *ctx)
}
ctx->make_space_for_cs_id = make_space_for_cs_id_simple;
+ ctx->insert_cs_id = insert_cs_id_simple;
if (ctx->intlv_mode != NONE &&
ctx->intlv_mode != NOHASH_2CH &&
@@ -1306,8 +1313,6 @@ static int calculate_cs_id(struct addr_ctx *ctx)
static int denormalize_addr(struct addr_ctx *ctx)
{
- u8 num_intlv_bits;
-
/* Return early if no interleaving. */
if (ctx->intlv_mode == NONE)
return 0;
@@ -1326,20 +1331,7 @@ static int denormalize_addr(struct addr_ctx *ctx)
return -EINVAL;
}
- if (num_intlv_bits > 0) {
- u64 temp_addr_i;
-
- /*
- * The pre-interleaved address consists of XXXXXXIIIYYYYY
- * where III is the ID for this CS, and XXXXXXYYYYY are the
- * address bits from the post-interleaved address.
- * "num_intlv_bits" has been calculated to tell us how many "I"
- * bits there are. "intlv_addr_bit" tells us how many "Y" bits
- * there are (where "I" starts).
- */
- temp_addr_i = (ctx->cs_id << ctx->intlv_addr_bit);
- ctx->ret_addr |= temp_addr_i;
- }
+ ctx->insert_cs_id(ctx);
return 0;
}
--
2.25.1
Move code to find the shift and mask values used in die and socket
interleaving into separate helper functions. These will be expanded for
future DF versions. Make the die_id_mask and socket_id_mask values u16
type to accommodate larger bitfields in future DF versions.
Also, move reading of the System Fabric ID Mask register into
get_masks(). This will be expanded for future DF versions.
Call get_masks() early since future DF versions may need these values
early.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Remove leading whitespace in function pointers.
* Include pr_debug() in failure.
v2->v3:
* Was patch 26 in v2.
* Remove early code related to "df_ops".
v1->v2:
* Moved from arch/x86 to EDAC.
* Added functions to data_fabric_ops.
drivers/edac/amd64_edac.c | 59 ++++++++++++++++++++++++++++++---------
1 file changed, 46 insertions(+), 13 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 7891c2e93d53..ac7919010063 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1073,7 +1073,10 @@ struct addr_ctx {
u32 reg_dram_offset;
u32 reg_base_addr;
u32 reg_limit_addr;
+ u32 reg_fab_id_mask0;
u16 cs_fabric_id;
+ u16 die_id_mask;
+ u16 socket_id_mask;
u16 nid;
u8 inst_id;
u8 map_num;
@@ -1089,8 +1092,11 @@ struct addr_ctx {
struct data_fabric_ops {
u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
+ u8 (*get_die_id_shift)(struct addr_ctx *ctx);
+ u8 (*get_socket_id_shift)(struct addr_ctx *ctx);
int (*get_intlv_mode)(struct addr_ctx *ctx);
int (*get_cs_fabric_id)(struct addr_ctx *ctx);
+ int (*get_masks)(struct addr_ctx *ctx);
void (*get_intlv_num_dies)(struct addr_ctx *ctx);
void (*get_intlv_num_sockets)(struct addr_ctx *ctx);
};
@@ -1185,12 +1191,37 @@ static int get_cs_fabric_id_df2(struct addr_ctx *ctx)
return 0;
}
+static int get_masks_df2(struct addr_ctx *ctx)
+{
+ /* Read D18F1x208 (SystemFabricIdMask). */
+ if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->reg_fab_id_mask0))
+ return -EINVAL;
+
+ ctx->die_id_mask = (ctx->reg_fab_id_mask0 >> 8) & 0xFF;
+ ctx->socket_id_mask = (ctx->reg_fab_id_mask0 >> 16) & 0xFF;
+
+ return 0;
+}
+
+static u8 get_die_id_shift_df2(struct addr_ctx *ctx)
+{
+ return (ctx->reg_fab_id_mask0 >> 24) & 0xF;
+}
+
+static u8 get_socket_id_shift_df2(struct addr_ctx *ctx)
+{
+ return (ctx->reg_fab_id_mask0 >> 28) & 0xF;
+}
+
struct data_fabric_ops df2_ops = {
.get_hi_addr_offset = get_hi_addr_offset_df2,
.get_intlv_mode = get_intlv_mode_df2,
.get_intlv_num_dies = get_intlv_num_dies_df2,
.get_intlv_num_sockets = get_intlv_num_sockets_df2,
.get_cs_fabric_id = get_cs_fabric_id_df2,
+ .get_masks = get_masks_df2,
+ .get_die_id_shift = get_die_id_shift_df2,
+ .get_socket_id_shift = get_socket_id_shift_df2,
};
struct data_fabric_ops *df_ops;
@@ -1281,7 +1312,6 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
static int calculate_cs_id(struct addr_ctx *ctx)
{
- u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
u8 die_id_bit = 0, sock_id_bit, cs_mask = 0;
/* If interleaved over more than 1 channel: */
@@ -1291,28 +1321,26 @@ static int calculate_cs_id(struct addr_ctx *ctx)
ctx->cs_id = ctx->cs_fabric_id & cs_mask;
}
- sock_id_bit = die_id_bit;
+ /* Return early if no die interleaving and no socket interleaving. */
+ if (!(ctx->intlv_num_dies || ctx->intlv_num_sockets))
+ return 0;
- /* Read D18F1x208 (SystemFabricIdMask). */
- if (ctx->intlv_num_dies || ctx->intlv_num_sockets)
- if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->tmp))
- return -EINVAL;
+ sock_id_bit = die_id_bit;
/* If interleaved over more than 1 die: */
if (ctx->intlv_num_dies) {
- sock_id_bit = die_id_bit + ctx->intlv_num_dies;
- die_id_shift = (ctx->tmp >> 24) & 0xF;
- die_id_mask = (ctx->tmp >> 8) & 0xFF;
+ u8 die_id_shift = df_ops->get_die_id_shift(ctx);
- ctx->cs_id |= ((ctx->cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
+ sock_id_bit = die_id_bit + ctx->intlv_num_dies;
+ ctx->cs_id |= ((ctx->cs_fabric_id & ctx->die_id_mask)
+ >> die_id_shift) << die_id_bit;
}
/* If interleaved over more than 1 socket: */
if (ctx->intlv_num_sockets) {
- socket_id_shift = (ctx->tmp >> 28) & 0xF;
- socket_id_mask = (ctx->tmp >> 16) & 0xFF;
+ u8 socket_id_shift = df_ops->get_socket_id_shift(ctx);
- ctx->cs_id |= ((ctx->cs_fabric_id & socket_id_mask)
+ ctx->cs_id |= ((ctx->cs_fabric_id & ctx->socket_id_mask)
>> socket_id_shift) << sock_id_bit;
}
@@ -1395,6 +1423,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
ctx.nid = nid;
ctx.inst_id = umc;
+ if (df_ops->get_masks(&ctx)) {
+ pr_debug("Failed to get masks");
+ return -EINVAL;
+ }
+
if (df_ops->get_cs_fabric_id(&ctx)) {
pr_debug("Failed to get CS Fabric ID");
return -EINVAL;
--
2.25.1
Move code that gets the CS Fabric ID into a separate helper function.
This will be expanded for future DF versions.
The bitfield used for this value may be larger than the 8 bits currently
used. So make it a u16 type which is large enough to hold all known
sizes of this bitfield across DF versions.
Also, call this function early as future DF versions may need the value
early.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Remove leading whitespace in function pointer.
* Include pr_debug() in failure.
v2->v3:
* Was patch 25 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Added function to data_fabric_ops.
drivers/edac/amd64_edac.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index e3db4e98fe58..7891c2e93d53 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1073,6 +1073,7 @@ struct addr_ctx {
u32 reg_dram_offset;
u32 reg_base_addr;
u32 reg_limit_addr;
+ u16 cs_fabric_id;
u16 nid;
u8 inst_id;
u8 map_num;
@@ -1089,6 +1090,7 @@ struct addr_ctx {
struct data_fabric_ops {
u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
int (*get_intlv_mode)(struct addr_ctx *ctx);
+ int (*get_cs_fabric_id)(struct addr_ctx *ctx);
void (*get_intlv_num_dies)(struct addr_ctx *ctx);
void (*get_intlv_num_sockets)(struct addr_ctx *ctx);
};
@@ -1172,11 +1174,23 @@ static void get_intlv_num_sockets_df2(struct addr_ctx *ctx)
ctx->intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1;
}
+static int get_cs_fabric_id_df2(struct addr_ctx *ctx)
+{
+ /* Read D18F0x50 (FabricBlockInstanceInformation3). */
+ if (df_indirect_read_instance(ctx->nid, 0, 0x50, ctx->inst_id, &ctx->tmp))
+ return -EINVAL;
+
+ ctx->cs_fabric_id = (ctx->tmp >> 8) & 0xFF;
+
+ return 0;
+}
+
struct data_fabric_ops df2_ops = {
.get_hi_addr_offset = get_hi_addr_offset_df2,
.get_intlv_mode = get_intlv_mode_df2,
.get_intlv_num_dies = get_intlv_num_dies_df2,
.get_intlv_num_sockets = get_intlv_num_sockets_df2,
+ .get_cs_fabric_id = get_cs_fabric_id_df2,
};
struct data_fabric_ops *df_ops;
@@ -1268,20 +1282,13 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
static int calculate_cs_id(struct addr_ctx *ctx)
{
u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
- u8 die_id_bit, sock_id_bit, cs_fabric_id, cs_mask = 0;
-
- /* Read D18F0x50 (FabricBlockInstanceInformation3). */
- if (df_indirect_read_instance(ctx->nid, 0, 0x50, ctx->inst_id, &ctx->tmp))
- return -EINVAL;
-
- cs_fabric_id = (ctx->tmp >> 8) & 0xFF;
- die_id_bit = 0;
+ u8 die_id_bit = 0, sock_id_bit, cs_mask = 0;
/* If interleaved over more than 1 channel: */
if (ctx->intlv_num_chan) {
die_id_bit = ctx->intlv_num_chan;
cs_mask = (1 << die_id_bit) - 1;
- ctx->cs_id = cs_fabric_id & cs_mask;
+ ctx->cs_id = ctx->cs_fabric_id & cs_mask;
}
sock_id_bit = die_id_bit;
@@ -1297,7 +1304,7 @@ static int calculate_cs_id(struct addr_ctx *ctx)
die_id_shift = (ctx->tmp >> 24) & 0xF;
die_id_mask = (ctx->tmp >> 8) & 0xFF;
- ctx->cs_id |= ((cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
+ ctx->cs_id |= ((ctx->cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
}
/* If interleaved over more than 1 socket: */
@@ -1305,7 +1312,8 @@ static int calculate_cs_id(struct addr_ctx *ctx)
socket_id_shift = (ctx->tmp >> 28) & 0xF;
socket_id_mask = (ctx->tmp >> 16) & 0xFF;
- ctx->cs_id |= ((cs_fabric_id & socket_id_mask) >> socket_id_shift) << sock_id_bit;
+ ctx->cs_id |= ((ctx->cs_fabric_id & socket_id_mask)
+ >> socket_id_shift) << sock_id_bit;
}
return 0;
@@ -1387,6 +1395,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
ctx.nid = nid;
ctx.inst_id = umc;
+ if (df_ops->get_cs_fabric_id(&ctx)) {
+ pr_debug("Failed to get CS Fabric ID");
+ return -EINVAL;
+ }
+
if (remove_dram_offset(&ctx)) {
pr_debug("Failed to remove DRAM offset");
return -EINVAL;
--
2.25.1
Move code used to calculate the CS ID into a separate helper function.
Drop redundant code comment about reading DF register.
The "num_intlv_bits" variable is left uninitialized as it will be removed
in a later patch.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Include pr_debug() on failure.
v2->v3:
* Was patch 23 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 100 ++++++++++++++++++++------------------
1 file changed, 52 insertions(+), 48 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index f5b1902e04ac..b2bcd8ea1a06 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1258,10 +1258,55 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
}
}
-static int denormalize_addr(struct addr_ctx *ctx)
+static int calculate_cs_id(struct addr_ctx *ctx)
{
u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
- u8 num_intlv_bits, cs_mask = 0;
+ u8 die_id_bit, sock_id_bit, cs_fabric_id, cs_mask = 0;
+
+ /* Read D18F0x50 (FabricBlockInstanceInformation3). */
+ if (df_indirect_read_instance(ctx->nid, 0, 0x50, ctx->inst_id, &ctx->tmp))
+ return -EINVAL;
+
+ cs_fabric_id = (ctx->tmp >> 8) & 0xFF;
+ die_id_bit = 0;
+
+ /* If interleaved over more than 1 channel: */
+ if (ctx->intlv_num_chan) {
+ die_id_bit = ctx->intlv_num_chan;
+ cs_mask = (1 << die_id_bit) - 1;
+ ctx->cs_id = cs_fabric_id & cs_mask;
+ }
+
+ sock_id_bit = die_id_bit;
+
+ /* Read D18F1x208 (SystemFabricIdMask). */
+ if (ctx->intlv_num_dies || ctx->intlv_num_sockets)
+ if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->tmp))
+ return -EINVAL;
+
+ /* If interleaved over more than 1 die: */
+ if (ctx->intlv_num_dies) {
+ sock_id_bit = die_id_bit + ctx->intlv_num_dies;
+ die_id_shift = (ctx->tmp >> 24) & 0xF;
+ die_id_mask = (ctx->tmp >> 8) & 0xFF;
+
+ ctx->cs_id |= ((cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
+ }
+
+ /* If interleaved over more than 1 socket: */
+ if (ctx->intlv_num_sockets) {
+ socket_id_shift = (ctx->tmp >> 28) & 0xF;
+ socket_id_mask = (ctx->tmp >> 16) & 0xFF;
+
+ ctx->cs_id |= ((cs_fabric_id & socket_id_mask) >> socket_id_shift) << sock_id_bit;
+ }
+
+ return 0;
+}
+
+static int denormalize_addr(struct addr_ctx *ctx)
+{
+ u8 num_intlv_bits;
/* Return early if no interleaving. */
if (ctx->intlv_mode == NONE)
@@ -1276,55 +1321,14 @@ static int denormalize_addr(struct addr_ctx *ctx)
ctx->make_space_for_cs_id(ctx);
+ if (calculate_cs_id(ctx)) {
+ pr_debug("Failed to calculate CS ID");
+ return -EINVAL;
+ }
+
if (num_intlv_bits > 0) {
- u8 die_id_bit, sock_id_bit, cs_fabric_id;
u64 temp_addr_i;
- /*
- * Read FabricBlockInstanceInformation3_CS[BlockFabricID].
- * This is the fabric id for this coherent slave. Use
- * umc/channel# as instance id of the coherent slave
- * for FICAA.
- */
- if (df_indirect_read_instance(ctx->nid, 0, 0x50, ctx->inst_id, &ctx->tmp))
- return -EINVAL;
-
- cs_fabric_id = (ctx->tmp >> 8) & 0xFF;
- die_id_bit = 0;
-
- /* If interleaved over more than 1 channel: */
- if (ctx->intlv_num_chan) {
- die_id_bit = ctx->intlv_num_chan;
- cs_mask = (1 << die_id_bit) - 1;
- ctx->cs_id = cs_fabric_id & cs_mask;
- }
-
- sock_id_bit = die_id_bit;
-
- /* Read D18F1x208 (SystemFabricIdMask). */
- if (ctx->intlv_num_dies || ctx->intlv_num_sockets)
- if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->tmp))
- return -EINVAL;
-
- /* If interleaved over more than 1 die. */
- if (ctx->intlv_num_dies) {
- sock_id_bit = die_id_bit + ctx->intlv_num_dies;
- die_id_shift = (ctx->tmp >> 24) & 0xF;
- die_id_mask = (ctx->tmp >> 8) & 0xFF;
-
- ctx->cs_id |= ((cs_fabric_id & die_id_mask)
- >> die_id_shift) << die_id_bit;
- }
-
- /* If interleaved over more than 1 socket. */
- if (ctx->intlv_num_sockets) {
- socket_id_shift = (ctx->tmp >> 28) & 0xF;
- socket_id_mask = (ctx->tmp >> 16) & 0xFF;
-
- ctx->cs_id |= ((cs_fabric_id & socket_id_mask)
- >> socket_id_shift) << sock_id_bit;
- }
-
/*
* The pre-interleaved address consists of XXXXXXIIIYYYYY
* where III is the ID for this CS, and XXXXXXYYYYY are the
--
2.25.1
Move code that makes a gap for the CS ID into a separate helper function.
The exact bits to use vary based on interleaving mode. New interleaving
modes in future DF versions will be added as new cases.
Also, introduce a helper function that does the bit manipulation to make
the gap. The current version of this function is "simple", and future
interleaving modes may reuse this or use a more advanced function.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Added glossary entry.
v2->v3:
* Was patch 22 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Added new function pointer to ctx struct.
drivers/edac/amd64_edac.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index c3342f0bec45..f5b1902e04ac 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -991,6 +991,7 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr)
/*
* Glossary of acronyms used in address translation for Zen-based systems
*
+ * CS = Coherent Slave
* DF = Data Fabric
*/
@@ -1081,6 +1082,7 @@ struct addr_ctx {
u8 intlv_num_sockets;
u8 cs_id;
int (*dehash_addr)(struct addr_ctx *ctx);
+ void (*make_space_for_cs_id)(struct addr_ctx *ctx);
};
struct data_fabric_ops {
@@ -1090,6 +1092,29 @@ struct data_fabric_ops {
void (*get_intlv_num_sockets)(struct addr_ctx *ctx);
};
+static void expand_bits(u8 start_bit, u8 num_bits, u64 *value)
+{
+ u64 temp1, temp2;
+
+ if (start_bit == 0) {
+ *value <<= num_bits;
+ return;
+ }
+
+ temp1 = *value & GENMASK_ULL(start_bit - 1, 0);
+ temp2 = (*value & GENMASK_ULL(63, start_bit)) << num_bits;
+ *value = temp1 | temp2;
+}
+
+static void make_space_for_cs_id_simple(struct addr_ctx *ctx)
+{
+ u8 num_intlv_bits = ctx->intlv_num_chan;
+
+ num_intlv_bits += ctx->intlv_num_dies;
+ num_intlv_bits += ctx->intlv_num_sockets;
+ expand_bits(ctx->intlv_addr_bit, num_intlv_bits, &ctx->ret_addr);
+}
+
static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
{
return (ctx->reg_dram_offset & GENMASK_ULL(31, 20)) << 8;
@@ -1120,6 +1145,8 @@ static int get_intlv_mode_df2(struct addr_ctx *ctx)
ctx->dehash_addr = dehash_addr_df2;
}
+ ctx->make_space_for_cs_id = make_space_for_cs_id_simple;
+
if (ctx->intlv_mode != NONE &&
ctx->intlv_mode != NOHASH_2CH &&
ctx->intlv_mode != DF2_HASH_2CH)
@@ -1247,13 +1274,11 @@ static int denormalize_addr(struct addr_ctx *ctx)
df_ops->get_intlv_num_dies(ctx);
df_ops->get_intlv_num_sockets(ctx);
- num_intlv_bits = ctx->intlv_num_chan;
- num_intlv_bits += ctx->intlv_num_dies;
- num_intlv_bits += ctx->intlv_num_sockets;
+ ctx->make_space_for_cs_id(ctx);
if (num_intlv_bits > 0) {
- u64 temp_addr_x, temp_addr_i, temp_addr_y;
u8 die_id_bit, sock_id_bit, cs_fabric_id;
+ u64 temp_addr_i;
/*
* Read FabricBlockInstanceInformation3_CS[BlockFabricID].
@@ -1308,11 +1333,8 @@ static int denormalize_addr(struct addr_ctx *ctx)
* bits there are. "intlv_addr_bit" tells us how many "Y" bits
* there are (where "I" starts).
*/
- temp_addr_y = ctx->ret_addr & GENMASK_ULL(ctx->intlv_addr_bit - 1, 0);
temp_addr_i = (ctx->cs_id << ctx->intlv_addr_bit);
- temp_addr_x = (ctx->ret_addr & GENMASK_ULL(63, ctx->intlv_addr_bit))
- << num_intlv_bits;
- ctx->ret_addr = temp_addr_x | temp_addr_i | temp_addr_y;
+ ctx->ret_addr |= temp_addr_i;
}
return 0;
--
2.25.1
DF3-based systems (Rome and later) support new interleaving modes and a
number of bit fields have changed or moved entirely. Add support for
these new modes and fields.
Refactoring should be minimal due to earlier changes, and most updates
will be additions.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Added glossary entry.
v2->v3:
* Was patch 30 in v2.
* Drop "df_regs" use.
* Set "df_ops" during module init.
v1->v2:
* Moved from arch/x86 to EDAC.
* Use function pointers as needed.
drivers/edac/amd64_edac.c | 188 +++++++++++++++++++++++++++++++++++++-
1 file changed, 186 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 34405c8940fb..d213f9ecab16 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -991,6 +991,7 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr)
/*
* Glossary of acronyms used in address translation for Zen-based systems
*
+ * COD = Cluster-on-Die
* CS = Coherent Slave
* DF = Data Fabric
*/
@@ -1062,6 +1063,11 @@ static int df_indirect_read_broadcast(u16 node, u8 func, u16 reg, u32 *lo)
enum intlv_modes {
NONE = 0x00,
NOHASH_2CH = 0x01,
+ NOHASH_4CH = 0x03,
+ NOHASH_8CH = 0x05,
+ HASH_COD4_2CH = 0x0C,
+ HASH_COD2_4CH = 0x0D,
+ HASH_COD1_8CH = 0x0E,
DF2_HASH_2CH = 0x21,
};
@@ -1074,6 +1080,7 @@ struct addr_ctx {
u32 reg_base_addr;
u32 reg_limit_addr;
u32 reg_fab_id_mask0;
+ u32 reg_fab_id_mask1;
u16 cs_fabric_id;
u16 die_id_mask;
u16 socket_id_mask;
@@ -1085,6 +1092,7 @@ struct addr_ctx {
u8 intlv_num_dies;
u8 intlv_num_sockets;
u8 cs_id;
+ u8 node_id_shift;
int (*dehash_addr)(struct addr_ctx *ctx);
void (*make_space_for_cs_id)(struct addr_ctx *ctx);
void (*insert_cs_id)(struct addr_ctx *ctx);
@@ -1245,6 +1253,168 @@ struct data_fabric_ops df2_ops = {
.get_component_id_mask = get_component_id_mask_df2,
};
+static u64 get_hi_addr_offset_df3(struct addr_ctx *ctx)
+{
+ return (ctx->reg_dram_offset & GENMASK_ULL(31, 12)) << 16;
+}
+
+static void make_space_for_cs_id_cod_hash(struct addr_ctx *ctx)
+{
+ u8 num_intlv_bits = ctx->intlv_num_chan;
+
+ num_intlv_bits += ctx->intlv_num_sockets;
+ expand_bits(ctx->intlv_addr_bit, 1, &ctx->ret_addr);
+ if (num_intlv_bits > 1)
+ expand_bits(12, num_intlv_bits - 1, &ctx->ret_addr);
+}
+
+static void insert_cs_id_cod_hash(struct addr_ctx *ctx)
+{
+ ctx->ret_addr |= ((ctx->cs_id & 0x1) << ctx->intlv_addr_bit);
+ ctx->ret_addr |= ((ctx->cs_id & 0xE) << 11);
+}
+
+static int dehash_addr_df3(struct addr_ctx *ctx)
+{
+ u8 hashed_bit, intlv_ctl_64k, intlv_ctl_2M, intlv_ctl_1G;
+
+ /* Read D18F0x3F8 (DfGlobalCtrl). */
+ if (df_indirect_read_broadcast(0, 0, 0x3F8, &ctx->tmp))
+ return -EINVAL;
+
+ intlv_ctl_64k = !!((ctx->tmp >> 20) & 0x1);
+ intlv_ctl_2M = !!((ctx->tmp >> 21) & 0x1);
+ intlv_ctl_1G = !!((ctx->tmp >> 22) & 0x1);
+
+ hashed_bit = (ctx->ret_addr >> 14) ^
+ ((ctx->ret_addr >> 18) & intlv_ctl_64k) ^
+ ((ctx->ret_addr >> 23) & intlv_ctl_2M) ^
+ ((ctx->ret_addr >> 32) & intlv_ctl_1G) ^
+ (ctx->ret_addr >> ctx->intlv_addr_bit);
+
+ hashed_bit &= BIT(0);
+
+ if (hashed_bit != ((ctx->ret_addr >> ctx->intlv_addr_bit) & BIT(0)))
+ ctx->ret_addr ^= BIT(ctx->intlv_addr_bit);
+
+ if (ctx->intlv_mode != HASH_COD2_4CH &&
+ ctx->intlv_mode != HASH_COD1_8CH)
+ return 0;
+
+ hashed_bit = (ctx->ret_addr >> 12) ^
+ ((ctx->ret_addr >> 16) & intlv_ctl_64k) ^
+ ((ctx->ret_addr >> 21) & intlv_ctl_2M) ^
+ ((ctx->ret_addr >> 30) & intlv_ctl_1G);
+
+ hashed_bit &= BIT(0);
+
+ if (hashed_bit != ((ctx->ret_addr >> 12) & BIT(0)))
+ ctx->ret_addr ^= BIT(12);
+
+ if (ctx->intlv_mode != HASH_COD1_8CH)
+ return 0;
+
+ hashed_bit = (ctx->ret_addr >> 13) ^
+ ((ctx->ret_addr >> 17) & intlv_ctl_64k) ^
+ ((ctx->ret_addr >> 22) & intlv_ctl_2M) ^
+ ((ctx->ret_addr >> 31) & intlv_ctl_1G);
+
+ hashed_bit &= BIT(0);
+
+ if (hashed_bit != ((ctx->ret_addr >> 13) & BIT(0)))
+ ctx->ret_addr ^= BIT(13);
+
+ return 0;
+}
+
+static int get_intlv_mode_df3(struct addr_ctx *ctx)
+{
+ ctx->intlv_mode = (ctx->reg_base_addr >> 2) & 0xF;
+
+ if (ctx->intlv_mode == HASH_COD4_2CH ||
+ ctx->intlv_mode == HASH_COD2_4CH ||
+ ctx->intlv_mode == HASH_COD1_8CH) {
+ ctx->make_space_for_cs_id = make_space_for_cs_id_cod_hash;
+ ctx->insert_cs_id = insert_cs_id_cod_hash;
+ ctx->dehash_addr = dehash_addr_df3;
+ } else {
+ ctx->make_space_for_cs_id = make_space_for_cs_id_simple;
+ ctx->insert_cs_id = insert_cs_id_simple;
+ }
+
+ return 0;
+}
+
+static u8 get_intlv_addr_sel_df3(struct addr_ctx *ctx)
+{
+ return (ctx->reg_base_addr >> 9) & 0x7;
+}
+
+static void get_intlv_num_dies_df3(struct addr_ctx *ctx)
+{
+ ctx->intlv_num_dies = (ctx->reg_base_addr >> 6) & 0x3;
+}
+
+static void get_intlv_num_sockets_df3(struct addr_ctx *ctx)
+{
+ ctx->intlv_num_sockets = (ctx->reg_base_addr >> 8) & 0x1;
+}
+
+static u8 get_die_id_shift_df3(struct addr_ctx *ctx)
+{
+ return ctx->node_id_shift;
+}
+
+static u8 get_socket_id_shift_df3(struct addr_ctx *ctx)
+{
+ return ((ctx->reg_fab_id_mask1 >> 8) & 0x3) + ctx->node_id_shift;
+}
+
+static int get_masks_df3(struct addr_ctx *ctx)
+{
+ /* Read D18F1x208 (SystemFabricIdMask). */
+ if (df_indirect_read_broadcast(ctx->nid, 1, 0x208, &ctx->reg_fab_id_mask0))
+ return -EINVAL;
+
+ /* Read D18F1x20C (SystemFabricIdMask1) */
+ if (df_indirect_read_broadcast(0, 1, 0x20C, &ctx->reg_fab_id_mask1))
+ return -EINVAL;
+
+ ctx->node_id_shift = ctx->reg_fab_id_mask1 & 0xF;
+
+ ctx->die_id_mask = (ctx->reg_fab_id_mask1 >> 16) & 0x7;
+ ctx->die_id_mask <<= ctx->node_id_shift;
+
+ ctx->socket_id_mask = (ctx->reg_fab_id_mask1 >> 24) & 0x7;
+ ctx->socket_id_mask <<= ctx->node_id_shift;
+
+ return 0;
+}
+
+static u16 get_dst_fabric_id_df3(struct addr_ctx *ctx)
+{
+ return ctx->reg_limit_addr & 0x3FF;
+}
+
+static u16 get_component_id_mask_df3(struct addr_ctx *ctx)
+{
+ return ctx->reg_fab_id_mask0 & 0x3FF;
+}
+
+struct data_fabric_ops df3_ops = {
+ .get_hi_addr_offset = get_hi_addr_offset_df3,
+ .get_intlv_mode = get_intlv_mode_df3,
+ .get_intlv_addr_sel = get_intlv_addr_sel_df3,
+ .get_intlv_num_dies = get_intlv_num_dies_df3,
+ .get_intlv_num_sockets = get_intlv_num_sockets_df3,
+ .get_cs_fabric_id = get_cs_fabric_id_df2,
+ .get_masks = get_masks_df3,
+ .get_die_id_shift = get_die_id_shift_df3,
+ .get_socket_id_shift = get_socket_id_shift_df3,
+ .get_dst_fabric_id = get_dst_fabric_id_df3,
+ .get_component_id_mask = get_component_id_mask_df3,
+};
+
struct data_fabric_ops *df_ops;
static int get_dram_offset_reg(struct addr_ctx *ctx)
@@ -1303,8 +1473,8 @@ static int get_intlv_addr_bit(struct addr_ctx *ctx)
{
u8 intlv_addr_sel = df_ops->get_intlv_addr_sel(ctx);
- /* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
- if (intlv_addr_sel > 3) {
+ /* {0, 1, 2, 3, 4} map to address bits {8, 9, 10, 11, 12} respectively */
+ if (intlv_addr_sel > 4) {
pr_debug("Invalid interleave address select %d.\n", intlv_addr_sel);
return -EINVAL;
}
@@ -1322,9 +1492,18 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
ctx->intlv_num_chan = 0;
break;
case NOHASH_2CH:
+ case HASH_COD4_2CH:
case DF2_HASH_2CH:
ctx->intlv_num_chan = 1;
break;
+ case NOHASH_4CH:
+ case HASH_COD2_4CH:
+ ctx->intlv_num_chan = 2;
+ break;
+ case NOHASH_8CH:
+ case HASH_COD1_8CH:
+ ctx->intlv_num_chan = 3;
+ break;
default:
/* Valid interleaving modes where checked earlier. */
break;
@@ -4197,14 +4376,17 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
} else if (pvt->model >= 0x30 && pvt->model <= 0x3f) {
fam_type = &family_types[F17_M30H_CPUS];
pvt->ops = &family_types[F17_M30H_CPUS].ops;
+ df_ops = &df3_ops;
break;
} else if (pvt->model >= 0x60 && pvt->model <= 0x6f) {
fam_type = &family_types[F17_M60H_CPUS];
pvt->ops = &family_types[F17_M60H_CPUS].ops;
+ df_ops = &df3_ops;
break;
} else if (pvt->model >= 0x70 && pvt->model <= 0x7f) {
fam_type = &family_types[F17_M70H_CPUS];
pvt->ops = &family_types[F17_M70H_CPUS].ops;
+ df_ops = &df3_ops;
break;
}
fallthrough;
@@ -4226,6 +4408,7 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
fam_type = &family_types[F17_M70H_CPUS];
pvt->ops = &family_types[F17_M70H_CPUS].ops;
fam_type->ctl_name = "F19h_M20h";
+ df_ops = &df3_ops;
break;
} else if (pvt->model >= 0x50 && pvt->model <= 0x5f) {
fam_type = &family_types[F19_M50H_CPUS];
@@ -4241,6 +4424,7 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
fam_type = &family_types[F19_CPUS];
pvt->ops = &family_types[F19_CPUS].ops;
family_types[F19_CPUS].ctl_name = "F19h";
+ df_ops = &df3_ops;
break;
default:
--
2.25.1
The reference code for DF2 hashing was changed to XOR the interleave
address bit rather than the CS ID. Match that here.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* No change.
v2->v3:
* Was patch 28 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
drivers/edac/amd64_edac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index e293aefd486e..601ececf5106 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1142,7 +1142,7 @@ static int dehash_addr_df2(struct addr_ctx *ctx)
(ctx->ret_addr >> 18) ^
(ctx->ret_addr >> 21) ^
(ctx->ret_addr >> 30) ^
- ctx->cs_id;
+ (ctx->ret_addr >> ctx->intlv_addr_bit);
hashed_bit &= BIT(0);
--
2.25.1
Redo the current CS ID calculations to match the reference code. Helper
functions are introduced that will be expanded for future DF versions.
Use u16 type for dst_fabric_id and component_id_mask values to
accommodate larger bitfields in future DF versions.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Remove leading whitespace in function pointers.
v2->v3:
* Was patch 27 in v2.
v1->v2:
* Moved from arch/x86 to EDAC.
* Added functions to data_fabric_ops.
drivers/edac/amd64_edac.c | 52 ++++++++++++++++++++++++++-------------
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index ac7919010063..e293aefd486e 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1092,6 +1092,8 @@ struct addr_ctx {
struct data_fabric_ops {
u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
+ u16 (*get_dst_fabric_id)(struct addr_ctx *ctx);
+ u16 (*get_component_id_mask)(struct addr_ctx *ctx);
u8 (*get_die_id_shift)(struct addr_ctx *ctx);
u8 (*get_socket_id_shift)(struct addr_ctx *ctx);
int (*get_intlv_mode)(struct addr_ctx *ctx);
@@ -1213,6 +1215,16 @@ static u8 get_socket_id_shift_df2(struct addr_ctx *ctx)
return (ctx->reg_fab_id_mask0 >> 28) & 0xF;
}
+static u16 get_dst_fabric_id_df2(struct addr_ctx *ctx)
+{
+ return ctx->reg_limit_addr & 0xFF;
+}
+
+static u16 get_component_id_mask_df2(struct addr_ctx *ctx)
+{
+ return (~(ctx->socket_id_mask | ctx->die_id_mask)) & 0xFF;
+}
+
struct data_fabric_ops df2_ops = {
.get_hi_addr_offset = get_hi_addr_offset_df2,
.get_intlv_mode = get_intlv_mode_df2,
@@ -1222,6 +1234,8 @@ struct data_fabric_ops df2_ops = {
.get_masks = get_masks_df2,
.get_die_id_shift = get_die_id_shift_df2,
.get_socket_id_shift = get_socket_id_shift_df2,
+ .get_dst_fabric_id = get_dst_fabric_id_df2,
+ .get_component_id_mask = get_component_id_mask_df2,
};
struct data_fabric_ops *df_ops;
@@ -1310,38 +1324,42 @@ static void get_intlv_num_chan(struct addr_ctx *ctx)
}
}
-static int calculate_cs_id(struct addr_ctx *ctx)
+static u8 calc_level_bits(u8 id, u8 level_mask, u8 shift, u8 mask, u8 num_bits)
{
- u8 die_id_bit = 0, sock_id_bit, cs_mask = 0;
+ return (((id & level_mask) >> shift) & mask) << num_bits;
+}
- /* If interleaved over more than 1 channel: */
- if (ctx->intlv_num_chan) {
- die_id_bit = ctx->intlv_num_chan;
- cs_mask = (1 << die_id_bit) - 1;
- ctx->cs_id = ctx->cs_fabric_id & cs_mask;
- }
+static int calculate_cs_id(struct addr_ctx *ctx)
+{
+ u16 dst_fabric_id = df_ops->get_dst_fabric_id(ctx);
+ u16 mask, num_intlv_bits = ctx->intlv_num_chan;
- /* Return early if no die interleaving and no socket interleaving. */
- if (!(ctx->intlv_num_dies || ctx->intlv_num_sockets))
- return 0;
+ mask = df_ops->get_component_id_mask(ctx);
+ ctx->cs_id = (ctx->cs_fabric_id & mask) - (dst_fabric_id & mask);
- sock_id_bit = die_id_bit;
+ mask = (1 << num_intlv_bits) - 1;
+ ctx->cs_id &= mask;
/* If interleaved over more than 1 die: */
if (ctx->intlv_num_dies) {
u8 die_id_shift = df_ops->get_die_id_shift(ctx);
- sock_id_bit = die_id_bit + ctx->intlv_num_dies;
- ctx->cs_id |= ((ctx->cs_fabric_id & ctx->die_id_mask)
- >> die_id_shift) << die_id_bit;
+ mask = (1 << ctx->intlv_num_dies) - 1;
+
+ ctx->cs_id |= calc_level_bits(ctx->cs_fabric_id, ctx->die_id_mask,
+ die_id_shift, mask, num_intlv_bits);
+
+ num_intlv_bits += ctx->intlv_num_dies;
}
/* If interleaved over more than 1 socket: */
if (ctx->intlv_num_sockets) {
u8 socket_id_shift = df_ops->get_socket_id_shift(ctx);
- ctx->cs_id |= ((ctx->cs_fabric_id & ctx->socket_id_mask)
- >> socket_id_shift) << sock_id_bit;
+ mask = (1 << ctx->intlv_num_sockets) - 1;
+
+ ctx->cs_id |= calc_level_bits(ctx->cs_fabric_id, ctx->socket_id_mask,
+ socket_id_shift, mask, num_intlv_bits);
}
return 0;
--
2.25.1
...this will be expanded for future Data Fabric versions.
Signed-off-by: Yazen Ghannam <[email protected]>
---
Link:
https://lore.kernel.org/r/[email protected]
v3->v4:
* Remove leading whitespace in function pointer.
v2->v3:
* Was patch 29 in v2.
v1->v2:
* New in v2.
drivers/edac/amd64_edac.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 601ececf5106..34405c8940fb 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1096,6 +1096,7 @@ struct data_fabric_ops {
u16 (*get_component_id_mask)(struct addr_ctx *ctx);
u8 (*get_die_id_shift)(struct addr_ctx *ctx);
u8 (*get_socket_id_shift)(struct addr_ctx *ctx);
+ u8 (*get_intlv_addr_sel)(struct addr_ctx *ctx);
int (*get_intlv_mode)(struct addr_ctx *ctx);
int (*get_cs_fabric_id)(struct addr_ctx *ctx);
int (*get_masks)(struct addr_ctx *ctx);
@@ -1172,6 +1173,11 @@ static int get_intlv_mode_df2(struct addr_ctx *ctx)
return 0;
}
+static u8 get_intlv_addr_sel_df2(struct addr_ctx *ctx)
+{
+ return (ctx->reg_base_addr >> 8) & 0x7;
+}
+
static void get_intlv_num_dies_df2(struct addr_ctx *ctx)
{
ctx->intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3;
@@ -1228,6 +1234,7 @@ static u16 get_component_id_mask_df2(struct addr_ctx *ctx)
struct data_fabric_ops df2_ops = {
.get_hi_addr_offset = get_hi_addr_offset_df2,
.get_intlv_mode = get_intlv_mode_df2,
+ .get_intlv_addr_sel = get_intlv_addr_sel_df2,
.get_intlv_num_dies = get_intlv_num_dies_df2,
.get_intlv_num_sockets = get_intlv_num_sockets_df2,
.get_cs_fabric_id = get_cs_fabric_id_df2,
@@ -1294,7 +1301,7 @@ static int get_dram_addr_map(struct addr_ctx *ctx)
static int get_intlv_addr_bit(struct addr_ctx *ctx)
{
- u8 intlv_addr_sel = (ctx->reg_base_addr >> 8) & 0x7;
+ u8 intlv_addr_sel = df_ops->get_intlv_addr_sel(ctx);
/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
if (intlv_addr_sel > 3) {
--
2.25.1
On Thu, Jan 27, 2022 at 08:40:58PM +0000, Yazen Ghannam wrote:
> Move the dehashing code into a separate helper function. Define a
> DF2-specific function for the current code. Specific helper functions
> will be added for future DF versions.
>
> The dehashing function used is based on the interleaving mode rather
> than the Data Fabric version. So save the function pointer in the
> context struct. The use of "df2" in the name of the current function is
> only because the interleaving mode using it only appears on Data Fabric
> 2 systems.
>
> Signed-off-by: Yazen Ghannam <[email protected]>
> ---
> Link:
> https://lore.kernel.org/r/[email protected]
>
> v3->v4:
> * Include pr_debug() on failure.
>
> v2->v3:
> * Was patch 12 in v2.
>
> v1->v2:
> * Moved from arch/x86 to EDAC.
> * Add new function pointer in ctx struct.
>
> drivers/edac/amd64_edac.c | 36 +++++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 350204eadb27..da2d0d9ce406 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1077,7 +1077,7 @@ struct addr_ctx {
> u8 map_num;
> u8 intlv_addr_bit;
> u8 cs_id;
> - bool hash_enabled;
> + int (*dehash_addr)(struct addr_ctx *ctx);
A function pointer in a context struct?!
> @@ -1357,18 +1372,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
> goto out_err;
> }
>
> - if (ctx.hash_enabled) {
> - /* Save some parentheses and grab ls-bit at the end. */
> - hashed_bit = (ctx.ret_addr >> 12) ^
> - (ctx.ret_addr >> 18) ^
> - (ctx.ret_addr >> 21) ^
> - (ctx.ret_addr >> 30) ^
> - ctx.cs_id;
> -
> - hashed_bit &= BIT(0);
> -
> - if (hashed_bit != ((ctx.ret_addr >> ctx.intlv_addr_bit) & BIT(0)))
> - ctx.ret_addr ^= BIT(ctx.intlv_addr_bit);
> + if (ctx.dehash_addr && ctx.dehash_addr(&ctx)) {
So you can just as well do:
if (ctx->intlv_mode == 8)
dehash_addr();
And dehash_addr() can inside determine whether df2 or df3.
Btw, that 8 looks like magic. It should be a #define.
What you have now looks a bit weird with those function pointers lumped
together with those other members of addr_ctx. Dunno, maybe it'll make
more sense when I read the rest first...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 27, 2022 at 08:41:09PM +0000, Yazen Ghannam wrote:
> @@ -1326,20 +1331,7 @@ static int denormalize_addr(struct addr_ctx *ctx)
> return -EINVAL;
> }
>
> - if (num_intlv_bits > 0) {
> - u64 temp_addr_i;
> -
> - /*
> - * The pre-interleaved address consists of XXXXXXIIIYYYYY
> - * where III is the ID for this CS, and XXXXXXYYYYY are the
> - * address bits from the post-interleaved address.
> - * "num_intlv_bits" has been calculated to tell us how many "I"
> - * bits there are. "intlv_addr_bit" tells us how many "Y" bits
> - * there are (where "I" starts).
> - */
That comment looks useful, why remove it?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 27, 2022 at 08:40:59PM +0000, Yazen Ghannam wrote:
> Move the DRAM limit check into a separate helper function.
You're still writing the "what" in commit messages - pls make a note to
write about "why" you're doing a change instead.
Because I don't see why you're doing this. Because
umc_normaddr_to_sysaddr() is supposed to call helper functions only?
Now if you had the "why" I wouldn't be wondering...
:)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 27, 2022 at 08:41:03PM +0000, Yazen Ghannam wrote:
> Move number of interleaved channel calculation to a separate helper
> function. Drop unused cases.
... or by looking at that change, I think you want to have each separate
step in the address massaging be a separate function. Which, if so,
makes sense but you have to say it...
...
> +static void get_intlv_num_chan(struct addr_ctx *ctx)
> +{
> + /* Save the log2(# of channels). */
> + switch (ctx->intlv_mode) {
> + case NONE:
> + ctx->intlv_num_chan = 0;
> + break;
> + case NOHASH_2CH:
> + case DF2_HASH_2CH:
> + ctx->intlv_num_chan = 1;
> + break;
> + default:
> + /* Valid interleaving modes where checked earlier. */
"were"
> + break;
> + }
> +}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 27, 2022 at 08:41:07PM +0000, Yazen Ghannam wrote:
> +static void expand_bits(u8 start_bit, u8 num_bits, u64 *value)
> +{
> + u64 temp1, temp2;
> +
> + if (start_bit == 0) {
As always
if (!<variable, etc>)
for 0/NULL tests.
> + *value <<= num_bits;
> + return;
> + }
> +
> + temp1 = *value & GENMASK_ULL(start_bit - 1, 0);
> + temp2 = (*value & GENMASK_ULL(63, start_bit)) << num_bits;
> + *value = temp1 | temp2;
> +}
> +
> +static void make_space_for_cs_id_simple(struct addr_ctx *ctx)
> +{
> + u8 num_intlv_bits = ctx->intlv_num_chan;
> +
> + num_intlv_bits += ctx->intlv_num_dies;
> + num_intlv_bits += ctx->intlv_num_sockets;
> + expand_bits(ctx->intlv_addr_bit, num_intlv_bits, &ctx->ret_addr);
> +}
void functions but they return values through their pointer arguments?
I'm sure you can design those better.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 27, 2022 at 08:41:12PM +0000, Yazen Ghannam wrote:
> Redo the current CS ID calculations to match the reference code. Helper
> functions are introduced that will be expanded for future DF versions.
"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."
Also, do not talk about what your patch does - that should hopefully be
visible in the diff itself. Rather, talk about *why* you're doing what
you're doing.
Just a reminder.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 27, 2022 at 08:41:01PM +0000, Yazen Ghannam wrote:
> Move code to find the interleave address bit into a separate helper
> function.
Same question: what's the point of this change?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 14, 2022 at 01:50:54PM +0100, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 08:41:07PM +0000, Yazen Ghannam wrote:
> > +static void expand_bits(u8 start_bit, u8 num_bits, u64 *value)
> > +{
> > + u64 temp1, temp2;
> > +
> > + if (start_bit == 0) {
>
> As always
>
> if (!<variable, etc>)
>
> for 0/NULL tests.
>
Will fix.
> > + *value <<= num_bits;
> > + return;
> > + }
> > +
> > + temp1 = *value & GENMASK_ULL(start_bit - 1, 0);
> > + temp2 = (*value & GENMASK_ULL(63, start_bit)) << num_bits;
> > + *value = temp1 | temp2;
> > +}
> > +
> > +static void make_space_for_cs_id_simple(struct addr_ctx *ctx)
> > +{
> > + u8 num_intlv_bits = ctx->intlv_num_chan;
> > +
> > + num_intlv_bits += ctx->intlv_num_dies;
> > + num_intlv_bits += ctx->intlv_num_sockets;
> > + expand_bits(ctx->intlv_addr_bit, num_intlv_bits, &ctx->ret_addr);
> > +}
>
> void functions but they return values through their pointer arguments?
> I'm sure you can design those better.
>
I guess this could be "addr = func(x, y, addr)", but why not just operate on
the value directly? There isn't an error condition here that's obvious to me.
Thanks,
Yazen
On Fri, Feb 11, 2022 at 11:47:31PM +0100, Borislav Petkov wrote:
...
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -1077,7 +1077,7 @@ struct addr_ctx {
> > u8 map_num;
> > u8 intlv_addr_bit;
> > u8 cs_id;
> > - bool hash_enabled;
> > + int (*dehash_addr)(struct addr_ctx *ctx);
>
> A function pointer in a context struct?!
>
> > @@ -1357,18 +1372,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
> > goto out_err;
> > }
> >
> > - if (ctx.hash_enabled) {
> > - /* Save some parentheses and grab ls-bit at the end. */
> > - hashed_bit = (ctx.ret_addr >> 12) ^
> > - (ctx.ret_addr >> 18) ^
> > - (ctx.ret_addr >> 21) ^
> > - (ctx.ret_addr >> 30) ^
> > - ctx.cs_id;
> > -
> > - hashed_bit &= BIT(0);
> > -
> > - if (hashed_bit != ((ctx.ret_addr >> ctx.intlv_addr_bit) & BIT(0)))
> > - ctx.ret_addr ^= BIT(ctx.intlv_addr_bit);
> > + if (ctx.dehash_addr && ctx.dehash_addr(&ctx)) {
>
> So you can just as well do:
>
> if (ctx->intlv_mode == 8)
> dehash_addr();
>
> And dehash_addr() can inside determine whether df2 or df3.
>
> Btw, that 8 looks like magic. It should be a #define.
>
> What you have now looks a bit weird with those function pointers lumped
> together with those other members of addr_ctx. Dunno, maybe it'll make
> more sense when I read the rest first...
>
Yeah, I think I got carried away with the function pointers. :P
The functions in the context struct are set based on interleaving mode rather
than Data Fabric version. Which is why it doesn't work to include them in the
"df_ops".
But I can do something like you suggest. There will be an unconditional call
to dehash_addr(). Helper functions will be called for specific interleave
modes. Otherwise, it'll return 0 if hashing isn't used.
Thanks,
Yazen
On Mon, Feb 14, 2022 at 01:10:49PM +0100, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 08:41:01PM +0000, Yazen Ghannam wrote:
> > Move code to find the interleave address bit into a separate helper
> > function.
>
> Same question: what's the point of this change?
>
Yes, I'll update this.
Similar goal as in other places. When the function seems sufficiently long
(subjective I know), break it up into helper functions.
I've been trying to decide based on logical steps. Do you have any general
recommendations or rule-of-thumb?
Thanks,
Yazen
On Mon, Feb 14, 2022 at 02:42:36PM +0100, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 08:41:12PM +0000, Yazen Ghannam wrote:
> > Redo the current CS ID calculations to match the reference code. Helper
> > functions are introduced that will be expanded for future DF versions.
>
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."
>
> Also, do not talk about what your patch does - that should hopefully be
> visible in the diff itself. Rather, talk about *why* you're doing what
> you're doing.
>
> Just a reminder.
>
Yep, thank you. I'll update this.
Thanks,
Yazen
On Mon, Feb 14, 2022 at 01:20:44PM +0100, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 08:41:03PM +0000, Yazen Ghannam wrote:
> > Move number of interleaved channel calculation to a separate helper
> > function. Drop unused cases.
>
> ... or by looking at that change, I think you want to have each separate
> step in the address massaging be a separate function. Which, if so,
> makes sense but you have to say it...
>
Yep, will do.
> ...
>
> > +static void get_intlv_num_chan(struct addr_ctx *ctx)
> > +{
> > + /* Save the log2(# of channels). */
> > + switch (ctx->intlv_mode) {
> > + case NONE:
> > + ctx->intlv_num_chan = 0;
> > + break;
> > + case NOHASH_2CH:
> > + case DF2_HASH_2CH:
> > + ctx->intlv_num_chan = 1;
> > + break;
> > + default:
> > + /* Valid interleaving modes where checked earlier. */
>
> "were"
Will fix.
Thanks,
Yazen
On Mon, Feb 14, 2022 at 02:09:07PM +0100, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 08:41:09PM +0000, Yazen Ghannam wrote:
> > @@ -1326,20 +1331,7 @@ static int denormalize_addr(struct addr_ctx *ctx)
> > return -EINVAL;
> > }
> >
> > - if (num_intlv_bits > 0) {
> > - u64 temp_addr_i;
> > -
> > - /*
> > - * The pre-interleaved address consists of XXXXXXIIIYYYYY
> > - * where III is the ID for this CS, and XXXXXXYYYYY are the
> > - * address bits from the post-interleaved address.
> > - * "num_intlv_bits" has been calculated to tell us how many "I"
> > - * bits there are. "intlv_addr_bit" tells us how many "Y" bits
> > - * there are (where "I" starts).
> > - */
>
> That comment looks useful, why remove it?
>
No firm reason. I just got the feeling it wasn't very useful. But I'll keep it
in case others find it helpful.
Thanks,
Yazen
On Mon, Feb 14, 2022 at 11:06:07AM +0100, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 08:40:59PM +0000, Yazen Ghannam wrote:
> > Move the DRAM limit check into a separate helper function.
>
> You're still writing the "what" in commit messages - pls make a note to
> write about "why" you're doing a change instead.
>
> Because I don't see why you're doing this. Because
> umc_normaddr_to_sysaddr() is supposed to call helper functions only?
>
> Now if you had the "why" I wouldn't be wondering...
>
> :)
>
Yes, you're right. I'll update this.
Yes, the goal is to break up the translation procedure into discrete
high-level steps.
Thanks,
Yazen
On Wed, Mar 09, 2022 at 09:50:06PM +0000, Yazen Ghannam wrote:
> But I can do something like you suggest. There will be an unconditional call
> to dehash_addr(). Helper functions will be called for specific interleave
> modes. Otherwise, it'll return 0 if hashing isn't used.
That sounds like proper design to me. A single function which dehashes
an address and that's the only thing it does but it takes care of all
possible cases.
Yap. :-)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Mar 09, 2022 at 10:12:00PM +0000, Yazen Ghannam wrote:
> Similar goal as in other places. When the function seems sufficiently long
> (subjective I know), break it up into helper functions.
>
> I've been trying to decide based on logical steps. Do you have any general
> recommendations or rule-of-thumb?
Documentation/process/coding-style.rst, 6) Functions has some good ideas
about it. To me, a function should do one thing and one thing only but
yes, the decision is subjective.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Mar 09, 2022 at 10:25:22PM +0000, Yazen Ghannam wrote:
> I guess this could be "addr = func(x, y, addr)", but why not just operate on
> the value directly? There isn't an error condition here that's obvious to me.
Because when you look at the callsite:
ctx->make_space_for_cs_id(ctx);
and compare it with the version I'm suggesting:
ctx->ret_addr = ctx->add_coherent_slave_id_slice(ctx);
my version says exactly what it does: it adds the bit slice of CS ID to
ret_addr. You don't even have to look at the function body in order to
know what's going on.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette