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

Hi,

On 8/4/26 14:50, Ben Horgan wrote:
> 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.

Yes, I just saw this as well, for some reason I missed that conversion. 
It's a very simple fix, of course.

Cheers,
Andre

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