Re: [PULL 19/92] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes

[Peter Maydell]

On Thu, 24 Sep 2020 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> The serial device has 8 registers, each 8-bit. The MemoryRegionOps
> 'serial_io_ops' is initialized with max_access_size=1, and all
> memory_region_init_io() callers correctly set the region size to
> 8 bytes:
> - serial_io_realize
> - serial_isa_realizefn
> - serial_pci_realize
> - multi_serial_pci_realize
>
> It is safe to assert the offset argument of serial_ioport_read()
> and serial_ioport_write() is always less than 8.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20200907015535.827885-2-f4bug@amsat.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index fd80ae5592..840da89de7 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -344,7 +344,7 @@ static void serial_ioport_write(void *opaque, hwaddr 
> addr, uint64_t val,
>  {
>      SerialState *s = opaque;
>
> -    addr &= 7;
> +    assert(size == 1 && addr < 8);
>      trace_serial_ioport_write(addr, val);
>      switch(addr) {
>      default:

Bug report https://bugs.launchpad.net/qemu/+bug/1904331
points out that the addition of this assert() makes obvious
that either the assert is wrong or some code later in the
function which is looking at size must be dead:
            if (size == 1) {
                s->divider = (s->divider & 0xff00) | val;
            } else {
                s->divider = val;
            }

Presumably it's the if() that should be fixed ?

thanks
-- PMM