[PATCH v1 00/11] arm_mpam: minor fixes at v7.2

[PATCH v1 00/11] arm_mpam: minor fixes at v7.2

[Ben Horgan]

This is a bunch of small fixes for hypothetical problems in the driver. They
were found by a mixture of messing around with a software model, inspection and
llm review.

The first two patches ensure the error interrupt isn't accidentally disabled
when register state is lost. Patch 3 keeps the feature accounting correct but
has no functional change. Patches 4 to 7 fix mbwu save/restore but this is not
relied on as we plan to just use resctrl assigned counters and these are
unassigned when a domain goes offline. Also, MSC afffinity of a region smaller
than a resctrl domain has not been seen. Patch 8 makes read_mon_cdp_safe()
behaviour the same for cdp_enabled and not. Patch 9 fixes an infinite loop in
a system using all the PARTIDs which is much bigger than anything than anything
I've seen. Patch 10 stops some allocations failing on similiarly big systems.

Based on v7.2-rc2

Ben Horgan (11):
  arm_mpam: Move MPAMF_ECR write helpers to allow reuse
  arm_mpam: Restore the error interrupt enable from mpam_cpu_online()
  arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth
    counters
  arm_mpam: Add missing mon_sel locking in MBWU restore
  arm_mpam: Ensure MBWU counters are reset on restore
  arm_mpam: Use __ris_msmon_read() for saving MBWU state
  arm_mpam: Initialize all of struct mon_read in
    mpam_restore_mbwu_state()
  arm_mpam: resctrl: Correct check that existing class is L3
  arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent
  arm_mpam: Don't loop forever if there is the maximum possible amount
    of PARTIDs
  arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg

 drivers/resctrl/mpam_devices.c | 124 ++++++++++++++++++++-------------
 drivers/resctrl/mpam_resctrl.c |   4 +-
 2 files changed, 76 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH v1 01/11] arm_mpam: Move MPAMF_ECR write helpers to allow reuse

[Ben Horgan]

In preparation for calling mpam_enable_msc_ecr() from the CPU hotplug
handlers move it higher in the file. Move mpam_disable_msc_ecr() too to
keep them together.

Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 38 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index b69f99488111..82966ca2a631 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1813,6 +1813,25 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
 	mutex_unlock(&msc->cfg_lock);
 }
 
+static int mpam_enable_msc_ecr(void *_msc)
+{
+	struct mpam_msc *msc = _msc;
+
+	__mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
+
+	return 0;
+}
+
+/* This can run in mpam_disable(), and the interrupt handler on the same CPU */
+static int mpam_disable_msc_ecr(void *_msc)
+{
+	struct mpam_msc *msc = _msc;
+
+	__mpam_write_reg(msc, MPAMF_ECR, 0);
+
+	return 0;
+}
+
 static void _enable_percpu_irq(void *_irq)
 {
 	int *irq = _irq;
@@ -2435,25 +2454,6 @@ static char *mpam_errcode_names[16] = {
 	[12 ... 15] = "Reserved"
 };
 
-static int mpam_enable_msc_ecr(void *_msc)
-{
-	struct mpam_msc *msc = _msc;
-
-	__mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
-
-	return 0;
-}
-
-/* This can run in mpam_disable(), and the interrupt handler on the same CPU */
-static int mpam_disable_msc_ecr(void *_msc)
-{
-	struct mpam_msc *msc = _msc;
-
-	__mpam_write_reg(msc, MPAMF_ECR, 0);
-
-	return 0;
-}
-
 static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
 {
 	u64 reg;
-- 
2.43.0

[PATCH v1 02/11] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Ben Horgan]

When all CPUs affine to an MSC are offline the MSC may lose register state
which the driver then restores when an affine CPU comes back online. The
error interrupt enable, MPAMF_ECR.INTEN, is missed.

Restore MPAMF_ECR at CPU online.

Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 82966ca2a631..acfa9a4dc2fc 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1852,8 +1852,14 @@ static int mpam_cpu_online(unsigned int cpu)
 		if (msc->reenable_error_ppi)
 			_enable_percpu_irq(&msc->reenable_error_ppi);
 
-		if (atomic_fetch_inc(&msc->online_refs) == 0)
+		if (atomic_fetch_inc(&msc->online_refs) == 0) {
+			mutex_lock(&msc->error_irq_lock);
+			if (msc->error_irq_hw_enabled)
+				mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
+			mutex_unlock(&msc->error_irq_lock);
+
 			mpam_reprogram_msc(msc);
+		}
 	}
 
 	if (mpam_resctrl_enabled)
-- 
2.43.0

Re: [PATCH v1 02/11] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Gavin Shan]

Hi Ben,

On 7/10/26 9:55 PM, Ben Horgan wrote:
> When all CPUs affine to an MSC are offline the MSC may lose register state
> which the driver then restores when an affine CPU comes back online. The
> error interrupt enable, MPAMF_ECR.INTEN, is missed.
> 
> Restore MPAMF_ECR at CPU online.
> 
> Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 82966ca2a631..acfa9a4dc2fc 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1852,8 +1852,14 @@ static int mpam_cpu_online(unsigned int cpu)
>   		if (msc->reenable_error_ppi)
>   			_enable_percpu_irq(&msc->reenable_error_ppi);
>   
> -		if (atomic_fetch_inc(&msc->online_refs) == 0)
> +		if (atomic_fetch_inc(&msc->online_refs) == 0) {
> +			mutex_lock(&msc->error_irq_lock);
> +			if (msc->error_irq_hw_enabled)
> +				mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
> +			mutex_unlock(&msc->error_irq_lock);
> +
>   			mpam_reprogram_msc(msc);
> +		}
>   	}
>   

I don't understand how this happened that MPAMF_ECR gets lost, but msc->error_irq_hw_enabled
is kept as true. mpam_disable_msc_ecr() is triggered in __mpam_irq_handler() or mpam_unregister_irqs().
Both are related to mpam_broken_work, which is invoked to disable the driver completely.
I don't see how MPAMF_ECR is cleared at CPU offline time.

>   	if (mpam_resctrl_enabled)

Thanks,
Gavin

Re: [PATCH v1 02/11] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Ben Horgan]

Hi Gavin,

On 7/16/26 04:59, Gavin Shan wrote:
> Hi Ben,
> 
> On 7/10/26 9:55 PM, Ben Horgan wrote:
>> When all CPUs affine to an MSC are offline the MSC may lose register state
>> which the driver then restores when an affine CPU comes back online. The
>> error interrupt enable, MPAMF_ECR.INTEN, is missed.
>>
>> Restore MPAMF_ECR at CPU online.
>>
>> Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 82966ca2a631..acfa9a4dc2fc 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1852,8 +1852,14 @@ static int mpam_cpu_online(unsigned int cpu)
>>           if (msc->reenable_error_ppi)
>>               _enable_percpu_irq(&msc->reenable_error_ppi);
>>   -        if (atomic_fetch_inc(&msc->online_refs) == 0)
>> +        if (atomic_fetch_inc(&msc->online_refs) == 0) {
>> +            mutex_lock(&msc->error_irq_lock);
>> +            if (msc->error_irq_hw_enabled)
>> +                mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
>> +            mutex_unlock(&msc->error_irq_lock);
>> +
>>               mpam_reprogram_msc(msc);
>> +        }
>>       }
>>   
> 
> I don't understand how this happened that MPAMF_ECR gets lost, but msc->error_irq_hw_enabled
> is kept as true. mpam_disable_msc_ecr() is triggered in __mpam_irq_handler() or mpam_unregister_irqs().
> Both are related to mpam_broken_work, which is invoked to disable the driver completely.
> I don't see how MPAMF_ECR is cleared at CPU offline time.

The reasoning is the same as to why mpam_reprogram_msc() is already run here. When all the CPUs
affine to an MSC at a cache are offline then the cache instance and so the MSC might be switched off
and lose register state. Hence, any registers we rely on need to set when the MSC comes back online.
For PSCI_CPU_SUSPEND this needs to be handled by the firmware and for PSCI_CPU_OFF the driver
handles it. Does that make any more sense?

Thanks,

Ben

> 
>>       if (mpam_resctrl_enabled)
> 
> Thanks,
> Gavin
> 

Re: [PATCH v1 02/11] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Gavin Shan]

Hi Ben,

On 7/16/26 6:59 PM, Ben Horgan wrote:
> On 7/16/26 04:59, Gavin Shan wrote:
>> On 7/10/26 9:55 PM, Ben Horgan wrote:
>>> When all CPUs affine to an MSC are offline the MSC may lose register state
>>> which the driver then restores when an affine CPU comes back online. The
>>> error interrupt enable, MPAMF_ECR.INTEN, is missed.
>>>
>>> Restore MPAMF_ECR at CPU online.
>>>
>>> Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>> ---
>>>    drivers/resctrl/mpam_devices.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index 82966ca2a631..acfa9a4dc2fc 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -1852,8 +1852,14 @@ static int mpam_cpu_online(unsigned int cpu)
>>>            if (msc->reenable_error_ppi)
>>>                _enable_percpu_irq(&msc->reenable_error_ppi);
>>>    -        if (atomic_fetch_inc(&msc->online_refs) == 0)
>>> +        if (atomic_fetch_inc(&msc->online_refs) == 0) {
>>> +            mutex_lock(&msc->error_irq_lock);
>>> +            if (msc->error_irq_hw_enabled)
>>> +                mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
>>> +            mutex_unlock(&msc->error_irq_lock);
>>> +
>>>                mpam_reprogram_msc(msc);
>>> +        }
>>>        }
>>>    
>>
>> I don't understand how this happened that MPAMF_ECR gets lost, but msc->error_irq_hw_enabled
>> is kept as true. mpam_disable_msc_ecr() is triggered in __mpam_irq_handler() or mpam_unregister_irqs().
>> Both are related to mpam_broken_work, which is invoked to disable the driver completely.
>> I don't see how MPAMF_ECR is cleared at CPU offline time.
> 
> The reasoning is the same as to why mpam_reprogram_msc() is already run here. When all the CPUs
> affine to an MSC at a cache are offline then the cache instance and so the MSC might be switched off
> and lose register state. Hence, any registers we rely on need to set when the MSC comes back online.
> For PSCI_CPU_SUSPEND this needs to be handled by the firmware and for PSCI_CPU_OFF the driver
> handles it. Does that make any more sense?
> 

Ok. You mean PSCI_CPU_OFF on the last associated CPU enforces a power-off on the MSC
either? In this case, the commit messages need to be clearer with something below.

   When all the associated CPUs to an MSC are offline, possibly caused by
   PSCI_CPU_OFF, the power supply to the MSC can be cut off and all registers,
   including MPAMF_ECR.INTEN, are lost. Afterwards, we need to explicitly set
   MPAMF_ECR.INTEN when the first associated CPU becomes online and the power
   supply to the MSC is recovered.

   Restore MPAMF_ECR.INTEN in mpam_cpu_online() when the first associated CPU
   of the MSC becomes online

> Thanks,
> 
> Ben
> 
>>
>>>        if (mpam_resctrl_enabled)
>>

Thanks,
Gavin

[PATCH v1 03/11] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Ben Horgan]

If there are memory bandwidth counters then there are 31 bit counters even
if there are also 44 bit counters or 63 bit counters.

Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
counters.

Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index acfa9a4dc2fc..11b10c3bc334 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 						mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
 					else
 						mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
-				} else {
-					mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
 				}
+
+				mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
 			}
 		}
 	}
-- 
2.43.0

Re: [PATCH v1 03/11] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Shaopeng Tan (Fujitsu)]

Hello Ben,

>If there are memory bandwidth counters then there are 31 bit counters even
>if there are also 44 bit counters or 63 bit counters.

Although 31-bit counters are always exist, aren't they never used simultaneously with 44-bit or 63-bit counters?
If the `mpam_feat_msmon_mbwu_31counter` feature is set to always be enabled, could that potentially cause a bug?

Best regards,
Shaopeng TAN


>Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
>counters.
>
>Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
>Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>---
> drivers/resctrl/mpam_devices.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>index acfa9a4dc2fc..11b10c3bc334 100644
>--- a/drivers/resctrl/mpam_devices.c
>+++ b/drivers/resctrl/mpam_devices.c
>@@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>                                                 mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
>                                         else
>                                                 mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
>-                               } else {
>-                                       mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>                                 }
>+
>+                               mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>                         }
>                 }
>         }
>-- 
>2.43.0
>

Re: [PATCH v1 03/11] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Ben Horgan]

Hi Shaopeng,

On 7/15/26 07:34, Shaopeng Tan (Fujitsu) wrote:
> Hello Ben,
> 
>> If there are memory bandwidth counters then there are 31 bit counters even
>> if there are also 44 bit counters or 63 bit counters.
> 
> Although 31-bit counters are always exist, aren't they never used simultaneously with 44-bit or 63-bit counters?
> If the `mpam_feat_msmon_mbwu_31counter` feature is set to always be enabled, could that potentially cause a bug?

Sure, potentially, any change of meaning of a feature flag can be a source of confusion and so bugs
but based on my analysis this patch causes no change of behaviour in the driver other than in the
lines it changes. I do, however, think it's clearer to have feature bits just indicate whether the
feature is present or not rather than adding extra meaning.

In a mismatched system, a class may end up using 31 bit counters even if some of the RIS support
long counters.

Thanks,

Ben



> 
> Best regards,
> Shaopeng TAN
> 
> 
>> Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
>> counters.
>>
>> Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>>  drivers/resctrl/mpam_devices.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index acfa9a4dc2fc..11b10c3bc334 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>                                                  mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
>>                                          else
>>                                                  mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
>> -                               } else {
>> -                                       mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>>                                  }
>> +
>> +                               mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>>                          }
>>                  }
>>          }
>> -- 
>> 2.43.0
>>

Re: [PATCH v1 03/11] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> If there are memory bandwidth counters then there are 31 bit counters even
> if there are also 44 bit counters or 63 bit counters.
> 
> Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
> counters.
> 
> Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

It might be worthwhile to put some context in the commit log helping readers
to correlate to the specification. What I found is something like below in
section 8.2.1.2 Long MBWU counter and capture of ARM IHI 0099A.a:

In MSMON_MBWU, the VALUE field is always 31 bits. If MSMON_MBWU_L is implemented,
the length of the VALUE field is either 63 or 44 bits as set by MPAMF_MBWUMON_IDR.LWD.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index acfa9a4dc2fc..11b10c3bc334 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>   						mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
>   					else
>   						mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
> -				} else {
> -					mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>   				}
> +
> +				mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>   			}
>   		}
>   	}

Thanks,
Gavin

Re: [PATCH v1 03/11] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Ben Horgan]

Hi Gavin,

On 7/16/26 05:18, Gavin Shan wrote:
> On 7/10/26 9:55 PM, Ben Horgan wrote:
>> If there are memory bandwidth counters then there are 31 bit counters even
>> if there are also 44 bit counters or 63 bit counters.
>>
>> Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
>> counters.
>>
>> Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
> 
> It might be worthwhile to put some context in the commit log helping readers
> to correlate to the specification. What I found is something like below in
> section 8.2.1.2 Long MBWU counter and capture of ARM IHI 0099A.a:
> 
> In MSMON_MBWU, the VALUE field is always 31 bits. If MSMON_MBWU_L is implemented,
> the length of the VALUE field is either 63 or 44 bits as set by MPAMF_MBWUMON_IDR.LWD.

How about?

When MPAMF_MSMON_IDR.MSMON_MBWU is 1, MSMON_MBWU is present and has a VALUE field of 31 bits. If
additionally, MPAMF_MBWUMON_IDR.HAS_LONG is 1, then MSMON_MBWU_L is also present and has a VALUE
field of 44 or 63 bits as indicated by MPAMF_MBWUMON_IDR.LWD.

Thanks,

Ben

> 
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index acfa9a4dc2fc..11b10c3bc334 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>                           mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
>>                       else
>>                           mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
>> -                } else {
>> -                    mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>>                   }
>> +
>> +                mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>>               }
>>           }
>>       }
> 
> Thanks,
> Gavin
> 

Re: [PATCH v1 03/11] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Gavin Shan]

Hi Ben,

On 7/16/26 7:26 PM, Ben Horgan wrote:
> On 7/16/26 05:18, Gavin Shan wrote:
>> On 7/10/26 9:55 PM, Ben Horgan wrote:
>>> If there are memory bandwidth counters then there are 31 bit counters even
>>> if there are also 44 bit counters or 63 bit counters.
>>>
>>> Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
>>> counters.
>>>
>>> Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>> ---
>>>    drivers/resctrl/mpam_devices.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> It might be worthwhile to put some context in the commit log helping readers
>> to correlate to the specification. What I found is something like below in
>> section 8.2.1.2 Long MBWU counter and capture of ARM IHI 0099A.a:
>>
>> In MSMON_MBWU, the VALUE field is always 31 bits. If MSMON_MBWU_L is implemented,
>> the length of the VALUE field is either 63 or 44 bits as set by MPAMF_MBWUMON_IDR.LWD.
> 
> How about?
> 
> When MPAMF_MSMON_IDR.MSMON_MBWU is 1, MSMON_MBWU is present and has a VALUE field of 31 bits. If
> additionally, MPAMF_MBWUMON_IDR.HAS_LONG is 1, then MSMON_MBWU_L is also present and has a VALUE
> field of 44 or 63 bits as indicated by MPAMF_MBWUMON_IDR.LWD.
> 

Looks good to me, thanks!

> Thanks,
> 
> Ben
> 

Thanks,
Gavin

>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index acfa9a4dc2fc..11b10c3bc334 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>>                            mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
>>>                        else
>>>                            mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
>>> -                } else {
>>> -                    mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>>>                    }
>>> +
>>> +                mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>>>                }
>>>            }
>>>        }
>>
>> Thanks,
>> Gavin
>>
> 

[PATCH v1 04/11] arm_mpam: Add missing mon_sel locking in MBWU restore

[Ben Horgan]

The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
that use MPAMCFG_MON_SEL. However, in mpam_restore_mbwu_state() mbwu_state
is accessed without holding the mon_sel_lock.

Add the missing locking.

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 11b10c3bc334..b34e2a368516 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1648,16 +1648,24 @@ static int mpam_restore_mbwu_state(void *_ris)
 	u64 val;
 	struct mon_read mwbu_arg;
 	struct mpam_msc_ris *ris = _ris;
+	struct mpam_msc *msc = ris->vmsc->msc;
 	struct mpam_class *class = ris->vmsc->comp->class;
 
 	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
+		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
+			return -EIO;
+
 		if (ris->mbwu_state[i].enabled) {
 			mwbu_arg.ris = ris;
 			mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
 			mwbu_arg.type = mpam_msmon_choose_counter(class);
 			mwbu_arg.val = &val;
 
+			mpam_mon_sel_unlock(msc);
+
 			__ris_msmon_read(&mwbu_arg);
+		} else {
+			mpam_mon_sel_unlock(msc);
 		}
 	}
 
-- 
2.43.0

Re: [PATCH v1 04/11] arm_mpam: Add missing mon_sel locking in MBWU restore

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
> that use MPAMCFG_MON_SEL. However, in mpam_restore_mbwu_state() mbwu_state
> is accessed without holding the mon_sel_lock.
> 
> Add the missing locking.
> 
> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 11b10c3bc334..b34e2a368516 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1648,16 +1648,24 @@ static int mpam_restore_mbwu_state(void *_ris)
>   	u64 val;
>   	struct mon_read mwbu_arg;
>   	struct mpam_msc_ris *ris = _ris;
> +	struct mpam_msc *msc = ris->vmsc->msc;
>   	struct mpam_class *class = ris->vmsc->comp->class;
>   
>   	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
> +		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> +			return -EIO;
> +

The code can be simplified by avoiding the unnecessary nested conditional statement,
something like below:

		if (!ris->mbwu_state[i].enabled) {
			mpam_mon_sel_unlock(msc);
			continue;
		}

		mwbu_arg.ris = ris;
                 mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
		mwbu_arg.type = mpam_msmon_choose_counter(class);
		mwbu_arg.val = &val;

                 mpam_mon_sel_unlock(msc);

                 __ris_msmon_read(&mwbu_arg);

>   		if (ris->mbwu_state[i].enabled) {
>   			mwbu_arg.ris = ris;
>   			mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>   			mwbu_arg.type = mpam_msmon_choose_counter(class);
>   			mwbu_arg.val = &val;
>   
> +			mpam_mon_sel_unlock(msc);
> +
>   			__ris_msmon_read(&mwbu_arg);
> +		} else {
> +			mpam_mon_sel_unlock(msc);
>   		}
>   	}
>   

Thanks,
Gavin

Re: [PATCH v1 04/11] arm_mpam: Add missing mon_sel locking in MBWU restore

[Ben Horgan]

Hi Gavin,

