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

[Chen, Yu C]

Hi Reinette,

On 8/20/2026 7:06 AM, Reinette Chatre wrote:
> 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"

OK, will change it.

> > 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"

OK.

> >   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?

Yes, erdt_scale is used only for limbo handler, and the cmrc::up_scale 
is actually
used by monitor count. Let me change the comment above.

> > +static int erdt_scale;
>
> Can the scale ever be negative? Could it be unsigned int?

It would not be negative, let me switch it to unsigned int, but..

> 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?

Yes, the legacy CPUID reports it as 32 bits, while CMRC is declared as 
64 bits.

In theory, we should change the scale type from unsigned int to u64 to 
accommodate
both the legacy CPUID and CMRC. However, it seems unlikely that the 
scale would
exceed 32 bits. If the scale were 32 bits, the L3 occupancy would be at 
least
2^32 − 1, which is about 4 GB. We have not yet seen platform with 4 GB 
of L3 cache.
So perhaps we can keep erdt_scale as unsigned int for now IMO.

> > +
> >   int erdt_get_max_rmid(void)
>
> Can this be negative?

It would not be negative, let me convert it into unsigned int.

> >   
> > +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.

OK, will check the code to fix them.

> > +	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.

OK, let add the explanation around 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?

On second thought, the cleanup needs to be performed. That is, the 
region-aware
RDT should enable CMT, MBA, and MBM collectively; otherwise, the system 
falls back
to the legacy interface. This could keep the code easier to maintain. I 
will address
this in the next version.

thanks,
Chenyu