hw/misc/bcm2835_rng: Specify valid memory access sizes

[PATCH] hw/misc/bcm2835_rng: Specify valid memory access sizes

Posted by Peter Maydell 4 weeks, 1 day ago

The BCM2835 RNG has 32-bit registers only; specify this in
the MemoryRegionOps so wrong-sized accesses are rejected rather
than getting to the assertions in the read and write functions.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3394
Fixes: 54a5ba13a9f ("target-arm: Implement BCM2835 hardware RNG")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/bcm2835_rng.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/misc/bcm2835_rng.c b/hw/misc/bcm2835_rng.c
index e4d2c224c8..bd4acade5b 100644
--- a/hw/misc/bcm2835_rng.c
+++ b/hw/misc/bcm2835_rng.c
@@ -93,6 +93,8 @@ static const MemoryRegionOps bcm2835_rng_ops = {
     .read = bcm2835_rng_read,
     .write = bcm2835_rng_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
 };
 
 static const VMStateDescription vmstate_bcm2835_rng = {
-- 
2.43.0

Re: [PATCH] hw/misc/bcm2835_rng: Specify valid memory access sizes

Posted by Philippe Mathieu-Daudé 3 weeks, 5 days ago

Hi Peter,

On 1/5/26 18:27, Peter Maydell wrote:
> The BCM2835 RNG has 32-bit registers only; specify this in
> the MemoryRegionOps so wrong-sized accesses are rejected rather
> than getting to the assertions in the read and write functions.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3394
> Fixes: 54a5ba13a9f ("target-arm: Implement BCM2835 hardware RNG")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/bcm2835_rng.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/misc/bcm2835_rng.c b/hw/misc/bcm2835_rng.c
> index e4d2c224c8..bd4acade5b 100644
> --- a/hw/misc/bcm2835_rng.c
> +++ b/hw/misc/bcm2835_rng.c
> @@ -93,6 +93,8 @@ static const MemoryRegionOps bcm2835_rng_ops = {
>       .read = bcm2835_rng_read,
>       .write = bcm2835_rng_write,
>       .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,

.impl is clearly 4 (why not set it? I tend to think the default
values -- where unset -- come from adapting pre-MemoryRegionOps models,
and today better be explicit and avoid implicit defaults here).

What is the rationale for max_access_size=4, that the ARM core is
a 32-bit CPU? I guess remember these peripherals are on an AXI bus
accessible by the videocore as 64-bit accesses. But QEMU doesn't
model that, and other peripherals are also limited to 32-bit max.
So good enough for now I suppose.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   };
>   
>   static const VMStateDescription vmstate_bcm2835_rng = {

Re: [PATCH] hw/misc/bcm2835_rng: Specify valid memory access sizes

Posted by Peter Maydell 3 weeks, 4 days ago

On Mon, 4 May 2026 at 13:29, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 1/5/26 18:27, Peter Maydell wrote:
> > The BCM2835 RNG has 32-bit registers only; specify this in
> > the MemoryRegionOps so wrong-sized accesses are rejected rather
> > than getting to the assertions in the read and write functions.
> >
> > Cc: qemu-stable@nongnu.org
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3394
> > Fixes: 54a5ba13a9f ("target-arm: Implement BCM2835 hardware RNG")
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/misc/bcm2835_rng.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/misc/bcm2835_rng.c b/hw/misc/bcm2835_rng.c
> > index e4d2c224c8..bd4acade5b 100644
> > --- a/hw/misc/bcm2835_rng.c
> > +++ b/hw/misc/bcm2835_rng.c
> > @@ -93,6 +93,8 @@ static const MemoryRegionOps bcm2835_rng_ops = {
> >       .read = bcm2835_rng_read,
> >       .write = bcm2835_rng_write,
> >       .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .valid.min_access_size = 4,
> > +    .valid.max_access_size = 4,
>
> .impl is clearly 4 (why not set it? I tend to think the default
> values -- where unset -- come from adapting pre-MemoryRegionOps models,
> and today better be explicit and avoid implicit defaults here).
>
> What is the rationale for max_access_size=4, that the ARM core is
> a 32-bit CPU? I guess remember these peripherals are on an AXI bus
> accessible by the videocore as 64-bit accesses. But QEMU doesn't
> model that, and other peripherals are also limited to 32-bit max.
> So good enough for now I suppose.

The rationale is that the registers are all 32 bit, and we're
already asserting that size == 4, so we know there's no code
out there in the wild that's trying to do accesses at different
sizes.

Adding

 +    .impl.min_access_size = 4,
 +    .impl.max_access_size = 4,

is probably a good idea here, I agree.

-- PMM

Re: [PATCH] hw/misc/bcm2835_rng: Specify valid memory access sizes

Posted by Philippe Mathieu-Daudé 3 weeks, 4 days ago

On 5/5/26 10:16, Peter Maydell wrote:
> On Mon, 4 May 2026 at 13:29, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 1/5/26 18:27, Peter Maydell wrote:
>>> The BCM2835 RNG has 32-bit registers only; specify this in
>>> the MemoryRegionOps so wrong-sized accesses are rejected rather
>>> than getting to the assertions in the read and write functions.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3394
>>> Fixes: 54a5ba13a9f ("target-arm: Implement BCM2835 hardware RNG")
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    hw/misc/bcm2835_rng.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/misc/bcm2835_rng.c b/hw/misc/bcm2835_rng.c
>>> index e4d2c224c8..bd4acade5b 100644
>>> --- a/hw/misc/bcm2835_rng.c
>>> +++ b/hw/misc/bcm2835_rng.c
>>> @@ -93,6 +93,8 @@ static const MemoryRegionOps bcm2835_rng_ops = {
>>>        .read = bcm2835_rng_read,
>>>        .write = bcm2835_rng_write,
>>>        .endianness = DEVICE_NATIVE_ENDIAN,
>>> +    .valid.min_access_size = 4,
>>> +    .valid.max_access_size = 4,
>>
>> .impl is clearly 4 (why not set it? I tend to think the default
>> values -- where unset -- come from adapting pre-MemoryRegionOps models,
>> and today better be explicit and avoid implicit defaults here).
>>
>> What is the rationale for max_access_size=4, that the ARM core is
>> a 32-bit CPU? I guess remember these peripherals are on an AXI bus
>> accessible by the videocore as 64-bit accesses. But QEMU doesn't
>> model that, and other peripherals are also limited to 32-bit max.
>> So good enough for now I suppose.
> 
> The rationale is that the registers are all 32 bit, and we're
> already asserting that size == 4, so we know there's no code
> out there in the wild that's trying to do accesses at different
> sizes.

My worry is about someone extending the model, allowing different
access sizes.

> 
> Adding
> 
>   +    .impl.min_access_size = 4,
>   +    .impl.max_access_size = 4,
> 
> is probably a good idea here, I agree.

Thanks,

Phil.