| Message ID | 20220127204115.384161-8-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: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 <yazen.ghannam@amd.com> > --- > Link: > https://lore.kernel.org/r/20211028175728.121452-12-yazen.ghannam@amd.com > > 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...
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 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. :-)
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? */
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 <yazen.ghannam@amd.com> --- Link: https://lore.kernel.org/r/20211028175728.121452-12-yazen.ghannam@amd.com 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(-)