Re: [PATCH v11 1/5] riscv_cbqri: Add capacity controller core

[Drew Fustini <fustini@kernel.org>]

On Thu, Nov 12, 2026 at 05:14:50PM +0100, Radim Krčmář wrote:

Hi Radim, thanks for the review.

> 2026-11-08T15:20:50-08:00, Drew Fustini <fustini@kernel.org>:
> > Add the core for RISC-V CBQRI capacity controllers: the register
> > layout, the capability probe, and the allocation operations that
> > program a capacity block mask for an RCID and read it back.
> >
> > A discovery layer maps the register block and describes the controller
> > in a cbqri_controller_info descriptor. riscv_cbqri_register_cc() probes
> > the controller from that descriptor and adds it to the controller list,
> > which stays private to the core. The core owns the controller state
> > after registration, so nothing in it depends on how the controller was
> > discovered. The caller keeps the register mapping alive.
> >
> > Co-developed-by: Adrien Ricciardi <aricciardi@baylibre.com>
> > Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
> > Signed-off-by: Drew Fustini <fustini@kernel.org>
> > ---
> > diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> > +static u64 cbqri_readq(void __iomem *addr)
> > +{
> > +	return lo_hi_readq(addr);
> > +}
> > +
> > +static void cbqri_clear_cunits(struct cbqri_controller *ctrl)
> > +{
> > +	if (ctrl->cc.cunits) {
> > +		writel(0, ctrl->base + CBQRI_CC_CUNITS_OFF);
> > +		writel(0, ctrl->base + CBQRI_CC_CUNITS_OFF + 4);
> 
> nit: lo_hi_writeq?

Good idea, I'll switch to that.

> > +int cbqri_apply_cache_config(struct cbqri_controller *ctrl, u32 closid,
> > +			     const struct cbqri_cc_config *cfg)
> > +{
> > [...]
> > +	mutex_lock(&ctrl->lock);
> > [...]
> > +out:
> > +	mutex_unlock(&ctrl->lock);
> > +	return err;
> 
> nit: guard the mutex?

Also a good idea, I'll switch for this function and also
cbqri_read_cache_config().

> 
> > +static int cbqri_probe_feature(struct cbqri_controller *ctrl, int reg_offset,
> > +			       int operation, int *status, bool *access_type_supported)
> > +{
> > +	const u64 active_mask = CBQRI_CONTROL_REGISTERS_OP_MASK |
> > +				CBQRI_CONTROL_REGISTERS_AT_MASK |
> > +				CBQRI_CONTROL_REGISTERS_RCID_MASK;
> > +	u64 reg, saved_reg;
> > +	int at;
> > +
> > +	/*
> > +	 * Default the output to false so the status==0 (feature not
> > +	 * implemented) path returns a deterministic value to the caller
> > +	 * rather than leaving an uninitialized bool.
> > +	 */
> > +	*access_type_supported = false;
> > +
> > +	/* Keep the initial register value to preserve the WPRI fields */
> > +	reg = cbqri_readq(ctrl->base + reg_offset);
> > +	saved_reg = reg;
> > +
> > +	/* Drain any in-flight firmware op before issuing our own write. */
> > +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &saved_reg) < 0) {
> > +		pr_err("BUSY timeout before probe operation\n");
> > +		return -EIO;
> > +	}
> > +
> > +	/*
> > +	 * Execute the requested operation with all active fields
> > +	 * (OP/AT/RCID) zeroed except OP itself. Every bit not in
> > +	 * active_mask is WPRI and gets carried over from saved_reg.
> > +	 */
> > +	reg = (saved_reg & ~active_mask) |
> > +	      FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation);
> > +	writel(lower_32_bits(reg), ctrl->base + reg_offset);
> > +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> > +		pr_err_ratelimited("BUSY timeout during operation\n");
> > +		return -EIO;
> > +	}
> > +
> > +	/* Get the operation status */
> > +	*status = FIELD_GET(CBQRI_CONTROL_REGISTERS_STATUS_MASK, reg);
> > +
> > +	/*
> > +	 * Check for AT support if the register is implemented
> > +	 * (if not, the status value will remain 0)
> > +	 */
> > +	if (*status != 0) {
> > +		/*
> > +		 * Re-issue operation with AT=CODE so the controller
> > +		 * latches AT=CODE on supported hardware (or resets it to 0
> > +		 * on hardware that doesn't). OP must be a defined CBQRI op
> > +		 * here. OP=0 is a no-op and would silently disable CDP.
> 
> OP=0 is a reserved for future standard use.
> (Hopefully just fails, but it could be worse if it gets implemented as
>  something other than no-op.)

I'll reword and drop the incorrect statement that OP=0 is a no-op.

> 
> > +		 */
> > +		reg = (saved_reg & ~active_mask) |
> > +		      FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation) |
> > +		      FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK,
> > +				 CBQRI_CONTROL_REGISTERS_AT_CODE);
> > +		writel(lower_32_bits(reg), ctrl->base + reg_offset);
> > +		if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> > +			pr_err("BUSY timeout setting AT field\n");
> > +			return -EIO;
> > +		}
> > +
> > +		/*
> > +		 * If the AT field value has been reset to zero,
> > +		 * then AT support is not present
> > +		 */
> > +		at = FIELD_GET(CBQRI_CONTROL_REGISTERS_AT_MASK, reg);
> 
> I think it would be safer to also check the status field.
> 
> I'm not too clear whether the spec actually allows returning 5 (invalid
> AT) when the AT is invalid, but an implementation that does that
> wouldn't surprise me.
> 
> (For example when an implementation supports data AT and a custom AT, so
> code AT isn't supported, but AT in general is -- it could keep the
> failing code AT and just report the error.)

