Messages in this thread | | | Date | Thu, 8 Jul 2021 15:35:17 -0400 | From | Yazen Ghannam <> | Subject | Re: [PATCH v2 03/31] EDAC/amd64: Don't use naked values for DF registers |
| |
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
| |