Re: [PATCH v2 14/15] arm_mpam: prevent MPAM-Fb accesses inside IRQ handler

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 7/3/26 12:54, Ben Horgan wrote:
> Hi Andre,
> 
> On 7/2/26 17:22, Andre Przywara wrote:
>> When an MPAM MSC gets into an error condition, it can trigger an error
>> IRQ. We cannot really do much about those errors, but we at least query
>> and log the error, then disable MPAM functionality.
>>
>> This error report relies on reading the MSC's error status register
>> (ESR) in the IRQ handler, which is not possible for MPAM-Fb based
>> MSC accesses, since they involve mailbox routines that might sleep.
>> The same is true for clearing the interrupt at the source, which
>> requires MSC access.
>>
>> For simplicity just skip the ESR read when the MSC is not using direct
>> MMIO accesses, and just ignore the pending interrupts. We will wrap up
>> MPAM functionality regardless, knowing the exact error value will not
>> change that.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 35 +++++++++++++++++++---------------
>>   1 file changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index b858ff389bff..4a088e6cd235 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -2639,7 +2639,7 @@ static int mpam_disable_msc_ecr(void *_msc)
>>   
>>   static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
>>   {
>> -	u64 reg;
>> +	u64 reg = 0;
>>   	u16 partid;
>>   	u8 errcode, pmg, ris;
>>   
>> @@ -2648,25 +2648,30 @@ static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
>>   					   &msc->accessibility)))
>>   		return IRQ_NONE;
>>   
>> -	mpam_msc_read_esr(msc, &reg);
>> +	/* MPAM-Fb MSC accesses cannot be done in atomic context. */
>> +	if (msc->iface == MPAM_IFACE_MMIO) {
>> +		mpam_msc_read_esr(msc, &reg);
>>   
>> -	errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
>> -	if (!errcode)
>> -		return IRQ_NONE;
>> +		errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
>> +		if (!errcode)
>> +			return IRQ_NONE;
>>   
>> -	/* Clear level triggered irq */
>> -	mpam_msc_clear_esr(msc);
>> +		/* Clear level triggered irq */
>> +		mpam_msc_clear_esr(msc);
>>   
>> -	partid = FIELD_GET(MPAMF_ESR_PARTID_MON, reg);
>> -	pmg = FIELD_GET(MPAMF_ESR_PMG, reg);
>> -	ris = FIELD_GET(MPAMF_ESR_RIS, reg);
>> +		partid = FIELD_GET(MPAMF_ESR_PARTID_MON, reg);
>> +		pmg = FIELD_GET(MPAMF_ESR_PMG, reg);
>> +		ris = FIELD_GET(MPAMF_ESR_RIS, reg);
>>   
>> -	pr_err_ratelimited("error irq from msc:%u '%s', partid:%u, pmg: %u, ris: %u\n",
>> -			   msc->id, mpam_errcode_names[errcode], partid, pmg,
>> -			   ris);
>> +		pr_err_ratelimited("error irq from msc:%u '%s', partid:%u, pmg: %u, ris: %u\n",
>> +				   msc->id, mpam_errcode_names[errcode], partid,
>> +				   pmg, ris);
>>   
>> -	/* Disable this interrupt. */
>> -	mpam_disable_msc_ecr(msc);
>> +		/* Disable this interrupt. */
>> +		mpam_disable_msc_ecr(msc);
> 
> As an error interrupt is final can we just disable the IRQ?

Doing that should be covered by mpam_unregister_irqs() as part of the 
mpam_broken_work, shouldn't it? Or do you want to do it earlier?

> Is it
> useful? I see there is a function disable_irq_no_sync().

If we want to do it earlier, the _nosync variant sounds promising, 
although the comment talks about it being nested, so I guess it would 
need to be balanced? Which might be tricky here, since I guess the IRQ 
would be disabled again in mpam_unregister_irqs()?

>> +	} else {
>> +		pr_err_ratelimited("unknown error irq from msc:%u\n", msc->id);
> 
> Should we report by irq number?
> As MSC may share interrupts we don't know which MSC caused the error irq
> at this point. On MMIO platforms we read the ESR to establish this.

I see what you mean, though I am not sure if the user would be able to 
make sense of any interrupt number? I would put it in anyway, more 
information doesn't hurt.

Cheers,
Andre

> Thanks,
> 
> Ben
> 
>> +	}
>>   
>>   	/* Are we racing with the thread disabling MPAM? */
>>   	if (!mpam_is_enabled())
>