[RFC PATCH 0/6] Introduce flexible CLOSID/RMID translation

[RFC PATCH 0/6] Introduce flexible CLOSID/RMID translation

[Dave Martin]

This series introduces an ID remapping scheme, as a preparatory step
towards improved support for MPAM hardware that implements the MPAM
PARTID Narrowing feature.

This series is not a complete implementation, but is sufficient to
provide additional resctrl monitoring groups beyond the number of PMGs
supported by the hardware (with some limitations; see "Limitations",
below).

Based on:
git://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git mpam/snapshot/v6.12-rc1


Background
==========

An MPAM Memory System Component (MSC) that does not implement the
PARTID Narrowing feature (Narrowing) provides independently
programmable resource controls for each PARTID up to the maximum PARTID
that the MSC supports.

An MSC that _does_ implement Narrowing performs a remapping step to
convert the PARTID from the incoming memory transaction ("request
PARTID", or "reqPARTID") to an internal ID ("internal PARTID", or
"intPARTID") before looking up and applying resource controls.  Such an
MSC does not necessarily need to provide the same number of
independently programmable resource control partitions (indexed by the
intPARTID) as that which the size of the supported PARTID ("reqPARTID")
space would otherwise require.  Instead, independently programmable
resource controls are provided for each intPARTID, accompanied by a
software programmable mapping that allows each reqPARTID to be mapped
independently to a selected intPARTID.

Typically, the number of intPARTIDs supported by a Narrowing MSC is
smaller than the number of reqPARTIDs.  Narrowing permits multiple
reqPARTIDs to be mapped to each intPARTID, so allowing the whole MPAM
PARTID space to be served.

A non-Narrowing MSC can be understood as implementing special case of
Narrowing, where the reqPARTID-to-intPARTID mapping is the identity
function and is not programmable.


The number of independently configurable control partitions at a given
MSC cannot exceed the number of intPARTIDs available, so the number of
resctrl CLOSIDs (which need to be independently controllable) must
typically be limited not to exceed the maximum number of intPARTIDs
available on any Narrowing MSC.

However, the presence of a programmable reqPARTID-to-intPARTID mapping
in each MSC creates an opportunity: because a group of multiple MPAM
PARTIDs can be mapped onto the same controls, they behave as a single
resource control partition.  Because the PARTID is used as a match
criterion for monitoring purposes along with the MPAM Performance
Monitoring Group (PMG) identifier assigned each memory system
transaction, the specific PARTID in the group can be used to
distinguish transactions for monitoring purposes, even within the same
resctrl control group and when the PMGs of the transactions are the
same.

This means that it becomes possible to provide a larger number of
resctrl monitoring groups than the number of PMGs supported by the MPAM
hardware implementation.


In order to take advantage of this, a more flexible mapping scheme is
required for mapping resctrl CLOSIDs and RMIDs to MPAM PARTIDs and PMGs
than the current MPAM driver provides.

Similar approaches have been suggested by other people
(see Acknowledgements).

However, since I was already working on this, I'm posting my approach
for comparison.


This Series
===========

This series introduces a remapping scheme as follows:

1) The resctrl partition identifiers are combined into a single
identifier (formally the Cartesian product of the CLOSID, Code/Data
partition where applicable, and RMID ID spaces).

2) The resulting identifier is then broken down into the MPAM PARTID
and PMG, in such a way that each possible <CLOSID, CDP index, RMID>
maps to a unique <PARTID, PMG> pair and vice versa, and so that any two
<PARTID, PMG> that differ only in the PMG value, map onto the same
<CLOSID, CDP index> pair.


This transformation could be done in intuitive way using bitwise
concatenation, shift and masking operations, but since the size of the
MPAM hardware ID spaces may limited, I have tried to generalise this
approach using multiplication in place of bit-shifts, to permit ID
spaces whose size is not a power of two.


Limitations
===========

This series aims to contain all translations within the mpam_resctrl.c
code.  This seems the most sensible place for it, since this is a
conversion scheme that will be used to interface the MPAM hardware with
the resctrl core code (but has nothing to do with the "pure" MPAM
architecture or the resctrl core code itself).

To simplify the interface between mpam_resctrl.c and mpam_devices.c,
the ID conversion scheme is arranged so that each resctrl CLOSID maps
to a contiguous sequence of MPAM PARTIDs.  This means that programming
controls for a CLOSID can be achieved by iterating over the appropriate
range of PARTIDs.

No change is made to program the reqPARTID-to-intPARTID mappings, so
the number of MPAM PARTIDs in use continues to be limited not to exceed
the maximum number of intPARTIDs available, for now.

Because a CLOSID may be fragmented across multiple PARTIDs, this series
will cause certain types of resource control to be applied incorrectly
to each individual PARTID instead of the whole resctrl control group.

Future patches will need to program the intPARTID mappings
appropriately and/or disable certain controls in order to restore the
expected regulation behaviour as seen by users of resctrl.


Patches
=======

Patches 1-3 are pending fixes / cleanups for the MPAM branch.  They
have nothing directly to do with this series, but I don't want to
maintain two versions of my development branch.  This seems as a good a
time as any to post them.

Patch 4 implements the ID remapping scheme.

Following these are two development patches which may be useful to
reviewers:

Patch 5 adds a kernel command-line parameter mpam.partid_per_closid=<n>
to allow the number of PARTIDs to map to each CLOSID.  This is intended
to be temporary, but since the patches contain no logic for choosing
this parameter, allowing it to be specified on the kernel command line
makes it easier to experiment with different configurations.

Patch 6 adds some debug printks to show how MPAM PARTIDs / PMGs are
actually being assigned, and shows when each MSC is reprogrammed.
Since we don't want to create unintentional ABI exposing these IDs, I
don't expect this to be upstreamed in any form.


Acknowledgements
================

Thanks to Shaopeng Tan for pointing out this opportunity and suggesting
a related idea, and to Zeng Heng who has also implemented something
similar. [1], [2], [3]


References
==========

Zeng Heng <zengheng4@huawei.com>:

[1] (v3) arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver
https://lore.kernel.org/linux-arm-kernel/20241207092136.2488426-1-zengheng4@huawei.com/

[2] (v2) arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver
https://lore.kernel.org/linux-arm-kernel/20241119135104.595630-1-zengheng4@huawei.com/

[3] (v1) arm_mpam: Introduce the definitions of intPARTID and reqPARTID
https://lore.kernel.org/linux-arm-kernel/20241114135037.918470-1-zengheng4@huawei.com/



Dave Martin (6):
  arm_mpam: Clean up config update checks in mpam_apply_config()
  arm_mpam: Fix read-back of cloned resource controls under CDP
    emulation
  arm_mpam: Delete unused function resctrl_arch_set_rmid()
  arm_mpam: Introduce flexible CLOSID/RMID translation
  arm_mpam: [NFU] Rework ID remapping to use a kernel command-line
    argument
  arm_mpam: [NFU] Development diagnostics for MPAM ID assignments

 arch/arm64/include/asm/mpam.h              |  21 +-
 drivers/platform/arm64/mpam/mpam_devices.c |  29 +++
 drivers/platform/arm64/mpam/mpam_resctrl.c | 214 +++++++++++++--------
 3 files changed, 168 insertions(+), 96 deletions(-)


base-commit: 4d17b37b670795954fd7a70c8ec83fb705a5e2ad
-- 
2.34.1

[RFC PATCH 1/6] arm_mpam: Clean up config update checks in mpam_apply_config()

[Dave Martin]

In mpam_apply_config(), a simple memcmp() test is used to check
whether the config passed by the caller is already installed or
not.

This check will never find a match except (very occasionally) by
accident, since the component's version of the struct contains
things that the caller won't pass or doesn't know (such as the
garbage collection record).  There might also be random padding.

