Hi Ben,

On 28/08/2025 17:13, Ben Horgan wrote:

On 8/22/25 16:30, James Morse wrote:

quoted

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.

quoted

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

quoted

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