On 7/16/26 05:29, Gavin Shan wrote:
> On 7/10/26 9:55 PM, Ben Horgan wrote:
>> The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
>> that use MPAMCFG_MON_SEL. However, in mpam_restore_mbwu_state() mbwu_state
>> is accessed without holding the mon_sel_lock.
>>
>> Add the missing locking.
>>
>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 11b10c3bc334..b34e2a368516 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1648,16 +1648,24 @@ static int mpam_restore_mbwu_state(void *_ris)
>>       u64 val;
>>       struct mon_read mwbu_arg;
>>       struct mpam_msc_ris *ris = _ris;
>> +    struct mpam_msc *msc = ris->vmsc->msc;
>>       struct mpam_class *class = ris->vmsc->comp->class;
>>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>> +        if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>> +            return -EIO;
>> +
> 
> The code can be simplified by avoiding the unnecessary nested conditional statement,
> something like below:
> 
>         if (!ris->mbwu_state[i].enabled) {
>             mpam_mon_sel_unlock(msc);
>             continue;
>         }
> 
>         mwbu_arg.ris = ris;
>                 mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>         mwbu_arg.type = mpam_msmon_choose_counter(class);
>         mwbu_arg.val = &val;
> 
>                 mpam_mon_sel_unlock(msc);
> 
>                 __ris_msmon_read(&mwbu_arg);
> 
>>           if (ris->mbwu_state[i].enabled) {
>>               mwbu_arg.ris = ris;
>>               mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>>               mwbu_arg.type = mpam_msmon_choose_counter(class);
>>               mwbu_arg.val = &val;
>>   +            mpam_mon_sel_unlock(msc);
>> +
>>               __ris_msmon_read(&mwbu_arg);
>> +        } else {
>> +            mpam_mon_sel_unlock(msc);
>>           }
>>       }
>>   

Ok. I think it will be clearer what's best here once Andre's error propagation in his MPAM-Fb series
has crystalized.

Thanks,

Ben

> 
> Thanks,
> Gavin
> 

Re: [PATCH v1 04/11] arm_mpam: Add missing mon_sel locking in MBWU restore

[Gavin Shan]

Hi Ben,

On 7/16/26 7:31 PM, Ben Horgan wrote:
> On 7/16/26 05:29, Gavin Shan wrote:
>> On 7/10/26 9:55 PM, Ben Horgan wrote:
>>> The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
>>> that use MPAMCFG_MON_SEL. However, in mpam_restore_mbwu_state() mbwu_state
>>> is accessed without holding the mon_sel_lock.
>>>
>>> Add the missing locking.
>>>
>>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>> ---
>>>    drivers/resctrl/mpam_devices.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index 11b10c3bc334..b34e2a368516 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -1648,16 +1648,24 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>        u64 val;
>>>        struct mon_read mwbu_arg;
>>>        struct mpam_msc_ris *ris = _ris;
>>> +    struct mpam_msc *msc = ris->vmsc->msc;
>>>        struct mpam_class *class = ris->vmsc->comp->class;
>>>          for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>>> +        if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>> +            return -EIO;
>>> +
>>
>> The code can be simplified by avoiding the unnecessary nested conditional statement,
>> something like below:
>>
>>          if (!ris->mbwu_state[i].enabled) {
>>              mpam_mon_sel_unlock(msc);
>>              continue;
>>          }
>>
>>          mwbu_arg.ris = ris;
>>                  mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>>          mwbu_arg.type = mpam_msmon_choose_counter(class);
>>          mwbu_arg.val = &val;
>>
>>                  mpam_mon_sel_unlock(msc);
>>
>>                  __ris_msmon_read(&mwbu_arg);
>>
>>>            if (ris->mbwu_state[i].enabled) {
>>>                mwbu_arg.ris = ris;
>>>                mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>>>                mwbu_arg.type = mpam_msmon_choose_counter(class);
>>>                mwbu_arg.val = &val;
>>>    +            mpam_mon_sel_unlock(msc);
>>> +
>>>                __ris_msmon_read(&mwbu_arg);
>>> +        } else {
>>> +            mpam_mon_sel_unlock(msc);
>>>            }
>>>        }
>>>    
> 
> Ok. I think it will be clearer what's best here once Andre's error propagation in his MPAM-Fb series
> has crystalized.
> 

Yeah, we can clean this up after that.

> Thanks,
> 
> Ben
> 

Thanks,
Gavin

Re: [PATCH v1 04/11] arm_mpam: Add missing mon_sel locking in MBWU restore

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
> that use MPAMCFG_MON_SEL. However, in mpam_restore_mbwu_state() mbwu_state
> is accessed without holding the mon_sel_lock.
> 
> Add the missing locking.
> 
> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

[Ben Horgan]

When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
zero in mpam_save_mbwu_state(). This is very likely to mean that the config
will mismatch when restoring and so the monitor will be reset. However, the
state may have been lost and so there are no guarantees. Ensure the reset
happens by setting the reset_on_next_read and remove the unnecessary writes
from mpam_save_mbwu_state().

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index b34e2a368516..222fc248067e 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1648,10 +1648,13 @@ static int mpam_restore_mbwu_state(void *_ris)
 	u64 val;
 	struct mon_read mwbu_arg;
 	struct mpam_msc_ris *ris = _ris;
+	struct msmon_mbwu_state *mbwu_state;
 	struct mpam_msc *msc = ris->vmsc->msc;
 	struct mpam_class *class = ris->vmsc->comp->class;
 
 	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
+		mbwu_state = &ris->mbwu_state[i];
+
 		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
 			return -EIO;
 
@@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
 			mwbu_arg.type = mpam_msmon_choose_counter(class);
 			mwbu_arg.val = &val;
 
+			mbwu_state->reset_on_next_read = true;
+
 			mpam_mon_sel_unlock(msc);
 
 			__ris_msmon_read(&mwbu_arg);
@@ -1696,15 +1701,11 @@ static int mpam_save_mbwu_state(void *arg)
 
 		cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
 		cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
-		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
 
-		if (mpam_ris_has_mbwu_long_counter(ris)) {
+		if (mpam_ris_has_mbwu_long_counter(ris))
 			val = mpam_msc_read_mbwu_l(msc);
-			mpam_msc_zero_mbwu_l(msc);
-		} else {
+		else
 			val = mpam_read_monsel_reg(msc, MBWU);
-			mpam_write_monsel_reg(msc, MBWU, 0);
-		}
 
 		cfg->mon = i;
 		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
-- 
2.43.0