This may result in MSCs being reprogrammed unnecessarily.

Instead, only compare fields that the caller specified.  If
anything is present in the caller's config and doesn't match the
installed config, paste it across.  If nothing was pasted across
then the MSC reprogramming step is skipped (as the previous code
attempted to do).

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

There are comments in the code suggesting a change of this sort.
I may or may not have gone in the right direction with this, and
I have only tried to clean up the behaviour rather than optimising.

No attempt is made to skip unnecessary MSC register updates if the MSC
reprogramming goes ahead.

NOT well tested, yet.
---
 drivers/platform/arm64/mpam/mpam_devices.c | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index 9463045c0011..41962dd1bb68 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -3120,6 +3120,29 @@ static void mpam_extend_config(struct mpam_class *class, struct mpam_config *cfg
 	}
 }
 
+#define maybe_update_config(cfg, feature, newcfg, member, changes) do { \
+	if (mpam_has_feature(feature, newcfg) &&			\
+	    (newcfg)->member != (cfg)->member) {			\
+		(cfg)->member = (newcfg)->member;			\
+		mpam_set_feature(feature, cfg);				\
+									\
+		(changes) |= (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);
+	maybe_update_config(cfg, mpam_feat_mbw_min, newcfg, mbw_min, changes);
+
+	return changes;
+}
+
 /* TODO: split into write_config/sync_config */
 /* TODO: add config_dirty bitmap to drive sync_config */
 int mpam_apply_config(struct mpam_component *comp, u16 partid,
-- 
2.34.1

[RFC PATCH 2/6] arm_mpam: Fix read-back of cloned resource controls under CDP emulation

[Dave Martin]

When reading resource control values through a resctrl schemata
file, the index into the configuration array in the MPAM driver
is currently translated incorrectly.

This means that when resctrl is mounted with the "cdp" option, the
wrong array entry may be returned or an out-of-bounds array access
may occur when reading back of control values for the MB resource.

Fix it by translating the index for affected resources as if they
were "DATA" resources.  ("CODE" would also work, but because
resctrl_arch_update_one() clones schemata writes across "CODE" and
"DATA" for affected resources, it does not matter which one is
read.)

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

This issue was introduced by the non-upstream commit
"arm_mpam: resctrl: Improve CDP emulation"

(commit b097a6193546dbbd27cd19d7e16e1e750ebb030f in
git://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git mpam/snapshot/v6.12-rc1 )
---
 drivers/platform/arm64/mpam/mpam_resctrl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index 76ddbce0ea9c..30f2caec11d7 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -1097,6 +1097,14 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 	dom = container_of(d, struct mpam_resctrl_dom, resctrl_ctrl_dom);
 	cprops = &res->class->props;
 
+	/*
+	 * When CDP is enabled, but the resource doesn't support it,
+	 * the control is cloned across both partids.
+	 * Pick one at random to read:
+	 */
+	if (mpam_resctrl_hide_cdp(r->rid))
+		type = CDP_DATA;
+
 	partid = resctrl_get_config_index(closid, type);
 	cfg = &dom->comp->cfg[partid];
 
-- 
2.34.1

[RFC PATCH 3/6] arm_mpam: Delete unused function resctrl_arch_set_rmid()

[Dave Martin]

The function resctrl_arch_set_rmid() appears to have been left
behind after earlier refactoring and is now unused.

To avoid needing to answer awkward questions about how this
function should apply partition ID namespace remappings, just get
rid of it.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

I'm assuming here that there are no pending branches that implement /
use this function.

When moving tasks between monitoring groups, resctrlfs currently
extracts the CLOSID from struct rdtgroup and passes it explicitly
to resctrl_arch_set_closid_rmid(), so there seems to be no need for
a separate helper that doesn't overwrite the arch's control
partition identifier.
---
 arch/arm64/include/asm/mpam.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/mpam.h
index e5f385767174..df725d2d9d05 100644
--- a/arch/arm64/include/asm/mpam.h
+++ b/arch/arm64/include/asm/mpam.h
@@ -93,20 +93,6 @@ static inline u64 mpam_get_regval(struct task_struct *tsk)
 #endif
 }
 
-static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
-{
-#ifdef CONFIG_ARM64_MPAM
-	u64 regval = mpam_get_regval(tsk);
-
-	regval &= ~MPAM1_EL1_PMG_D;
-	regval &= ~MPAM1_EL1_PMG_I;
-	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid);
-	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid);
-
-	WRITE_ONCE(task_thread_info(tsk)->mpam_partid_pmg, regval);
-#endif
-}
-
 static inline void mpam_thread_switch(struct task_struct *tsk)
 {
 	u64 oldregval;
-- 
2.34.1

[RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID translation

[Dave Martin]

Currently, the MPAM driver uses the resctrl CLOSID directly as the
MPAM Partition Identifier (PARTID; possibly modified to give
different PARTIDs to code and data when Code/Data Partitioning /
CDP is enabled), and uses the resctrl RMID directly as the MPAM
Performance Monitoring Group identifier (PMG).

In preparation for using the MPAM PARTID Narrowing feature to allow
more resctrl monitoring groups to be provided than the number of
PMG values supported by the hardware, a more flexible remapping
scheme is needed.

Factor out ID translation operations in the MPAM resctrl glue code
into a couple of self-contained helpers, and call them as
appropriate.

The translation scheme has a single parameter, partid_per_closid,
which is currently hard-wired to 1.

As a result, this patch should have no effect on functionality.

Logic to determine / control the mapping parameters can be
introduced later.

The ID transformation may be visualised as follows:

without CDP:
  +---------------------------------------------------+-----------+
  |                       CLOSID                      |    RMID   |
  +---------------------------------------------------+-------+---+
  |                           PARTID                  :       |PMG|
  +-----------------------------------------------------------+---+

and with CDP, where the "CDP" field is 0 for the data sub-
partition and 1 for the code sub-partition:
  +-----------------------------------------------+---+-----------+
  |                     CLOSID                    |CDP|    RMID   |
  +-----------------------------------------------+---+-------+---+
  |                           PARTID                  :       |PMG|
  +-----------------------------------------------------------+---+

where each box represents a non-negative integer spanning some
fixed range starting from zero, and horizontal concatenation
denotes multiplying the value denoted by the left-hand box by the
span of the right-hand box, and adding the result to the value
denoted by the right-hand box; thus, most-significant fields are on
the left.

(Except for the span of each field not necessarily being a power of
two, this is conceptually identical to concatenation and cutting of
bitfields.)

The dotted segment indicates the least significant part of PARTID,
which spans partid_per_closid PARTIDs and discriminates monitoring
groups that use the same MPAM PMG value.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 drivers/platform/arm64/mpam/mpam_resctrl.c | 200 +++++++++++----------
 1 file changed, 110 insertions(+), 90 deletions(-)

diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index 30f2caec11d7..0473286ec65a 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -156,6 +156,18 @@ static bool mpam_resctrl_hide_cdp(enum resctrl_res_level rid)
 	return cdp_enabled && !resctrl_arch_get_cdp_enabled(rid);
 }
 
+static unsigned int partid_per_closid = 1;
+
+static unsigned int mpam_num_pmg(void)
+{
+	return mpam_pmg_max + 1;
+}
+
+static unsigned int mpam_num_rmid(void)
+{
+	return mpam_num_pmg() * partid_per_closid;
+}
+
 /*
  * MSC may raise an error interrupt if it sees an out or range partid/pmg,
  * and go on to truncate the value. Regardless of what the hardware supports,
@@ -163,35 +175,71 @@ static bool mpam_resctrl_hide_cdp(enum resctrl_res_level rid)
  */
 u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)
 {
-	return mpam_partid_max + 1;
+	u32 res = (mpam_partid_max + 1) / partid_per_closid;
+
+	WARN_ON(res < 1);
+	return res;
 }
 
