Re: [PATCH v4 06/10] arm_mpam: propagate MSC access errors for state saving function

[Ben Horgan <ben.horgan@arm.com>]

Hi Sudeep, Andre,

On 7/24/26 13:27, Sudeep Holla wrote:
> On Fri, Jul 24, 2026 at 01:19:14PM +0200, Andre Przywara wrote:
>> Hi,
>>
>> On 7/24/26 12:07, Sudeep Holla wrote:
>>> On Thu, Jul 23, 2026 at 05:54:50PM +0200, Andre Przywara wrote:
>>>> Allow the mpam_save_mbwu_state() function to return an error, and
>>>> propagate read and write errors from the lower level up.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>   drivers/resctrl/mpam_devices.c | 29 ++++++++++++++++++++++-------
>>>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>>> index bcff53477133..6329443c451f 100644
>>>> --- a/drivers/resctrl/mpam_devices.c
>>>> +++ b/drivers/resctrl/mpam_devices.c
>>>> @@ -1833,22 +1833,37 @@ static int mpam_save_mbwu_state(void *arg)
>>>>   		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
>>>>   			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
>>>> -		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
>>>> -		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
>>>> -		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
>>>> -		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>>> +		ret = mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
>>>> +		if (ret)
>>>> +			return ret;
>>>
>>> How does it work in general with PCC. Now that you can fail at any point,
>>> what happens to the write that occurs before a failed read like above one.
>>> Who will take care of erasing those new writes or it doesn't matter ?

At least in this particular case it doesn't matter. The MON_SEL just configures which instance of
the monitor we are reading or writing from and we'll just write MON_SEL again next time we want to
interact with a monitor.

>>> Just checking as I don't have much knowledge on MPAM intrinsics.
>>
>> TBH I don't know, but I think we consider MPAM botched at this point, and
>> just stop the driver, similar to an error IRQ? But I am not sure this is
>> properly implemented at this point. The focus of these first six patches was
>> merely to lay the dirty groundwork for *being able* to handle errors, and do
>> this now rather than in the future.
>>

I've just been discussing the error handling with James and trying to work out what we can do
without limiting our options going forward. Ideally, if there are transient errors we'd like to
retry in the kernel if it's not taking "too long" and otherwise report the error back to user space.
For catastrophic errors, such as when the scp doesn't reply at all or that no MPAM commands are
going to work again we should disable MPAM. As resctrl does not anticipate errors willy nilly from
arch code it ends up reporting the info/last_cmd_status as "ok" even in situations when the MSC
accesses have failed. I notice this in at least rdtgroup_schemata_write(). Before we can report
proper failure information from resctrl and update last_cmd_status based on the error status from
the architecture code it doesn't make sense to report the errors to user space. We would likely want
resctrl to understand -ETIMEDOUT as well. As such, this leaves us the option of just nuking MPAM on
any error reported from the MPAM firmware interface. For now, we needn't do any retries and just
error out on the first error. It should be possible to just schedule mpam_broken_work from mpam_fb.c
on error.

Sorry for prompting the writing of these error handling patches but they will likely come in useful
in the future.

Thanks,

Ben

> 
> Well, I agree to some extent, but PCC adds that failure case before which
> it wasn't there. So, it is hard to claim that it was botched up before so
> let it be. I will let James/Ben to decide if it was already botched up or
> PCC addition makes it fragile in terms of error handling.