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 <pbonz...@redhat.com> wrote:
>>>
>>> From: Philippe Mathieu-Daudé <f4...@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é <f4...@amsat.org>
>>> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
>>> Message-Id: <20200907015535.827885-2-f4...@amsat.org>
>>> Signed-off-by: Paolo Bonzini <pbonz...@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
>
>
