Re: [PATCH v5 08/10] arm_mpam: add MPAM-Fb MSC firmware access support

[Andre Przywara <andre.przywara@arm.com>]

Hey Srivathsa,

On 7/29/26 18:13, Srivathsa L Rao wrote:
> Hi Andre,
> 
> On 7/29/2026 7:11 PM, Andre Przywara wrote:
>> The Arm MPAM Firmware-backed (Fb) Profile document[1] describes an
>> alternative way of accessing the "Memory System Components" (MSC) in an
>> MPAM enabled system.
>>
>> Normally the MSCs are MMIO mapped, but in some implementations this
>> might not be possible (MSC located outside of the local socket, MSC
>> mapped secure-only) or desirable (direct MMIO access too slow or needs
>> to be mediated through a control processor). MPAM-fb standardises a
>> protocol to abstract MSC accesses, building on the SCMI protocol.
>>
>> Add functions that do an MSC read or write access by redirecting the
>> request through a firmware interface. For now this done via an ACPI
>> PCC shared memory and mailbox combination.
>>
>> Since the protocol used is only a small subset of the full SCMI spec,
>> and the SCMI protocol has no full ACPI support anyway, open-code the
>> (simple) SCMI message generation, for just the fields we need.
>>
>> [1] https://developer.arm.com/documentation/den0144/latest
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   drivers/resctrl/Makefile        |   2 +-
>>   drivers/resctrl/mpam_devices.c  |  52 ++++++--
>>   drivers/resctrl/mpam_fb.c       | 214 ++++++++++++++++++++++++++++++++
>>   drivers/resctrl/mpam_internal.h |  22 ++++
>>   include/linux/arm_mpam.h        |   2 +-
>>   5 files changed, 279 insertions(+), 13 deletions(-)
>>   create mode 100644 drivers/resctrl/mpam_fb.c
>>
>> diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile
>> index 4f6d0e81f9b8..097c036724e9 100644
>> --- a/drivers/resctrl/Makefile
>> +++ b/drivers/resctrl/Makefile
>> @@ -1,5 +1,5 @@
>>   obj-$(CONFIG_ARM64_MPAM_DRIVER)            += mpam.o
>> -mpam-y                        += mpam_devices.o
>> +mpam-y                        += mpam_devices.o mpam_fb.o
>>   mpam-$(CONFIG_ARM64_MPAM_RESCTRL_FS)        += mpam_resctrl.o
>>   ccflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG)    += -DDEBUG
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/ 
>> mpam_devices.c
>> index f6910ab3bbc2..abe1e628928f 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c

[ ... ]

>> +static int mpam_fb_send_request(struct mpam_pcc_chan *pcc_chan, u32 
>> msc_id,
>> +                u16 reg, u32 *result, int mpam_fb_command)
>> +{
>> +    unsigned int token = atomic_inc_return(&mpam_fb_token);
>> +    struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_shmem;
>> +    struct pcc_mbox_chan *chan;
>> +    void __iomem *payload_ofs;
>> +    u32 status;
>> +    int ret;
>> +
>> +    if (!pcc_chan)
>> +        return -ENODEV;
>> +
>> +    chan = pcc_chan->pcc_chan;
>> +
>> +    /* prune token to fit into the 10 bits inside the command 
>> register */
>> +    token = FIELD_GET(MPAM_MSC_TOKEN_MASK,
>> +              FIELD_PREP(MPAM_MSC_TOKEN_MASK, token));
>> +
>> +    guard(mutex)(&pcc_chan->pcc_chan_lock);
>> +
>> +    switch (mpam_fb_command) {
>> +    case MPAM_MSC_WRITE_CMD:
>> +        mpam_fb_build_write_message(msc_id, reg, *result,
>> +                        token, chan->shmem);
>> +        break;
>> +    case MPAM_MSC_READ_CMD:
>> +        mpam_fb_build_read_message(msc_id, reg, token, chan->shmem);
>> +        break;
>> +    case MPAM_PROTOCOL_VERSION_CMD:
>> +        mpam_fb_build_version_message(token, chan->shmem);
>> +        break;
>> +    default:
>> +        dev_err(pcc_chan->pcc_cl.dev, "unsupported MPAM-Fb command 
>> %d\n",
>> +            mpam_fb_command);
>> +        ret = -EINVAL;
>> +        goto out_err;
>> +    }
>> +
>> +    ret = mbox_send_message(chan->mchan, NULL);
>> +    if (ret < 0)
>> +        goto out_err;
>> +
>> +    pcc_shmem = chan->shmem;
>> +    payload_ofs = chan->shmem + sizeof(*pcc_shmem);
>> +    status = readl(&pcc_shmem->command);
>> +    if (FIELD_GET(MPAM_MSC_TOKEN_MASK, status) != token) {
>> +        ret = -ETIMEDOUT;
>> +
>> +        goto out_err;
>> +    }
>> +
>> +    ret = readl(payload_ofs + 0x0);
>> +    if (ret < 0) {
>> +        switch (ret) {
>> +        case MPAM_FB_ERR_NOT_SUPPORTED:
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        case MPAM_FB_ERR_INVALID_PARAMETERS:
>> +            ret = -EINVAL;
>> +            break;
>> +        case MPAM_FB_ERR_NOT_FOUND:
>> +            ret = -ENOENT;
>> +            break;
>> +        case MPAM_FB_ERR_OUT_OF_RANGE:
>> +            ret = -ERANGE;
>> +            break;
> 
> While testing v4 on a QEMU setup with a fake PCC-backed MSC, I added a
> small error injection mechanism to verify the firmware response status
> code translations in mpam_fb_send_request(). I injected each defined
> MPAM_FB_ERR_* code and observed the following.

Ah, very nice, thanks for doing this!

>> +        default:
>> +            ret = -EINVAL;
>> +        }
>> +
>> +        goto out_err;
>> +    }
> 
> MPAM_FB_ERR_BUSY (-6) falls through to this default and gets -EINVAL.
> Would -EAGAIN be more appropriate here?  Callers could then maybe add a 

Oh, sure, I somehow missed that, even though it's one of the more 
obvious mappings and probably even the most useful one.

Thanks for catching that, added.

Cheers,
Andre

> short retry loop inside mpam_fb_send_request() itself, or in the probe 
> path, convert -EAGAIN to -EPROBE_DEFER so the driver core retries probe 
> automatically.
> 
> The other unhandled codes also collapse to -EINVAL, like EPROTO, EBUSY, 
> I guess that can come later.
> 
>> +
>> +    if (mpam_fb_command != MPAM_MSC_WRITE_CMD)
>> +        *result = readl(payload_ofs + 0x4);
>> +
>> +    return 0;
>> +
>> +out_err:
>> +    mpam_fb_disable_mpam(ret);
>> +
>> +    return ret;
>> +}
>> +
>> +int mpam_fb_send_read_request(struct mpam_msc *msc, u16 reg, u32 
>> *result)
>> +{
>> +    return mpam_fb_send_request(msc->pcc_chan, msc->mpam_fb_msc_id,
>> +                    reg, result, MPAM_MSC_READ_CMD);
>> +}
>> +
>> +int mpam_fb_send_write_request(struct mpam_msc *msc, u16 reg, u32 value)
>> +{
>> +    return mpam_fb_send_request(msc->pcc_chan, msc->mpam_fb_msc_id,
>> +                    reg, &value, MPAM_MSC_WRITE_CMD);
>> +}
>> +
>> +int mpam_fb_get_protocol_version(struct mpam_msc *msc)
>> +{
>> +    u32 version;
>> +    int ret;
>> +
>> +    ret = mpam_fb_send_request(msc->pcc_chan, 0,
>> +                   0, &version, MPAM_PROTOCOL_VERSION_CMD);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return version;
>> +}
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/ 
>> mpam_internal.h
>> index 2b81b6b0bf4e..a2193e7df57c 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -11,6 +11,7 @@
>>   #include <linux/io.h>
>>   #include <linux/jump_label.h>
>>   #include <linux/llist.h>
>> +#include <linux/mailbox_client.h>
>>   #include <linux/mutex.h>
>>   #include <linux/resctrl.h>
>>   #include <linux/spinlock.h>
>> @@ -57,6 +58,15 @@ struct mpam_garbage {
>>       struct platform_device    *pdev;
>>   };
>> +struct mpam_pcc_chan {
>> +    struct list_head    pcc_chans;
>> +    struct mbox_client    pcc_cl;
>> +    struct pcc_mbox_chan    *pcc_chan;
>> +    struct mutex        pcc_chan_lock; /* only one message at a time */
>> +    struct kref        refcount;
>> +    int            subspace_id;
>> +};
>> +
>>   struct mpam_msc {
>>       /* member of mpam_all_msc */
>>       struct list_head    all_msc_list;
>> @@ -66,6 +76,8 @@ struct mpam_msc {
>>       /* Not modified after mpam_is_enabled() becomes true */
>>       enum mpam_msc_iface    iface;
>> +    struct mpam_pcc_chan    *pcc_chan;
>> +    int            mpam_fb_msc_id;    /* in its own name space */
>>       u32            nrdy_usec;
>>       cpumask_t        accessibility;
>>       bool            has_extd_esr;
>> @@ -484,6 +496,9 @@ extern u8 mpam_pmg_max;
>>   void mpam_enable(struct work_struct *work);
>>   void mpam_disable(struct work_struct *work);
>> +/* helper function to call from outside mpam_devices.c */
>> +void mpam_fb_disable_mpam(int err);
>> +
>>   /* Reset all the RIS in a class under cpus_read_lock() */
>>   void mpam_reset_class_locked(struct mpam_class *class);
>> @@ -511,6 +526,13 @@ static inline void 
>> mpam_resctrl_offline_cpu(unsigned int cpu) { }
>>   static inline void mpam_resctrl_teardown_class(struct mpam_class 
>> *class) { }
>>   #endif /* CONFIG_RESCTRL_FS */
>> +/* MPAM-Fb Firmware-backed protocol wrappers */
>> +int mpam_fb_send_read_request(struct mpam_msc *msc, u16 reg, u32 
>> *result);
>> +int mpam_fb_send_write_request(struct mpam_msc *msc, u16 reg, u32 
>> value);
>> +int mpam_fb_get_protocol_version(struct mpam_msc *msc);
>> +
>> +#define MPAM_FB_PROT_HEADER_LEN        sizeof(u32)
>> +
>>   /*
>>    * MPAM MSCs have the following register layout. See:
>>    * Arm Memory System Resource Partitioning and Monitoring (MPAM) System
>> diff --git a/include/linux/arm_mpam.h b/include/linux/arm_mpam.h
>> index f92a36187a52..002f56e15362 100644
>> --- a/include/linux/arm_mpam.h
>> +++ b/include/linux/arm_mpam.h
>> @@ -12,7 +12,7 @@ struct mpam_msc;
>>   enum mpam_msc_iface {
>>       MPAM_IFACE_MMIO,    /* a real MPAM MSC */
>> -    MPAM_IFACE_PCC,        /* a fake MPAM MSC */
>> +    MPAM_IFACE_PCC,        /* using the MPAM-Fb firmware redirection */
>>   };
>>   enum mpam_class_types {
> 
> Best Regards,
> Srivathsa