Re: [PATCH RFC v3 04/11] RISC-V: QoS: add CBQRI hardware interface

[Drew Fustini <fustini@kernel.org>]

On Thu, Apr 30, 2026 at 04:15:05PM -0700, Reinette Chatre wrote:
> Hi Drew,
> 
> On 4/14/26 6:53 PM, Drew Fustini wrote:
> > diff --git a/arch/riscv/kernel/qos/internal.h b/arch/riscv/kernel/qos/internal.h
> > new file mode 100644
> > index 000000000000..edbcbd9471b1
> > --- /dev/null
> > +++ b/arch/riscv/kernel/qos/internal.h
> 
> ...
> 
> > +
> > +struct cbqri_config {
> > +	u64 cbm; /* capacity block mask */
> > +	u64 rbwb; /* reserved bandwidth blocks */
> > +};
> 
> Is this struct necessary? From what I can tell it is used to pass a parameter to
> cbqri_apply_cache_config() and cbqri_apply_bw_config() where each just picks the
> one member of interest. Could parameter of interest just be provided directly to
> cbqri_apply_cache_config() and cbqri_apply_bw_config() without bouncing it
> through this struct first?

Agreed, the struct is unnecessary indirection. The two callers each
write one field and read it back immediately. I will drop it and pass
cbm / rbwb directly.

> > +
> > +#endif /* _ASM_RISCV_QOS_INTERNAL_H */
> > diff --git a/arch/riscv/kernel/qos/qos_resctrl.c b/arch/riscv/kernel/qos/qos_resctrl.c
> > new file mode 100644
> > index 000000000000..6d294f2f2504
> > --- /dev/null
> > +++ b/arch/riscv/kernel/qos/qos_resctrl.c
> > @@ -0,0 +1,432 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#define pr_fmt(fmt) "qos: resctrl: " fmt
> > +
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/ioport.h>
> > +#include <linux/resctrl.h>
> > +#include <linux/riscv_qos.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <asm/csr.h>
> > +#include <asm/qos.h>
> > +#include "internal.h"
> > +
> > +static struct cbqri_resctrl_res cbqri_resctrl_resources[RDT_NUM_RESOURCES];
> > +
> > +static bool exposed_alloc_capable;
> > +/* CDP (code data prioritization) on x86 is AT (access type) on RISC-V */
> > +static bool exposed_cdp_l2_capable;
> > +static bool exposed_cdp_l3_capable;
> > +static bool is_cdp_l2_enabled;
> > +static bool is_cdp_l3_enabled;
> 
> These CDP values could also be handled as part of struct cbqri_resctrl_res, similar
> to how CDP is managed with struct rdt_hw_resource. There does seem to be a hidden
> assumption in this implementation that if *any* cache controller at particular level
> supports CDP (which will cause global exposed_cdp_l{2,3}_capable to be true) then all cache
> controllers at that level are assumed to support CDP even though this property is
> enumerated separately for every cache controller making it possible for this
> support to not be uniform.
> 
> ...

Good point. This is fixed in v4 which I was about to send. The
controller-level cap is now stored on each controller as
cc.supports_alloc_at_code and cc.supports_alloc_at_data, and
cbqri_resctrl_pick_caches() rejects a heterogeneous-CDP set at the same
cache level. Therefore, resctrl never sees a global-capable bit that
some controllers cannot honour. I will take a look at folding this into
struct cbqri_resctrl_res to make it simpler.

> > +/* Perform capacity allocation control operation on capacity controller */
> > +static int cbqri_cc_alloc_op(struct cbqri_controller *ctrl, int operation, int rcid,
> > +			     enum resctrl_conf_type type)
> > +{
> > +	int reg_offset = CBQRI_CC_ALLOC_CTL_OFF;
> > +	int status;
> > +	u64 reg;
> > +
> > +	reg = ioread64(ctrl->base + reg_offset);
> > +	reg &= ~CBQRI_CONTROL_REGISTERS_OP_MASK;
> > +	reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation);
> > +	reg &= ~CBQRI_CONTROL_REGISTERS_RCID_MASK;
> > +	reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_RCID_MASK, rcid);
> > +
> > +	/* CBQRI capacity AT is only supported on L2 and L3 caches for now */
> > +	if (ctrl->type == CBQRI_CONTROLLER_TYPE_CAPACITY &&
> > +	    ((ctrl->cache.cache_level == 2 && is_cdp_l2_enabled) ||
> > +	    (ctrl->cache.cache_level == 3 && is_cdp_l3_enabled))) {
> > +		reg &= ~CBQRI_CONTROL_REGISTERS_AT_MASK;
> > +		switch (type) {
> > +		case CDP_CODE:
> > +			reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK,
> > +					  CBQRI_CONTROL_REGISTERS_AT_CODE);
> > +			break;
> > +		case CDP_DATA:
> > +		default:
> > +			reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK,
> > +					  CBQRI_CONTROL_REGISTERS_AT_DATA);
> > +			break;
> > +		}
> > +	}
> 
> There does not seem to be any special enabling when resctrl enables CDP via
> resctrl_arch_set_cdp_enabled() so I assume CDP is always enabled? Does that mean
> that when CDP is disabled from resctrl perspective (thus, both is_cdp_l2_enabled
> and is_cdp_l3_enabled are false, but exposed_cdp_l2_capable and exposed_cdp_l3_capable
> are true) then the default behavior is that allocation applies to both code and data
> and it is not necessary to set both CBQRI_CONTROL_REGISTERS_AT_CODE and
> CBQRI_CONTROL_REGISTERS_AT_DATA?

Good question. I think this is a bug. resctrl_arch_set_cdp_enabled() is
implemented in v4 and toggles the is_cdp_l{2,3}_enabled flags. When CDP
is disabled, the AT field is left at 0 in cbqri_cc_alloc_op(). But BQRI
encodes AT=0 as data, so it only programs the data config and silently
leaves code config at whatever the hardware default is.

The fix would be to issue two CONFIG_LIMIT ops, AT=data and AT=code, for
the same cbm when supports_alloc_at_code is true and CDP is disabled.
Thanks for catching this.

[..]
> > +	/*
> > +	 * max_rmid is used by resctrl_arch_system_num_rmid_idx()
> > +	 * Find the smallest mcid_count amongst all controllers.
> > +	 */
> > +	max_rmid = min(max_rmid, ctrl->mcid_count);
> 
> This computation appears to include all controllers, whether they support monitoring or not.
> What will ctrl->mcid_count be on a controller that does not support monitoring?

Good catch. I will gate the minimum on ctrl->mon_capable.

Thanks,
Drew