Re: [PATCH RFC v5 10/18] riscv_cbqri: Add bandwidth controller monitoring device ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] FIELD_MODIFY() with 32-bit GENMASK clobbers the upper 32 bits of u64 registers on 32-bit architectures.
--

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

riscv_cbqri: Add bandwidth controller monitoring device ops

This commit adds bandwidth controller monitoring primitives. It allocates a
per-MCID software accumulator to extend the 62-bit hardware counter into a
64-bit byte total and handles overflow when reading the counter. It also
provides a function to discover the single mon-capable bandwidth controller.

> diff --git a/drivers/resctrl/cbqri_internal.h b/drivers/resctrl/cbqri_internal.h
> index 48f8a6e1923e2..555512cf7eab9 100644
> --- a/drivers/resctrl/cbqri_internal.h
> +++ b/drivers/resctrl/cbqri_internal.h
[ ... ]
> @@ -63,12 +63,26 @@
>  #define CBQRI_CC_MON_CTL_OP_CONFIG_EVENT 1
>  #define CBQRI_CC_MON_CTL_OP_READ_COUNTER 2
>  
> +#define CBQRI_BC_MON_CTL_OP_CONFIG_EVENT 1
>  #define CBQRI_BC_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)
> +/* Bandwidth usage monitoring event IDs (CBQRI spec Table 10) */
> +#define CBQRI_BC_EVT_ID_TOTAL_READ_WRITE  1
> +
> +/* bc_mon_ctr_val layout (CBQRI spec section 4.3, Figure 7) */
> +#define CBQRI_BC_MON_CTR_VAL_CTR_MASK    GENMASK_ULL(61, 0)
> +#define CBQRI_BC_MON_CTR_VAL_INVALID     BIT_ULL(62)
> +#define CBQRI_BC_MON_CTR_VAL_OVF         BIT_ULL(63)
> +
> +/*
> + * mon_ctl field masks (CC and BC share an identical OP/MCID/EVT_ID/STATUS
> + * layout). Keep all masks u64-wide so FIELD_MODIFY() on a u64 register
> + * never zero-extends a 32-bit ~mask and clobbers the WPRI/STATUS/BUSY
> + * fields in bits 63:32 for future support of RV32
> + */
> +#define CBQRI_MON_CTL_OP_MASK        GENMASK_ULL(4, 0)
> +#define CBQRI_MON_CTL_MCID_MASK      GENMASK_ULL(19, 8)
> +#define CBQRI_MON_CTL_EVT_ID_MASK    GENMASK_ULL(27, 20)

[Severity: High]
This isn't a bug introduced by this patch, but does the same fix need to be
applied to other masks?

It looks like the CBQRI_CONTROL_REGISTERS_*_MASK macros (for example,
CBQRI_CONTROL_REGISTERS_OP_MASK) continue to use the 32-bit GENMASK().

When these 32-bit masks are passed to FIELD_MODIFY() on a u64 variable in
functions like cbqri_cc_alloc_op(), standard C promotion rules will zero-extend
the 32-bit ~mask value. This would clear bits 63:32 of the register, clobbering
the WPRI, STATUS, and BUSY fields when written back to the device via
iowrite64().

Should these other macros also be upgraded to GENMASK_ULL() to prevent silent
hardware state corruption on 32-bit architectures like RV32?

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