Re: [PATCH v2 2/4] RAS/AMD/ATL: Expand helpers for adding and removing base and hole

[Yazen Ghannam]

On 3/27/24 14:52, John Allen wrote:
> The existing helpers for adding and removing the base and legacy MMIO

The remove helper is newly introduced in this patch, correct?

> hole operate on the ret_addr stored in the addr_ctx struct. This works
> for the existing use case as adding and removing the base and hole is
> only done once for an address that is already stored in ret_addr as part
> of translation.

I think it would help to give a bit more context and to describe things
more generally.

"The ret_addr field in struct addr_ctx contains the intermediate value of
the returned address as it passes through multiple steps in the
translation process. Currently, the "add DRAM base and legacy hole"
step is only done once, so it operates directly on the intermediate
value."

> However, in the Data Fabric 4.5 non-power-of-2 cases, we will add and
> remove the base and hole temporarily to check for correctness on
> addresses other than ret_addr multiple times throughout translation.

Don't say "we". Keep the voice passive.

"However, for X cases, the "add DRAM base and legacy hole" step, and its
inverse, need to be done for multiple temporary addresses values. During
this process, the intermediate should not be lost, so the ret_addr value
cannot be reused."

> Modify the helpers to take any address as a parameter and return the
> resulting address after adding or removing the hole. This will allow for
> more simply adding and removing the base and hole from any address
> without having to store them in the ret_addr field of the addr_ctx
> struct.

"Update the existing 'add' helper to operate on an arbitrary address.
And introduce a new 'remove' helper to do the inverse operations."

How do these sound?

> Signed-off-by: John Allen <john.allen@xxxxxxx>
> ---
> v2:
>    - Make remove_base_and_hole the inverse of add_base_and_hole, removing
>      the hole first and then removing the base.
>    - Improve commit description.
> ---
>   drivers/ras/amd/atl/core.c     | 43 ++++++++++++++++++++++------------
>   drivers/ras/amd/atl/internal.h |  3 +++
>   2 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ras/amd/atl/core.c b/drivers/ras/amd/atl/core.c
> index 63513d972c07..d45f9948c0ab 100644
> --- a/drivers/ras/amd/atl/core.c
> +++ b/drivers/ras/amd/atl/core.c
> @@ -49,15 +49,26 @@ static bool legacy_hole_en(struct addr_ctx *ctx)
>   	return FIELD_GET(DF_LEGACY_MMIO_HOLE_EN, reg);
>   }
>   
> -static int add_legacy_hole(struct addr_ctx *ctx)
> +static u64 add_legacy_hole(struct addr_ctx *ctx, u64 addr)
>   {
>   	if (!legacy_hole_en(ctx))
> -		return 0;
> +		return addr;
>   
> -	if (ctx->ret_addr >= df_cfg.dram_hole_base)
> -		ctx->ret_addr += (BIT_ULL(32) - df_cfg.dram_hole_base);
> +	if (addr >= df_cfg.dram_hole_base)
> +		addr += (BIT_ULL(32) - df_cfg.dram_hole_base);
>   
> -	return 0;
> +	return addr;
> +}
> +
> +static u64 remove_legacy_hole(struct addr_ctx *ctx, u64 addr)
> +{
> +	if (!legacy_hole_en(ctx))
> +		return addr;
> +
> +	if (addr >= df_cfg.dram_hole_base)
> +		addr -= (BIT_ULL(32) - df_cfg.dram_hole_base);
> +
> +	return addr;
>   }
>   
>   static u64 get_base_addr(struct addr_ctx *ctx)
> @@ -72,14 +83,16 @@ static u64 get_base_addr(struct addr_ctx *ctx)
>   	return base_addr << DF_DRAM_BASE_LIMIT_LSB;
>   }
>   
> -static int add_base_and_hole(struct addr_ctx *ctx)
> +u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr)
>   {
> -	ctx->ret_addr += get_base_addr(ctx);
> -
> -	if (add_legacy_hole(ctx))
> -		return -EINVAL;
> +	addr += get_base_addr(ctx);
> +	return add_legacy_hole(ctx, addr);
> +}
>   
> -	return 0;
> +u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr)
> +{
> +	remove_legacy_hole(ctx, addr);
> +	return addr - get_base_addr(ctx);
>   }
>   
>   static bool late_hole_remove(struct addr_ctx *ctx)
> @@ -123,14 +136,14 @@ unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsig
>   	if (denormalize_address(&ctx))
>   		return -EINVAL;
>   
> -	if (!late_hole_remove(&ctx) && add_base_and_hole(&ctx))
> -		return -EINVAL;
> +	if (!late_hole_remove(&ctx))
> +		ctx.ret_addr = add_base_and_hole(&ctx, ctx.ret_addr);
>   
>   	if (dehash_address(&ctx))
>   		return -EINVAL;
>   
> -	if (late_hole_remove(&ctx) && add_base_and_hole(&ctx))
> -		return -EINVAL;
> +	if (late_hole_remove(&ctx))
> +		ctx.ret_addr = add_base_and_hole(&ctx, ctx.ret_addr);
>   
>   	if (addr_over_limit(&ctx))
>   		return -EINVAL;
> diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
> index 1413c8ddc6c5..05b870fcb24e 100644
> --- a/drivers/ras/amd/atl/internal.h
> +++ b/drivers/ras/amd/atl/internal.h
> @@ -236,6 +236,9 @@ int dehash_address(struct addr_ctx *ctx);
>   unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsigned long addr);
>   unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err);
>   
> +u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr);
> +u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr);
> +
>   /*
>    * Make a gap in @data that is @num_bits long starting at @bit_num.
>    * e.g. data		= 11111111'b

Besides the commit message notes, I think this looks good.

Reviewed-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>

Thanks,
Yazen