Re: [PATCH v5 10/10] arm_mpam: detect and enable MPAM-Fb PCC support

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

Hi Lee,

On 7/30/26 03:34, Lee Trager wrote:
> On 7/29/26 6:41 AM, Andre Przywara wrote:
> 
>> @@ -2231,7 +2316,7 @@ static void mpam_msc_drv_remove(struct 
>> platform_device *pdev)
>>   static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device 
>> *pdev)
>>   {
>>       int err;
>> -    u32 tmp;
>> +    u32 pcc_subspace_id;
>>       struct mpam_msc *msc;
>>       struct resource *msc_res;
>>       struct device *dev = &pdev->dev;
>> @@ -2279,7 +2364,7 @@ static struct mpam_msc 
>> *do_mpam_msc_drv_probe(struct platform_device *pdev)
>>       if (err)
>>           return ERR_PTR(err);
>> -    if (device_property_read_u32(&pdev->dev, "pcc-channel", &tmp))
>> +    if (device_property_read_u32(dev, "pcc-channel", &pcc_subspace_id))
>>           msc->iface = MPAM_IFACE_MMIO;
>>       else
>>           msc->iface = MPAM_IFACE_PCC;
>> @@ -2295,6 +2380,36 @@ static struct mpam_msc 
>> *do_mpam_msc_drv_probe(struct platform_device *pdev)
>>           }
>>           msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>>           msc->mapped_hwpage = io;
>> +    } else if (msc->iface == MPAM_IFACE_PCC) {
>> +        int ret;
>> +
>> +        msc->pcc_chan = mpam_pcc_chan_get(dev, pcc_subspace_id);
>> +        if (IS_ERR(msc->pcc_chan)) {
>> +            pr_err("Failed to request MSC PCC channel\n");
>> +            return ERR_CAST(msc->pcc_chan);
>> +        }
>> +
>> +        if (msc->pcc_chan->pcc_chan->shmem_size < 
>> MPAM_FB_MAX_MSG_SIZE) {
> 
> I think the PCC shared-memory size undercounts the required region. From 
> my understanding shmem_size is the length of the complete PCC shared- 
> memory region advertised by the PCCT and mapped by the PCC driver. 
> MPAM_FB_MAX_MSG_SIZE is the 4-byte protocol header stored in pcc_shmem- 
>  >command + the 16-byte write payload.
> 
> However, mpam_fb_build_write_message() places the payload after the 
> complete 16 byte struct acpi_pcct_ext_pcc_shared_memory. The largest 
> request therefore occupies 32 bytes of shared memory region(16-byte PCC 
> header+ 16-byte write payload). Thus a firmware region between 20 and 31 
> bytes currently passes the check but overruns when constructing a write 
> request. I think this should be

Ah, you are right, there are indeed multiple things at play here.

 > if (msc->pcc_chan->pcc_chan->shmem_size <>
>      sizeof(struct acpi_pcct_ext_pcc_shared_memory) +
>      MPAM_FB_MAX_MSG_SIZE - MPAM_FB_PROT_HEADER_LEN)

I simplified that by just dropping MPAM_FB_PROT_HEADER_LEN from the 
MPAM_FB_MAX_MSG_SIZE definition.

Thanks for having a thorough look!

Cheers,
Andre

> 
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/ 
>> mpam_internal.h
>> index a2193e7df57c..e926b7feb72a 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -77,7 +77,6 @@ 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;
>> @@ -532,6 +531,11 @@ 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)
>> +/* The longest message is MPAM_MSC_WRITE, with 4 parameters. */
>> +#define MPAM_FB_MAX_MSG_SIZE    (MPAM_FB_PROT_HEADER_LEN + 4 * 
>> sizeof(u32))
>> +
>> +#define MPAM_FB_VERSION_MAJOR_MASK    GENMASK(31, 16)
>> +#define MPAM_FB_VERSION_MINOR_MASK    GENMASK(15, 0)
>>   /*
>>    * MPAM MSCs have the following register layout. See: