Re: [RFC PATCH 20/36] arm_mpam: Probe the hardware features resctrl supports

[James Morse <james.morse@arm.com>]

Hi Ben,

On 24/07/2025 16:08, Ben Horgan wrote:
> On 7/11/25 19:36, James Morse wrote:
>> Expand the probing support with the control and monitor types
>> we can use with resctrl.

>> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/
>> mpam_devices.c
>> index 8646fb85ad09..61911831ab39 100644
>> --- a/drivers/platform/arm64/mpam/mpam_devices.c
>> +++ b/drivers/platform/arm64/mpam/mpam_devices.c

>> @@ -645,6 +659,137 @@ static struct mpam_msc_ris *mpam_get_or_create_ris(struct mpam_msc
>> *msc,
>>       return found;
>>   }
>>   +/*
>> + * IHI009A.a has this nugget: "If a monitor does not support automatic behaviour
>> + * of NRDY, software can use this bit for any purpose" - so hardware might not
>> + * implement this - but it isn't RES0.
>> + *
>> + * Try and see what values stick in this bit. If we can write either value,
>> + * its probably not implemented by hardware.
>> + */
>> +#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)            \
>> +do {                                        \
>> +    u32 now;                                \
>> +    u64 mon_sel;                                \
>> +    bool can_set, can_clear;                        \
>> +    struct mpam_msc *_msc = _ris->vmsc->msc;                \
>> +                                        \
>> +    if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(_msc))) {            \
>> +        _result = false;                        \
>> +        break;                                \
>> +    }                                    \
>> +    mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |            \
>> +          FIELD_PREP(MSMON_CFG_MON_SEL_RIS, _ris->ris_idx);        \
>> +    mpam_write_monsel_reg(_msc, CFG_MON_SEL, mon_sel);            \
>> +                                        \
>> +    mpam_write_monsel_reg(_msc, _mon_reg, MSMON___NRDY);            \
>> +    now = mpam_read_monsel_reg(_msc, _mon_reg);                \
>> +    can_set = now & MSMON___NRDY;                        \
>> +                                        \
>> +    mpam_write_monsel_reg(_msc, _mon_reg, 0);                \
>> +    now = mpam_read_monsel_reg(_msc, _mon_reg);                \
>> +    can_clear = !(now & MSMON___NRDY);                    \
>> +    mpam_mon_sel_inner_unlock(_msc);                    \
>> +                                        \
>> +    _result = (!can_set || !can_clear);                    \
>> +} while (0)

> It is a bit surprising that something that looks like a function modifies a boolean passed
> by value.

> Consider continuing the pattern you have above:
> #define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)
> _mpam_ris_hw_probe_hw_nrdy(_ris, MSMON##_mon_reg, _result)
> 
> with signature:
> void _mpam_ris_hw_probe_hw_nrdy(struct mpam_msc *msc, u16 reg, bool *hw_managed);
> 
> and using the _mpam functions from the new _mpam_ris_hw_probe_hw_nrdy().

With that, it may as well return the result.
Done.


Thanks,

James