-u32 resctrl_arch_system_num_rmid_idx(void)
+static void mpam_resctrl_partid_range(u32 closid, enum resctrl_conf_type type,
+				      const struct rdt_resource *r,
+				      u16 *min_partid, u16 *max_partid)
 {
-	u8 closid_shift = fls(mpam_pmg_max);
-	u32 num_partid = resctrl_arch_get_num_closid(NULL);
+	u16 base_partid = closid;
+	u16 span = 1;
+
+	if (cdp_enabled) {
+		base_partid *= 2;
+		if (mpam_resctrl_hide_cdp(r->rid) ||
+		    type == CDP_NONE)
+			span *= 2;
+	}
 
-	return num_partid << closid_shift;
+	*min_partid = base_partid * partid_per_closid;
+	if (max_partid)
+		*max_partid = *min_partid + (span * partid_per_closid - 1);
 }
 
-u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
+static void mpam_resctrl_hwid(u32 closid, u32 rmid,
+			      u16 *partid_d, u16 *partid_i, u8 *pmg)
 {
-	u8 closid_shift = fls(mpam_pmg_max);
+	const u16 pmg_hi = rmid / mpam_num_pmg();
+	const u16 pmg_lo = rmid % mpam_num_pmg();
+	u16 base_partid = closid;
+	u16 partid_stride = 0;
+
+	if (cdp_enabled) {
+		base_partid *= 2;
+		partid_stride = 1;
+	}
 
-	BUG_ON(closid_shift > 8);
+	partid_stride *= partid_per_closid;
+	base_partid *= partid_per_closid;
+	base_partid += pmg_hi;
 
-	return (closid << closid_shift) | rmid;
+	*partid_d = base_partid;
+	if (partid_i)
+		*partid_i = base_partid + partid_stride;
+
+	*pmg = pmg_lo;
 }
 
-void resctrl_arch_rmid_idx_decode(u32 idx, u32 *closid, u32 *rmid)
+u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
 {
-	u8 closid_shift = fls(mpam_pmg_max);
-	u32 pmg_mask = ~(~0 << closid_shift);
+	return closid * mpam_num_rmid() + rmid;
+}
 
-	BUG_ON(closid_shift > 8);
+u32 resctrl_arch_system_num_rmid_idx(void)
+{
+	u32 num_closid = resctrl_arch_get_num_closid(NULL);
 
-	*closid = idx >> closid_shift;
-	*rmid = idx & pmg_mask;
+	return resctrl_arch_rmid_idx_encode(num_closid, 0);
+}
+
+void resctrl_arch_rmid_idx_decode(u32 idx, u32 *closid, u32 *rmid)
+{
+	*closid = idx / mpam_num_rmid();
+	*rmid = idx % mpam_num_rmid();
 }
 
 void resctrl_arch_sched_in(struct task_struct *tsk)
@@ -203,21 +251,14 @@ void resctrl_arch_sched_in(struct task_struct *tsk)
 
 void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid, u32 rmid)
 {
+	u16 partid_d, partid_i;
+	u8 pmg;
+
 	BUG_ON(closid > U16_MAX);
 	BUG_ON(rmid > U8_MAX);
 
-	if (!cdp_enabled) {
-		mpam_set_cpu_defaults(cpu, closid, closid, rmid, rmid);
-	} else {
-		/*
-		 * When CDP is enabled, resctrl halves the closid range and we
-		 * use odd/even partid for one closid.
-		 */
-		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
-		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
-
-		mpam_set_cpu_defaults(cpu, partid_d, partid_i, rmid, rmid);
-	}
+	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &pmg);
+	mpam_set_cpu_defaults(cpu, partid_d, partid_i, pmg, pmg);
 }
 
 void resctrl_arch_sync_cpu_closid_rmid(void *info)
@@ -236,41 +277,38 @@ void resctrl_arch_sync_cpu_closid_rmid(void *info)
 
 void resctrl_arch_set_closid_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
 {
+	u16 partid_d, partid_i;
+	u8 pmg;
+
 	BUG_ON(closid > U16_MAX);
 	BUG_ON(rmid > U8_MAX);
 
-	if (!cdp_enabled) {
-		mpam_set_task_partid_pmg(tsk, closid, closid, rmid, rmid);
-	} else {
-		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
-		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
-
-		mpam_set_task_partid_pmg(tsk, partid_d, partid_i, rmid, rmid);
-	}
+	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &pmg);
+	mpam_set_task_partid_pmg(tsk, partid_d, partid_i, pmg, pmg);
 }
 
 bool resctrl_arch_match_closid(struct task_struct *tsk, u32 closid)
 {
+	u16 min_partid, max_partid;
 	u64 regval = mpam_get_regval(tsk);
 	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
 
-	if (cdp_enabled)
-		tsk_closid >>= 1;
-
-	return tsk_closid == closid;
+	mpam_resctrl_partid_range(closid, CDP_NONE, NULL,
+				  &min_partid, &max_partid);
+	return tsk_closid >= min_partid && tsk_closid <= max_partid;
 }
 
 /* The task's pmg is not unique, the partid must be considered too */
 bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
 {
+	u16 partid_d;
+	u8 pmg;
 	u64 regval = mpam_get_regval(tsk);
-	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
-	u32 tsk_rmid = FIELD_GET(MPAM1_EL1_PMG_D, regval);
-
-	if (cdp_enabled)
-		tsk_closid >>= 1;
+	u16 tsk_partid_d = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
+	u8 tsk_pmg = FIELD_GET(MPAM1_EL1_PMG_D, regval);
 
-	return (tsk_closid == closid) && (tsk_rmid == rmid);
+	mpam_resctrl_hwid(closid, rmid, &partid_d, NULL, &pmg);
+	return (tsk_partid_d == partid_d) && (tsk_pmg == pmg);
 }
 
 struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l)
@@ -370,6 +408,7 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r, struct rdt_mon_domain *d,
 	struct mpam_resctrl_dom *dom;
 	u32 mon = *(u32 *)arch_mon_ctx;
 	enum mpam_device_features type;
+	u16 partid_d, partid_i, pmg;
 
 	resctrl_arch_rmid_read_context_check();
 
@@ -391,27 +430,23 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r, struct rdt_mon_domain *d,
 	if (cfg.mon == USE_RMID_IDX)
 		cfg.mon = resctrl_arch_rmid_idx_encode(closid, rmid);
 
+	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &cfg.pmg);
 	cfg.match_pmg = true;
