Re: [PATCH v6 5/9] x86/resctrl: Parse ACPI CMRC table

[Reinette Chatre]

Hi Chenyu,

On 7/25/26 2:23 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.

nit: "an RMD" -> "a monitoring domain"

> 
> Parse each CMRC sub-table, ioremap its register window, and save a copy of the
> CMRC table in the corresponding ERDT domain entry so that later monitoring code

nit: drop "later"

> can read the counters via MMIO.
> 
> Suggested-by: Tony Luck <tony.luck@xxxxxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> Tested-by: Hongyu Ning <hongyu.ning@xxxxxxxxxxxxxxx>
> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxx>
> ---

...

> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 575f8408a9e7..e60c2aea7ebd 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -49,6 +49,8 @@ DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
>  DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
>  DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>  
> +int erdt_get_scale(void);
> +
>  static inline bool resctrl_arch_alloc_capable(void)
>  {
>  	return rdt_alloc_capable;
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index 6257869d0db2..422618991927 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -23,6 +23,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. */
> @@ -33,11 +34,19 @@ static u16 first_rmdd_domain_id;
>  
>  static int erdt_max_rmid;
>  
> +/* Scale to bytes for the monitoring counters when ERDT is enabled. */

hmmm ... when looking ahead at patch #9 this does not seem to be how this value is used?
Instead, when a monitoring counter is read it is scaled using the per-domain
acpi_erdt_cmrc::up_scale?

Instead this seems to be the scale used to set/initialize resctrl_rmid_realloc_threshold
that is used by the limbo handler?

> +static int erdt_scale;

Can the scale ever be negative? Could it be unsigned int? Actually, looks like the original
MSR based scale obtained via CPUID.(EAX=0FH,ECX=1H) is 32 bits while this new scale value
from CMRC is 64 bits. The existing code can thus not accommodate the new values and need to
be updated?

> +
>  int erdt_get_max_rmid(void)

Can this be negative?

>  {
>  	return erdt_max_rmid;
>  }
>  
> +int erdt_get_scale(void)
> +{
> +	return erdt_scale;
> +}
> +
>  static void __iomem *erdt_ioremap(phys_addr_t base, u32 num_pages, const char *desc)
>  {
>  	void __iomem *addr;
> @@ -67,6 +76,7 @@ static void erdt_iounmap_domain(struct erdt_domain_info *domain)
>  static void cleanup_one_domain(struct erdt_domain_info *d)
>  {
>  	erdt_iounmap_domain(d);
> +	kfree(d->cmrc);
>  	kfree(d);
>  }
>  
> @@ -100,6 +110,43 @@ 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 (cmrc->header.length < sizeof(*cmrc)) {
> +		pr_warn(FW_BUG "Truncated CMRC subtable\n");

Please note there is inconsistency wrt "subtable" vs "sub-table" in error messages.

> +		return -EIO;
> +	}
> +
> +	if (cmrc->index_fn != CMRC_SUPPORTED_INDEX_FN) {
> +		pr_info("Unsupported CMRC index function %u\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, cmrc->header.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;
> +	}
> +
> +	erdt_scale = max_t(int, erdt_scale, cmrc->up_scale);

Please add a comment to describe why maximum of all domains' scale value is used. This comment
may be best placed at global definition of erdt_scale.

> +
> +	return 0;
> +}
> +
>  static inline struct acpi_subtbl_hdr_16 *rmdd_subtbl(struct acpi_erdt_rmdd *rmdd)
>  {
>  	return (void *)rmdd + sizeof(*rmdd);
> @@ -166,6 +213,16 @@ static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>  				goto cleanup;
>  
>  			subtbl_mask |= BIT(ACPI_ERDT_TYPE_CACD);
> +			break;
> +		case ACPI_ERDT_TYPE_CMRC:
> +			/*
> +			 * Only one CMRC is supported per domain as there is no
> +			 * method to distinguish different CMRCs within a domain.
> +			 */
> +			if (!(subtbl_mask & BIT(ACPI_ERDT_TYPE_CMRC)) &&
> +			    !cmrc_init(subtbl, domain_info))
> +				subtbl_mask |= BIT(ACPI_ERDT_TYPE_CMRC);

How is cmrc_init() failure handled?

> +
>  			break;
>  		default:
>  			break;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index bd437c3e5bf0..27f28b0fead8 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -24,10 +24,12 @@
>  /*
>   * Index into erdt_domain_info::base[] for each MMIO region.
>   * @ERDT_MMIO_RMDD_CREG: RMDD control register base address
> + * @ERDT_MMIO_CMRC_BASE: CMRC monitoring register base address
>   */
>  enum erdt_mmio_type {
>  	ERDT_MMIO_RMDD_CREG,
> -	ERDT_MMIO_LAST = ERDT_MMIO_RMDD_CREG
> +	ERDT_MMIO_CMRC_BASE,
> +	ERDT_MMIO_LAST = ERDT_MMIO_CMRC_BASE
>  };
>  
>  #define ERDT_MMIO_NUM_TYPES	(ERDT_MMIO_LAST + 1)
> @@ -35,6 +37,7 @@ enum erdt_mmio_type {
>  /**
>   * struct erdt_domain_info - Per-domain ERDT information
>   * @base:	Array of ioremapped MMIO region base addresses, indexed by ERDT_MMIO_* type
> + * @cmrc:	Copy of the ACPI CMRC sub-table for this domain
>   * @cpu_mask:	CPUs belonging to this resource management domain
>   * @max_rmid:	Maximum RMID supported by this domain
>   * @dom_id:	L3 cache ID shared by all CPUs in this domain (-1 if unset)
> @@ -42,6 +45,7 @@ enum erdt_mmio_type {
>   */
>  struct erdt_domain_info {
>  	void __iomem		*base[ERDT_MMIO_NUM_TYPES];
> +	struct acpi_erdt_cmrc	*cmrc;
>  	struct cpumask		cpu_mask;
>  	u32			max_rmid;
>  	int			dom_id;

Reinette