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

[Andre Przywara]

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: