Re: [PATCH 14/33] arm_mpam: Probe hardware to find the supported partid/pmg values

[Ben Horgan <ben.horgan@arm.com>]

Hi Gavin,

On 11/10/25 23:26, Gavin Shan wrote:
> Hi Ben,
> 
> On 11/9/25 10:43 AM, Gavin Shan wrote:
>> On 11/7/25 10:34 PM, Ben Horgan wrote:
>>> From: James Morse <james.morse@arm.com>
>>>
>>> CPUs can generate traffic with a range of PARTID and PMG values,
>>> but each MSC may also have its own maximum size for these fields.
>>> Before MPAM can be used, the driver needs to probe each RIS on
>>> each MSC, to find the system-wide smallest value that can be used.
>>> The limits from requestors (e.g. CPUs) also need taking into account.
>>>
>>> While doing this, RIS entries that firmware didn't describe are created
>>> under MPAM_CLASS_UNKNOWN.
>>>
>>> This adds the low level MSC write accessors.
>>>
>>> While we're here, implement the mpam_register_requestor() call
>>> for the arch code to register the CPU limits. Future callers of this
>>> will tell us about the SMMU and ITS.
>>>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Reviewed-by: Ben Horgan <ben.horgan@arm.com>
>>> Tested-by: Fenghua Yu <fenghuay@nvidia.com>
>>> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>>> Tested-by: Peter Newman <peternewman@google.com>
>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>> ---
>>> Changes since v3:
>>>  From Jonathan:
>>> Stray comma in printk
>>> Unnecessary braces
>>> ---
>>>   drivers/resctrl/mpam_devices.c  | 148 +++++++++++++++++++++++++++++++-
>>>   drivers/resctrl/mpam_internal.h |   6 ++
>>>   include/linux/arm_mpam.h        |  14 +++
>>>   3 files changed, 167 insertions(+), 1 deletion(-)
[...]
>>>   static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>>   {
>>>       u64 idr;
>>> +    u16 partid_max;
>>> +    u8 ris_idx, pmg_max;
>>> +    struct mpam_msc_ris *ris;
>>>       struct device *dev = &msc->pdev->dev;
>>>       lockdep_assert_held(&msc->probe_lock);
>>> @@ -464,6 +564,40 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>>           return -EIO;
>>>       }
>>> +    /* Grab an IDR value to find out how many RIS there are */
>>> +    mutex_lock(&msc->part_sel_lock);
>>> +    idr = mpam_msc_read_idr(msc);
>>> +    mutex_unlock(&msc->part_sel_lock);
>>> +
>>> +    msc->ris_max = FIELD_GET(MPAMF_IDR_RIS_MAX, idr);
>>> +
>>> +    /* Use these values so partid/pmg always starts with a valid
>>> value */
>>> +    msc->partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
>>> +    msc->pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
>>> +
>>> +    for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
>>> +        mutex_lock(&msc->part_sel_lock);
>>> +        __mpam_part_sel(ris_idx, 0, msc);
>>> +        idr = mpam_msc_read_idr(msc);
>>> +        mutex_unlock(&msc->part_sel_lock);
>>> +
>>> +        partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
>>> +        pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
>>> +        msc->partid_max = min(msc->partid_max, partid_max);
>>> +        msc->pmg_max = min(msc->pmg_max, pmg_max);
>>> +
>>> +        mutex_lock(&mpam_list_lock);
>>> +        ris = mpam_get_or_create_ris(msc, ris_idx);
>>> +        mutex_unlock(&mpam_list_lock);
>>> +        if (IS_ERR(ris))
>>> +            return PTR_ERR(ris);
>>> +    }
>>> +
>>> +    spin_lock(&partid_max_lock);
>>> +    mpam_partid_max = min(mpam_partid_max, msc->partid_max);
>>> +    mpam_pmg_max = min(mpam_pmg_max, msc->pmg_max);
>>> +    spin_unlock(&partid_max_lock);
>>> +
> 
> mpam_register_requestor() could be used here to avoid the capacities
> (maximal PARTIDs and PMGs) are unexpectedly lowered.
> 

I agree that this is somewhat surprising that without a requestor the
driver supports 1 PARTID and 1 PMG, but it is intentional behaviour. The
driver is only intended to be fully functional when a requestor
(external to this base driver) registers itself and I don't want to add
a dual meaning to this registration. This will be more obvious once the
rest of the mpam support is added.

Thanks,

Ben