Re: [PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

[Shaopeng Tan (Fujitsu)]

Hello Ben,

>When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
>zero in mpam_save_mbwu_state(). This is very likely to mean that the config
>will mismatch when restoring and so the monitor will be reset. However, the
>state may have been lost and so there are no guarantees. Ensure the reset
>happens by setting the reset_on_next_read and remove the unnecessary writes
>from mpam_save_mbwu_state().
>
>Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>---
> drivers/resctrl/mpam_devices.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>index b34e2a368516..222fc248067e 100644
>--- a/drivers/resctrl/mpam_devices.c
>+++ b/drivers/resctrl/mpam_devices.c
>@@ -1648,10 +1648,13 @@ static int mpam_restore_mbwu_state(void *_ris)
>         u64 val;
>         struct mon_read mwbu_arg;
>         struct mpam_msc_ris *ris = _ris;
>+       struct msmon_mbwu_state *mbwu_state;
>         struct mpam_msc *msc = ris->vmsc->msc;
>         struct mpam_class *class = ris->vmsc->comp->class;
> 
>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>+               mbwu_state = &ris->mbwu_state[i];
>+
>                 if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>                         return -EIO;
> 
>@@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
>                         mwbu_arg.type = mpam_msmon_choose_counter(class);
>                         mwbu_arg.val = &val;
> 
>+                       mbwu_state->reset_on_next_read = true;
>+
>                         mpam_mon_sel_unlock(msc);

        for (i = 0; i < ris->props.num_mbwu_mon; i++) {
+               mbwu_state = &ris->mbwu_state[i];
+
                if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
                        return -EIO;

-               if (ris->mbwu_state[i].enabled) {
+               if (mbwu_state->enabled) {  //this line might need refactoring
                        mwbu_arg.ris = ris;
-                       mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
+                       mwbu_arg.ctx = &mbwu_state->cfg;  //and this line
                        mwbu_arg.type = mpam_msmon_choose_counter(class);
                        mwbu_arg.val = &val;
 
+                       mbwu_state->reset_on_next_read = true;
+
                        mpam_mon_sel_unlock(msc);


Best regards,
Shaopeng TAN

>                         __ris_msmon_read(&mwbu_arg);
>@@ -1696,15 +1701,11 @@ static int mpam_save_mbwu_state(void *arg)
> 
>                 cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>                 cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
>-               mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
> 
>-               if (mpam_ris_has_mbwu_long_counter(ris)) {
>+               if (mpam_ris_has_mbwu_long_counter(ris))
>                         val = mpam_msc_read_mbwu_l(msc);
>-                       mpam_msc_zero_mbwu_l(msc);
>-               } else {
>+               else
>                         val = mpam_read_monsel_reg(msc, MBWU);
>-                       mpam_write_monsel_reg(msc, MBWU, 0);
>-               }
> 
>                 cfg->mon = i;
>                 cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>-- 
>2.43.0

Re: [PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

[Ben Horgan]

Hi Shaopeng,

On 7/15/26 08:49, Shaopeng Tan (Fujitsu) wrote:
> Hello Ben,
> 
>> When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
>> zero in mpam_save_mbwu_state(). This is very likely to mean that the config
>> will mismatch when restoring and so the monitor will be reset. However, the
>> state may have been lost and so there are no guarantees. Ensure the reset
>> happens by setting the reset_on_next_read and remove the unnecessary writes
>>from mpam_save_mbwu_state().
>>
>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>>  drivers/resctrl/mpam_devices.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index b34e2a368516..222fc248067e 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1648,10 +1648,13 @@ static int mpam_restore_mbwu_state(void *_ris)
>>          u64 val;
>>          struct mon_read mwbu_arg;
>>          struct mpam_msc_ris *ris = _ris;
>> +       struct msmon_mbwu_state *mbwu_state;
>>          struct mpam_msc *msc = ris->vmsc->msc;
>>          struct mpam_class *class = ris->vmsc->comp->class;
>>  
>>          for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>> +               mbwu_state = &ris->mbwu_state[i];
>> +
>>                  if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>                          return -EIO;
>>  
>> @@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
>>                          mwbu_arg.type = mpam_msmon_choose_counter(class);
>>                          mwbu_arg.val = &val;
>>  
>> +                       mbwu_state->reset_on_next_read = true;
>> +
>>                          mpam_mon_sel_unlock(msc);
> 
>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
> +               mbwu_state = &ris->mbwu_state[i];
> +
>                 if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>                         return -EIO;
> 
> -               if (ris->mbwu_state[i].enabled) {
> +               if (mbwu_state->enabled) {  //this line might need refactoring

I'm unclear on what you are trying to point out in this email. Please can you explain.

Thanks,

Ben

>                         mwbu_arg.ris = ris;
> -                       mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
> +                       mwbu_arg.ctx = &mbwu_state->cfg;  //and this line
>                         mwbu_arg.type = mpam_msmon_choose_counter(class);
>                         mwbu_arg.val = &val;
>  
> +                       mbwu_state->reset_on_next_read = true;
> +
>                         mpam_mon_sel_unlock(msc);
> 
> 
> Best regards,
> Shaopeng TAN
> 
>>                          __ris_msmon_read(&mwbu_arg);
>> @@ -1696,15 +1701,11 @@ static int mpam_save_mbwu_state(void *arg)
>>  
>>                  cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>>                  cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
>> -               mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>  
>> -               if (mpam_ris_has_mbwu_long_counter(ris)) {
>> +               if (mpam_ris_has_mbwu_long_counter(ris))
>>                          val = mpam_msc_read_mbwu_l(msc);
>> -                       mpam_msc_zero_mbwu_l(msc);
>> -               } else {
>> +               else
>>                          val = mpam_read_monsel_reg(msc, MBWU);
>> -                       mpam_write_monsel_reg(msc, MBWU, 0);
>> -               }
>>  
>>                  cfg->mon = i;
>>                  cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>> -- 
>> 2.43.0

Re: [PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

[Shaopeng Tan (Fujitsu)]

Hello Ben,

>>> When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
>>> zero in mpam_save_mbwu_state(). This is very likely to mean that the config
>>> will mismatch when restoring and so the monitor will be reset. However, the
>>> state may have been lost and so there are no guarantees. Ensure the reset
>>> happens by setting the reset_on_next_read and remove the unnecessary writes
>>>from mpam_save_mbwu_state().
>>>
>>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>> ---
>>>  drivers/resctrl/mpam_devices.c | 13 +++++++------
>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index b34e2a368516..222fc248067e 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -1648,10 +1648,13 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>          u64 val;
>>>          struct mon_read mwbu_arg;
>>>          struct mpam_msc_ris *ris = _ris;
>>> +       struct msmon_mbwu_state *mbwu_state;
>>>          struct mpam_msc *msc = ris->vmsc->msc;
>>>          struct mpam_class *class = ris->vmsc->comp->class;
>>>  
>>>          for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>>> +               mbwu_state = &ris->mbwu_state[i];
>>> +
>>>                  if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>>                          return -EIO;
>>>  
>>> @@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>                          mwbu_arg.type = mpam_msmon_choose_counter(class);
>>>                          mwbu_arg.val = &val;
>>>  
>>> +                       mbwu_state->reset_on_next_read = true;
>>> +
>>>                          mpam_mon_sel_unlock(msc);
>>
>>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>> +               mbwu_state = &ris->mbwu_state[i];
>> +
>>                 if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>                         return -EIO;
>>
>> -               if (ris->mbwu_state[i].enabled) {
>> +               if (mbwu_state->enabled) {  //this line might need refactoring
>
>I'm unclear on what you are trying to point out in this email. Please can you explain.
>
>Thanks,
>
>Ben

Inside the for loop, you introduced `mbwu_state = &ris->mbwu_state[i];` a few lines earlier, 
and then used `mbwu_state->reset_on_next_read = true;` shortly after.
Therefore, using mbwu_state->enabled and mbwu_state->cfg keeps 
the whole block consistent and avoids repeating ris->mbwu_state[i] multiple times. 

Best regards,
Shaopeng TAN

>>                         mwbu_arg.ris = ris;
>> -                       mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>> +                       mwbu_arg.ctx = &mbwu_state->cfg;  //and this line
>>                         mwbu_arg.type = mpam_msmon_choose_counter(class);
>>                         mwbu_arg.val = &val;
>> 
>> +                       mbwu_state->reset_on_next_read = true;
>> +
>>                         mpam_mon_sel_unlock(msc);
>>
>>
>> Best regards,
>> Shaopeng TAN
>>
>>>                          __ris_msmon_read(&mwbu_arg);
>>> @@ -1696,15 +1701,11 @@ static int mpam_save_mbwu_state(void *arg)
>>>  
>>>                  cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>>>                  cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
>>> -               mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>>  
>>> -               if (mpam_ris_has_mbwu_long_counter(ris)) {
>>> +               if (mpam_ris_has_mbwu_long_counter(ris))
>>>                          val = mpam_msc_read_mbwu_l(msc);
>>> -                       mpam_msc_zero_mbwu_l(msc);
>>> -               } else {
>>> +               else
>>>                          val = mpam_read_monsel_reg(msc, MBWU);
>>> -                       mpam_write_monsel_reg(msc, MBWU, 0);
>>> -               }
>>>  
>>>                  cfg->mon = i;
>>>                  cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>>> --
>>> 2.43.0
>
>

Re: [PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

[Ben Horgan]

Hi Shaopeng,

On 7/16/26 01:26, Shaopeng Tan (Fujitsu) wrote:
> Hello Ben,
> 
>>>> When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
>>>> zero in mpam_save_mbwu_state(). This is very likely to mean that the config
>>>> will mismatch when restoring and so the monitor will be reset. However, the
>>>> state may have been lost and so there are no guarantees. Ensure the reset
>>>> happens by setting the reset_on_next_read and remove the unnecessary writes
>>> >from mpam_save_mbwu_state().
>>>>
>>>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>>> ---
>>>>  drivers/resctrl/mpam_devices.c | 13 +++++++------
>>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>>> index b34e2a368516..222fc248067e 100644
>>>> --- a/drivers/resctrl/mpam_devices.c
>>>> +++ b/drivers/resctrl/mpam_devices.c
>>>> @@ -1648,10 +1648,13 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>>          u64 val;
>>>>          struct mon_read mwbu_arg;
>>>>          struct mpam_msc_ris *ris = _ris;
>>>> +       struct msmon_mbwu_state *mbwu_state;
>>>>          struct mpam_msc *msc = ris->vmsc->msc;
>>>>          struct mpam_class *class = ris->vmsc->comp->class;
>>>>  
>>>>          for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>>>> +               mbwu_state = &ris->mbwu_state[i];
>>>> +
>>>>                  if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>>>                          return -EIO;
>>>>  
>>>> @@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>>                          mwbu_arg.type = mpam_msmon_choose_counter(class);
>>>>                          mwbu_arg.val = &val;
>>>>  
>>>> +                       mbwu_state->reset_on_next_read = true;
>>>> +
>>>>                          mpam_mon_sel_unlock(msc);
>>>
>>>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>>> +               mbwu_state = &ris->mbwu_state[i];
>>> +
>>>                 if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>>                         return -EIO;
>>>
>>> -               if (ris->mbwu_state[i].enabled) {
>>> +               if (mbwu_state->enabled) {  //this line might need refactoring
>>
>> I'm unclear on what you are trying to point out in this email. Please can you explain.
>>
>> Thanks,
>>
>> Ben
> 
> Inside the for loop, you introduced `mbwu_state = &ris->mbwu_state[i];` a few lines earlier, 
> and then used `mbwu_state->reset_on_next_read = true;` shortly after.
> Therefore, using mbwu_state->enabled and mbwu_state->cfg keeps 
> the whole block consistent and avoids repeating ris->mbwu_state[i] multiple times. 

Thanks for explaining. Indeed, continuing to use the ris->mbwu_state[i] once it's assigned to a
local variable does make this a bit messy.

Ben

> 
> Best regards,
> Shaopeng TAN
> 
>>>                         mwbu_arg.ris = ris;
>>> -                       mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>>> +                       mwbu_arg.ctx = &mbwu_state->cfg;  //and this line
>>>                         mwbu_arg.type = mpam_msmon_choose_counter(class);
>>>                         mwbu_arg.val = &val;
>>>
>>> +                       mbwu_state->reset_on_next_read = true;
>>> +
>>>                         mpam_mon_sel_unlock(msc);
>>>
>>>
>>> Best regards,
>>> Shaopeng TAN
>>>
>>>>                          __ris_msmon_read(&mwbu_arg);
>>>> @@ -1696,15 +1701,11 @@ static int mpam_save_mbwu_state(void *arg)
>>>>  
>>>>                  cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>>>>                  cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
>>>> -               mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>>>  
>>>> -               if (mpam_ris_has_mbwu_long_counter(ris)) {
>>>> +               if (mpam_ris_has_mbwu_long_counter(ris))
>>>>                          val = mpam_msc_read_mbwu_l(msc);
>>>> -                       mpam_msc_zero_mbwu_l(msc);
>>>> -               } else {
>>>> +               else
>>>>                          val = mpam_read_monsel_reg(msc, MBWU);
>>>> -                       mpam_write_monsel_reg(msc, MBWU, 0);
>>>> -               }
>>>>  
>>>>                  cfg->mon = i;
>>>>                  cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>>>> --
>>>> 2.43.0
>>
>>

[PATCH v1 06/11] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[Ben Horgan]

mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
correction value. However, the type of counter to read is determined by the
RIS rather than the class and overflow is not taken into account. Fix this
and mitigate against further divergence by using the same helper as used
for user reads, __ris_msmon_read().

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 222fc248067e..a49f426aefc0 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1029,12 +1029,6 @@ struct mon_read {
 	bool				waited_timeout;
 };
 
-static bool mpam_ris_has_mbwu_long_counter(struct mpam_msc_ris *ris)
-{
-	return (mpam_has_feature(mpam_feat_msmon_mbwu_63counter, &ris->props) ||
-		mpam_has_feature(mpam_feat_msmon_mbwu_44counter, &ris->props));
-}
-
 static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
 {
 	int retry = 3;
@@ -1687,6 +1681,7 @@ static int mpam_save_mbwu_state(void *arg)
 	struct mpam_msc_ris *ris = arg;
 	struct msmon_mbwu_state *mbwu_state;
 	struct mpam_msc *msc = ris->vmsc->msc;
+	struct mpam_class *class = ris->vmsc->comp->class;
 
 	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
 		mbwu_state = &ris->mbwu_state[i];
@@ -1702,18 +1697,32 @@ static int mpam_save_mbwu_state(void *arg)
 		cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
 		cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
 
-		if (mpam_ris_has_mbwu_long_counter(ris))
-			val = mpam_msc_read_mbwu_l(msc);
-		else
-			val = mpam_read_monsel_reg(msc, MBWU);
-
 		cfg->mon = i;
 		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
 		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
 		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
-		mbwu_state->correction += val;
 		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
 		mpam_mon_sel_unlock(msc);
+
+		if (mbwu_state->enabled) {
+			struct mon_read mbwu_arg = {
+				mbwu_arg.ris = ris,
+				mbwu_arg.ctx = cfg,
+				mbwu_arg.type = mpam_msmon_choose_counter(class),
+				mbwu_arg.val = &val,
+			};
+
+			val = 0;
+			__ris_msmon_read(&mbwu_arg);
+
+			if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
+				return -EIO;
+
+			mbwu_state->reset_on_next_read = true;
+			if (!mbwu_arg.err)
+				mbwu_state->correction = val;
+			mpam_mon_sel_unlock(msc);
+		}
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH v1 06/11] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
> correction value. However, the type of counter to read is determined by the
> RIS rather than the class and overflow is not taken into account. Fix this
> and mitigate against further divergence by using the same helper as used
> for user reads, __ris_msmon_read().
> 
> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 222fc248067e..a49f426aefc0 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1029,12 +1029,6 @@ struct mon_read {
>   	bool				waited_timeout;
>   };
>   
> -static bool mpam_ris_has_mbwu_long_counter(struct mpam_msc_ris *ris)
> -{
> -	return (mpam_has_feature(mpam_feat_msmon_mbwu_63counter, &ris->props) ||
> -		mpam_has_feature(mpam_feat_msmon_mbwu_44counter, &ris->props));
> -}
> -
>   static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
>   {
>   	int retry = 3;
> @@ -1687,6 +1681,7 @@ static int mpam_save_mbwu_state(void *arg)
>   	struct mpam_msc_ris *ris = arg;
>   	struct msmon_mbwu_state *mbwu_state;
>   	struct mpam_msc *msc = ris->vmsc->msc;
> +	struct mpam_class *class = ris->vmsc->comp->class;
>   
>   	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>   		mbwu_state = &ris->mbwu_state[i];
> @@ -1702,18 +1697,32 @@ static int mpam_save_mbwu_state(void *arg)
>   		cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>   		cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
>   
> -		if (mpam_ris_has_mbwu_long_counter(ris))
> -			val = mpam_msc_read_mbwu_l(msc);
> -		else
> -			val = mpam_read_monsel_reg(msc, MBWU);
> -
>   		cfg->mon = i;
>   		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>   		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>   		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
> -		mbwu_state->correction += val;
>   		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>   		mpam_mon_sel_unlock(msc);
> +
> +		if (mbwu_state->enabled) {
> +			struct mon_read mbwu_arg = {
> +				mbwu_arg.ris = ris,
> +				mbwu_arg.ctx = cfg,
> +				mbwu_arg.type = mpam_msmon_choose_counter(class),
> +				mbwu_arg.val = &val,
> +			};
> +
> +			val = 0;
> +			__ris_msmon_read(&mbwu_arg);
> +
> +			if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> +				return -EIO;
> +
> +			mbwu_state->reset_on_next_read = true;
> +			if (!mbwu_arg.err)
> +				mbwu_state->correction = val;
> +			mpam_mon_sel_unlock(msc);
> +		}
>   	}

The unnecessary nested conditional statement can be avoided by
skipping the disabled MBWU state.

		if (!mbwu_state->enabled) {
			mpam_mon_sel_unlock(msc);
			continue;
		}

		mpam_mon_sel_unlock(msc);

		val = 0;
		mbwu_arg.ris = ris;
		mbwu_arg.ctx = cfg;
		mbwu_arg.type = mpam_msmon_choose_counter(class);
		mbwu_arg.val = &val
		__ris_msmon_read(&mbwu_arg);

		:
		:


>   
>   	return 0;

Thanks,
Gavin

Re: [PATCH v1 06/11] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
> correction value. However, the type of counter to read is determined by the
> RIS rather than the class and overflow is not taken into account. Fix this
> and mitigate against further divergence by using the same helper as used
> for user reads, __ris_msmon_read().
> 
> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH v1 07/11] arm_mpam: Initialize all of struct mon_read in mpam_restore_mbwu_state()

[Ben Horgan]

m->err may be read before initialization in __ris_msmon_read() when called
from mpam_restore_mbwu_state().

Initialize the whole struct mon_read in mpam_restore_mbwu_state() and fix
the spelling of mbwu in the name.

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index a49f426aefc0..c9adc450f087 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1640,7 +1640,6 @@ static int mpam_restore_mbwu_state(void *_ris)
 {
 	int i;
 	u64 val;
-	struct mon_read mwbu_arg;
 	struct mpam_msc_ris *ris = _ris;
 	struct msmon_mbwu_state *mbwu_state;
 	struct mpam_msc *msc = ris->vmsc->msc;
@@ -1653,16 +1652,18 @@ static int mpam_restore_mbwu_state(void *_ris)
 			return -EIO;
 
 		if (ris->mbwu_state[i].enabled) {
-			mwbu_arg.ris = ris;
-			mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
-			mwbu_arg.type = mpam_msmon_choose_counter(class);
-			mwbu_arg.val = &val;
+			struct mon_read mbwu_arg = {
+				.ris = ris,
+				.ctx = &ris->mbwu_state[i].cfg,
+				.type = mpam_msmon_choose_counter(class),
+				.val = &val
+			};
 
 			mbwu_state->reset_on_next_read = true;
 
 			mpam_mon_sel_unlock(msc);
 
-			__ris_msmon_read(&mwbu_arg);
+			__ris_msmon_read(&mbwu_arg);
 		} else {
 			mpam_mon_sel_unlock(msc);
 		}
-- 
2.43.0

Re: [PATCH v1 07/11] arm_mpam: Initialize all of struct mon_read in mpam_restore_mbwu_state()

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> m->err may be read before initialization in __ris_msmon_read() when called
> from mpam_restore_mbwu_state().
> 
> Initialize the whole struct mon_read in mpam_restore_mbwu_state() and fix
> the spelling of mbwu in the name.
> 
> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH v1 08/11] arm_mpam: resctrl: Correct check that existing class is L3

[Ben Horgan]

The class used to back mbm_total_bytes can be either at the L3 or the
memory. If a platform had candidate classes at both the memory and the L3
then, as the check for whether the existing class is L3 or not in
counter_update_class() is broken, the class that will be chosen depends on
which order the classes are considered and so the probe order.

Fix the check.

Fixes: 1458c4f05335 ("arm_mpam: resctrl: Add support for csu counters")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_resctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
index 226ff6f532fa..59dea750ff56 100644
--- a/drivers/resctrl/mpam_resctrl.c
+++ b/drivers/resctrl/mpam_resctrl.c
@@ -931,7 +931,7 @@ static void counter_update_class(enum resctrl_event_id evt_id,
 	struct mpam_class *existing_class = mpam_resctrl_counters[evt_id].class;
 
 	if (existing_class) {
-		if (class->level == 3) {
+		if (existing_class->level == 3) {
 			pr_debug("Existing class is L3 - L3 wins\n");
 			return;
 		}
-- 
2.43.0

Re: [PATCH v1 08/11] arm_mpam: resctrl: Correct check that existing class is L3

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> The class used to back mbm_total_bytes can be either at the L3 or the
> memory. If a platform had candidate classes at both the memory and the L3
> then, as the check for whether the existing class is L3 or not in
> counter_update_class() is broken, the class that will be chosen depends on
> which order the classes are considered and so the probe order.
> 
> Fix the check.
> 
> Fixes: 1458c4f05335 ("arm_mpam: resctrl: Add support for csu counters")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_resctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH v1 09/11] arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent

[Ben Horgan]

When read_mon_cdp_safe() is called with cdp_enabled equal to false then the
reading returned in val is the sum of the monitor values for the given
component. However, when cdp_enabled equal to true it is again the sum of
the monitor values but with the initial value of val also added.

Change the cdp_enabled equals true case to match the false case. All
callers zero the val before calling and so no functional change is
anticipated.

Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_resctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
index 59dea750ff56..d97565479014 100644
--- a/drivers/resctrl/mpam_resctrl.c
+++ b/drivers/resctrl/mpam_resctrl.c
@@ -487,7 +487,7 @@ static int read_mon_cdp_safe(struct mpam_resctrl_mon *mon, struct mpam_component
 		if (err)
 			return err;
 
-		*val += code_val + data_val;
+		*val = code_val + data_val;
 		return 0;
 	}
 
-- 
2.43.0

Re: [PATCH v1 09/11] arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> When read_mon_cdp_safe() is called with cdp_enabled equal to false then the
> reading returned in val is the sum of the monitor values for the given
> component. However, when cdp_enabled equal to true it is again the sum of
> the monitor values but with the initial value of val also added.
> 
> Change the cdp_enabled equals true case to match the false case. All
> callers zero the val before calling and so no functional change is
> anticipated.
> 
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_resctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH v1 10/11] arm_mpam: Don't loop forever if there is the maximum possible amount of PARTIDs

[Ben Horgan]

On a theoretical platform with the maximum possible number of usable
PARTIDs, 0XFFFF, the loops iterating over PARTID in mpam_reset_ris() and
mpam_reprogram_ris() will never reach their termination condition leading
to a system hang when register the cpu hotplug handlers when MPAM is
enabling.

Fix these loops.

Fixes: f188a36ca241 ("arm_mpam: Reset MSC controls from cpuhp callbacks")
Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index c9adc450f087..9412ffde1cf1 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1735,7 +1735,7 @@ static int mpam_save_mbwu_state(void *arg)
  */
 static int mpam_reset_ris(void *arg)
 {
-	u16 partid, partid_max;
+	u16 partid_max;
 	struct mpam_config reset_cfg = {};
 	struct mpam_msc_ris *ris = arg;
 
@@ -1745,7 +1745,7 @@ static int mpam_reset_ris(void *arg)
 	spin_lock(&partid_max_lock);
 	partid_max = mpam_partid_max;
 	spin_unlock(&partid_max_lock);
-	for (partid = 0; partid <= partid_max; partid++)
+	for (u32 partid = 0; partid <= partid_max; partid++)
 		mpam_reprogram_ris_partid(ris, partid, &reset_cfg);
 
 	return 0;
@@ -1792,7 +1792,6 @@ static int __write_config(void *arg)
 
 static void mpam_reprogram_msc(struct mpam_msc *msc)
 {
-	u16 partid;
 	bool reset;
 	struct mpam_config *cfg;
 	struct mpam_msc_ris *ris;
@@ -1816,7 +1815,7 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
 		arg.comp = ris->vmsc->comp;
 		arg.ris = ris;
 		reset = true;
-		for (partid = 0; partid <= mpam_partid_max; partid++) {
+		for (u32 partid = 0; partid <= mpam_partid_max; partid++) {
 			cfg = &ris->vmsc->comp->cfg[partid];
 			if (!bitmap_empty(cfg->features, MPAM_FEATURE_LAST))
 				reset = false;
-- 
2.43.0

Re: [PATCH v1 10/11] arm_mpam: Don't loop forever if there is the maximum possible amount of PARTIDs

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> On a theoretical platform with the maximum possible number of usable
> PARTIDs, 0XFFFF, the loops iterating over PARTID in mpam_reset_ris() and
> mpam_reprogram_ris() will never reach their termination condition leading
> to a system hang when register the cpu hotplug handlers when MPAM is
> enabling.
> 
> Fix these loops.
> 
> Fixes: f188a36ca241 ("arm_mpam: Reset MSC controls from cpuhp callbacks")
> Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH v1 11/11] arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg

[Ben Horgan]

Each component cfg is sized by the size of a per-PARTID multiplied by the
number of PARTIDs. On a platform sized to push the limits of the
specification, the allocation using kzalloc_objs() will consistently
fail. Change to use kvzmalloc_objs() so that large allocations fall back to
vmalloc() based allocations.

Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 9412ffde1cf1..9a7954b7a2af 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -157,7 +157,7 @@ static void mpam_free_garbage(void)
 		if (iter->pdev)
 			devm_kfree(&iter->pdev->dev, iter->to_free);
 		else
-			kfree(iter->to_free);
+			kvfree(iter->to_free);
 	}
 }
 
@@ -2667,7 +2667,7 @@ static int __allocate_component_cfg(struct mpam_component *comp)
 	if (comp->cfg)
 		return 0;
 
-	comp->cfg = kzalloc_objs(*comp->cfg, mpam_partid_max + 1);
+	comp->cfg = kvzalloc_objs(*comp->cfg, mpam_partid_max + 1);
 	if (!comp->cfg)
 		return -ENOMEM;
 
-- 
2.43.0

Re: [PATCH v1 11/11] arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> Each component cfg is sized by the size of a per-PARTID multiplied by the
> number of PARTIDs. On a platform sized to push the limits of the
> specification, the allocation using kzalloc_objs() will consistently
> fail. Change to use kvzmalloc_objs() so that large allocations fall back to
> vmalloc() based allocations.
> 
> Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_devices.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v1 00/11] arm_mpam: minor fixes at v7.2

[Gavin Shan]

On 7/10/26 9:55 PM, Ben Horgan wrote:
> This is a bunch of small fixes for hypothetical problems in the driver. They
> were found by a mixture of messing around with a software model, inspection and
> llm review.
> 
> The first two patches ensure the error interrupt isn't accidentally disabled
> when register state is lost. Patch 3 keeps the feature accounting correct but
> has no functional change. Patches 4 to 7 fix mbwu save/restore but this is not
> relied on as we plan to just use resctrl assigned counters and these are
> unassigned when a domain goes offline. Also, MSC afffinity of a region smaller
> than a resctrl domain has not been seen. Patch 8 makes read_mon_cdp_safe()
> behaviour the same for cdp_enabled and not. Patch 9 fixes an infinite loop in
> a system using all the PARTIDs which is much bigger than anything than anything
> I've seen. Patch 10 stops some allocations failing on similiarly big systems.
> 
> Based on v7.2-rc2
> 
> Ben Horgan (11):
>    arm_mpam: Move MPAMF_ECR write helpers to allow reuse
>    arm_mpam: Restore the error interrupt enable from mpam_cpu_online()
>    arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth
>      counters
>    arm_mpam: Add missing mon_sel locking in MBWU restore
>    arm_mpam: Ensure MBWU counters are reset on restore
>    arm_mpam: Use __ris_msmon_read() for saving MBWU state
>    arm_mpam: Initialize all of struct mon_read in
>      mpam_restore_mbwu_state()
>    arm_mpam: resctrl: Correct check that existing class is L3
>    arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent
>    arm_mpam: Don't loop forever if there is the maximum possible amount
>      of PARTIDs
>    arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg
> 
>   drivers/resctrl/mpam_devices.c | 124 ++++++++++++++++++++-------------
>   drivers/resctrl/mpam_resctrl.c |   4 +-
>   2 files changed, 76 insertions(+), 52 deletions(-)
> 

Tested on NVidia's grace-hopper machine and looks good: (a) kunit_tests passed;
(b) l3 cache partitioning and MBW (soft) limiting work as expected; (c) llc_occupancy
monitor counter alters when workload is attached and running.

Tested-by: Gavin Shan <gshan@redhat.com>

Thanks,
Gavin