Re: [External] [PATCH RFC v2 08/17] RISC-V: QoS: add resctrl interface for CBQRI controllers

[Drew Fustini]

On Mon, Feb 02, 2026 at 12:12:28PM +0800, yunhui cui wrote:
> Hi Drew,

Hi, thanks for your review, sorry I had this reply in draft for awhile
and failed to actually send it. All good points from you and I've been
working on fixing up the code.

> On Thu, Jan 29, 2026 at 4:28 AM Drew Fustini <fustini@xxxxxxxxxx> wrote:
> >
> > Add interface for CBQRI controller drivers to make use of the resctrl
> > filesystem.
> >
> > Co-developed-by: Adrien Ricciardi <aricciardi@xxxxxxxxxxxx>
> > Signed-off-by: Adrien Ricciardi <aricciardi@xxxxxxxxxxxx>
> > Signed-off-by: Drew Fustini <fustini@xxxxxxxxxx>
> > ---
> >  arch/riscv/kernel/qos/qos_resctrl.c | 1192 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 1192 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/qos/qos_resctrl.c b/arch/riscv/kernel/qos/qos_resctrl.c
> > new file mode 100644
> > index 000000000000..d500098599d2
> > --- /dev/null
> > +++ b/arch/riscv/kernel/qos/qos_resctrl.c
> > @@ -0,0 +1,1192 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#define pr_fmt(fmt) "qos: resctrl: " fmt
> > +
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/riscv_qos.h>
> > +#include <linux/resctrl.h>
> > +#include <linux/types.h>
> > +#include <asm/csr.h>
> > +#include <asm/qos.h>
> > +#include "internal.h"
> > +
> > +#define MAX_CONTROLLERS 6
> > +static struct cbqri_controller controllers[MAX_CONTROLLERS];
> 
> Switch to dynamic allocation? Remove MAX_CONTROLLERS.

Yes, I am reworking the implementation to dynamically allocate the
cbqri_controller array based on the number of controllers actually in
the system.

