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

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

Hi Andre,

On 7/10/26 08:31, Andre Przywara wrote:
> Hi,
> 
> On 7/9/26 15:27, Ben Horgan wrote:
>> 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@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?
>>
>>
>> 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.
> 
> Maybe, but masking and disabling are not the same thing, and ONESHOT
> most probably takes care of any imbalances. Whereas when we just disable
> the IRQ here on some occasions, without telling anyone, it may leave it
> in a weird state?

Ok, that could be problematic.

> 
>>
>>>
>>>> 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?
> 
> According to the description of free_irq(), it does disable the IRQ when
> no user is left. I'd assume it does this via the same mechanism we are
> trying to use, which would cause an imbalance?

Can we get around this by using a ONESHOT threaded interrupt and just
using the threaded part to do mpam_disable_msc_ecr() and queue the
mpam_broken_work?

Thanks,

Ben

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