Re: [PATCH v3 13/16] arm_mpam: prepare mon_sel locking for MPAM-Fb

[Andre Przywara]

Hi Ben,

On 7/15/26 17:16, Ben Horgan wrote:
> Hi Andre,
>
> On 7/10/26 15:45, Andre Przywara wrote:
> > The MSC MON_SEL register needs to be accessed from hardirq for the overflow
> > interrupt, and when taking an IPI to access these registers on platforms
> > where MSC are not accesible from every CPU. This makes an irqsave
> > spinlock the obvious lock to protect these registers. On systems with SCMI
> > mailboxes it must be able to sleep, meaning a mutex must be used. The
> > SCMI platforms can't support an overflow interrupt.
> > Clearly these two can't exist for one MSC at the same time.
>
> Is an MPAM-Fb platform using overflow interrupts definitely not possible? I would have thought that
> as long as things were fast enough that threaded interrupts could be used. I do agree that it would
> make things harder and the increased latency may make it unworkable.

I guess they are possible, but we don't support them at the moment, 
right? And as you said a while ago, we'll cross that bridge when we come 
to it. I am sure we will find a nifty solution then, but don't need to 
sweat it now. I will reword that to make it clear.

> > Change the mon_sel locking wrapper function to only use a spinlock when
> > the MSC is accessed directly via MMIO. In case of MPAM-Fb, we use a
> > mutex, but only if we are in a sleepable context. If that's not the
> > case, we return an error. This should not happen, as MPAM-Fb by design
> > does not require an MSC access to happen from a specific CPU, so there
> > is no need for any IPIs or preemption disabling to satisfy CPU
> > constraints.
>
> It'd be good to mention that on the rare occasion that all cpus are no_hz_full and resctrl does the
> monitor read via IPI that we already bail out early.

OK, if you say so ;-)

Cheers,
Andre

> And since overflow interrupts are not supported at the moment
> > anyway, we also wouldn't meet the other case.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> >   drivers/resctrl/mpam_internal.h | 28 +++++++++++++++++++++++-----
> >   1 file changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> > index 04d1a59f02af..7b6e0df904f8 100644
> > --- a/drivers/resctrl/mpam_internal.h
> > +++ b/drivers/resctrl/mpam_internal.h
> > @@ -126,6 +126,7 @@ struct mpam_msc {
> >   	 */
> >   	raw_spinlock_t		_mon_sel_lock;
> >   	unsigned long		_mon_sel_flags;
> > +	struct mutex		mon_sel_mutex;
> >   
> >   	void __iomem		*mapped_hwpage;
> >   	size_t			mapped_hwpage_sz;
> > @@ -139,27 +140,44 @@ struct mpam_msc {
> >   /* Returning false here means accesses to mon_sel must fail and report an error. */
> >   static inline bool __must_check mpam_mon_sel_lock(struct mpam_msc *msc)
> >   {
> > -	/* Locking will require updating to support a firmware backed interface */
> > -	if (WARN_ON_ONCE(msc->iface != MPAM_IFACE_MMIO))
> > +	if (msc->iface == MPAM_IFACE_MMIO) {
> > +		raw_spin_lock_irqsave(&msc->_mon_sel_lock, msc->_mon_sel_flags);
> > +
> > +		return true;
> > +	}
> > +
> > +	if (!preemptible())
> >   		return false;
> >   
> > -	raw_spin_lock_irqsave(&msc->_mon_sel_lock, msc->_mon_sel_flags);
> > +	mutex_lock(&msc->mon_sel_mutex);
> > +
> >   	return true;
> >   }
> >   
> >   static inline void mpam_mon_sel_unlock(struct mpam_msc *msc)
> >   {
> > -	raw_spin_unlock_irqrestore(&msc->_mon_sel_lock, msc->_mon_sel_flags);
> > +	if (msc->iface == MPAM_IFACE_MMIO) {
> > +		raw_spin_unlock_irqrestore(&msc->_mon_sel_lock,
> > +					   msc->_mon_sel_flags);
> > +
> > +		return;
> > +	}
> > +
> > +	mutex_unlock(&msc->mon_sel_mutex);
> >   }
> >   
> >   static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
> >   {
> > -	lockdep_assert_held_once(&msc->_mon_sel_lock);
> > +	if (msc->iface == MPAM_IFACE_MMIO)
> > +		lockdep_assert_held_once(&msc->_mon_sel_lock);
> > +	else
> > +		lockdep_assert_held_once(&msc->mon_sel_mutex);
> >   }
> >   
> >   static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc)
> >   {
> >   	raw_spin_lock_init(&msc->_mon_sel_lock);
> > +	mutex_init(&msc->mon_sel_mutex);
> >   }
> >   
> >   /* Bits for mpam features bitmaps */