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

[Drew Fustini <fustini@kernel.org>]

On Fri, Jan 16, 2026 at 05:20:00PM +0000, Radim Krčmář wrote:
> Sorry, I changed emails, and this series flew under my radar, since
> qemu-riscv lore public inbox has been silent for a month and I was
> happily oblivious that it's a bug on their side.
> 
> I'm addressing only the access size issue in this mail and I'll look at
> the rest next week.
> 
> 2026-01-05T13:54:21-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/releases/tag/v1.0
> > 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 const MemoryRegionOps riscv_cbqri_cc_ops = {
> > +    .read = riscv_cbqri_cc_read,
> > +    .write = riscv_cbqri_cc_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .valid.min_access_size = 4,
> > +    .valid.max_access_size = 8,
> > +    .impl.min_access_size = 8,
> 
> The .impl.min_access_size should be 4, since access_with_adjusted_size
> doesn't seem to do what we want.

Ok, thanks.

> > +    .impl.max_access_size = 8,
> 
> I think we can reuse the current 8 byte iplementation by adding
> something like the following hack for 4 byte accesses:
> 
>   static uint64_t riscv_cbqri_cc_read_wrapper(void *opaque, hwaddr addr,
>                                               unsigned size)
>   {
>       uint64_t value = riscv_cbqri_br_read(opaque, addr & ~0x7UL, 8);
>       if (size == 4) {
>           if (addr & 0x7) {
>               return value >> 32;
>           } else {
>               return value & 0xffffffff;
>           }
>       }
>       return value;
>   }
> 
> read has no side-effects, so the wrapper should be conceptually correct.

Thank you very much for the suggestion. I'll try it out and get a new
revision ready.

> 
> 
>   static uint64_t riscv_cbqri_cc_write_wrapper(void *opaque, hwaddr addr,
>                                                uint64_t value, unsigned size)
>   {
>       if (size == 4) {
>           uint64_t reg = riscv_cbqri_br_read(opaque, addr & ~0x7UL, 8);
>           if (addr & 0x7) {
>               value = value << 32 | reg & 0xffffffff;
>           } else {
>               value = value | reg & ~0xffffffffUL;
>           }
>       }
> 
>       riscv_cbqri_cc_write(opaque, addr & ~0x7UL, value, 8);
>   }
>  
> The control registers will execute an operation on a write to the upper
> half as well, which doesn't seem desirable, but it's unspecified afaik.
> The upper half of control registers has no fields that software would
> ever want to write, so I think it's not really a practical issue,
> especially since we're basically a skeleton implementation.

Thanks for the suggestion, I think that is an acceptable solution. The
main point of the Qemu implementation is to exercise the Linux kernel
code ahead of silicon that implements CBQRI.

> Another solution would be to implement just 4 byte registers, and handle
> 8 byte writes as a combination of two 4 byte -- the spec hints that this
> is an expected implementation, but with all registers being defined as 8
> byte, I think my hack would need "clarifications" to make it incorrect.

Thanks for also suggesting this. I agree that the previous suggestion is
sufficent.

-Drew