Re: [PATCH RFC v5 09/18] riscv_cbqri: Add bandwidth controller probe and allocation device ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The standalone read functions (`cbqri_read_rbwb` and `cbqri_read_mweight`) are vulnerable to the silent READ_LIMIT no-op hardware bug described in the commit message.
- [High] The bandwidth allocation operation inadvertently preserves the Access Type (AT) field left by firmware or prior probe operations, potentially applying limits to the wrong access type.
- [Medium] Modifying the hardware staging register before verifying the controller is ready (BUSY=0) can cause silent corruption of the unmodified field.
--

commit 43b9fe0d5868fdc96ff8dc83b2e5d5819da5ef2e
Author: Drew Fustini <fustini@kernel.org>

riscv_cbqri: Add bandwidth controller probe and allocation device ops

This commit adds support for CBQRI bandwidth controller discovery and
allocation control knobs (Rbwb and Mweight). It implements software caches
and pre-write sentinels to prevent stale data leakage caused by a silent
hardware bug during READ_LIMIT operations.

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> index 90a95fd111909..2a80197764a9c 100644
> --- a/drivers/resctrl/cbqri_devices.c
> +++ b/drivers/resctrl/cbqri_devices.c

[ ... ]

> @@ -135,6 +193,41 @@ int cbqri_mon_op(struct cbqri_controller *ctrl, int reg_offset,
>  	return 0;
>  }
>  
> +/*
> + * Perform bandwidth allocation control operation on bandwidth controller.
> + * Caller must hold ctrl->lock.
> + */
> +static int cbqri_bc_alloc_op(struct cbqri_controller *ctrl, int operation, int rcid)
> +{
> +	int reg_offset = CBQRI_BC_ALLOC_CTL_OFF;
> +	int status;
> +	u64 reg;
> +
> +	lockdep_assert_held(&ctrl->lock);
> +
> +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> +		pr_err_ratelimited("BUSY timeout before starting operation\n");
> +		return -EIO;
> +	}
> +	FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_OP_MASK, &reg, operation);
> +	FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_RCID_MASK, &reg, rcid);
> +	iowrite64(reg, ctrl->base + reg_offset);

[Severity: High]
Does this operation inadvertently preserve the Access Type (AT) field left over
by firmware or prior probe operations?

Since this reads the control register and modifies only OP and RCID via
FIELD_MODIFY, the AT bits are left untouched. If the field was left set to
a specific type (such as CBQRI_CONTROL_REGISTERS_AT_CODE during probe),
would the limits applied here silently inherit that type, throttling only
Code bandwidth while leaving Data bandwidth unconstrained?

[ ... ]

> @@ -266,6 +359,152 @@ int cbqri_read_cache_config(struct cbqri_controller *ctrl, u32 closid,
>  	return err;
>  }
>  
> +/*
> + * Apply a per-RCID update to one field (Rbwb or Mweight) of bc_bw_alloc.
> + * bc_bw_alloc packs both fields, so both halves are seeded from the
> + * authoritative software caches before CONFIG_LIMIT. This avoids the
> + * silent READ_LIMIT no-op window where stale data from a prior op's
> + * RCID could leak into the unmodified field. The verify step uses an
> + * inverted-value sentinel to confirm hardware accepted the target field.
> + *
> + * Caller must hold ctrl->lock.
> + */
> +static int cbqri_apply_bc_field(struct cbqri_controller *ctrl, u32 closid,
> +				enum cbqri_bc_field field, u64 val)
> +{
> +	u64 rbwb = ctrl->rbwb_cache[closid];
> +	u64 mweight = ctrl->mweight_cache[closid];
> +	u64 readback;
> +	int ret;
> +
> +	lockdep_assert_held(&ctrl->lock);
> +
> +	if (field == CBQRI_BC_FIELD_RBWB)
> +		rbwb = val;
> +	else
> +		mweight = val;
> +
> +	cbqri_set_bc_bw_alloc(ctrl, rbwb, mweight);
> +
> +	ret = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_CONFIG_LIMIT, closid);

[Severity: Medium]
Could modifying the hardware staging register before verifying the controller
is ready cause a silent corruption of the unmodified field?

If a previous operation timed out but is still executing, the BUSY flag would
be 1, and the hardware might ignore the cbqri_set_bc_bw_alloc() write.

If that prior operation then completes while cbqri_bc_alloc_op() polls for
BUSY=0, the new CONFIG_LIMIT would be issued. Could the hardware then apply
the stale staging register payload?

If the modified field in the stale payload happens to match the target value,
the post-write sentinel verification would falsely succeed, while the
unmodified field is silently corrupted.

[ ... ]

> +/*
> + * Read the Rbwb (reserved bandwidth blocks) for closid via READ_LIMIT.
> + */
> +int cbqri_read_rbwb(struct cbqri_controller *ctrl, u32 closid, u64 *rbwb_out)
> +{
> +	int err;
> +
> +	mutex_lock(&ctrl->lock);
> +	err = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> +	if (err == 0)
> +		*rbwb_out = cbqri_get_rbwb(ctrl);
> +	mutex_unlock(&ctrl->lock);
> +	return err;
> +}
> +
> +/*
> + * Read the Mweight (opportunistic weight) for closid via READ_LIMIT.
> + */
> +int cbqri_read_mweight(struct cbqri_controller *ctrl, u32 closid, u64 *mweight_out)
> +{
> +	int err;
> +
> +	mutex_lock(&ctrl->lock);
> +	err = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> +	if (err == 0)
> +		*mweight_out = cbqri_get_mweight(ctrl);
> +	mutex_unlock(&ctrl->lock);
> +	return err;
> +}

[Severity: High]
Are these standalone read functions vulnerable to the same silent READ_LIMIT
no-op hardware bug described in the commit message?

Since they execute a bare READ_LIMIT without the pre-write sentinel validation
used in cbqri_apply_bc_field(), could they return stale data from a prior
operation if the hardware bug triggers?

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