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

[Andre Przywara]

Hi Jonathan,

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

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