| Message ID | 20210623192002.3671647-4-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD MCA Address Translation Updates | expand |
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. Thx.
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
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index e9342d7d693f..b94067e3952b 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -996,6 +996,11 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr) /* Protect the PCI config register pairs used for DF indirect access. */ static DEFINE_MUTEX(df_indirect_mutex); +struct df_reg { + u8 func; + u16 offset; +}; + /* * Data Fabric Indirect Access uses FICAA/FICAD. * @@ -1006,7 +1011,7 @@ static DEFINE_MUTEX(df_indirect_mutex); * Fabric Indirect Configuration Access Data (FICAD): There are FICAD LO * and FICAD HI registers but so far we only need the LO register. */ -static int amd_df_indirect_read(u16 node, u8 func, u16 reg, u8 instance_id, u32 *lo) +static int amd_df_indirect_read(u16 node, struct df_reg reg, u8 instance_id, u32 *lo) { struct pci_dev *F4; u32 ficaa; @@ -1020,8 +1025,8 @@ static int amd_df_indirect_read(u16 node, u8 func, u16 reg, u8 instance_id, u32 goto out; ficaa = 1; - ficaa |= reg & 0x3FC; - ficaa |= (func & 0x7) << 11; + ficaa |= reg.offset & 0x3FC; + ficaa |= (reg.func & 0x7) << 11; ficaa |= instance_id << 16; mutex_lock(&df_indirect_mutex); @@ -1043,6 +1048,33 @@ static int amd_df_indirect_read(u16 node, u8 func, u16 reg, u8 instance_id, u32 return err; } +enum df_reg_names { + /* Function 0 */ + FAB_BLK_INST_INFO_3, + DRAM_HOLE_CTL, + DRAM_BASE_ADDR, + DRAM_LIMIT_ADDR, + DRAM_OFFSET, + + /* Function 1 */ + SYS_FAB_ID_MASK, +}; + +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; + if (amd_df_indirect_read(nid, reg, umc, &tmp)) goto out_err; /* Check if address range is valid. */ @@ -1096,8 +1130,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr goto out_err; } - /* Read D18F0x114 (DramLimitAddress). */ - if (amd_df_indirect_read(nid, 0, 0x114 + (8 * base), umc, &tmp)) + reg = df_regs[DRAM_LIMIT_ADDR]; + reg.offset += base * 8; + if (amd_df_indirect_read(nid, reg, umc, &tmp)) goto out_err; intlv_num_sockets = (tmp >> 8) & 0x1; @@ -1153,7 +1188,7 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr * umc/channel# as instance id of the coherent slave * for FICAA. */ - if (amd_df_indirect_read(nid, 0, 0x50, umc, &tmp)) + if (amd_df_indirect_read(nid, df_regs[FAB_BLK_INST_INFO_3], umc, &tmp)) goto out_err; cs_fabric_id = (tmp >> 8) & 0xFF; @@ -1168,9 +1203,8 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr sock_id_bit = die_id_bit; - /* Read D18F1x208 (SystemFabricIdMask). */ if (intlv_num_dies || intlv_num_sockets) - if (amd_df_indirect_read(nid, 1, 0x208, umc, &tmp)) + if (amd_df_indirect_read(nid, df_regs[SYS_FAB_ID_MASK], umc, &tmp)) goto out_err; /* If interleaved over more than 1 die. */ @@ -1209,7 +1243,7 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr /* If legacy MMIO hole enabled */ if (lgcy_mmio_hole_en) { - if (amd_df_indirect_read(nid, 0, 0x104, umc, &tmp)) + if (amd_df_indirect_read(nid, df_regs[DRAM_HOLE_CTL], umc, &tmp)) goto out_err; dram_hole_base = tmp & GENMASK(31, 24);
AMD Data Fabric registers are defined using a combination of PCI function number and offset. Define a struct to hold these values, and update the DF Indirect Access function to accept a struct of this type. Update the address translation code to include a list of the needed DF registers using this new format. Define an enumeration to give the registers more human-readable names. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- Link: https://lkml.kernel.org/r/20210507190140.18854-2-Yazen.Ghannam@amd.com v1->v2: * Moved from arch/x86 to EDAC. drivers/edac/amd64_edac.c | 60 ++++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 13 deletions(-)