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

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 <[email protected]>
>> ---
>>   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 */
>