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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Chenyu,

On 9/17/26 9:50 PM, Chen Yu wrote:
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 8f6edcdcfd87..9a32ed418c33 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);
>  
> +unsigned int erdt_get_scale(void);
> +

Adding this prototype to asm header file seems out of place. One needs to look
at later patches to learn this is because of upcoming use in
resctrl_arch_round_mon_val(). Beyond that, resctrl_arch_round_mon_val() also
later needs erdt_cpu_has() that even more looks like the wrong thing to do
when it comes to the asm header file.

resctrl_arch_round_mon_val() is used in two places, during system initialization
and when user space updates resctrl_rmid_realloc_threshold via a write to
"max_threshold_occupancy". Neither is a hot path requiring this to be inline
code. 

Aiming to keep resctrl_arch_round_mon_val() as an inline function is causing this
ERDT support to be unnecessarily fragmented. Could you please add a preparatory
patch that moves resctrl_arch_round_mon_val() to a c file and add its prototype
to include/linux/resctrl.h? This means that a change to MPAM driver is also needed
that I do not expect objection against. To make this easier it would help to
place the stub among the more stable resctrl_arch_* calls in MPAM driver.

>  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 249ba547d7c8..a8a7417c2f82 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. */
> @@ -37,11 +38,26 @@ static u16 first_rmdd_domain_id;
>   */
>  static unsigned int erdt_max_rmid;
>  
> +/*
> + * Used only by the limbo handler to round resctrl_rmid_realloc_threshold.

This implies erdt_scale is used by limbo handler but limbo handler only uses
resctrl_rmid_realloc_threshold directly, no?

> + * resctrl_rmid_realloc_threshold is a single global value, and
> + * resctrl_arch_round_mon_val() takes no domain argument, so a single scale has
> + * to be derived from the per-domain cmrc->up_scale. max() is chosen because the

This does not sound right. Using the fact that a function does not take an argument as
a motivation just makes one wonder why the function cannot just be changed?

"resctrl_rmid_realloc_threshold is a single global value" is accurate and the reason
why it needs to stay that way is because it is exposed to user space as such. That
was done before RDT introduced per domain scaling. If keeping it a global is ok for
ERDT then please highlight this, otherwise resctrl needs an enhancement.

Apart from above it looks like introduction of erdt_scale and erdt_get_scale() would
benefit from a separate commit. The comment above clearly notes its complexity but
there is no mention of it in changelog. 

> + * rounding is a floor: a larger scale yields a slightly lower threshold, i.e. an
> + * RMID has to drop to a slightly lower occupancy before it is reused.
> + */
> +static unsigned int erdt_scale;
> +
>  unsigned int erdt_get_max_rmid(void)
>  {
>  	return erdt_max_rmid;
>  }
>  
> +unsigned int erdt_get_scale(void)
> +{
> +	return erdt_scale;
> +}
> +
>  static void __iomem *erdt_ioremap(resource_size_t base, u32 num_pages, const char *desc)
>  {
>  	void __iomem *addr;
> @@ -71,6 +87,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);
>  }
>  
> @@ -105,6 +122,49 @@ 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)

Same comment as for cacd_init().

> +{
> +	struct acpi_erdt_cmrc *cmrc = (struct acpi_erdt_cmrc *)subtbl;
> +
> +	if (cmrc->header.length < sizeof(*cmrc)) {
> +		pr_warn(FW_BUG "Truncated CMRC sub-table\n");
> +		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;
> +	}
> +
> +	/* resctrl scales monitoring values with an unsigned int. */
> +	if (cmrc->up_scale > UINT_MAX) {
> +		pr_warn(FW_BUG "Insane CMRC up_scale value 0x%llx\n", cmrc->up_scale);
> +		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(erdt_scale, cmrc->up_scale);
> +
> +	return 0;
> +}
> +
>  static inline struct acpi_subtbl_hdr_16 *rmdd_subtbl(struct acpi_erdt_rmdd *rmdd)
>  {
>  	return (void *)rmdd + sizeof(*rmdd);
> @@ -170,6 +230,19 @@ static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>  
>  			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.
> +			 */

Please note how this comment style is different from comment used to describe parsing
of other RMDD sub-tables (before or after "case"). Please stick one style and use it
consistently.

> +			if (subtbl_mask & BIT(ACPI_ERDT_TYPE_CMRC))
> +				break;
> +
> +			if (cmrc_init(subtbl, domain_info))
> +				goto cleanup;
> +
> +			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 2e8fb36ad804..26c3e0c546ad 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,12 +37,14 @@ enum erdt_mmio_type {
>  /**
>   * struct erdt_domain_info - Per-domain ERDT information
>   * @base:	Array of ioremapped MMIO region base addresses, indexed by ERDT_MMIO_*
> + * @cmrc:	Copy of the ACPI CMRC sub-table for this domain
>   * @cpu_mask:	CPUs belonging to this resource management domain
>   * @dom_id:	L3 cache ID shared by all CPUs in this domain (-1 if unset)
>   * @entry:	Links into the global domain_info_list
>   */
>  struct erdt_domain_info {
>  	void __iomem		*base[ERDT_MMIO_NUM_TYPES];
> +	struct acpi_erdt_cmrc	*cmrc;
>  	struct cpumask		cpu_mask;
>  	int			dom_id;
>  	struct list_head	entry;

Reinette