That seems reasonable. I will change it to only report AT support when
the AT field reads back as code with status = success.

> > +		if (at == CBQRI_CONTROL_REGISTERS_AT_CODE)
> > +			*access_type_supported = true;
> > +	}
> > +
> > +	/*
> > +	 * Restore the original register value.
> > +	 * Clear OP to avoid re-triggering the probe op.
> > +	 */
> > +	saved_reg &= ~CBQRI_CONTROL_REGISTERS_OP_MASK;
> > +	writel(lower_32_bits(saved_reg), ctrl->base + reg_offset);
> 
> It would be better to do nothing than trigger an illegal OP.
> All our writes should modify the non-WPRI bits anyway, so restoring them
> does nothing.

I agree. I'll drop the restore write.

> > +	if (cbqri_wait_busy_flag(ctrl, reg_offset, NULL) < 0) {
> > +		pr_err("BUSY timeout restoring register value\n");
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int cbqri_probe_cc(struct cbqri_controller *ctrl)
> > +{
> > [...]
> > +
> > +	/*
> > +	 * The driver programs the v1.0 register layout. Controllers that
> > +	 * report 0.x differ only in capability bits that read as zero, so
> > +	 * they are accepted. Anything newer than 1.0 may move fields.
> > +	 */
> > +	if (ver_major > 1 || (ver_major == 1 && ver_minor > 0)) {
> 
> I think we could accept ver_major == 1 even with ver_minor > 0 since the
> minor specifications shouldn't be doing backward-incompatible changes.
> 
> (If we fix the OP=0, we shouldn't be invoking any other behavior that
>  might change.)

Good point. I'll skip the ver_minor check and reject ver_major > 1.

> I'd be more careful about accepting anything pre-1.0, since I would
> expect there was a breaking change in the spec somewhere, and we could
> program it incorrectly...  Do we even care about pre-1.0 hardware?

I went through the tagged drafts in the riscv-cbqri repo, v0.9.1 through
v1.0-rc3. The registers and encodings that this driver uses are the same
in all of them. RPFX/P were added in bits that had been reserved.

If I add that information to the comment, then would that be acceptable
to you?

Thanks,
Drew