diff mbox series

[v2,07/31] EDAC/amd64: Define functions for DramOffset

Message ID 20210623192002.3671647-8-yazen.ghannam@amd.com (mailing list archive)
State New, archived
Headers show
Series AMD MCA Address Translation Updates | expand

Commit Message

Yazen Ghannam June 23, 2021, 7:19 p.m. UTC 
Add helper functions to read the DramOffset register and to remove the
offset from the calculated address.

The helper functions will be expanded in future DF versions.

Rename the "base" variable to "map_num" to indicate that this is the
address map number. An address map is defined with a base and limit
value. The map_num variable is used to select the proper base and limit
registers to use for the address translation.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20210507190140.18854-4-Yazen.Ghannam@amd.com

v1->v2:
* Moved from arch/x86 to EDAC.
* Add function to data_fabric_ops.

 drivers/edac/amd64_edac.c | 57 +++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 14 deletions(-)

Comments

Borislav Petkov June 30, 2021, 5:27 p.m. UTC | #1 
On Wed, Jun 23, 2021 at 07:19:38PM +0000, Yazen Ghannam wrote:
> Add helper functions to read the DramOffset register and to remove the
> offset from the calculated address.
> 
> The helper functions will be expanded in future DF versions.
> 
> Rename the "base" variable to "map_num" to indicate that this is the
> address map number. An address map is defined with a base and limit
> value. The map_num variable is used to select the proper base and limit
> registers to use for the address translation.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> Link:
> https://lkml.kernel.org/r/20210507190140.18854-4-Yazen.Ghannam@amd.com
> 
> v1->v2:
> * Moved from arch/x86 to EDAC.
> * Add function to data_fabric_ops.
> 
>  drivers/edac/amd64_edac.c | 57 +++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index f769353ef7e4..389f0621e885 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1079,16 +1079,26 @@ static struct df_reg df_regs[] = {
>  	[SYS_FAB_ID_MASK]	=	{1, 0x208},
>  };
>  
> +/* Use "reg_" prefix for raw register values. */
>  struct addr_ctx {
>  	u64 ret_addr;
> +	u32 reg_dram_offset;
>  	u16 nid;
>  	u8 inst_id;
> +	u8 map_num;
>  };
>  
>  struct data_fabric_ops {
> +	u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
>  };
>  
> +static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
> +{
> +	return (ctx->reg_dram_offset & GENMASK_ULL(31, 20)) << 8;
> +}
> +
>  struct data_fabric_ops df2_ops = {

Those "df2" ops are for UMC v2? 

> +	.get_hi_addr_offset		=	&get_hi_addr_offset_df2,
						^

No need for the "&".

Also, if you define a member ->get_hi_addr_offset in df2_ops, then you
need to define that member in *every* df_ops structure you have so that
there's no NULL pointers.

I guess that's coming...
Yazen Ghannam July 8, 2021, 8:08 p.m. UTC | #2 
On Wed, Jun 30, 2021 at 07:27:07PM +0200, Borislav Petkov wrote:
> On Wed, Jun 23, 2021 at 07:19:38PM +0000, Yazen Ghannam wrote:

...

> >  struct data_fabric_ops {
> > +	u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
> >  };
> >  
> > +static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
> > +{
> > +	return (ctx->reg_dram_offset & GENMASK_ULL(31, 20)) << 8;
> > +}
> > +
> >  struct data_fabric_ops df2_ops = {
> 
> Those "df2" ops are for UMC v2? 
>

No, Data Fabric 2 is used in the first Zen products (Ryzen, Naples,
etc.)

> > +	.get_hi_addr_offset		=	&get_hi_addr_offset_df2,
> 						^
> 
> No need for the "&".
>

Okay.

> Also, if you define a member ->get_hi_addr_offset in df2_ops, then you
> need to define that member in *every* df_ops structure you have so that
> there's no NULL pointers.
> 
> I guess that's coming...
>

Yes, a function goes into df_ops only when details change between Data
Fabric versions, i.e. different bitfields, registers, etc., but the flow
is otherwise the same.

I'll give the NULL pointer concern more thought. I've tried to cover all
code paths and use cases testing on real hardware. But it's possible
something can be missed with how configurable our systems can be.

Thanks,
Yazen
diff mbox series

Patch

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index f769353ef7e4..389f0621e885 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1079,16 +1079,26 @@  static struct df_reg df_regs[] = {
 	[SYS_FAB_ID_MASK]	=	{1, 0x208},
 };
 
+/* Use "reg_" prefix for raw register values. */
 struct addr_ctx {
 	u64 ret_addr;
+	u32 reg_dram_offset;
 	u16 nid;
 	u8 inst_id;
+	u8 map_num;
 };
 
 struct data_fabric_ops {
+	u64 (*get_hi_addr_offset)(struct addr_ctx *ctx);
 };
 
+static u64 get_hi_addr_offset_df2(struct addr_ctx *ctx)
+{
+	return (ctx->reg_dram_offset & GENMASK_ULL(31, 20)) << 8;
+}
+
 struct data_fabric_ops df2_ops = {
+	.get_hi_addr_offset		=	&get_hi_addr_offset_df2,
 };
 
 struct data_fabric_ops *df_ops;
@@ -1100,6 +1110,35 @@  static int set_df_ops(struct addr_ctx *ctx)
 	return 0;
 }
 
+static int get_dram_offset_reg(struct addr_ctx *ctx)
+{
+	if (amd_df_indirect_read(ctx->nid, df_regs[DRAM_OFFSET],
+				 ctx->inst_id, &ctx->reg_dram_offset))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int remove_dram_offset(struct addr_ctx *ctx)
+{
+	if (get_dram_offset_reg(ctx))
+		return -EINVAL;
+
+	ctx->map_num = 0;
+
+	/* Remove HiAddrOffset from normalized address, if enabled: */
+	if (ctx->reg_dram_offset & BIT(0)) {
+		u64 hi_addr_offset = df_ops->get_hi_addr_offset(ctx);
+
+		if (ctx->ret_addr >= hi_addr_offset) {
+			ctx->ret_addr -= hi_addr_offset;
+			ctx->map_num = 1;
+		}
+	}
+
+	return 0;
+}
+
 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;
@@ -1109,7 +1148,7 @@  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
 	u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
 	u8 intlv_addr_sel, intlv_addr_bit;
 	u8 num_intlv_bits, hashed_bit;
-	u8 lgcy_mmio_hole_en, base = 0;
+	u8 lgcy_mmio_hole_en;
 	u8 cs_mask, cs_id = 0;
 	bool hash_enabled = false;
 
@@ -1128,21 +1167,11 @@  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
 	if (set_df_ops(&ctx))
 		return -EINVAL;
 
-	if (amd_df_indirect_read(nid, df_regs[DRAM_OFFSET], umc, &tmp))
+	if (remove_dram_offset(&ctx))
 		goto out_err;
 
-	/* Remove HiAddrOffset from normalized address, if enabled: */
-	if (tmp & BIT(0)) {
-		u64 hi_addr_offset = (tmp & GENMASK_ULL(31, 20)) << 8;
-
-		if (norm_addr >= hi_addr_offset) {
-			ctx.ret_addr -= hi_addr_offset;
-			base = 1;
-		}
-	}
-
 	reg = df_regs[DRAM_BASE_ADDR];
-	reg.offset += base * 8;
+	reg.offset += ctx.map_num * 8;
 	if (amd_df_indirect_read(nid, reg, umc, &tmp))
 		goto out_err;
 
@@ -1166,7 +1195,7 @@  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
 	}
 
 	reg = df_regs[DRAM_LIMIT_ADDR];
-	reg.offset += base * 8;
+	reg.offset += ctx.map_num * 8;
 	if (amd_df_indirect_read(nid, reg, umc, &tmp))
 		goto out_err;