Messages in this thread | | | Date | Wed, 9 Mar 2022 21:50:06 +0000 | From | Yazen Ghannam <> | Subject | Re: [PATCH v4 07/24] EDAC/amd64: Define function to dehash address |
| |
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
| |