-	cfg.pmg = rmid;
+	cfg.pmg = pmg;
 	cfg.opts = resctrl_evt_config_to_mpam(dom->mbm_local_evt_cfg);
 
 	if (irqs_disabled()) {
 		/* Check if we can access this domain without an IPI */
 		err = -EIO;
 	} else {
-		if (cdp_enabled) {
-			cfg.partid = closid << 1;
-			err = mpam_msmon_read(dom->comp, &cfg, type, val);
-			if (err)
-				return err;
+		cfg.partid = partid_d;
+		err = mpam_msmon_read(dom->comp, &cfg, type, val);
 
-			cfg.partid += 1;
+		if (partid_i != partid_d) {
+			cfg.partid = partid_i;
 			err = mpam_msmon_read(dom->comp, &cfg, type, &cdp_val);
 			if (!err)
 				*val += cdp_val;
-		} else {
-			cfg.partid = closid;
-			err = mpam_msmon_read(dom->comp, &cfg, type, val);
 		}
 	}
 
@@ -423,6 +458,7 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
 {
 	struct mon_cfg cfg;
 	struct mpam_resctrl_dom *dom;
+	u16 partid_d, partid_i;
 
 	if (eventid != QOS_L3_MBM_LOCAL_EVENT_ID)
 		return;
@@ -433,14 +469,13 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
 
 	dom = container_of(d, struct mpam_resctrl_dom, resctrl_mon_dom);
 
-	if (cdp_enabled) {
-		cfg.partid = closid << 1;
-		mpam_msmon_reset_mbwu(dom->comp, &cfg);
+	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &cfg.pmg);
 
-		cfg.partid += 1;
-		mpam_msmon_reset_mbwu(dom->comp, &cfg);
-	} else {
-		cfg.partid = closid;
+	cfg.partid = partid_d;
+	mpam_msmon_reset_mbwu(dom->comp, &cfg);
+
+	if (partid_i != partid_d) {
+		cfg.partid = partid_i;
 		mpam_msmon_reset_mbwu(dom->comp, &cfg);
 	}
 }
@@ -1081,7 +1116,7 @@ static void mpam_resctrl_exit(void)
 u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 			    u32 closid, enum resctrl_conf_type type)
 {
-	u32 partid;
+	u16 partid;
 	struct mpam_config *cfg;
 	struct mpam_props *cprops;
 	struct mpam_resctrl_res *res;
@@ -1097,15 +1132,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 	dom = container_of(d, struct mpam_resctrl_dom, resctrl_ctrl_dom);
 	cprops = &res->class->props;
 
-	/*
-	 * When CDP is enabled, but the resource doesn't support it,
-	 * the control is cloned across both partids.
-	 * Pick one at random to read:
-	 */
-	if (mpam_resctrl_hide_cdp(r->rid))
-		type = CDP_DATA;
-
-	partid = resctrl_get_config_index(closid, type);
+	mpam_resctrl_partid_range(closid, type, r, &partid, NULL);
 	cfg = &dom->comp->cfg[partid];
 
 	switch (r->rid) {
@@ -1126,7 +1153,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 		return -EINVAL;
 	}
 
-	if (!r->alloc_capable || partid >= resctrl_arch_get_num_closid(r) ||
+	if (!r->alloc_capable || partid > mpam_partid_max ||
 	    !mpam_has_feature(configured_by, cfg))
 		return resctrl_get_default_ctrl(r);
 
@@ -1147,8 +1174,8 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 			    u32 closid, enum resctrl_conf_type t, u32 cfg_val)
 {
-	int err;
-	u32 partid;
+	int err = 0;
+	u16 partid, min_partid, max_partid;
 	struct mpam_config cfg;
 	struct mpam_props *cprops;
 	struct mpam_resctrl_res *res;
@@ -1163,8 +1190,10 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 	dom = container_of(d, struct mpam_resctrl_dom, resctrl_ctrl_dom);
 	cprops = &res->class->props;
 
-	partid = resctrl_get_config_index(closid, t);
-	if (!r->alloc_capable || partid >= resctrl_arch_get_num_closid(r))
+	mpam_resctrl_partid_range(closid, t, r, &min_partid, &max_partid);
+	if (!r->alloc_capable ||
+	    min_partid > mpam_partid_max ||
+	    max_partid > mpam_partid_max)
 		return -EINVAL;
 
 	cfg.features = 0;
@@ -1190,22 +1219,13 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 		return -EINVAL;
 	}
 
-	/*
-	 * When CDP is enabled, but the resource doesn't support it, we need to
-	 * apply the same configuration to the other partid.
-	 */
-	if (mpam_resctrl_hide_cdp(r->rid)) {
-		partid = resctrl_get_config_index(closid, CDP_CODE);
+	for (partid = min_partid; partid <= max_partid; partid++) {
 		err = mpam_apply_config(dom->comp, partid, &cfg);
 		if (err)
-			return err;
-
-		partid = resctrl_get_config_index(closid, CDP_DATA);
-		return mpam_apply_config(dom->comp, partid, &cfg);
-
-	} else {
-		return mpam_apply_config(dom->comp, partid, &cfg);
+			break;
 	}
+
+	return err;
 }
 
 /* TODO: this is IPI heavy */
-- 
2.34.1

[RFC PATCH 5/6] arm_mpam: [NFU] Rework ID remapping to use a kernel command-line argument

[Dave Martin]

Hacking the source is an acceptable interface for experimentation,
but not very convenient.

Instead, add a kernel command-line parameter
mpam_partid_per_closid=<n> to specify how many PARTIDs are used to
provide monitoring groups for each resctrl CLOSID.

This change is not intended for upstream.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 drivers/platform/arm64/mpam/mpam_resctrl.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index 0473286ec65a..870efbf39f6f 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -3,6 +3,9 @@
 
 #define pr_fmt(fmt) "mpam: resctrl: " fmt
 
+#undef KBUILD_MODNAME
+#define KBUILD_MODNAME "mpam"
+
 #include <linux/arm_mpam.h>
 #include <linux/cacheinfo.h>
 #include <linux/cpu.h>
@@ -10,6 +13,7 @@
 #include <linux/errno.h>
 #include <linux/limits.h>
 #include <linux/list.h>
+#include <linux/moduleparam.h>
 #include <linux/printk.h>
 #include <linux/rculist.h>
 #include <linux/resctrl.h>
@@ -158,6 +162,24 @@ static bool mpam_resctrl_hide_cdp(enum resctrl_res_level rid)
 
 static unsigned int partid_per_closid = 1;
 
+static int mpam_resctrl_partid_per_closid_set(const char *val,
+					      const struct kernel_param *kp)
+{
+	/*
+	 * 16 in an arbitrary maximum, sufficient for experimentation
+	 * but not ridiculously large:
+	 */
+	return param_set_uint_minmax(val, kp, 1, 16);
+}
+
+static const struct kernel_param_ops mpam_resctrl_partid_per_closid_ops  = {
+	.set = mpam_resctrl_partid_per_closid_set,
+	.get = param_get_uint,
+};
+
+device_param_cb(partid_per_closid, &mpam_resctrl_partid_per_closid_ops,
+		&partid_per_closid, 0444);
+
 static unsigned int mpam_num_pmg(void)
 {
 	return mpam_pmg_max + 1;
-- 
2.34.1

[RFC PATCH 6/6] arm_mpam: [NFU] Development diagnostics for MPAM ID assignments

[Dave Martin]

Purely for development purposes, add some printks to show what MPAM
IDs actually get assigned and configured.

This would otherwise be observable only though statistical
measurements of performance (or not observable at all).

This change is not intended for upstream.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/mpam.h              | 7 +++++++
 drivers/platform/arm64/mpam/mpam_devices.c | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/mpam.h
index df725d2d9d05..4ae2e635e261 100644
--- a/arch/arm64/include/asm/mpam.h
+++ b/arch/arm64/include/asm/mpam.h
@@ -66,6 +66,9 @@ static inline void mpam_set_cpu_defaults(int cpu, u16 partid_d, u16 partid_i,
 	default_val |= FIELD_PREP(MPAM1_EL1_PMG_I, pmg_i);
 
 	WRITE_ONCE(per_cpu(arm64_mpam_default, cpu), default_val);
+
+	pr_info("MPAM: CPU%d -> (D=%d:%d, I=%d:%d)\n",
+		cpu, partid_d, pmg_d, partid_i, pmg_i);
 }
 
 static inline void mpam_set_task_partid_pmg(struct task_struct *tsk,
@@ -81,6 +84,10 @@ static inline void mpam_set_task_partid_pmg(struct task_struct *tsk,
 	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, pmg_i);
 
 	WRITE_ONCE(task_thread_info(tsk)->mpam_partid_pmg, regval);
+
+	pr_info("MPAM: task %s[%d/%d] -> (D=%d:%d, I=%d:%d)\n",
+		current->comm, current->pid, current->tgid,
+		partid_d, pmg_d, partid_i, pmg_i);
 #endif
 }
 
diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index 41962dd1bb68..dd84c0aa50f6 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -3158,6 +3158,12 @@ int mpam_apply_config(struct mpam_component *comp, u16 partid,
 
 	mpam_extend_config(comp->class, cfg);
 
+	pr_info("mpam_apply_config(): comp %d partid %d cfg={0x%x: 0x%lx, 0x%lx, %u, %u}\n",
+		comp->comp_id, partid, cfg->features,
+		(unsigned long)cfg->cpbm,
+		(unsigned long)cfg->mbw_pbm,
+		cfg->mbw_min, cfg->mbw_max);
+
 	if (!mpam_update_config(&comp->cfg[partid], cfg))
 		return 0;
 
-- 
2.34.1

Re: [RFC PATCH 1/6] arm_mpam: Clean up config update checks in mpam_apply_config()

[Dave Martin]

Hi all,

On Thu, Dec 12, 2024 at 03:39:55PM +0000, Dave Martin wrote:
> In mpam_apply_config(), a simple memcmp() test is used to check
> whether the config passed by the caller is already installed or
> not.
> 
> This check will never find a match except (very occasionally) by
> accident, since the component's version of the struct contains
> things that the caller won't pass or doesn't know (such as the
> garbage collection record).  There might also be random padding.
> 
> This may result in MSCs being reprogrammed unnecessarily.
> 
> Instead, only compare fields that the caller specified.  If
> anything is present in the caller's config and doesn't match the
> installed config, paste it across.  If nothing was pasted across
> then the MSC reprogramming step is skipped (as the previous code
> attempted to do).
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> ---
> 
> There are comments in the code suggesting a change of this sort.
> I may or may not have gone in the right direction with this, and
> I have only tried to clean up the behaviour rather than optimising.
> 
> No attempt is made to skip unnecessary MSC register updates if the MSC
> reprogramming goes ahead.
> 
> NOT well tested, yet.
> ---

Oops, git rebase didn't spot that this patch had already been applied,
due to adjacent hunks inserted in the meantime from other patches.

This patch can be dropped from this series.

[...]

Cheers
---Dave

RE: [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID translation

[Shaopeng Tan (Fujitsu)]

Hello Dave,

> Currently, the MPAM driver uses the resctrl CLOSID directly as the MPAM
> Partition Identifier (PARTID; possibly modified to give different PARTIDs to
> code and data when Code/Data Partitioning / CDP is enabled), and uses the
> resctrl RMID directly as the MPAM Performance Monitoring Group identifier
> (PMG).
> 
> In preparation for using the MPAM PARTID Narrowing feature to allow more
> resctrl monitoring groups to be provided than the number of PMG values
> supported by the hardware, a more flexible remapping scheme is needed.
> 
> Factor out ID translation operations in the MPAM resctrl glue code into a couple
> of self-contained helpers, and call them as appropriate.
> 
> The translation scheme has a single parameter, partid_per_closid, which is
> currently hard-wired to 1.
> 
> As a result, this patch should have no effect on functionality.
> 
> Logic to determine / control the mapping parameters can be introduced later.
> 
> The ID transformation may be visualised as follows:
> 
> without CDP:
>   +---------------------------------------------------+-----------+
>   |                       CLOSID                      |    RMID   |
>   +---------------------------------------------------+-------+---+
>   |                           PARTID                  :       |PMG|
>   +-----------------------------------------------------------+---+
> 
> and with CDP, where the "CDP" field is 0 for the data sub- partition and 1 for
> the code sub-partition:
>   +-----------------------------------------------+---+-----------+
>   |                     CLOSID                    |CDP|    RMID   |
>   +-----------------------------------------------+---+-------+---+
>   |                           PARTID                  :       |PMG|
>   +-----------------------------------------------------------+---+
> 
> where each box represents a non-negative integer spanning some fixed range
> starting from zero, and horizontal concatenation denotes multiplying the value
> denoted by the left-hand box by the span of the right-hand box, and adding the
> result to the value denoted by the right-hand box; thus, most-significant fields
> are on the left.
> 
> (Except for the span of each field not necessarily being a power of two, this is
> conceptually identical to concatenation and cutting of
> bitfields.)
> 
> The dotted segment indicates the least significant part of PARTID, which spans
> partid_per_closid PARTIDs and discriminates monitoring groups that use the
> same MPAM PMG value.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  drivers/platform/arm64/mpam/mpam_resctrl.c | 200
> +++++++++++----------
>  1 file changed, 110 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c
> b/drivers/platform/arm64/mpam/mpam_resctrl.c
> index 30f2caec11d7..0473286ec65a 100644
> --- a/drivers/platform/arm64/mpam/mpam_resctrl.c
> +++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
> @@ -156,6 +156,18 @@ static bool mpam_resctrl_hide_cdp(enum
> resctrl_res_level rid)
>  	return cdp_enabled && !resctrl_arch_get_cdp_enabled(rid);
>  }
> 
> +static unsigned int partid_per_closid = 1;
> +
> +static unsigned int mpam_num_pmg(void)
> +{
> +	return mpam_pmg_max + 1;
> +}
> +
> +static unsigned int mpam_num_rmid(void) {
> +	return mpam_num_pmg() * partid_per_closid; }
> +
>  /*
>   * MSC may raise an error interrupt if it sees an out or range partid/pmg,
>   * and go on to truncate the value. Regardless of what the hardware supports,
> @@ -163,35 +175,71 @@ static bool mpam_resctrl_hide_cdp(enum
> resctrl_res_level rid)
>   */
>  u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)  {
> -	return mpam_partid_max + 1;
> +	u32 res = (mpam_partid_max + 1) / partid_per_closid;

If there is a remainder, are you going to ignore the remainder PARTIDs?
It might be better to notify the user with a warning message.

> +	WARN_ON(res < 1);
> +	return res;
>  }

I think it is better to check whether PARTID narrowing feature is supported or not before using this function. 
If it is checked here, since this function is called multiple times, the message should be printed only once by WARN_ON_ONCE(). 

Also, if the partid_per_closid is greater than (mpam_partid_max + 1), returning "0" will not work properly. 
To switch to default behavior (PARTID narrowing disabled), should it return mapm_patid_max+ 1)?

> -u32 resctrl_arch_system_num_rmid_idx(void)
> +static void mpam_resctrl_partid_range(u32 closid, enum resctrl_conf_type
> type,
> +				      const struct rdt_resource *r,
> +				      u16 *min_partid, u16 *max_partid)
>  {
> -	u8 closid_shift = fls(mpam_pmg_max);
> -	u32 num_partid = resctrl_arch_get_num_closid(NULL);
> +	u16 base_partid = closid;
> +	u16 span = 1;
> +
> +	if (cdp_enabled) {
> +		base_partid *= 2;
> +		if (mpam_resctrl_hide_cdp(r->rid) ||
> +		    type == CDP_NONE)
> +			span *= 2;
> +	}
> 
> -	return num_partid << closid_shift;
> +	*min_partid = base_partid * partid_per_closid;
> +	if (max_partid)
> +		*max_partid = *min_partid + (span * partid_per_closid - 1);
>  }

I think the min_partid/max_partid is different depending on the type (CDP_DATA or CDP_CODE).

> -u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
> +static void mpam_resctrl_hwid(u32 closid, u32 rmid,
> +			      u16 *partid_d, u16 *partid_i, u8 *pmg)
>  {
> -	u8 closid_shift = fls(mpam_pmg_max);
> +	const u16 pmg_hi = rmid / mpam_num_pmg();
> +	const u16 pmg_lo = rmid % mpam_num_pmg();
> +	u16 base_partid = closid;
> +	u16 partid_stride = 0;
> +
> +	if (cdp_enabled) {
> +		base_partid *= 2;
> +		partid_stride = 1;
> +	}
> 
> -	BUG_ON(closid_shift > 8);
> +	partid_stride *= partid_per_closid;
> +	base_partid *= partid_per_closid;
> +	base_partid += pmg_hi;
> 
> -	return (closid << closid_shift) | rmid;
> +	*partid_d = base_partid;
> +	if (partid_i)
> +		*partid_i = base_partid + partid_stride;
> +
> +	*pmg = pmg_lo;
>  }
> 
> -void resctrl_arch_rmid_idx_decode(u32 idx, u32 *closid, u32 *rmid)
> +u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
>  {
> -	u8 closid_shift = fls(mpam_pmg_max);
> -	u32 pmg_mask = ~(~0 << closid_shift);
> +	return closid * mpam_num_rmid() + rmid; }
> 
> -	BUG_ON(closid_shift > 8);
> +u32 resctrl_arch_system_num_rmid_idx(void)
> +{
> +	u32 num_closid = resctrl_arch_get_num_closid(NULL);
> 
> -	*closid = idx >> closid_shift;
> -	*rmid = idx & pmg_mask;
> +	return resctrl_arch_rmid_idx_encode(num_closid, 0); }
> +
> +void resctrl_arch_rmid_idx_decode(u32 idx, u32 *closid, u32 *rmid) {
> +	*closid = idx / mpam_num_rmid();
> +	*rmid = idx % mpam_num_rmid();
>  }
> 
>  void resctrl_arch_sched_in(struct task_struct *tsk) @@ -203,21 +251,14 @@
> void resctrl_arch_sched_in(struct task_struct *tsk)
> 
>  void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid, u32 rmid)
> {
> +	u16 partid_d, partid_i;
> +	u8 pmg;
> +
>  	BUG_ON(closid > U16_MAX);
>  	BUG_ON(rmid > U8_MAX);
> 
> -	if (!cdp_enabled) {
> -		mpam_set_cpu_defaults(cpu, closid, closid, rmid, rmid);
> -	} else {
> -		/*
> -		 * When CDP is enabled, resctrl halves the closid range and
> we
> -		 * use odd/even partid for one closid.
> -		 */
> -		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
> -		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
> -
> -		mpam_set_cpu_defaults(cpu, partid_d, partid_i, rmid, rmid);
> -	}
> +	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &pmg);
> +	mpam_set_cpu_defaults(cpu, partid_d, partid_i, pmg, pmg);
>  }
> 
>  void resctrl_arch_sync_cpu_closid_rmid(void *info) @@ -236,41 +277,38 @@
> void resctrl_arch_sync_cpu_closid_rmid(void *info)
> 
>  void resctrl_arch_set_closid_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
> {
> +	u16 partid_d, partid_i;
> +	u8 pmg;
> +
>  	BUG_ON(closid > U16_MAX);
>  	BUG_ON(rmid > U8_MAX);
> 
> -	if (!cdp_enabled) {
> -		mpam_set_task_partid_pmg(tsk, closid, closid, rmid, rmid);
> -	} else {
> -		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
> -		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
> -
> -		mpam_set_task_partid_pmg(tsk, partid_d, partid_i, rmid,
> rmid);
> -	}
> +	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &pmg);
> +	mpam_set_task_partid_pmg(tsk, partid_d, partid_i, pmg, pmg);
>  }
> 
>  bool resctrl_arch_match_closid(struct task_struct *tsk, u32 closid)  {
> +	u16 min_partid, max_partid;
>  	u64 regval = mpam_get_regval(tsk);
>  	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
> 
> -	if (cdp_enabled)
> -		tsk_closid >>= 1;
> -
> -	return tsk_closid == closid;
> +	mpam_resctrl_partid_range(closid, CDP_NONE, NULL,
> +				  &min_partid, &max_partid);
> +	return tsk_closid >= min_partid && tsk_closid <= max_partid;
>  }
> 
>  /* The task's pmg is not unique, the partid must be considered too */  bool
> resctrl_arch_match_rmid(struct task_struct *tsk, u32 closid, u32 rmid)  {
> +	u16 partid_d;
> +	u8 pmg;
>  	u64 regval = mpam_get_regval(tsk);
> -	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
> -	u32 tsk_rmid = FIELD_GET(MPAM1_EL1_PMG_D, regval);
> -
> -	if (cdp_enabled)
> -		tsk_closid >>= 1;
> +	u16 tsk_partid_d = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
> +	u8 tsk_pmg = FIELD_GET(MPAM1_EL1_PMG_D, regval);
> 
> -	return (tsk_closid == closid) && (tsk_rmid == rmid);
> +	mpam_resctrl_hwid(closid, rmid, &partid_d, NULL, &pmg);
> +	return (tsk_partid_d == partid_d) && (tsk_pmg == pmg);
>  }
> 
>  struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l)
> @@ -370,6 +408,7 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r,
> struct rdt_mon_domain *d,
>  	struct mpam_resctrl_dom *dom;
>  	u32 mon = *(u32 *)arch_mon_ctx;
>  	enum mpam_device_features type;
> +	u16 partid_d, partid_i, pmg;
> 
>  	resctrl_arch_rmid_read_context_check();
> 
> @@ -391,27 +430,23 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r,
> struct rdt_mon_domain *d,
>  	if (cfg.mon == USE_RMID_IDX)
>  		cfg.mon = resctrl_arch_rmid_idx_encode(closid, rmid);
> 
> +	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &cfg.pmg);
>  	cfg.match_pmg = true;
> -	cfg.pmg = rmid;
> +	cfg.pmg = pmg;
>  	cfg.opts = resctrl_evt_config_to_mpam(dom->mbm_local_evt_cfg);
> 
>  	if (irqs_disabled()) {
>  		/* Check if we can access this domain without an IPI */
>  		err = -EIO;
>  	} else {
> -		if (cdp_enabled) {
> -			cfg.partid = closid << 1;
> -			err = mpam_msmon_read(dom->comp, &cfg, type,
> val);
> -			if (err)
> -				return err;
> +		cfg.partid = partid_d;
> +		err = mpam_msmon_read(dom->comp, &cfg, type, val);
> 
> -			cfg.partid += 1;
> +		if (partid_i != partid_d) {
> +			cfg.partid = partid_i;
>  			err = mpam_msmon_read(dom->comp, &cfg, type,
> &cdp_val);
>  			if (!err)
>  				*val += cdp_val;
> -		} else {
> -			cfg.partid = closid;
> -			err = mpam_msmon_read(dom->comp, &cfg, type,
> val);
>  		}
>  	}
> 
> @@ -423,6 +458,7 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r,
> struct rdt_mon_domain *d,  {
>  	struct mon_cfg cfg;
>  	struct mpam_resctrl_dom *dom;
> +	u16 partid_d, partid_i;
> 
>  	if (eventid != QOS_L3_MBM_LOCAL_EVENT_ID)
>  		return;
> @@ -433,14 +469,13 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r,
> struct rdt_mon_domain *d,
> 
>  	dom = container_of(d, struct mpam_resctrl_dom, resctrl_mon_dom);
> 
> -	if (cdp_enabled) {
> -		cfg.partid = closid << 1;
> -		mpam_msmon_reset_mbwu(dom->comp, &cfg);
> +	mpam_resctrl_hwid(closid, rmid, &partid_d, &partid_i, &cfg.pmg);
> 
> -		cfg.partid += 1;
> -		mpam_msmon_reset_mbwu(dom->comp, &cfg);
> -	} else {
> -		cfg.partid = closid;
> +	cfg.partid = partid_d;
> +	mpam_msmon_reset_mbwu(dom->comp, &cfg);
> +
> +	if (partid_i != partid_d) {
> +		cfg.partid = partid_i;
>  		mpam_msmon_reset_mbwu(dom->comp, &cfg);
>  	}
>  }
> @@ -1081,7 +1116,7 @@ static void mpam_resctrl_exit(void)
>  u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
>  			    u32 closid, enum resctrl_conf_type type)  {
> -	u32 partid;
> +	u16 partid;
>  	struct mpam_config *cfg;
>  	struct mpam_props *cprops;
>  	struct mpam_resctrl_res *res;
> @@ -1097,15 +1132,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r,
> struct rdt_ctrl_domain *d,
>  	dom = container_of(d, struct mpam_resctrl_dom, resctrl_ctrl_dom);
>  	cprops = &res->class->props;
> 
> -	/*
> -	 * When CDP is enabled, but the resource doesn't support it,
> -	 * the control is cloned across both partids.
> -	 * Pick one at random to read:
> -	 */
> -	if (mpam_resctrl_hide_cdp(r->rid))
> -		type = CDP_DATA;
> -
> -	partid = resctrl_get_config_index(closid, type);
> +	mpam_resctrl_partid_range(closid, type, r, &partid, NULL);
>  	cfg = &dom->comp->cfg[partid];
> 
>  	switch (r->rid) {
> @@ -1126,7 +1153,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r,
> struct rdt_ctrl_domain *d,
>  		return -EINVAL;
>  	}
> 
> -	if (!r->alloc_capable || partid >= resctrl_arch_get_num_closid(r) ||
> +	if (!r->alloc_capable || partid > mpam_partid_max ||
>  	    !mpam_has_feature(configured_by, cfg))
>  		return resctrl_get_default_ctrl(r);
> 
> @@ -1147,8 +1174,8 @@ u32 resctrl_arch_get_config(struct rdt_resource *r,
> struct rdt_ctrl_domain *d,  int resctrl_arch_update_one(struct rdt_resource *r,
> struct rdt_ctrl_domain *d,
>  			    u32 closid, enum resctrl_conf_type t, u32 cfg_val)
> {
> -	int err;
> -	u32 partid;
> +	int err = 0;
> +	u16 partid, min_partid, max_partid;
>  	struct mpam_config cfg;
>  	struct mpam_props *cprops;
>  	struct mpam_resctrl_res *res;
> @@ -1163,8 +1190,10 @@ int resctrl_arch_update_one(struct rdt_resource *r,
> struct rdt_ctrl_domain *d,
>  	dom = container_of(d, struct mpam_resctrl_dom, resctrl_ctrl_dom);
>  	cprops = &res->class->props;
> 
> -	partid = resctrl_get_config_index(closid, t);
> -	if (!r->alloc_capable || partid >= resctrl_arch_get_num_closid(r))
> +	mpam_resctrl_partid_range(closid, t, r, &min_partid, &max_partid);
> +	if (!r->alloc_capable ||
> +	    min_partid > mpam_partid_max ||
> +	    max_partid > mpam_partid_max)
>  		return -EINVAL;
> 
>  	cfg.features = 0;
> @@ -1190,22 +1219,13 @@ int resctrl_arch_update_one(struct rdt_resource *r,
> struct rdt_ctrl_domain *d,
>  		return -EINVAL;
>  	}
> 
> -	/*
> -	 * When CDP is enabled, but the resource doesn't support it, we need
> to
> -	 * apply the same configuration to the other partid.
> -	 */
> -	if (mpam_resctrl_hide_cdp(r->rid)) {
> -		partid = resctrl_get_config_index(closid, CDP_CODE);
> +	for (partid = min_partid; partid <= max_partid; partid++) {
>  		err = mpam_apply_config(dom->comp, partid, &cfg);
>  		if (err)
> -			return err;
> -
> -		partid = resctrl_get_config_index(closid, CDP_DATA);
> -		return mpam_apply_config(dom->comp, partid, &cfg);
> -
> -	} else {
> -		return mpam_apply_config(dom->comp, partid, &cfg);
> +			break;
>  	}
> +
> +	return err;
>  }

For a mixture of MSCs system, MSCs that do not support PARTID narrowing and support Maximum Partition feature may not work properly.

>  /* TODO: this is IPI heavy */
> --
> 2.34.1

I know this is not a complete implementation.
Mapping relationship between intPARTIDs and reqPARTIDs need to be written to register MPAMCFG_INTPARTID/MPAMCFG_PART_SEL.

Best regards,
Shaopeng TAN

Re: [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID translation

[Dave Martin]

Hi,

On Fri, Dec 20, 2024 at 05:01:30AM +0000, Shaopeng Tan (Fujitsu) wrote:
> Hello Dave,

[...]

> > diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > b/drivers/platform/arm64/mpam/mpam_resctrl.c
> > index 30f2caec11d7..0473286ec65a 100644
> > --- a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > +++ b/drivers/platform/arm64/mpam/mpam_resctrl.c

[...]

> > @@ -163,35 +175,71 @@ static bool mpam_resctrl_hide_cdp(enum
> > resctrl_res_level rid)
> >   */
> >  u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)  {
> > -	return mpam_partid_max + 1;
> > +	u32 res = (mpam_partid_max + 1) / partid_per_closid;
> 
> If there is a remainder, are you going to ignore the remainder PARTIDs?
> It might be better to notify the user with a warning message.

You are right: the remainder is just thrown away, because the remaining
PARTIDs would not be sufficient for a whole resctrl control group.

It would be a good idea to print a warning somewhere though; I will try
to add that.

[...]

> I think it is better to check whether PARTID narrowing feature is
> supported or not before using this function. 
> If it is checked here, since this function is called multiple times,
> the message should be printed only once by WARN_ON_ONCE(). 
> 
> Also, if the partid_per_closid is greater than (mpam_partid_max + 1),
> returning "0" will not work properly.
> To switch to default behavior (PARTID narrowing disabled), should it
> return mapm_patid_max+ 1)?

The idea is that partid_per_closid is set by some configuration logic
that is not implemented yet.  This will be a bit more complicated than
just exposing this variable directly as a kernel parameter.

Currently I just implemented a basic range check to prevent the kernel
parameter being set to stupid values, but I didn't try to make it
completely robust.


In any case, since partid_per_closid defaults to 1, the behavior of
this function should just match the non-PARTID-Narrowing behaviour
unless the user requested a different value.

Bearing that in mind, I think that a "bad" value of partid_per_closid
should not be seen here, when the rest of the implementation is
complete.

Does this sound sensible?

It could make sense to have a WARN_ON_ONCE() here anyway, though,
since this code is not on a fast path.


> > -u32 resctrl_arch_system_num_rmid_idx(void)
> > +static void mpam_resctrl_partid_range(u32 closid, enum resctrl_conf_type
> > type,
> > +				      const struct rdt_resource *r,
> > +				      u16 *min_partid, u16 *max_partid)
> >  {
> > -	u8 closid_shift = fls(mpam_pmg_max);
> > -	u32 num_partid = resctrl_arch_get_num_closid(NULL);
> > +	u16 base_partid = closid;
> > +	u16 span = 1;
> > +
> > +	if (cdp_enabled) {
> > +		base_partid *= 2;
> > +		if (mpam_resctrl_hide_cdp(r->rid) ||
> > +		    type == CDP_NONE)
> > +			span *= 2;
> > +	}
> > 
> > -	return num_partid << closid_shift;
> > +	*min_partid = base_partid * partid_per_closid;
> > +	if (max_partid)
> > +		*max_partid = *min_partid + (span * partid_per_closid - 1);
> >  }
> 
> I think the min_partid/max_partid is different depending on the type
> (CDP_DATA or CDP_CODE).

You're right!  Looking at my fixup patches from the end of December,
it looks like I already encountered this problem and wrote a
workaround, but I didn't post it at the time.

[...]

> > @@ -1190,22 +1219,13 @@ int resctrl_arch_update_one(struct rdt_resource *r,
> > struct rdt_ctrl_domain *d,
> >  		return -EINVAL;
> >  	}
> > 
> > -	/*
> > -	 * When CDP is enabled, but the resource doesn't support it, we need
> > to
> > -	 * apply the same configuration to the other partid.
> > -	 */
> > -	if (mpam_resctrl_hide_cdp(r->rid)) {
> > -		partid = resctrl_get_config_index(closid, CDP_CODE);
> > +	for (partid = min_partid; partid <= max_partid; partid++) {
> >  		err = mpam_apply_config(dom->comp, partid, &cfg);
> >  		if (err)
> > -			return err;
> > -
> > -		partid = resctrl_get_config_index(closid, CDP_DATA);
> > -		return mpam_apply_config(dom->comp, partid, &cfg);
> > -
> > -	} else {
> > -		return mpam_apply_config(dom->comp, partid, &cfg);
> > +			break;
> >  	}
> > +
> > +	return err;
> >  }
> 
> For a mixture of MSCs system, MSCs that do not support PARTID
> narrowing and support Maximum Partition feature may not work
> properly.

This is true.

For this series, we just trust that the user understands what they are
doing, but the complete implementation will need additional checks.

My plan was to do the appropriate checks when the driver starts up,
to decide whether it is safe to set partid_per_closid to a value
different from 1.

Once we get to this code, the decision has already been taken and so
the value of partid_per_closid (and the resulting mapping of resctrl
groups onto PARTIDs) can be trusted as being appropriate for the
hardware.

Can you see any problems with this approach?


[...]

> I know this is not a complete implementation.
> Mapping relationship between intPARTIDs and reqPARTIDs need to be
> written to register MPAMCFG_INTPARTID/MPAMCFG_PART_SEL.
> 
> Best regards,
> Shaopeng TAN

Agreed.  I have some prototype code for that, but I need to split up
the patches and clean it up before posting it.

Thanks for the review.

Cheers
---Dave

RE: [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID translation

[Shaopeng Tan (Fujitsu)]

Hello Dave,

> Hi,
> 
> On Fri, Dec 20, 2024 at 05:01:30AM +0000, Shaopeng Tan (Fujitsu) wrote:
> > Hello Dave,
> 
> [...]
> 
> > > diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > > b/drivers/platform/arm64/mpam/mpam_resctrl.c
> > > index 30f2caec11d7..0473286ec65a 100644
> > > --- a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > > +++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
> 
> [...]
> 
> > > @@ -163,35 +175,71 @@ static bool mpam_resctrl_hide_cdp(enum
> > > resctrl_res_level rid)
> > >   */
> > >  u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)  {
> > > -	return mpam_partid_max + 1;
> > > +	u32 res = (mpam_partid_max + 1) / partid_per_closid;
> >
> > If there is a remainder, are you going to ignore the remainder PARTIDs?
> > It might be better to notify the user with a warning message.
> 
> You are right: the remainder is just thrown away, because the remaining
> PARTIDs would not be sufficient for a whole resctrl control group.
> 
> It would be a good idea to print a warning somewhere though; I will try to add
> that.
> 
> [...]
> 
> > I think it is better to check whether PARTID narrowing feature is
> > supported or not before using this function.
> > If it is checked here, since this function is called multiple times,
> > the message should be printed only once by WARN_ON_ONCE().
> >
> > Also, if the partid_per_closid is greater than (mpam_partid_max + 1),
> > returning "0" will not work properly.
> > To switch to default behavior (PARTID narrowing disabled), should it
> > return mapm_patid_max+ 1)?
> 
> The idea is that partid_per_closid is set by some configuration logic that is not
> implemented yet.  This will be a bit more complicated than just exposing this
> variable directly as a kernel parameter.
> 
> Currently I just implemented a basic range check to prevent the kernel
> parameter being set to stupid values, but I didn't try to make it completely
> robust.
> 
> 
> In any case, since partid_per_closid defaults to 1, the behavior of this function
> should just match the non-PARTID-Narrowing behaviour unless the user
> requested a different value.
> 
> Bearing that in mind, I think that a "bad" value of partid_per_closid should not
> be seen here, when the rest of the implementation is complete.
> 
> Does this sound sensible?
> 
> It could make sense to have a WARN_ON_ONCE() here anyway, though, since
> this code is not on a fast path.

It sounds sensible.

> > > -u32 resctrl_arch_system_num_rmid_idx(void)
> > > +static void mpam_resctrl_partid_range(u32 closid, enum
> > > +resctrl_conf_type
> > > type,
> > > +				      const struct rdt_resource *r,
> > > +				      u16 *min_partid, u16 *max_partid)
> > >  {
> > > -	u8 closid_shift = fls(mpam_pmg_max);
> > > -	u32 num_partid = resctrl_arch_get_num_closid(NULL);
> > > +	u16 base_partid = closid;
> > > +	u16 span = 1;
> > > +
> > > +	if (cdp_enabled) {
> > > +		base_partid *= 2;
> > > +		if (mpam_resctrl_hide_cdp(r->rid) ||
> > > +		    type == CDP_NONE)
> > > +			span *= 2;
> > > +	}
> > >
> > > -	return num_partid << closid_shift;
> > > +	*min_partid = base_partid * partid_per_closid;
> > > +	if (max_partid)
> > > +		*max_partid = *min_partid + (span * partid_per_closid - 1);
> > >  }
> >
> > I think the min_partid/max_partid is different depending on the type
> > (CDP_DATA or CDP_CODE).
> 
> You're right!  Looking at my fixup patches from the end of December, it looks
> like I already encountered this problem and wrote a workaround, but I didn't
> post it at the time.
> 
> [...]
> 
> > > @@ -1190,22 +1219,13 @@ int resctrl_arch_update_one(struct
> > > rdt_resource *r, struct rdt_ctrl_domain *d,
> > >  		return -EINVAL;
> > >  	}
> > >
> > > -	/*
> > > -	 * When CDP is enabled, but the resource doesn't support it, we need
> > > to
> > > -	 * apply the same configuration to the other partid.
> > > -	 */
> > > -	if (mpam_resctrl_hide_cdp(r->rid)) {
> > > -		partid = resctrl_get_config_index(closid, CDP_CODE);
> > > +	for (partid = min_partid; partid <= max_partid; partid++) {
> > >  		err = mpam_apply_config(dom->comp, partid, &cfg);
> > >  		if (err)
> > > -			return err;
> > > -
> > > -		partid = resctrl_get_config_index(closid, CDP_DATA);
> > > -		return mpam_apply_config(dom->comp, partid, &cfg);
> > > -
> > > -	} else {
> > > -		return mpam_apply_config(dom->comp, partid, &cfg);
> > > +			break;
> > >  	}
> > > +
> > > +	return err;
> > >  }
> >
> > For a mixture of MSCs system, MSCs that do not support PARTID
> > narrowing and support Maximum Partition feature may not work properly.
> 
> This is true.
> 
> For this series, we just trust that the user understands what they are doing, but
> the complete implementation will need additional checks.
> 
> My plan was to do the appropriate checks when the driver starts up, to decide
> whether it is safe to set partid_per_closid to a value different from 1.
> 
> Once we get to this code, the decision has already been taken and so the value
> of partid_per_closid (and the resulting mapping of resctrl groups onto PARTIDs)
> can be trusted as being appropriate for the hardware.
> 
> Can you see any problems with this approach?

This approach sounds sensible.

> 
> [...]
> 
> > I know this is not a complete implementation.
> > Mapping relationship between intPARTIDs and reqPARTIDs need to be
> > written to register MPAMCFG_INTPARTID/MPAMCFG_PART_SEL.
> >
> > Best regards,
> > Shaopeng TAN
> 
> Agreed.  I have some prototype code for that, but I need to split up the patches
> and clean it up before posting it.
> 
> Thanks for the review.

I will review the rest of the prototype code when it is posted.
 
Best regards,
Shaopeng TAN