> > +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> > +                          u32 closid, u32 rmid, enum resctrl_event_id eventid,
> > +                          u64 *val, void *arch_mon_ctx)
> > +{
> > +       /*
> > +        * The current Qemu implementation of CBQRI capacity and bandwidth
> > +        * controllers do not emulate the utilization of resources over
> > +        * time. Therefore, Qemu currently sets the invalid bit in
> > +        * cc_mon_ctr_val and bc_mon_ctr_val, and there is no meaningful
> > +        * value other than 0 to return for reading an RMID (e.g. MCID in
> > +        * CBQRI terminology)
> > +        */
> > +
> > +       return 0;
> 
> Implement per the spec's description directly, not as this comment states?

Good point that this should actually perform the operation to read the
value, even if Qemu is just setting the invalid bit as there is no real
value implemented in Qemu.

> > +/*
> > + * Note: for the purposes of the CBQRI proof-of-concept, debug logging
> > + * has been left in this function that detects the properties of CBQRI
> > + * capable controllers in the system. pr_info calls would be removed
> > + * before submitting non-RFC patches.
> > + */
> > +static int cbqri_probe_controller(struct cbqri_controller_info *ctrl_info,
> > +                                 struct cbqri_controller *ctrl)
> > +{
> > +       int err = 0, status;
> > +       u64 reg;
> > +
> > +       pr_info("controller info: type=%d addr=0x%lx size=%lu max-rcid=%u max-mcid=%u",
> > +               ctrl_info->type, ctrl_info->addr, ctrl_info->size,
> > +               ctrl_info->rcid_count, ctrl_info->mcid_count);
> > +
> > +       /* max_rmid is used by resctrl_arch_system_num_rmid_idx() */
> > +       max_rmid = ctrl_info->mcid_count;
> 
> Get the min of all controllers?

Yes, I will change the logic to do that.

> > +static int qos_init_domain_ctrlval(struct rdt_resource *r, struct rdt_ctrl_domain *d)
> > +{
> > +       struct cbqri_resctrl_res *hw_res;
> > +       struct cbqri_resctrl_dom *hw_dom;
> > +       u64 *dc;
> > +       int err = 0;
> > +       int i;
> > +
> > +       hw_res = container_of(r, struct cbqri_resctrl_res, resctrl_res);
> > +       if (!hw_res)
> > +               return -ENOMEM;
> > +
> > +       hw_dom = container_of(d, struct cbqri_resctrl_dom, resctrl_ctrl_dom);
> > +       if (!hw_dom)
> > +               return -ENOMEM;
> > +
> > +       dc = kmalloc_array(hw_res->max_rcid, sizeof(*hw_dom->ctrl_val),
> > +                          GFP_KERNEL);
> > +       if (!dc)
> > +               return -ENOMEM;
> > +
> > +       hw_dom->ctrl_val = dc;
> > +
> > +       for (i = 0; i < hw_res->max_rcid; i++, dc++) {
> > +               err = resctrl_arch_update_one(r, d, i, 0, resctrl_get_default_ctrl(r));
> > +               if (err)
> > +                       return 0;
> 
> return 0 ?

Ah, yes, I will update to return error instead of silencing it.

> > +static int qos_resctrl_add_controller_domain(struct cbqri_controller *ctrl, int *id)
> > +{
> > +       struct rdt_ctrl_domain *domain = NULL;
> > +       struct cbqri_resctrl_res *cbqri_res = NULL;
> > +       struct rdt_resource *res = NULL;
> > +       int internal_id = *id;
> > +       int err = 0;
> > +
> > +       domain = qos_new_domain(ctrl);
> > +       if (!domain)
> > +               return -ENOSPC;
> > +       if (ctrl->ctrl_info->type == CBQRI_CONTROLLER_TYPE_CAPACITY) {
> > +               cpumask_copy(&domain->hdr.cpu_mask, &ctrl->ctrl_info->cache.cpu_mask);
> > +               if (ctrl->ctrl_info->cache.cache_level == 2) {
> > +                       cbqri_res = &cbqri_resctrl_resources[RDT_RESOURCE_L2];
> > +                       cbqri_res->max_rcid = ctrl->ctrl_info->rcid_count;
> > +                       cbqri_res->max_mcid = ctrl->ctrl_info->mcid_count;
> > +                       res = &cbqri_res->resctrl_res;
> > +                       res->mon.num_rmid = ctrl->ctrl_info->mcid_count;
> > +                       res->rid = RDT_RESOURCE_L2;
> > +                       res->name = "L2";
> > +                       res->alloc_capable = ctrl->alloc_capable;
> > +                       res->mon_capable = ctrl->mon_capable;
> > +                       res->schema_fmt = RESCTRL_SCHEMA_BITMAP;
> > +                       res->ctrl_scope = RESCTRL_L2_CACHE;
> > +                       res->cache.arch_has_sparse_bitmasks = false;
> > +                       res->cache.arch_has_per_cpu_cfg = false;
> > +                       res->cache.cbm_len = ctrl->cc.ncblks;
> > +                       res->cache.shareable_bits = resctrl_get_default_ctrl(res);
> > +                       res->cache.min_cbm_bits = 1;
> > +               } else if (ctrl->ctrl_info->cache.cache_level == 3) {
> > +                       cbqri_res = &cbqri_resctrl_resources[RDT_RESOURCE_L3];
> > +                       cbqri_res->max_rcid = ctrl->ctrl_info->rcid_count;
> > +                       cbqri_res->max_mcid = ctrl->ctrl_info->mcid_count;
> > +                       res = &cbqri_res->resctrl_res;
> > +                       res->mon.num_rmid = ctrl->ctrl_info->mcid_count;
> > +                       res->rid = RDT_RESOURCE_L3;
> > +                       res->name = "L3";
> > +                       res->schema_fmt = RESCTRL_SCHEMA_BITMAP;
> > +                       res->ctrl_scope = RESCTRL_L3_CACHE;
> > +                       res->alloc_capable = ctrl->alloc_capable;
> > +                       res->mon_capable = ctrl->mon_capable;
> > +                       res->cache.arch_has_sparse_bitmasks = false;
> > +                       res->cache.arch_has_per_cpu_cfg = false;
> > +                       res->cache.cbm_len = ctrl->cc.ncblks;
> > +                       res->cache.shareable_bits = resctrl_get_default_ctrl(res);
> > +                       res->cache.min_cbm_bits = 1;
> > +               } else {
> > +                       pr_warn("%s(): unknown cache level %d", __func__,
> > +                               ctrl->ctrl_info->cache.cache_level);
> > +                       err = -ENODEV;
> > +                       goto err_free_domain;
> > +               }
> > +       } else if (ctrl->ctrl_info->type == CBQRI_CONTROLLER_TYPE_BANDWIDTH) {
> > +               if (ctrl->alloc_capable) {
> > +                       cbqri_res = &cbqri_resctrl_resources[RDT_RESOURCE_MBA];
> > +                       cbqri_res->max_rcid = ctrl->ctrl_info->rcid_count;
> > +                       cbqri_res->max_mcid = ctrl->ctrl_info->mcid_count;
> > +                       res = &cbqri_res->resctrl_res;
> > +                       res->mon.num_rmid = ctrl->ctrl_info->mcid_count;
> > +                       res->rid = RDT_RESOURCE_MBA;
> > +                       res->name = "MB";
> > +                       res->schema_fmt = RESCTRL_SCHEMA_RANGE;
> > +                       res->ctrl_scope = RESCTRL_L3_CACHE;
> > +                       res->alloc_capable = ctrl->alloc_capable;
> > +                       res->mon_capable = false;
> > +                       res->membw.delay_linear = true;
> > +                       res->membw.arch_needs_linear = true;
> > +                       res->membw.throttle_mode = THREAD_THROTTLE_UNDEFINED;
> > +                       // The minimum percentage allowed by the CBQRI spec
> > +                       res->membw.min_bw = 1;
> > +                       // The maximum percentage allowed by the CBQRI spec
> > +                       res->membw.max_bw = 80;
> > +                       res->membw.bw_gran = 1;
> > +               }
> 
> Wrap a function.

I am guessing you mean to break up this long function into a couple of
smaller functions? I will give that a try for the next rev.

> > +       } else {
> > +               pr_warn("%s(): unknown resource %d", __func__, ctrl->ctrl_info->type);
> > +               err = -ENODEV;
> > +               goto err_free_domain;
> > +       }
> > +
> > +       domain->hdr.id = internal_id;
> > +       err = qos_init_domain_ctrlval(res, domain);
> > +       if (err)
> > +               goto err_free_domain;
> > +
> > +       if (cbqri_res) {
> > +               list_add_tail(&domain->hdr.list, &cbqri_res->resctrl_res.ctrl_domains);
> > +               *id = internal_id;
> > +               err = resctrl_online_ctrl_domain(res, domain);
> > +               if (err) {
> > +                       pr_warn("%s(): failed to online cbqri_res domain", __func__);
> > +                       goto err_free_domain;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +
> > +err_free_domain:
> > +       pr_warn("%s(): err_free_domain", __func__);
> > +       kfree(container_of(domain, struct cbqri_resctrl_dom, resctrl_ctrl_dom));
> 
> free hw_dom->ctrl_val ？

I'll take a closer look at the error cleanup path and fix in the next
rev.

Thanks,
Drew