Re: [PATCH v3 23/29] arm_mpam: Add mpam_msmon_read() to read monitor value

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

Hi Shaopeng,

On 11/7/25 05:01, Shaopeng Tan (Fujitsu) wrote:
> Hi Ben,
> 
>> Hi Shaopeng,
>>
>> On 11/5/25 08:32, Shaopeng Tan (Fujitsu) wrote:
>>> Hello James,
>>>
>>>> Reading a monitor involves configuring what you want to monitor, and
>>>> reading the value. Components made up of multiple MSC may need values
>>>> from each MSC. MSCs may take time to configure, returning 'not ready'.
>>>> The maximum 'not ready' time should have been provided by firmware.
>>>>
>>>> Add mpam_msmon_read() to hide all this. If (one of) the MSC returns
>>>> not ready, then wait the full timeout value before trying again.
>>>>
>>>> CC: Shanker Donthineni <sdonthineni@nvidia.com>
>>>> Signed-off-by: James Morse <james.morse@arm.com>
>> [...]
>>>> +/* Call with MSC lock held */
>>>> +static void __ris_msmon_read(void *arg) {
>>>> +	u64 now;
>>>> +	bool nrdy = false;
>>>> +	struct mon_read *m = arg;
>>>> +	struct mon_cfg *ctx = m->ctx;
>>>> +	struct mpam_msc_ris *ris = m->ris;
>>>> +	struct mpam_props *rprops = &ris->props;
>>>> +	struct mpam_msc *msc = m->ris->vmsc->msc;
>>>> +	u32 mon_sel, ctl_val, flt_val, cur_ctl, cur_flt;
>>>> +
>>>> +	if (!mpam_mon_sel_lock(msc)) {
>>>> +		m->err = -EIO;
>>>> +		return;
>>>> +	}
>>>> +	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL,
>>>> ctx->mon) |
>>>> +		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
>>>> +	mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
>>>> +
>>>> +	/*
>>>> +	 * Read the existing configuration to avoid re-writing the same values.
>>>> +	 * This saves waiting for 'nrdy' on subsequent reads.
>>>> +	 */
>>>> +	read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
>>>> +	clean_msmon_ctl_val(&cur_ctl);
>>>> +	gen_msmon_ctl_flt_vals(m, &ctl_val, &flt_val);
>>>> +	if (cur_flt != flt_val || cur_ctl != (ctl_val | MSMON_CFG_x_CTL_EN))
>>>> +		write_msmon_ctl_flt_vals(m, ctl_val, flt_val);
>>>
>>> When reading the CSU counter of a different control group, the counter is
>> cleared to 0 by the write_msmon_ctl_flt_vals().
>>>
>>>> +	switch (m->type) {
>>>> +	case mpam_feat_msmon_csu:
>>>> +		now = mpam_read_monsel_reg(msc, CSU);
>>>> +		if (mpam_has_feature(mpam_feat_msmon_csu_hw_nrdy,
>>>> rprops))
>>>> +			nrdy = now & MSMON___NRDY;
>>>> +		break;
>>>
>>> The first time read the counter(MSMON_CSU), all bits except nrdy are 0.
>>
>>
>> I'm trying to understand your problem. Isn't what you are describing the
>> intended behaviour of the nrdy bit? It takes some time to get a count of the
>> cache utilization so if it's not ready this is set and the driver retries.
> 
> I apologize for not explain it correctly.
> The key point is that for some SOC chip, if `mpam_feat_msmon_csu_hw_nrdy` is false,
> the NRDY bit value of counter(MSMON_CSU) cannot be set to 'nrdy'.
> 'nrdy' will keep its initial value(false).

Ok, so if I understand correctly, your hardware takes some time to
generate the csu monitor value but doesn't have a hardware control of
the nrdy bit to mark the value as unreliable. When this bit is under
software control then the behaviour is implementation defined. Some what
wierdly the spec says "If a monitor does not support automatic behavior
of NRDY, software can use this bit for any purpose.". I would expect a
quirk for your platform could be added to delay after CSU configuration
but this is out of scope for this series.


> 
>>> This means when check the cache monitoring value the first time, cat
>>> mon_data/mon_L3_0*/llc_occupancy the result will be 0.
>>> From the second time, the result will return to normal.
>>
>> This is not expected. On creating a new ctrl_mon group, generating some
>> memory traffic and checking the llc_occumpancy I see a non-zero value on my
>> setup. Not sure why you wouldn't.
> 
> Best regards,
> Shaopeng TAN
> 

Thanks,

Ben