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

Drew Fustini
2026-05-01T04:45:24Z

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