Messages in this thread Patch in this message | | | Date | Fri, 11 Feb 2022 23:47:31 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v4 07/24] EDAC/amd64: Define function to dehash address |
| |
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...
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |