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

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

Hi Jonathan,

On 7/28/26 01:22, Jonathan Cameron wrote:
> On Thu, 23 Jul 2026 17:54:52 +0200
> Andre Przywara <andre.przywara@arm.com> 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>
> Just a couple of suggestions that you can ignore if you like
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Thanks!

> 
>> diff --git a/drivers/resctrl/mpam_fb.c b/drivers/resctrl/mpam_fb.c
>> new file mode 100644
>> index 000000000000..ec4bf0dd9182
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_fb.c
>> @@ -0,0 +1,197 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2024 Arm Ltd.
> 
> Arm has either been working on this for rather a long time or that's a cut
> and paste.  Even if 2024 is a good start given changes during revisions
> seems reasonable to include 2026 as well.

===================
Date:   Tue May 21 11:26:16 2024 +0100

     firmware: arm_scmi: add MPAM-FB SCMI protocol stub
========================

So yes, it's been in the making for a while, although in a different 
shape and form.
But will surely add the current year to the range.

>> +struct mpam_fb_read_payload {
>> +	u32 msc_id;
>> +	u32 flags;
>> +	u32 reg_offset;
>> +} __packed;
>> +
>> +struct mpam_fb_write_payload {
> 
> Given these are only used in the specific functions that build the
> messages you could move the structure definitions to be local to those
> functions.
> 
> Alternatively you could move all these to the header and get the advantage
> of using a union to get the maximum message size rather than a number
> currently picked out of the air.

A union sounds nice, but it feels a bit over the top, and I think 
keeping the structs local to their users trumps this. After all this is 
what mpam_fb.c does: deal with the protocol internals, without bothering 
the rest of the code.

> 
>> +	u32 msc_id;
>> +	u32 flags;
>> +	u32 reg_offset;
>> +	u32 value;
>> +} __packed;
> 
> 
> 
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
>> index b3a6ed9ed175..e4686ff0ba2e 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
> 
> ...
> 
>> +#define MPAM_FB_PROT_HEADER_LEN	sizeof(u32)
>> +#define MPAM_FB_MAX_MSG_SIZE	(MPAM_FB_PROT_HEADER_LEN + 4 * sizeof(u32))
> 
> Doesn't seem to be used until patch 10.  Might be more sensible to bring it
> in there. A spec reference might also be good for this.  I think longest
> is current the MPAM_MSC_WRITE.  A note above that one option would be
> to make this explicit by having the message structure definitions in this
> header.

Yes, but I will not boil the ocean, and just add a comment.

Thanks,
Andre