Re: [PATCH 3/7] hw/riscv: implement CBQRI capacity controller

[Drew Fustini <fustini@kernel.org>]

On Thu, Nov 20, 2025 at 08:25:44PM +0100, Radim Krčmář wrote:
> 2025-11-19T16:42:19-08:00, Drew Fustini <fustini@kernel.org>:
> > From: Nicolas Pitre <npitre@baylibre.com>
> >
> > Implement a capacity 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, Occupancy
> >   - Capacity allocation ops: CONFIG_LIMIT, READ_LIMIT, FLUSH_RCID
> >
> > Link: https://github.com/riscv-non-isa/riscv-cbqri/blob/main/riscv-cbqri.pdf
> > Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> > [fustini: add fields introduced in the ratified spec: cunits, rpfx, p]
> > Signed-off-by: Drew Fustini <fustini@kernel.org>
> > ---
> > diff --git a/hw/riscv/cbqri_capacity.c b/hw/riscv/cbqri_capacity.c
> > [...]
> > +static void riscv_cbqri_cc_write_alloc_ctl(RiscvCbqriCapacityState *cc,
> > +                                           uint64_t value)
> > +{
> >[...]
> > +    if (rcid >= cc->nb_rcids) {
> > +        status = CC_ALLOC_STATUS_INVAL_RCID;
> > +    } else if (atv && !is_valid_at(cc, at)) {
> > +        status = CC_ALLOC_STATUS_INVAL_AT;
> > +    } else if (op == CC_ALLOC_OP_CONFIG_LIMIT &&
> > +               cc->supports_alloc_op_config_limit) {
> > +        status = alloc_blockmask_config(cc, rcid, at, &busy);
> > +    } else if (op == CC_ALLOC_OP_READ_LIMIT &&
> > +               cc->supports_alloc_op_read_limit) {
> > +        status = alloc_blockmask_read(cc, rcid, at, &busy);
> > +    } else if (op == CC_ALLOC_OP_FLUSH_RCID &&
> > +               cc->supports_alloc_op_flush_rcid) {
> > +        status = alloc_blockmask_init(cc, rcid, at, false, &busy);
> 
> The spec says the following about flush:
> 
>   "The configured capacity block allocation or the capacity unit limit
>    is not changed by this operation."
> 
> Limits are not implemented, so I think it's supposed to be a nop.

Thanks, that makes sense. I'll change it for the next revision.

> > [...]
> > +static uint64_t riscv_cbqri_cc_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    RiscvCbqriCapacityState *cc = opaque;
> > +    uint64_t value = 0;
> > +
> > +    assert((addr % 8) == 0);
> > +    assert(size == 8);
> 
> Is there a plan to extend support for 32 bit operations?
> 
>   "The CBQRI registers are defined so that software can perform two
>    individual 4 byte accesses."

Good question. Daniel also commented about the asserts for size == 8
while your MemoryRegionOps has .valid.min_access_size = 4. It does seem
like from the spec that these CBQRI controllers do need to support 4
byte accesses.

However, I need to understand better how to accomplish that. I'm
uncertain if I need to add code to the read and write hooks for that, or
if something in MemoryRegionOps can handle it automatically.

> > [...]
> > +    case A_CC_BLOCK_MASK:
> > +        if (cc->ncblks == 0) {
> > +            break;
> > +        }
> > +        /* fallthrough */
> > +    default:
> > +        unsigned int blkmask_slot = (addr - A_CC_BLOCK_MASK) / 8;
> > +        if (blkmask_slot >= (cc->ncblks + 63) / 64) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "%s: out of bounds (addr=0x%x)",
> > +                          __func__, (uint32_t)addr);
> > +            break;
> > +        }
> > +        value = cc->alloc_blockmasks[blkmask_slot];
> 
> There is supposed to be a cc_cunits registers after the cc_block_mask.

Good point, I need to update the code to account for cc_units.

> > [...]
> > +static void riscv_cbqri_cc_realize(DeviceState *dev, Error **errp)
> > +{
> > +    RiscvCbqriCapacityState *cc = RISCV_CBQRI_CC(dev);
> > +
> > +    if (!cc->mmio_base) {
> > +        error_setg(errp, "mmio_base property not set");
> > +        return;
> > +    }
> > +
> > +    assert(cc->mon_counters == NULL);
> > +    cc->mon_counters = g_new0(MonitorCounter, cc->nb_mcids);
> > +
> > +    assert(cc->alloc_blockmasks == NULL);
> > +    uint64_t *end = get_blockmask_location(cc, cc->nb_rcids, 0);
> > +    unsigned int blockmasks_size = end - cc->alloc_blockmasks;
> > +    cc->alloc_blockmasks = g_new0(uint64_t, blockmasks_size);
> > +
> > +    memory_region_init_io(&cc->mmio, OBJECT(dev), &riscv_cbqri_cc_ops,
> > +                          cc, TYPE_RISCV_CBQRI_CC".mmio", 4 * 1024);
> 
> Shouldn't the region size take cc->ncblks into account?
> (A bitmask for 2^16 ids is 8kB.)

cc_block_mask field is BMW / 8. In the case of NCBLKS of 12 and NCBLKS
of 16, both end up with a BMW of 64 which would be 8 bytes. I think the
the only reason the allocation is 4KB is that is meant to be aligned to
the page size. Otherwise, the capacity controller register layout is
pretty small.

> > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &cc->mmio);
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, cc->mmio_base);
> > +}
> > +
> > +static void riscv_cbqri_cc_reset(DeviceState *dev)
> > +{
> > +    RiscvCbqriCapacityState *cc = RISCV_CBQRI_CC(dev);
> > +
> > +    cc->cc_mon_ctl = 0;
> > +    cc->cc_alloc_ctl = 0;
> 
> Only the busy field must be reset to 0.  I think the decision warrants a
> comment that we're zeroing more to have simpler code.

Okay, will do.

> > +
> > +    /* assign all capacity only to rcid0 */
> > +    for (unsigned int rcid = 0; rcid < cc->nb_rcids; rcid++) {
> 
> rcid != 0 are unspecified on reset, so I would prefer not to touch them.

Okay, that is a good point that the spec does state that. I'll fix this
in the next rev.

> > +        bool any_at = false;
> > +
> > +        if (cc->supports_at_data) {
> > +            alloc_blockmask_init(cc, rcid, CC_AT_DATA,
> > +                                 rcid == 0, NULL);
> > +            any_at = true;
> > +        }
> > +        if (cc->supports_at_code) {
> > +            alloc_blockmask_init(cc, rcid, CC_AT_CODE,
> > +                                 rcid == 0, NULL);
> > +            any_at = true;
> > +        }
> > +        if (!any_at) {
> > +            alloc_blockmask_init(cc, rcid, 0,
> > +                                 rcid == 0, NULL);
> > +        }
> > +    }
> 
> I think it looks a bit better if AT values were expressed as a bitfield
> of size 8: (untested)
> 
>     unsigned long at = find_next_bit(&cc->supported_at_mask, 8, 0);
> 
>     do {
>         alloc_blockmask_init(cc, rcid, at < 8 ? at : 0, rcid == 0, NULL);
>         at = find_next_bit(&cc->supported_at_mask, 8, at + 1);
>     } while (at < 8);

Thanks for the suggestion. I will give that a try.

Drew