Re: [PATCH v7 4/6] hw/riscv: implement CBQRI bandwidth controller

[Drew Fustini <fustini@kernel.org>]

On Mon, Nov 09, 2026 at 10:09:29AM -0300, Daniel Henrique Barboza wrote:
> On 11/1/2026 5:25 PM, Drew Fustini wrote:
> > From: Nicolas Pitre <npitre@baylibre.com>
> > 
> > Implement a bandwidth controller according to the Capacity and Bandwidth
> > QoS Register Interface (CBQRI) which supports these capabilities:
> > 
> >    - Number of access types: 2 (code and data)
> >    - Usage monitoring operations: CONFIG_EVENT, READ_COUNTER
> >    - Event IDs supported: None, Total read/write byte count, Total
> >                           read byte count, Total write byte count
> >    - Bandwidth allocation operations: CONFIG_LIMIT, READ_LIMIT
> > 
> > The controller gives the Linux resctrl support for CBQRI a device to
> > configure and read back bandwidth limits per RCID on the virt machine.
> > Only the register interface is modeled. The configured limits are not
> > enforced and the monitoring counters are always marked INVALID because
> > QEMU does not model memory bandwidth usage.
> > 
> > Link: https://github.com/riscv-non-isa/riscv-cbqri/releases/tag/v1.0
> > Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> > [fustini: add fields introduced in the ratified spec: rpfx and p]
> > Signed-off-by: Drew Fustini <fustini@kernel.org>
> > ---
> 
> LGTM overall.  A few comments:

Thanks for reviewing!

[..]
> > +typedef struct BandwidthAllocation {
> > +    uint32_t Rbwb:16;
> > +    uint32_t Mweight:8;
> > +    uint32_t sharedAT:3;
> > +    bool useShared:1;
> > +} BandwidthAllocation;
[..]
> I believe BandwidthAllocation could be renamed to RiscvCbrqiBandwidthAllocation for
> naming consistency.

Good idea, I'll change it.

