Re: [PATCH v2 03/31] EDAC/amd64: Don't use naked values for DF registers

[Yazen Ghannam]

On Fri, Jun 25, 2021 at 05:21:08PM +0200, Borislav Petkov wrote:
> On Wed, Jun 23, 2021 at 07:19:34PM +0000, Yazen Ghannam wrote:
> > +static struct df_reg df_regs[] = {
> > +	/* D18F0x50 (FabricBlockInstanceInformation3_CS) */
> > +	[FAB_BLK_INST_INFO_3]	=	{0, 0x50},
> > +	/* D18F0x104 (DramHoleControl) */
> > +	[DRAM_HOLE_CTL]		=	{0, 0x104},
> > +	/* D18F0x110 (DramBaseAddress) */
> > +	[DRAM_BASE_ADDR]	=	{0, 0x110},
> > +	/* D18F0x114 (DramLimitAddress) */
> > +	[DRAM_LIMIT_ADDR]	=	{0, 0x114},
> > +	/* D18F0x1B4 (DramOffset) */
> > +	[DRAM_OFFSET]		=	{0, 0x1B4},
> > +	/* D18F1x208 (SystemFabricIdMask) */
> > +	[SYS_FAB_ID_MASK]	=	{1, 0x208},
> > +};
> > +
> >  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
> >  {
> >  	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
> > @@ -1059,8 +1091,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
> >  	u8 cs_mask, cs_id = 0;
> >  	bool hash_enabled = false;
> >  
> > -	/* Read D18F0x1B4 (DramOffset), check if base 1 is used. */
> > -	if (amd_df_indirect_read(nid, 0, 0x1B4, umc, &tmp))
> > +	struct df_reg reg;
> > +
> > +	if (amd_df_indirect_read(nid, df_regs[DRAM_OFFSET], umc, &tmp))
> >  		goto out_err;
> >  
> >  	/* Remove HiAddrOffset from normalized address, if enabled: */
> > @@ -1073,8 +1106,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
> >  		}
> >  	}
> >  
> > -	/* Read D18F0x110 (DramBaseAddress). */
> > -	if (amd_df_indirect_read(nid, 0, 0x110 + (8 * base), umc, &tmp))
> > +	reg = df_regs[DRAM_BASE_ADDR];
> > +	reg.offset += base * 8;
> 
> So this looks weird: you have a df_regs[] array of all those different
> DF registers which I'd assume is a read-only thing because, well, those
> func and offset things are immutable, i.e., hw registers offsets etc.
> 
> But then here you go and and modify the offset.
> 
> And that df_regs array is globally visible in the driver and if some
> later functionality decides to use it, it'll see the modified offset.
> 
> IOW, I'd make that array read only (const) and use local vars instead to
> pass down to amd_df_indirect_read().
> 
> And I'm also questioning what the point is for that df_reg thing?
> 
> You have them defined but then you have to change them.
> 
> I.e., you can just as well pass in func and offset separately and be
> done with it.
> 
> But maybe there's something else happening in the patches which comes
> later and which will make me go, ahaa.
>

You're right that the values should be immutable. The changes done here
are only for this pair of base/limit registers. Most of the time we'll
only use 2 pairs (4 registers). But some systems will need to look at 16
pairs, and so this current approach seemed nicer than writing out 32
registers with mostly redundant information.

I was trying to make the code more "self-documenting" and move away from
magic numbers, etc. But it all looks okay to me, so I'm not sure which
way to go (magic numbers + code comments, something else, etc.).

So I'm inclined to stick with passing in the func/offset values and
dropping the df_regs thing.

Thanks,
Yazen