Hi Drew,
On 26-10-2026 23:17, Drew Fustini wrote:
> 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/releases/tag/v1.0
> Signed-off-by: Nicolas Pitre <[email protected]>
> [fustini: add fields introduced in the ratified spec: cunits, rpfx, p]
> Reviewed-by: Radim Krčmář <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
> ---
> MAINTAINERS | 1 +
> hw/riscv/cbqri_capacity.c | 727
> ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 728 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2984779ce43a..f230cdbbe1b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -408,6 +408,7 @@ M: Nicolas Pitre <[email protected]>
> M: Drew Fustini <[email protected]>
> L: [email protected]
> S: Supported
> +F: hw/riscv/cbqri_capacity.c
> F: include/hw/riscv/cbqri.h
>
> RISC-V SiFive (Xsf*) extensions
> diff --git a/hw/riscv/cbqri_capacity.c b/hw/riscv/cbqri_capacity.c
> new file mode 100644
> index 000000000000..7ea20532b950
> --- /dev/null
> +++ b/hw/riscv/cbqri_capacity.c
> @@ -0,0 +1,727 @@
> +/*
> + * RISC-V Capacity and Bandwidth QoS Register Interface
> + * URL: https://github.com/riscv-non-isa/riscv-cbqri/releases/tag/v1.0
> + *
> + * Copyright (c) 2023 BayLibre SAS
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This file contains the Capacity-controller QoS Register Interface.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/bitmap.h"
> +#include "hw/core/qdev-properties.h"
> +#include "hw/core/sysbus.h"
> +#include "target/riscv/cpu.h"
> +#include "hw/riscv/cbqri.h"
[...]
> +static void riscv_cbqri_cc_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + RiscvCbqriCapacityState *cc = opaque;
> +
> + assert((addr % 8) == 0);
> +
The same comments in the riscv_cbqri_cc_read() below are applicable
here as well.
> + switch (addr) {
> + case A_CC_CAPABILITIES:
> + /* read-only register */
> + break;
> + case A_CC_MON_CTL:
> + riscv_cbqri_cc_write_mon_ctl(cc, value);
> + break;
> + case A_CC_ALLOC_CTL:
> + riscv_cbqri_cc_write_alloc_ctl(cc, value);
> + break;
> + case A_CC_MON_CTR_VAL:
> + /* read-only register */
> + break;
> + case A_CC_BLOCK_MASK:
> + if (cc->ncblks == 0) {
> + break;
> + }
> + /* fallthrough */
> + default:
> + uint32_t blkmask_slot = (addr - A_CC_BLOCK_MASK) / 8;
> + if (blkmask_slot == get_slots(cc)) {
> + /* this is cc_cunits register */
> + break;
> + } else if (blkmask_slot > get_slots(cc)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: out of bounds (addr=0x%x)",
> + __func__, (uint32_t)addr);
> + break;
> + }
> + cc->alloc_blockmasks[blkmask_slot] = value;
> + }
> +}
> +
> +static uint64_t riscv_cbqri_cc_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + RiscvCbqriCapacityState *cc = opaque;
> + uint64_t value = 0;
> +
> + assert((addr % 8) == 0);
> +
To access the 64 bit CBQRI register, the Linux kernel is performing two
32 bit reads. The register offsets that are passed to this function are 0x00,
0x04, 0x08 ... The assert is triggered for the addr 0x04, 0x0c.. and the
QEMU exists.
This check should have been:
assert((addr %4) == 0)
The switch statement uses the register offsets that are 8-byte aligned.
A_CC_CAPABILITIES (0x00), A_CC_MON_CTL (0x08) etc.
Since the kernel invokes read twice, the second readl() has the offset addr
0x04. This does not match any offset and falls into the default case.
The following change fixes the problem.
hwaddr reg_addr = addr & ~7ULL; /* align down to 8-byte boundary */> +
switch (addr) {
switch (reg_addr) {> + case A_CC_CAPABILITIES:
> + value = FIELD_DP64(value, CC_CAPABILITIES, VER_MAJOR,
> + RISCV_CBQRI_VERSION_MAJOR);
> + value = FIELD_DP64(value, CC_CAPABILITIES, VER_MINOR,
> + RISCV_CBQRI_VERSION_MINOR);
> + value = FIELD_DP64(value, CC_CAPABILITIES, NCBLKS,
> + cc->ncblks);
> + value = FIELD_DP64(value, CC_CAPABILITIES, FRCID,
> + cc->supports_alloc_op_flush_rcid);
> + value = FIELD_DP64(value, CC_CAPABILITIES, CUNITS, 0);
> + value = FIELD_DP64(value, CC_CAPABILITIES, RPFX, 0);
> + value = FIELD_DP64(value, CC_CAPABILITIES, P, 0);
> + break;
> + case A_CC_MON_CTL:
> + value = cc->cc_mon_ctl;
> + break;
> + case A_CC_ALLOC_CTL:
> + value = cc->cc_alloc_ctl;
> + break;
> + case A_CC_MON_CTR_VAL:
> + value = cc->cc_mon_ctr_val;
> + break;
> + 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 == get_slots(cc)) {
> + /*
> + * The cc_cunits register is always the slot following the
> + * last slot of cc_blockmask register. Capacity units are
> + * not supported by this implementation so must return 0.
> + */
> + value = 0;
> + break;
> + } else if (blkmask_slot > get_slots(cc)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: out of bounds (addr=0x%x)",
> + __func__, (uint32_t)addr);
> + break;
> + }
> + value = cc->alloc_blockmasks[blkmask_slot];
> + }
> +
The logic within the switch statement always computes the full 64-bit register
value. But the Linux kernel requested for 32 bits. The kernel is creating a
64-bit value with the two 32-bit values. This should return only the relevant
half.
The following code snippet fixed the problem.
/* Return the correct 32-bit */
if (size == 4) {
if (addr & 4)
return value >> 32; // high word
else
return value & 0xffffffff; // low word
}
Thanks,
Prajwal N
