Re: [PATCH v4 07/24] EDAC/amd64: Define function to dehash address

[Yazen Ghannam <yazen.ghannam@amd.com>]

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