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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `FIELD_MODIFY()` combined with `GENMASK()` zero-extends and silently clears the upper 32 bits of 64-bit control registers on 32-bit architectures.
- [High] `cbqri_probe_feature()` executes invalid hardware commands and writes to reserved bits when misapplied to the Monitoring Control Register (`MON_CTL`).
- [Medium] The secondary AT=CODE probe in `cbqri_probe_feature()` fails to re-clear the RCID field, executing on potentially uninitialized data.
--

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

riscv_cbqri: Add capacity controller monitoring device ops
    
Add the CC monitoring primitives. cbqri_init_mon_counters() pre-arms
every MCID with the Occupancy event so a subsequent READ_COUNTER
just snapshots the live counter without re-configuring the slot.

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> index dc76a146e34df..e46b02d2c50d7 100644
> --- a/drivers/resctrl/cbqri_devices.c
> +++ b/drivers/resctrl/cbqri_devices.c

[ ... ]

> @@ -361,6 +402,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);

Does reusing cbqri_probe_feature() for the Monitoring Control Register
(MON_CTL) execute invalid hardware commands and write to reserved bits?

Looking at cbqri_probe_feature(), it issues the operation while clearing the
RCID but ignores the EVT_ID field. This means it issues a counter read using
whatever uninitialized value the bootloader left in EVT_ID. Hardware is
expected to reject a counter read for an unconfigured event, returning an
error status instead of success.

Won't this expected failure cause the probe to falsely conclude monitoring is
unsupported?

Additionally, cbqri_probe_feature() unconditionally attempts to verify Access
Type (AT) support by modifying bits 7:5. Since the MON_CTL register layout
does not contain an AT field, does this modify implicitly reserved bits and
risk hardware faults?

Also, in the secondary AT=CODE probe within cbqri_probe_feature(), it appears
the RCID field is not re-cleared:

cbqri_probe_feature() {
    ...
    reg = saved_reg;
    FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_OP_MASK, &reg, operation);
    FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_AT_MASK, &reg,
                 CBQRI_CONTROL_REGISTERS_AT_CODE);
    iowrite64(reg, ctrl->base + reg_offset);
    ...
}

If the bootloader left an out-of-bounds or invalid value in the RCID field,
will this secondary probe execute on that invalid ID and potentially cause a
hardware fault?

> +	if (err)
> +		return err;

[ ... ]

> diff --git a/drivers/resctrl/cbqri_internal.h b/drivers/resctrl/cbqri_internal.h
> index 6a581a7e417be..b1169ffc599fb 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)

Can FIELD_MODIFY() combined with GENMASK() zero-extend and silently clear the
upper 32 bits of 64-bit control registers on 32-bit architectures?

Since CBQRI_MON_CTL_OP_MASK and CBQRI_MON_CTL_MCID_MASK are defined using
GENMASK(), they yield an unsigned long (32-bit on RV32).

When FIELD_MODIFY() does *(_reg_p) &= ~(_mask) on the 64-bit u64 reg, the
bitwise negation of the 32-bit mask is bitwise ANDed with reg. C integer
promotion rules zero-extend the unsigned 32-bit value to something like
0x00000000FFFFFFE0ULL.

Will this inadvertently clear bits 63:32 of the register to 0 before it is
written back via iowrite64(), potentially corrupting the STATUS field, the
BUSY bit, and any hardware Write-Preserve-Read-Ignore fields?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-ssqosid-cbqri-rqsc-v7-0-v4-0-eb53831ef683@kernel.org?part=8