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

[Drew Fustini <fustini@kernel.org>]

On Wed, Oct 28, 2026 at 07:02:32PM +0530, Prajwal N wrote:
> Hi Drew,
> 
> On 26-10-2026 23:17, Drew Fustini wrote:
> > 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/releases/tag/v1.0
> > Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> > [fustini: add fields introduced in the ratified spec: cunits, rpfx, p]
> > Reviewed-by: Radim Krčmář <radim.krcmar@oss.qualcomm.com>
> > Signed-off-by: Drew Fustini <fustini@kernel.org>
> > ---
> >  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 <npitre@baylibre.com>
> >  M: Drew Fustini <fustini@kernel.org>
> >  L: qemu-riscv@nongnu.org
> >  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

Thank you for reviewing. Did you see that riscv_cbqri_cc_ops actually
invokes riscv_cbqri_cc_read_wrapper() and
riscv_cbqri_cc_write_wrapper()?

Those wrappers were added in v5 based on Radim's feedback on v4.

Thanks,
Drew