Re: [PATCH] hw/char/bcm2835_aux: Allow less than 32-bit accesses

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 2 Oct 2020 at 19:10, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The "BCM2835 ARM Peripherals" datasheet [*] chapter 2
> ("Auxiliaries: UART1 & SPI1, SPI2"), list the register
> sizes as 3/8/16/32 bits. We assume this means this
> peripheral allows 8-bit accesses.
>
> This was not an issue until commit 5d971f9e67 which reverted
> ("memory: accept mismatching sizes in memory_region_access_valid").
>
> The model is implemented as 32-bit accesses (see commit 97398d900c,
> all registers are 32-bit) so replace MemoryRegionOps.valid as
> MemoryRegionOps.impl, and re-introduce MemoryRegionOps.valid
> with a 8/32-bit range.
>
> [*] https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
>
> Fixes: 97398d900c ("bcm2835_aux: add emulation of BCM2835 AUX (aka UART1) block")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Noticed while running Trusted Firmware-A on the raspi3:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg680115.html
> ---
>  hw/char/bcm2835_aux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
> index ee3dd40e3c..dade2ab5fd 100644
> --- a/hw/char/bcm2835_aux.c
> +++ b/hw/char/bcm2835_aux.c
> @@ -249,7 +249,9 @@ static const MemoryRegionOps bcm2835_aux_ops = {
>      .read = bcm2835_aux_read,
>      .write = bcm2835_aux_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>  };

We don't seem to document the exact semantics you get for
a write with a size smaller than the impl.min_access_size.
Looking at the implementation in softmmu/memory.c, the
answer seems to be "it's turned into a write at the larger
size where the other bits in the write are zeroes".
Those semantics seem OK for this device (though there are
devices where they would not be, I suspect).
(The other plausible implementation would have been
"we do a read-modify-write sequence", which would not be
OK for this device, since it has some "device state changes
on read" registers like AUX_MU_IO_REG.)

We should probably clarify the comments in the MemoryRegionOps
struct to nail down the behaviour when the .impl constraints
are tighter than the .valid ones, but for this patch:

Applied to target-arm.next, thanks.

-- PMM