Re: [PATCH v5 05/10] x86/resctrl: Parse ACPI CMRC table

[Reinette Chatre]

Hi Chenyu,

On 7/1/26 6:46 AM, Chen Yu wrote:
> The CMRC (Cache Monitoring Registers for CPU Agents Description)
> sub-table of ERDT describes the MMIO registers used to read
> cache monitoring counters (e.g. LLC occupancy) for an RMD.

Please use the entire line length available (please check all changelogs).
Some context for this request:
https://lore.kernel.org/lkml/20250916105447.GCaMlB976WLxHHeNMD@fat_crate.local/

> 
> Parse each CMRC sub-table, ioremap its register window, and save
> the CMRC pointer in the corresponding ERDT domain entry so that
> later monitoring code can read the counters via MMIO.

Is the "save the CMRC pointer ..." referring to how resctrl makes
a copy of the CMRC table? If so, that is very different from
"saving a pointer".

> 
> Suggested-by: Tony Luck <tony.luck@xxxxxxxxx>
> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxx>
> Tested-by: Hongyu Ning <hongyu.ning@xxxxxxxxxxxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> ---
> v4->v5:
>   No change.    
> ---
>  arch/x86/kernel/cpu/resctrl/erdt.c     | 44 ++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index 6c1df7e43eab..a5754d64fcc1 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -26,6 +26,7 @@ static LIST_HEAD(domain_info_list);
>  static bool __erdt_enabled;
>  
>  #define ERDT_VALID_VERSION		1
> +#define CMRC_SUPPORTED_INDEX_FN		1
>  #define RMDD_FLAG_CPU_L3_DOMAIN		BIT(0)
>  
>  /* Bitmask of valid sub-tables found in the first RMDD, used to ensure all RMDDs match. */
> @@ -79,6 +80,7 @@ static void cleanup_one_domain(struct erdt_domain_info *d)
>  {
>  	erdt_iounmap_domain(d);
>  	free_cpumask_var(d->cpu_mask);
> +	kfree(d->cmrc);
>  	kfree(d);
>  }
>  
> @@ -112,6 +114,41 @@ static __init int cacd_init(struct acpi_subtbl_hdr_16 *subtbl,
>  	return 0;
>  }
>  
> +static __init int cmrc_init(struct acpi_subtbl_hdr_16 *subtbl,
> +			    struct erdt_domain_info *domain_info)
> +{
> +	struct acpi_erdt_cmrc *cmrc = (struct acpi_erdt_cmrc *)subtbl;
> +
> +	if (subtbl->length < sizeof(*cmrc)) {

I think it will be helpful if the ERDT parsing code uses consistent patterns. Here,
for example, now that a pointer of accurate type is available via cmrc, can cmrc
be referenced instead to be consistent with cacd_init()?

> +		pr_warn(FW_BUG "Truncated CMRC subtable\n");
> +		return -EIO;
> +	}
> +
> +	if (cmrc->index_fn != CMRC_SUPPORTED_INDEX_FN) {
> +		pr_info("Unsupported CMRC index function %d\n", cmrc->index_fn);
> +		return -EIO;
> +	}
> +
> +	if (!cmrc->clump_size) {
> +		pr_warn(FW_BUG "CMRC clump_size is zero\n");
> +		return -EIO;
> +	}
> +
> +	domain_info->base[ERDT_MMIO_CMRC_BASE] =
> +		erdt_ioremap(cmrc->cmt_reg_base, cmrc->cmt_reg_size, "CMRC base");
> +	if (!domain_info->base[ERDT_MMIO_CMRC_BASE])
> +		return -EIO;
> +
> +	domain_info->cmrc = kmemdup(cmrc, subtbl->length, GFP_KERNEL);
> +	if (!domain_info->cmrc) {
> +		iounmap(domain_info->base[ERDT_MMIO_CMRC_BASE]);
> +		domain_info->base[ERDT_MMIO_CMRC_BASE] = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline struct acpi_subtbl_hdr_16 *rmdd_subtbl(struct acpi_erdt_rmdd *rmdd)
>  {
>  	return (void *)rmdd + sizeof(*rmdd);
> @@ -178,6 +215,13 @@ static __init bool parse_rmdd_entry(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>  				goto cleanup;
>  
>  			subtbl_mask |= BIT(ACPI_ERDT_TYPE_CACD);
> +			break;
> +		case ACPI_ERDT_TYPE_CMRC:
> +			/* TBD: Only 1 CMRR per domain is allowed? */

Who are you asking this question to?

> +			if (!(subtbl_mask & BIT(ACPI_ERDT_TYPE_CMRC)) &&
> +			    !cmrc_init(subtbl, domain_info))
> +				subtbl_mask |= BIT(ACPI_ERDT_TYPE_CMRC);
> +
>  			break;
>  		default:
>  			break;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 7d9100b7648f..3dab05c38003 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -36,6 +36,7 @@ enum erdt_mmio_type {
>  
>  struct erdt_domain_info {
>  	void __iomem		*base[ERDT_MMIO_NUM_TYPES];
> +	struct acpi_erdt_cmrc	*cmrc;
>  	cpumask_var_t		cpu_mask;
>  	int			max_rmid;
>  	struct list_head	list;

Reinette