> > +#define RISCV_CBQRI_BC(obj) \
> > +    OBJECT_CHECK(RiscvCbqriBandwidthState, (obj), TYPE_RISCV_CBQRI_BC)
> > +
> > +/*
> > + * All bandwidth allocation records are contiguous to simplify
> > + * allocation. The first one is used to hold the BC_BW_ALLOC register
> > + * content, followed by respective records for each AT per RCID.
> > + *
> > + * Records are indexed by the AT encoding, so each RCID has one record
> > + * per AT value whether or not the controller supports the code access
> > + * type. is_valid_at() rejects an unsupported AT before a record is
> > + * selected.
> > + */
> > +static unsigned int bw_alloc_index(uint32_t rcid, uint32_t at)
> > +{
> > +    assert(at < BC_NB_ATS);
> 
> Please use g_assert() instead of assert().

Okay, I will change it.

[..]
> > +static void riscv_cbqri_bc_class_init(ObjectClass *klass, const void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    dc->realize = riscv_cbqri_bc_realize;
> > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > +    dc->desc = "RISC-V CBQRI Bandwidth Controller";
> > +    device_class_set_props(dc, riscv_cbqri_bc_properties);
> > +    dc->legacy_reset = riscv_cbqri_bc_reset;
> 
> We're moving away from legacy_reset for new devices.  We're now declaring a Resettable
> interface and setting reset phases.
> 
> For this device a simple way of doing it is to slightly change th reset callback params:
> 
> 
> static void riscv_cbqri_bc_reset(Object *obj, ResetType type)
> 
> And then during class_init:
> 
>     ResettableClass *rc = RESETTABLE_CLASS(klass);
> 
>     rc->phases.hold = riscv_cbqri_bc_reset;

Thanks for letting me know. I will switch to the Resettable interface.

[..]
> > +DeviceState *riscv_cbqri_bc_create(hwaddr addr,
> > +                                   const RiscvCbqriBandwidthCaps *caps)
> > +{
> > +    DeviceState *dev = qdev_new(TYPE_RISCV_CBQRI_BC);
> > +
> > +    qdev_prop_set_uint64(dev, "mmio_base", addr);
> > +    qdev_prop_set_uint16(dev, "max_mcids", caps->nb_mcids);
> > +    qdev_prop_set_uint16(dev, "max_rcids", caps->nb_rcids);
> > +    qdev_prop_set_uint16(dev, "nbwblks", caps->nbwblks);
> > +    qdev_prop_set_uint16(dev, "mrbwb", caps->mrbwb);
> > +
> > +    qdev_prop_set_bit(dev, "at_code",
> > +                      caps->supports_at_code);
> > +    qdev_prop_set_bit(dev, "alloc_op_config_limit",
> > +                      caps->supports_alloc_op_config_limit);
> > +    qdev_prop_set_bit(dev, "alloc_op_read_limit",
> > +                      caps->supports_alloc_op_read_limit);
> > +    qdev_prop_set_bit(dev, "mon_op_config_event",
> > +                      caps->supports_mon_op_config_event);
> > +    qdev_prop_set_bit(dev, "mon_op_read_counter",
> > +                      caps->supports_mon_op_read_counter);
> > +    qdev_prop_set_bit(dev, "mon_evt_id_none",
> > +                      caps->supports_mon_evt_id_none);
> > +    qdev_prop_set_bit(dev, "mon_evt_id_rdwr_count",
> > +                      caps->supports_mon_evt_id_rdwr_count);
> > +    qdev_prop_set_bit(dev, "mon_evt_id_rdonly_count",
> > +                      caps->supports_mon_evt_id_rdonly_count);
> > +    qdev_prop_set_bit(dev, "mon_evt_id_wronly_count",
> > +                      caps->supports_mon_evt_id_wronly_count);
> > +
> > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > +
> > +    return dev;
> > +}
> 
> We don't have any callers for riscv_cbqri_bc_create() in the series so I can't
> see how the intended use would be.  In particular with the RiscvCbqriBandwidthCaps *caps
> attribute.

Good catch. They were for the fixed example SoC for v1 back in the 2023
v1. v2 switched to -device and nothing has called them since.

> Note that riscv_cbqri_bc_create() is blindly applying all settings from 'caps' to
> the device.  This is fine, but note that extra care is required when retrieving 'caps'
> w.r.t the defaults established.
> 
> E.g. in riscv_cbqri_bc_properties[] we have:
> 
> > +    DEFINE_PROP_UINT16("max_mcids", RiscvCbqriBandwidthState, nb_mcids, 256),
> 
> > +    DEFINE_PROP_UINT16("mrbwb", RiscvCbqriBandwidthState, mrbwb, 819),
> > +
> > +    DEFINE_PROP_BOOL("at_code", RiscvCbqriBandwidthState,
> > +                     supports_at_code, true),
> 
> 
> Now let's say that we'll add the device in the 'virt' machine, and I want to change just
> nb_mcids to 512 and nothing else. If I do this:
> 
> 
> RiscvCbqriBandwidthCaps caps = {.nb_mcids = 512};
> 
> (...)
> 
> riscv_cbqri_bc_create(addr, &caps);
> 
> 
> 
> I'll create a TYPE_RISCV_CBQRI_BC nb_mcids = 512 and everything else will be zeroed, disregarding
> all defaults from riscv_cbqri_bc_properties[].
> 
> 
> My suggestion here is to create a riscv_cbqri_bc_caps_init(RiscvCbqriBandwidthCaps *caps) helper
> that callers can use to set the struct defaults first.  We would then do:
> 
> 
> RiscvCbqriBandwidthCaps caps;
> 
> riscv_cbqri_bc_caps_init(caps);
> caps.nb_mcids = 512;
> 
> (...)
> 
> riscv_cbqri_bc_create(addr, &caps);
> 
> 
> We have precedent for this pattern - most RISC-V boards use riscv_boot_info_init() to init
> a boot_info struct.

Thanks for the guidance. I will add riscv_cbqri_bc_caps_init() and
riscv_cbqri_cc_caps_init() to set the property defaults in the caps
struct.

Thanks,
Drew