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

[Drew Fustini <fustini@kernel.org>]

On Tue, May 12, 2026 at 01:58:59AM +0000, sashiko-bot@kernel.org wrote:
[..] 
> > @@ -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?

I will change it so every write masks the controller's input fields
clean before ORing the intended values.

> 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?

Bits 7:5 in MON_CTL are reserved zero and should not be hazardous to write.

> 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?

I will use (saved_reg & ~active_mask) so RCID/MCID will start from 0.

> 
> > +	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?

RV32 cannot reach this code today. RISCV_CBQRI_DRIVER selects
RISCV_ISA_SSQOSID, which depends on 64BIT, so unsigned long is always
64-bit where these masks are evaluated.

However, I will widen OP/MCID/EVT_ID to GENMASK_ULL too so the masks
stay correct if the driver is ever ported.

-Drew