Re: [PATCH v10 3/7] RAS/AMD/ATL: Add unified UMC address translation interface

[Borislav Petkov]

On Tue, Sep 22, 2026 at 03:23:13PM -0500, Yazen Ghannam wrote:
> The library exposes a single function for UMC address translation. Adding a
> new translation would mean adding and plumbing another export. Callers also
> have no way to request more than one representation of an address in a
> single call.
> 
> Add a single entry point, amd_translate_umc_mca_addr(), that operates
> entirely through struct atl_err. Expand the struct with a pair of
> operation bitmaps and the per-operation output fields.
> 
> The caller provides the error inputs and requests one or more operations.
> Each operation is attempted independently. The library sets a validation
> bit and fills the matching output for each operation that succeeds. The
> caller must check the validation bit before consuming an output value.
> 
> Group the error identifiers (normalized address, socket, and UMC bank ID)
> into struct atl_umc_addr. Lay it out to match the PRM parameter buffer
> inputs that every handler shares, and embed it in the buffer. A caller can
> then build the buffer straight from its own struct atl_err. Move the buffer
> definition to internal.h so callers outside prm.c can do so.
> 
> Route the interface through the always-built RAS core so the library
> remains optional. The core provides the entry point and a
> register/unregister pair. The library registers its implementation on load.
> The entry point does nothing (no validation bits set) when the library is
> absent.
> 
> Start with a single operation, ATL_OP_SPA, that produces the System
> Physical Address. Try the PRM handler first. Fall back to the native Data
> Fabric translation if PRM is unavailable and the Data Fabric revision
> supports it.
> 
> Leave the existing amd_convert_umc_mca_addr_to_sys_addr() interface in
> place for now.

Please remove text explaining the code.

> Assisted-by: LLM

Those tags are not needed anymore. Yeah, that was quick:

  1e035297ffda ("docs: remove Assisted-by references")

...

> diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
> index befc616d5e8a..91c02ef5dc11 100644
> --- a/drivers/ras/amd/atl/umc.c
> +++ b/drivers/ras/amd/atl/umc.c
> @@ -416,3 +416,45 @@ unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err)
>  
>  	return norm_to_sys_addr(socket_id, die_id, coh_st_inst_id, addr);
>  }
> +
> +/*
> + * Translate a UMC MCA error address into one or more representations as
> + * requested by the caller.
> + *
> + * The caller sets the input values and requests one or more operations
> + * through @err->requested. @err->valid is cleared on entry. Each operation
> + * is attempted independently. The corresponding bit in @err->valid is set
> + * and the related output field is filled for each operation that succeeds.
> + * The caller must check @err->valid before consuming an output value.
> + *
> + * The PRM handlers consume @err->umc_addr directly. Only MI300 needs its
> + * MCA_ADDR value converted to a normalized address first; see get_addr().
> + * MI300 platforms provide no PRM handlers, so the conversion is left to the
> + * native fallback path.

Hm, ok. Sounds like a whole protocol. Lemme get to the end of this and see
whether this is not too overengineered for no good reason...


> + * Registered with the RAS core as the UMC address translator; see
> + * amd_translate_umc_mca_addr().

Unnecessary text.

> + */
> +void amd_atl_umc_translate_addr(struct atl_err *err)
> +{
> +	err->socket_id = topology_physical_package_id(err->cpu);
> +	err->valid     = 0;
> +
> +	if (err->requested & ATL_OP_SPA) {

You can make those operations a number and this then can become a switch-case.

Should be easier, like a dispatch table.

> +		struct param_buf p_buf = {
> +			.addr		= err->umc_addr,
> +			.out_buf	= &err->spa,
> +		};
> +
> +		if (!prm_umc_norm_to_addr(norm_to_sys_guid, &p_buf)) {
> +			err->valid |= ATL_OP_SPA;
> +		} else if (!df_cfg.flags.prm_only) {
> +			unsigned long spa = convert_umc_mca_addr_to_sys_addr(err);
> +
> +			if (!IS_ERR_VALUE(spa)) {
> +				err->spa = spa;
> +				err->valid |= ATL_OP_SPA;
> +			}
> +		}
> +	}
> +}

...

> diff --git a/include/linux/ras.h b/include/linux/ras.h
> index 468941bfe855..eac8cf39ddd5 100644
> --- a/include/linux/ras.h
> +++ b/include/linux/ras.h
> @@ -3,6 +3,8 @@
>  #define __RAS_H__
>  
>  #include <asm/errno.h>
> +#include <linux/bits.h>
> +#include <linux/stddef.h>
>  #include <linux/uuid.h>
>  #include <linux/cper.h>
>  
> @@ -35,10 +37,26 @@ static inline void
>  log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev) { return; }
>  #endif
>  
> +/* Operations requested and completed through amd_translate_umc_mca_addr(). */
> +#define ATL_OP_SPA		BIT(0)	/* System Physical Address */

verify_comment_style: WARNING: No tail comments please.

Please audit your whole set.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette