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

[Drew Fustini <fustini@kernel.org>]

On Mon, Mar 02, 2026 at 06:55:10PM +0000, Radim Krcmar wrote:
> 2026-02-01T15:58:10-08:00, Drew Fustini <fustini@kernel.org>:
> > 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
> >
> > 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>
> > ---
> > diff --git a/hw/riscv/cbqri_bandwidth.c b/hw/riscv/cbqri_bandwidth.c
> > [...]
> > +static uint32_t bandwidth_config(RiscvCbqriBandwidthState *bc,
> > +                                 uint32_t rcid, uint32_t at,
> > +                                 bool *busy)
> > +{
> > +    BandwidthAllocation *bw_alloc = get_bw_alloc(bc, rcid, at);
> > +
> > +    /*
> > +     * Bandwidth is allocated in multiples of bandwidth blocks, and the
> > +     * value in Rbwb must be at least 1 and must not exceed MRBWB value.
> > +     */
> > +    if (bc->bw_allocations[0].Rbwb < 1) {
> > +        return BC_ALLOC_STATUS_INVAL_OP;
> > +    } else if (bc->bw_allocations[0].Rbwb > bc->mrbwb) {
> > +        return BC_ALLOC_STATUS_INVAL_OP;
> > +    }
> 
> "the sum of Rbwb allocated across all RCIDs must not exceed MRBWB".

Ah, so it should be checking the sum of all Rbwb and not sure the
current rcid. I'll fix.

> 
> > [...]
> > +static void riscv_cbqri_bc_write_mon_ctl(RiscvCbqriBandwidthState *bc,
> > +                                         uint64_t value)
> > +{
> > [...]
> > +    if (mcid >= bc->nb_mcids) {
> > +        status = BC_MON_CTL_STATUS_INVAL_MCID;
> > +    } else if (op == BC_MON_OP_CONFIG_EVENT &&
> > +               bc->supports_mon_op_config_event) {
> > +        if (evt_id == BC_EVT_ID_None &&
> > +            bc->supports_mon_evt_id_none) {
> > +            bc->mon_counters[mcid].active = false;
> > +            status = BC_MON_CTL_STATUS_SUCCESS;
> > +        } else if ((evt_id == BC_EVT_ID_RDWR_count &&
> > +                    bc->supports_mon_evt_id_rdwr_count) ||
> > +                   (evt_id == BC_EVT_ID_RDONLY_count &&
> > +                    bc->supports_mon_evt_id_rdonly_count) ||
> > +                   (evt_id == BC_EVT_ID_WRONLY_count &&
> > +                    bc->supports_mon_evt_id_wronly_count)) {
> > +            if (atv && !is_valid_at(bc, at)) {
> > +                status = BC_MON_CTL_STATUS_INVAL_AT;
> > +            } else {
> > +                bc->mon_counters[mcid].ctr_val =
> > +                    FIELD_DP64(0, BC_MON_CTR_VAL, INVALID, 1);
> 
> This caught my attention even in the capacity controller.
> Maybe a it's worth a short comment that we set INVALID, because we don't
> actually do any bookkeeping?

Good point, I'll add that.

> 
> > [...]
> > +DeviceState *riscv_cbqri_bc_create(hwaddr addr,
> > +                                   const RiscvCbqriBandwidthCaps *caps,
> > +                                   const char *target_name)
> > +{
> > +    DeviceState *dev = qdev_new(TYPE_RISCV_CBQRI_BC);
> > +
> > +    qdev_prop_set_uint64(dev, "mmio_base", addr);
> > +    qdev_prop_set_string(dev, "target", target_name);
> > +    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);
> 
> Missing mrbwb.

Thanks, I'll add that.

Drew