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

[Ben Horgan]

Hi Andre,

On 7/9/26 13:06, Andre Przywara wrote:
> 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@xxxxxxx>
>>> ---
>>>   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?


I think leaving it to mpam_broken_work risks having the hard irq handler
run again and again once we get an error interrupt. I'm just looking at
the code today but I think this is analogous to using IRQF_ONESHOT to
mask the interrupt until the threaded handler is finished.

> 
>> 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()?

mpam_unregister_irqs() disables the interrupt by clearing the msc enable
in mpam_disable_msc_ecr() rather than disabling by irq. I don't see how
this would cause a problem. Or does free_percpu_irq() or devm_free_irq()
do something incompatible?

> 
>>> +    } 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())
>>
>