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

[Philippe Mathieu-Daudé]

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
> 
>