On Thu, Nov 20, 2025 at 08:25:44PM +0100, Radim Krčmář wrote:
> 2025-11-19T16:42:19-08:00, Drew Fustini <[email protected]>:
> > From: Nicolas Pitre <[email protected]>
> >
> > 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 <[email protected]>
> > [fustini: add fields introduced in the ratified spec: cunits, rpfx, p]
> > Signed-off-by: Drew Fustini <[email protected]>
> > ---
> > 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
