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

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

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?

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

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