Re: [PATCH v8 00/11] arm_mpam: Add MPAM-Fb firmware support

[Ben Horgan <ben.horgan@arm.com>]

Hi Andre,

On 8/4/26 13:28, Andre Przywara wrote:
> Hi,
> 
> On 8/4/26 12:06, Andre Przywara wrote:
>> this is v8 of the MPAM-Fb code, for firmware based MSC accesses.
>> It fixes a potential use-after-free of the dev pointer on error
>> prints, and disables the IRQ on the irqchip side in case doing it on
>> the device side doesn't work. The rest are cosmetic fixes and adding
>> the tags from the diligent reviewers and testers, many thanks for that!
>> I dropped tags from the error IRQ patch (10/11) due to changes.
>> Find the changelog below. Based on v7.2-rc1.
>>
>> =======================
>> The Arm MPAM specification defines Memory System Components (MSCs),
>> which are devices that are programmed through an MMIO register frame. In
>> some occasions this turned out to be too limiting: the MSC might be
>> located behind a separate bus system (for instance inside an on-board
>> controller), it might be mapped secure-only, or in a different processor
>> socket without direct MMIO mapping. Also the MMIO access might be too slow
>> or it would need to be filtered or otherwise access controlled. Finally
>> there might be bugs in the MSC integration, which require a mediating
>> firmware to be accessible.
> 
> 
> For the records, I looked at the sashiko complaints, by my analysis most comments fall into one of
> those categories:
> 
> - Does this new early return / error return .... break MPAM logic?
> It's concerned about just bailing out from the middle of a sequence of MPAM read or writes, leaving
> the MSC in a misconfigured state. This is true, but doesn't apply, because we tear down MPAM
> completely upon an MPAM-Fb access returning an error. And if the communication between the kernel
> and an MSC breaks down completely, there isn't much we can do anyway: attempts to recover would fail
> as well.
> 
> - Since xxx() now returns an error code, do the callers need
>   to be updated to check it?
> - Are there missing error checks in xxx() related to the new error
>   return values?
> Yes, this is done in one of the following patches. The series is constructed so that only the final
> patch enables MPAM-Fb, before that there is no way MPAM-Fb could be instantiated, so only MMIO
> accesses are happening. They always return 0, so no error could occur and error paths are never
> exercised.
> This is used to construct the functionality gradually, and allow multiple, but smaller patches.
> Mentioned in the cover letter.

Just to add to this. The mpam_reset_component_locked() doesn't propagate the error, in
reset_arch_reset_all_ctrls() this is ok as this is only called when resctrl is being disabled. The
other place this is called from is the cpu hotplug handlers which aren't called by a user. So there
is no user to propagate the error up to.

The one valid concern as far as I can see is that mpam_disable_msc_ecr() always returns 0 and so the
disable_irq_nosync() can never be called.

Thanks,

Ben

> 
> - This is a pre-existing issue, but ....
> I haven't checked in detail, but from previous rounds those concerns were addressed by Ben's fixes,
> which IIUC are on its way already. This series is based on v7.2-rc1, so those fixes are not applied
> here.
> 
> Hope that helps,
> Cheers,
> Andre
> 
>> To accommodate all those different use cases, the MPAM-Fb specification
>> [1] describes an alternative way to access MSCs. Accesses to an MSC
>> would be wrapped in a message and communicated to the system using a
>> shared-memory/mailbox system mostly mimicking the Arm SCMI spec.
>> For ACPI systems, this would be abstracted through an ACPI PCC channel,
>> which provides the shared-memory region and the mailbox trigger. We can
>> lean on existing ACPI parsing code to register with these two
>> subsystems, but cannot rely on the existing SCMI code in the kernel.
>> This means we somewhat need to open code a very simplified SCMI handler,
>> which just provides enough functionality for the very basic subset of
>> SCMI that the MPAM-Fb spec requires.
>>
>> The first seven patches rework all MSC access wrappers to propagate error
>> information. Pure MMIO based MSC accesses would never fail, but the
>> MPAM-Fb access can go wrong in multiple ways. The patches have been split
>> up purely for reviewing reasons, if the number is a problem, we could as
>> well squash them. Please note that until the very last patch of this series
>> any MSC accesses would always only return 0, it's only the final enablement
>> of MPAM-Fb that could possibly introduce errors. Hence all former patches
>> can add error handling gradually, those code paths wouldn't be triggered
>> before patch 11/11.
>> Patch 8/11 solves a nasty problem: At the moment we protect stateful MSC
>> register accesses (mon_sel) through a spinlock. Unfortunately the mailbox
>> subsystem and the slow nature of the communication through this channel
>> forbid MPAM-Fb access in atomic context. So this patch keeps using a
>> spinlock for MMIO based accesses, but reverts to a mutex otherwise.
>> We just deny taking the lock for MPAM-Fb in atomic context, ideally we
>> wouldn't need that (no need to IPI another core when the MSC access does
>> not need to be local to one particular core), or we simply deny that part
>> of the functionality (access through perf).
>> Patch 9/11 adds the code to redirect MSC accesses through the
>> PCC shmem/mailbox system.
>> Patch 10/11 reworks the error interrupt handler to use a threaded IRQ for
>> MPAM-Fb, to be able to do MPAM-Fb MSC accesses inside (which might sleep).
>> The final patch 11/11 then adds the code to detect and store the PCC
>> channel information from the ACPI tables, and eventually enables
>> MPAM-Fb accesses.
>>
>> This would enable systems where some MSCs are not accessible via MMIO to
>> use those components anyway.
>>
>> Please have a look and test!
>>
>> Cheers,
>> Andre
>>
>> [1] https://developer.arm.com/documentation/den0144/latest
>>
>> Changes in v8:
>> - disable error IRQ on irqchip level when device side fails
>> - use MSC dev pointer for dev_err() instead of mailbox one
>> - smaller white space and formatting fixes
>> - reorder MPAM-Fb definitions
>> - rename error translation function to mpam_fb_to_linux_errno()
>> - add review and test tags
>>
>> Changes in v7:
>> - add tags
>> - add new patch to propagate errors in mpam_reprogram_ris_partid()
>> - prevent loop when mpam_diable() tries MSC accesses again
>> - register MMIO error IRQ handler without IRQF_ONESHOT
>> - re-use existing "err" variable instead of declaring "ret"
>> - initialise mon_sel_lock later, to wait for interface decision
>> - convert timeout units for nominal latency, between us and ms
>>   Changes in v6:
>> - keep using hard IRQ handler for MMIO based accesses
>> - move shmem buffer size and MPAM-Fb version check into mpam_fb.c
>> - drop mutex guard in mpam_fb_send_request(), we use gotos anyway
>> - factor out MPAM-Fb protocol error translation
>> - add MPAM_FB_ERR_BUSY error code to translation
>> - explicitly report also MPAM-Fb protocol error number when disabling MPAM
>> - remove unneeded MPAM-Fb details from mpam_internal.h
>> - drop leftover mpam_fb_msc_id usage
>>
>> Changes in v5:
>> - do not initialise _high variables early
>> - fix whitespace issues
>> - add Jonathan's Reviewed-by: tags
>> - init only either the spinlock OR the mutex for the mon_sel_lock
>> - drop warning about mapped_hwpage_sz for MPAM-Fb in mpam_msc_zero_mbwu_l()
>> - change IRQ handler to threaded handler, to allow MPAM-Fb MSC accesses
>> - disallow per-CPU IRQs (PPIs) for MPAM-Fb
>> - trigger MPAM disabling on MPAM-Fb protocol errors
>> - fix MPAM_PROTOCOL_VERSION_CMD name
>> - use PCC driver defined symbol for PCC_CHAN_FLAGS_IRQ
>> - add comment about usage of PCC's .signature field
>> - check for illegal MPAM-Fb command before sending request
>> - drop CPU accessibility check for MPAM-Fb based MSC accesses
>> - move MPAM-Fb protocol message structs inside build functions
>> - add comment about largest message size
>> - drop extra msc->mpam_fb_msc_id, in favour using pdev->id
>> - use already defined local dev variable instead of &pdev->dev
>> - use FIELD_GET() macro for MPAM-Fb protocol check
>> - drop devm_mutex_init() for pcc_chan_lock
>>
>> Changes in v4:
>> - unify error check patterns
>> - use scoped_guard(mutex) and ACQUIRE to simplify error handling
>> - use struct kref for PCC channel refcount
>> - drop unused mailbox rx_callback function
>> - increase size of props[] array to hold new msc_id property
>> - set mailbox TX timeout based on ACPI MPAM table field
>> - use devm_mutex_init() to allow automatic cleanup
>> - avoid overwriting error value for arm,not-ready-us property read
>> - prune MPAM-Fb protocol token to fit into bitmask inside register
>> - check return value for AIDR read
>> - use two separate MPAM-Fb protocol payload structs
>> - prune header list for mpam_fb.c
>> - drop "nrdy handled as an error" change
>> - fix message size field in MPAM-Fb protocol shmem area
>> - drop message size return from MPAM-Fb message build functions
>> - bring back IRQ flag in MPAM-Fb protocol header
>>
>> Changes in v3:
>> - drop inner/outer mon_sel lock patch, replace with simpler version
>> - harmonise code patterns in error propagation changes
>> - drop mon_sel lock before erroring out in mpam_ris_hw_probe_csu_nrdy()
>> - refactor mpam_msc_read_mbwu_l() to return an error
>> - drop special NRDY handling in __ris_msmon_read()
>> - add IRQ numbers in error interrupt handler to help pinpoint failure
>> - refactor MPAM-Fb message generation to accommodate more than read/write
>> - check MPAM-Fb protocol version at probe time
>> - drop mpam_fb.h header, merge into mpam_internal.h
>> - translate MPAM-Fb error code in Linux codes where needed
>> - clear IRQ flag bit in MPAM-Fb protocol header
>> - drop unneeded endianness conversions when crafting MPAM-Fb message
>> - use C struct to model MPAM-Fb message payload
>>
>> Changes in v2:
>> - add patches to add error propagation to MSC access wrappers
>> - drop former patch 1/5 (not needed)
>> - drop lock in mpam_reprogram_msc(), to avoid double lock
>> - add support for multiple MSCs per PCC channel
>> - adjust SCMI protocol code to use a PCC subtype 3 channel
>> - let PCC code handle the PCC channel negotiation (due to subtype 3)
>> - drop SCMI names in shmem offsets, and use existing PCC type 3 struct
>> - adjust shmem field offsets to match MPAM-Fb spec, not pure SCMI
>> - prevent MPAM-Fb calls inside atomic smp_call_function_any() payload
>> - skip all MSC accesses inside the IRQ handler when using MPAM-Fb
>>
>> Andre Przywara (11):
>>    arm_mpam: let low level MSC accessors return an error
>>    arm_mpam: propagate MSC access errors for hw_probe functions
>>    arm_mpam: propagate MSC access errors for MBWU counters
>>    arm_mpam: propagate MSC access errors for msmon helpers
>>    arm_mpam: propagate MSC access errors for __ris_msmon_read()
>>    arm_mpam: propagate MSC access errors for state saving function
>>    arm_mpam: propagate MSC access errors for mpam_reprogram_ris_partid()
>>    arm_mpam: prepare mon_sel locking for MPAM-Fb
>>    arm_mpam: add MPAM-Fb MSC firmware access support
>>    arm_mpam: change MPAM-Fb error IRQ to use a threaded IRQ handler
>>    arm_mpam: detect and enable MPAM-Fb PCC support
>>
>>   drivers/resctrl/Makefile        |   2 +-
>>   drivers/resctrl/mpam_devices.c  | 825 ++++++++++++++++++++++++--------
>>   drivers/resctrl/mpam_fb.c       | 252 ++++++++++
>>   drivers/resctrl/mpam_internal.h |  62 ++-
>>   include/linux/arm_mpam.h        |   2 +-
>>   5 files changed, 926 insertions(+), 217 deletions(-)
>>   create mode 100644 drivers/resctrl/mpam_fb.c
>>
>>
>> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
>