| Message ID | 20220127204115.384161-17-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD MCA Address Translation Updates | expand |
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.
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 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.
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;
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 <yazen.ghannam@amd.com> --- Link: https://lore.kernel.org/r/20211028175728.121452-22-yazen.ghannam@amd.com 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(-)