| Message ID | 20220127204115.384161-2-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD MCA Address Translation Updates | expand |
On Thu, Jan 27, 2022 at 08:40:52PM +0000, Yazen Ghannam wrote: > Define a stub to hold operations for different Data Fabric versions. > This will be filled in following patches. > > Set the operations at init-time as appropriate for each model/family > group. > > Also, start a glossary of acronyms used in the translation code. > > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> > --- > Link: > https://lore.kernel.org/r/20211028175728.121452-6-yazen.ghannam@amd.com > > v3->v4: > * Started glossary. > * Included pr_debug() for failing case. > > v2->v3: > * Was patch 6 in v2. > * "df_ops" is set at init time. > > v1->v2: > * New in v2. > > drivers/edac/amd64_edac.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index fba609ada0e6..639dfbea3348 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -988,6 +988,12 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr) > return csrow; > } > > +/* > + * Glossary of acronyms used in address translation for Zen-based systems > + * > + * DF = Data Fabric Yeah, "DF: Data Fabric" is probably easier to parse, without that weird spacing and equals sign. > + */ > + > /* Protect the PCI config register pairs used for DF indirect access. */ > static DEFINE_MUTEX(df_indirect_mutex); > > @@ -1058,6 +1064,14 @@ struct addr_ctx { > u8 inst_id; > }; > > +struct data_fabric_ops { > +}; I know that this is not the only example but we have struct definitions interspersed with functions in the .c file while former should be all in the header. It is a lot cleaner to have definitions and inline functions in the header and the actual functionality in the C file but I leave it up to you to decide what you prefer. > + > +struct data_fabric_ops df2_ops = { > +}; > + > +struct data_fabric_ops *df_ops; > + > 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; > @@ -1072,6 +1086,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr > > struct addr_ctx ctx; > > + if (!df_ops) { > + pr_debug("Data Fabric Operations not set"); That probably wants to be a WARN_ON_ONCE() so that it is loud and prominent when it happens...
On Fri, Feb 11, 2022 at 08:10:43PM +0100, Borislav Petkov wrote: > > +/* > > + * Glossary of acronyms used in address translation for Zen-based systems > > + * > > + * DF = Data Fabric > > Yeah, "DF: Data Fabric" is probably easier to parse, without that weird > spacing and equals sign. > Okay, will change. > > + */ > > + > > /* Protect the PCI config register pairs used for DF indirect access. */ > > static DEFINE_MUTEX(df_indirect_mutex); > > > > @@ -1058,6 +1064,14 @@ struct addr_ctx { > > u8 inst_id; > > }; > > > > +struct data_fabric_ops { > > +}; > > I know that this is not the only example but we have struct definitions > interspersed with functions in the .c file while former should be all in > the header. It is a lot cleaner to have definitions and inline functions > in the header and the actual functionality in the C file but I leave it > up to you to decide what you prefer. > Makes sense. I'll make the change. > > + > > +struct data_fabric_ops df2_ops = { > > +}; > > + > > +struct data_fabric_ops *df_ops; > > + > > 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; > > @@ -1072,6 +1086,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr > > > > struct addr_ctx ctx; > > > > + if (!df_ops) { > > + pr_debug("Data Fabric Operations not set"); > > That probably wants to be a WARN_ON_ONCE() so that it is loud and > prominent when it happens... > I'd like to keep this a debug message because address translation updates will almost certainly lag general EDAC enablement. For example, Family 19h Model 10h is enabled now, but df_ops won't be set. The translation code will return early here, and the EDAC message will say that the tranlsation failed. But this isn't a bug that needs a WARNing. Rather it's just that the feature isn't enabled yet for new systems. Thanks, Yazen
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index fba609ada0e6..639dfbea3348 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -988,6 +988,12 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr) return csrow; } +/* + * Glossary of acronyms used in address translation for Zen-based systems + * + * DF = Data Fabric + */ + /* Protect the PCI config register pairs used for DF indirect access. */ static DEFINE_MUTEX(df_indirect_mutex); @@ -1058,6 +1064,14 @@ struct addr_ctx { u8 inst_id; }; +struct data_fabric_ops { +}; + +struct data_fabric_ops df2_ops = { +}; + +struct data_fabric_ops *df_ops; + 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; @@ -1072,6 +1086,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr struct addr_ctx ctx; + if (!df_ops) { + pr_debug("Data Fabric Operations not set"); + return -EINVAL; + } + memset(&ctx, 0, sizeof(ctx)); /* Start from the normalized address */ @@ -3958,6 +3977,7 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt) if (pvt->model >= 0x10 && pvt->model <= 0x2f) { fam_type = &family_types[F17_M10H_CPUS]; pvt->ops = &family_types[F17_M10H_CPUS].ops; + df_ops = &df2_ops; break; } else if (pvt->model >= 0x30 && pvt->model <= 0x3f) { fam_type = &family_types[F17_M30H_CPUS]; @@ -3976,6 +3996,7 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt) case 0x18: fam_type = &family_types[F17_CPUS]; pvt->ops = &family_types[F17_CPUS].ops; + df_ops = &df2_ops; if (pvt->fam == 0x18) family_types[F17_CPUS].ctl_name = "F18h";
Define a stub to hold operations for different Data Fabric versions. This will be filled in following patches. Set the operations at init-time as appropriate for each model/family group. Also, start a glossary of acronyms used in the translation code. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- Link: https://lore.kernel.org/r/20211028175728.121452-6-yazen.ghannam@amd.com v3->v4: * Started glossary. * Included pr_debug() for failing case. v2->v3: * Was patch 6 in v2. * "df_ops" is set at init time. v1->v2: * New in v2. drivers/edac/amd64_edac.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)