Message-ID: <c169aec6-54d1-4ac5-886c-306dad7f6040@arm.com>
Date: Wed, 10 Sep 2025 20:29:56 +0100
From: James Morse <james.morse@....com>
To: Ben Horgan <ben.horgan@....com>, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org,
 devicetree@...r.kernel.org
Cc: shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
 Shanker Donthineni <sdonthineni@...dia.com>, fenghuay@...dia.com,
 baisheng.gao@...soc.com, Jonathan Cameron <jonathan.cameron@...wei.com>,
 Rob Herring <robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
 Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
 <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 24/33] arm_mpam: Allow configuration to be applied and
 restored during cpu online

Hi Ben,

On 28/08/2025 17:13, Ben Horgan wrote:
> On 8/22/25 16:30, James Morse wrote:
>> When CPUs come online the original configuration should be restored.
>> Once the maximum partid is known, allocate an configuration array for
>> each component, and reprogram each RIS configuration from this.
>>
>> The MPAM spec describes how multiple controls can interact. To prevent
>> this happening by accident, always reset controls that don't have a
>> valid configuration. This allows the same helper to be used for
>> configuration and reset.

> What in particular are you worried about here?

'other' controls being left in an unknown state - meaning the one you did set, is useless.

In a sane world, the thing writing the controls would write all the supported registers.
In practice, resctrl only knows about bitmaps. The glue code could provide all the other
values, but I figured it was better for the driver to do it. I'm sure they'll add other
control types, and it would be a nuisance to update multiple callers if there is ever more
than one.

Another angle down here is mismatched components/devices mean a control type could be
hidden if its not available everywhere - so the caller may not be aware of all the
controls it was supposed to provide.


> It does seem a bit
> wasteful that to update a single control in a ris all the controls in
> that ris are updated.

I don't think anyone would ever build something that supports all these. One is most
likely, pushing to three for platforms that support CPOR and CMIN/MAX. By the time you've
taken the IPI to access a cache MSC, the cost of an additional register access is negligible.


> This is needed for reset and restore but do we
> really want if we are just changing one control, e.g. the cache portion
> bitmap.

The original config has been blown away by this point, but we do have the bitmap of what
changed. I guess this is an emergent effect of __write_config() originating from the reset
helper, and the 'empty config' being used to reset devices.

I'd like to keep it as a single function that actually touches these registers.
I'll change to generate a 'maximal' config instead of empty for reset - which pulls the
policy on those values out, and drops the '0 for reset'.

huh ... that is what the ALL_FEATURES macro you pointed out was for ...
I suspect it was the bitmaps that are larger than a u32 that made this hard.



>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index b424af666b1e..8f6df2406c22 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c

>> @@ -1960,6 +2094,79 @@ void mpam_enable(struct work_struct *work)
>>  		mpam_enable_once();
>>  }
>>  
>> +struct mpam_write_config_arg {
>> +	struct mpam_msc_ris *ris;
>> +	struct mpam_component *comp;
>> +	u16 partid;
>> +};
>> +
>> +static int __write_config(void *arg)
>> +{
>> +	struct mpam_write_config_arg *c = arg;
>> +
>> +	mpam_reprogram_ris_partid(c->ris, c->partid, &c->comp->cfg[c->partid]);
>> +
>> +	return 0;
>> +}
>> +
>> +#define maybe_update_config(cfg, feature, newcfg, member, changes) do { \
>> +	if (mpam_has_feature(feature, newcfg) &&			\
>> +	    (newcfg)->member != (cfg)->member) {			\
>> +		(cfg)->member = (newcfg)->member;			\
>> +		cfg->features |= (1 << feature);			\
>> +									\
>> +		(changes) |= (1 << feature);				\
>> +	}								\
>> +} while (0)
>> +
>> +static mpam_features_t mpam_update_config(struct mpam_config *cfg,
>> +					  const struct mpam_config *newcfg)
>> +{
>> +	mpam_features_t changes = 0;
>> +
>> +	maybe_update_config(cfg, mpam_feat_cpor_part, newcfg, cpbm, changes);
>> +	maybe_update_config(cfg, mpam_feat_mbw_part, newcfg, mbw_pbm, changes);
>> +	maybe_update_config(cfg, mpam_feat_mbw_max, newcfg, mbw_max, changes);
>> +
>> +	return changes;
>> +}
>> +
>> +/* TODO: split into write_config/sync_config */
>> +/* TODO: add config_dirty bitmap to drive sync_config */

> Any changes to come for these TODO comments?

No time. The dirty bitmap was to help with the problem you highlighted above. Separating
into write/sync was to make it easier to support the firmware-backed thing, which can be
a problem for another day.

I'll drop these.


Thanks,

James

Powered by blists - more mailing lists