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

[Andre Przywara]

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?

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