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.
> [...]
> +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."
> + switch (addr) {
> [...]
> + 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.
> + }
> +
> + return value;
> +}
> [...]
> +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.)
> + 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.
> +
> + /* 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.
> + 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.
