Re: [PATCH RFC v5 08/18] riscv_cbqri: Add capacity controller monitoring device ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Using GENMASK() instead of GENMASK_ULL() for 64-bit register fields causes the upper 32 bits of the register to be unintentionally cleared on 32-bit architectures.
- [High] Monitoring support is incorrectly probed by issuing a READ_COUNTER operation for an invalid event on an unconfigured slot, which may falsely fail and disable monitoring.
- [Medium] Probing monitoring capabilities reuses cbqri_probe_feature() on MON_CTL, illegally writing to reserved (WPRI) bits to test for Access Type (AT) support.
- [Medium] Lack of bounds checking on mcid_count allows arbitrarily large loops that can block the CPU during hotplug, and causes implicit hardware ID aliasing.
--

commit 1c3fe5b330888927a5ac94480749c86c852d54d3
Author: Drew Fustini <fustini@kernel.org>
Subject: riscv_cbqri: Add capacity controller monitoring device ops

This commit introduces capacity monitoring operations for the RISC-V capacity
controller. It provides functions to pre-arm hardware slots with monitoring
events and execute monitoring register operations. It also updates the probe
sequence to detect capacity and bandwidth monitoring capabilities.

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> index a97511825eedf..90a95fd111909 100644
> --- a/drivers/resctrl/cbqri_devices.c
> +++ b/drivers/resctrl/cbqri_devices.c
[ ... ]
> @@ -369,6 +407,28 @@ static int cbqri_probe_cc(struct cbqri_controller *ctrl)
>  	}
>  	cpus_read_unlock();
>  
> +	/* Probe monitoring features */
> +	err = cbqri_probe_feature(ctrl, CBQRI_CC_MON_CTL_OFF,
> +				  CBQRI_CC_MON_CTL_OP_READ_COUNTER, &status,
> +				  &has_mon_at_code);

[Severity: Medium]
Is it safe to reuse cbqri_probe_feature() for the MON_CTL register here?

cbqri_probe_feature() attempts to verify Access Type (AT) support by writing
to the AT field bits. However, the MON_CTL register does not define an AT
field, which means this implicitly writes to reserved (WPRI) bits. Could
this helper be updated to conditionally skip the AT probe sequence for
monitoring registers?

[Severity: High]
Will this probe sequence falsely disable monitoring on hardware?

This invokes cbqri_probe_feature() using the READ_COUNTER operation, which
zeroes out all other fields including EVT_ID. This sends a read command for
the NONE event (EVT_ID=0) to an unconfigured slot, which the hardware may
reject with an error status. If the hardware returns an error, this check
will fail and disable monitoring. Could this probe instead use
CONFIG_EVENT or handle expected failure codes?

> +	if (err)
> +		return err;
> +
[ ... ]
> @@ -436,6 +496,28 @@ static int cbqri_probe_controller(struct cbqri_controller *ctrl)
>  	return err;
>  }
>  
> +/*
> + * Pre-arm every MCID with the Occupancy event so a subsequent READ_COUNTER
> + * just snapshots the live counter rather than re-configuring the slot.
> + * Called once per CC during resctrl-side cpuhp online for the L3 monitoring
> + * domain.
> + */
> +int cbqri_init_mon_counters(struct cbqri_controller *ctrl)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < ctrl->mcid_count; i++) {
> +		mutex_lock(&ctrl->lock);
> +		err = cbqri_mon_op(ctrl, CBQRI_CC_MON_CTL_OFF,
> +				   CBQRI_CC_MON_CTL_OP_CONFIG_EVENT,
> +				   i, CBQRI_CC_EVT_ID_OCCUPANCY, NULL);

[Severity: Medium]
Does mcid_count need bounds checking against the physical hardware limit?

The ctrl->mcid_count value is used as the loop boundary, but if firmware
supplies an erroneously large value, this loop could block the CPU for an
extended period during hotplug. 

Additionally, because the MCID field mask is 12 bits, any mcid_count larger
than 4096 will be truncated in cbqri_mon_op(), causing logical MCIDs to
incorrectly alias to the same physical hardware slot. Should an upper bound be
enforced on mcid_count?

> +		mutex_unlock(&ctrl->lock);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
> +}
> +
>  void cbqri_controller_destroy(struct cbqri_controller *ctrl)
>  {
>  	/*
> diff --git a/drivers/resctrl/cbqri_internal.h b/drivers/resctrl/cbqri_internal.h
> index cce13026a11ed..e776b0267837a 100644
> --- a/drivers/resctrl/cbqri_internal.h
> +++ b/drivers/resctrl/cbqri_internal.h
[ ... ]
> @@ -39,6 +41,20 @@
>  #define CBQRI_CC_ALLOC_CTL_OP_READ_LIMIT   2
>  #define CBQRI_CC_ALLOC_CTL_STATUS_SUCCESS  1
>  
> +#define CBQRI_CC_MON_CTL_OP_CONFIG_EVENT 1
> +#define CBQRI_CC_MON_CTL_OP_READ_COUNTER 2
> +
> +/* mon_ctl field masks (CC and BC share an identical OP/MCID/EVT_ID/STATUS layout) */
> +#define CBQRI_MON_CTL_OP_MASK        GENMASK(4, 0)
> +#define CBQRI_MON_CTL_MCID_MASK      GENMASK(19, 8)
> +#define CBQRI_MON_CTL_EVT_ID_MASK    GENMASK(27, 20)

[Severity: High]
Will using GENMASK() here inadvertently clear the upper 32 bits of the 64-bit
control register on 32-bit architectures?

When used with FIELD_MODIFY(), ~(_mask) will evaluate to a 32-bit unsigned
value which gets zero-extended to 64 bits. When this is bitwise ANDed with
the 64-bit register variable, it will wipe out the upper 32 bits, corrupting
the STATUS field before the register is written back. Should these 64-bit
field masks be defined with GENMASK_ULL() instead?

> +#define CBQRI_MON_CTL_STATUS_MASK    GENMASK_ULL(38, 32)
> +#define CBQRI_MON_CTL_STATUS_SUCCESS 1
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-ssqosid-cbqri-rqsc-v7-0-v5-0-78d3a7ba9dbe@kernel.org?part=8