| Message ID | 20200924092314.1722645-20-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/92] tests: add missing genh dependency | expand |
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
On 18/11/20 16:40, Peter Maydell wrote: > 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 ? It can be dropped, because serial_io_ops has .impl = { .min_access_size = 1, .max_access_size = 1, }, Therefore, a 16-bit write to addr==0 is automatically split into an 8-byte write to addr==0 and one to addr=1. Together, the two set the full 16 bits of s->divider. Thanks, Paolo
On 11/18/20 6:08 PM, Paolo Bonzini wrote: > On 18/11/20 16:40, Peter Maydell wrote: >> 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 ? > > It can be dropped, because serial_io_ops has > > .impl = { > .min_access_size = 1, > .max_access_size = 1, > }, > > Therefore, a 16-bit write to addr==0 is automatically split into an > 8-byte write to addr==0 and one to addr=1. Together, the two set the > full 16 bits of s->divider. Since commit 5ec3a23e6c8 ("serial: convert PIO to new memory api read/write") =) > > Thanks, > > Paolo > >
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: @@ -485,7 +485,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size) SerialState *s = opaque; uint32_t ret; - addr &= 7; + assert(size == 1 && addr < 8); switch(addr) { default: case 0: