Re: [PATCH v3 14/16] arm_mpam: add MPAM-Fb MSC firmware access support

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

Hi Niyas,

On 7/14/26 14:27, Niyas Sait wrote:
> On 10/07/2026 15:45, Andre Przywara wrote:
> 
>> +#define MPAM_VERSION_MSG_SIZE    (PCC_TYPE3_MSG_PAYLOAD_OFS)
>> +#define MPAM_READ_MSG_SIZE    (PCC_TYPE3_MSG_PAYLOAD_OFS + 3 * 
>> sizeof(u32))
>> +#define MPAM_WRITE_MSG_SIZE    (PCC_TYPE3_MSG_PAYLOAD_OFS + 4 * 
>> sizeof(u32))
> 
> I think these lengths are wrong for ACPI extended PCC shared memory.
> 
> Length should be command + payload, and should not include the payload 
> offset within the PCC shared memory region.
> 
> I think this should be something like
> 
> #define MPAM_VERSION_MSG_SIZE  sizeof(u32)
> #define MPAM_READ_MSG_SIZE     (sizeof(u32) + 3 * sizeof(u32))
> #define MPAM_WRITE_MSG_SIZE    (sizeof(u32) + 4 * sizeof(u32))

Yes, that's right, that's just the MPAM-Fb visible part of the protocol 
message, not the potentially preceding SCMI header.
Adjusted it like that.

> 
>> +
>> +static int mpam_fb_build_version_message(unsigned int token,
>> +                     void __iomem *msg_buf)
>> +{
>> +    struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem = msg_buf;
>> +
>> +    writel_relaxed(0, &pcc_shmem->flags);
> 
> Here the flags are always 0.
> 
> If the PCCT advertises interrupt based completion, I think we need to 
> set PCC_CMD_COMPLETION_NOTIFY here. Otherwise the platform can process 
> the request without generating an interrupt back to the AP, and the PCC 
> driver will time out waiting for completion.

So yeah, Ben commented on the previous version that we don't know if the 
platform requires an IRQ or not, so we better leave that 0.
But on platforms that do require one, we probably must set that flag. 
TBH it's a bit odd to have it, since it muddles transport and protocol, 
I'd say. In the kernel it shows because the client doesn't  have easy 
access to the PCCT options, that's hidden away by the mailbox abstraction.
I will bring the flag back, under the assumption that an agent not 
supporting interrupts would ignore it.
One other way I can think of is to peek into the mbox_chan mailbox 
controller structure and look at the txdone_method member, to check 
whether it says polling or not. But that sounds like cheating and 
violating the abstraction. If anyone has a better idea, I am all ears.

>> +
>> +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);
> 
> MPAM-Fb token is a 10-bit field in the message header, so this needs to 
> be masked before using it. Otherwise once the token exceeds 1023, the 
> platform returns the wrapped 10-bit token but Linux compares it against 
> the original value and will probably time out.

Ouch, that's a good one, thanks for spotting this! I confirmed it by 
initialising the token value to 1000, and indeed it stops with an error 
after 24 transactions. Fixed now.

Many thanks,
Andre

> 
> May be something like:
> 
> unsigned int token = atomic_inc_return(&mpam_fb_token) & 
> FIELD_MAX(MPAM_MSC_TOKEN_MASK);
> 
> 
> Thanks,
> Niyas