linux-kernel - Re: [PATCH v2 17/29] arm_mpam: Extend reset logic to allow devices to be reset any time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c94e9d28-c678-4397-aad4-b17ee6ba179c@arm.com>
Date: Thu, 2 Oct 2025 19:02:32 +0100
From: James Morse <james.morse@....com>
To: Fenghua Yu <fenghuay@...dia.com>, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org
Cc: D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 baisheng.gao@...soc.com, Jonathan Cameron <jonathan.cameron@...wei.com>,
 Rob Herring <robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
 Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
 <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH v2 17/29] arm_mpam: Extend reset logic to allow devices to
 be reset any time

Hi Fenghua,

On 25/09/2025 08:16, Fenghua Yu wrote:
> On 9/10/25 13:42, James Morse wrote:
>> cpuhp callbacks aren't the only time the MSC configuration may need to
>> be reset. Resctrl has an API call to reset a class.
>> If an MPAM error interrupt arrives it indicates the driver has
>> misprogrammed an MSC. The safest thing to do is reset all the MSCs
>> and disable MPAM.
>>
>> Add a helper to reset RIS via their class. Call this from mpam_disable(),
>> which can be scheduled from the error interrupt handler.


>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index e7faf453b5d7..a9d3c4b09976 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>>   @@ -1340,8 +1338,56 @@ static void mpam_enable_once(void)
>>              mpam_partid_max + 1, mpam_pmg_max + 1);
>>   }
>>   +static void mpam_reset_component_locked(struct mpam_component *comp)
>> +{
>> +    struct mpam_msc *msc;
>> +    struct mpam_vmsc *vmsc;
>> +    struct mpam_msc_ris *ris;
>> +
>> +    lockdep_assert_cpus_held();
>> +
>> +    guard(srcu)(&mpam_srcu);
>> +   
> 
> Nested locks on mpam_srcu in this call chain:
> 
> mpam_disable() -> mpam_reset_class() -> mpam_reset_class_locked() -> mpam_component_locked()

These are allowed to nest like this.


> There are redundant locks on mpam_srcu in mpam_disabled(), mpam_reset_class_locked(), and
> mpam_reset_component_locked().
> 
> It's better to guard mpam_srcu only in the top function mpam_disable() for simpler logic
> and lower overhead.

These things don't block, so there no real overhead. Shuffling them around to avoid the
harmless nesting would likely complicate the flow, not simplify it.

In the reset case you point at here, the resctrl code would need to take the srcu lock
before calling it - which is exposing the innards of what that function does.



>> list_for_each_entry_srcu(vmsc, &comp->vmsc, comp_list,
>> +                 srcu_read_lock_held(&mpam_srcu)) {
>> +        msc = vmsc->msc;
>> +
>> +        list_for_each_entry_srcu(ris, &vmsc->ris, vmsc_list,
>> +                     srcu_read_lock_held(&mpam_srcu)) {
>> +            if (!ris->in_reset_state)
>> +                mpam_touch_msc(msc, mpam_reset_ris, ris);
>> +            ris->in_reset_state = true;
>> +        }
>> +    }
>> +}


>> +/*
>> + * Called in response to an error IRQ.
>> + * All of MPAMs errors indicate a software bug, restore any modified
>> + * controls to their reset values.
>> + */
>>   void mpam_disable(struct work_struct *ignored)
>>   {
>> +    int idx;
>> +    struct mpam_class *class;
>>       struct mpam_msc *msc, *tmp;
>>         mutex_lock(&mpam_cpuhp_state_lock);
>> @@ -1351,6 +1397,12 @@ void mpam_disable(struct work_struct *ignored)
>>       }
>>       mutex_unlock(&mpam_cpuhp_state_lock);
>>   +    idx = srcu_read_lock(&mpam_srcu);

> It's better to change to guard(srcu)(&mpam_srcu);

For this one - absolutely not.
The guard() thing allows the toolchain to decide when to drop the lock. Further down in
this same function is an attempt to free the memory that got deferred. Guess what happens
if you call synchronize_srcu() while still in an srcu read side section....


>> +    list_for_each_entry_srcu(class, &mpam_classes, classes_list,
>> +                 srcu_read_lock_held(&mpam_srcu))
>> +        mpam_reset_class(class);
>> +    srcu_read_unlock(&mpam_srcu, idx);
>> +
>>       mutex_lock(&mpam_list_lock);
>>       list_for_each_entry_safe(msc, tmp, &mpam_all_msc, all_msc_list)
>>           mpam_msc_destroy(msc);


Thanks,

James