[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial device
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices |
Date: |
Thu, 2 Nov 2017 16:04:47 -0400 (EDT) |
----- Original Message -----
> From: "Philippe Mathieu-Daudé" <address@hidden>
> To: "Paolo Bonzini" <address@hidden>, "Mike Nawrocki" <address@hidden>, "Avi
> Kivity"
> <address@hidden>, "Peter Maydell" <address@hidden>, "Konrad Frederic"
> <address@hidden>
> Cc: address@hidden, address@hidden
> Sent: Thursday, November 2, 2017 8:28:09 PM
> Subject: Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial
> devices
>
> Hi Mike, Paolo.
>
> >> diff --git a/hw/char/serial.c b/hw/char/serial.c
> >> index 376bd2f240..c16c19a406 100644
> >> --- a/hw/char/serial.c
> >> +++ b/hw/char/serial.c
> >> @@ -1005,7 +1005,7 @@ static void serial_mm_write(void *opaque, hwaddr
> >> addr,
> >> uint64_t value, unsigned size)
> >> {
> >> SerialState *s = opaque;
> >> - value &= ~0u >> (32 - (size * 8));
> >> + value &= ~((uint64_t)0) >> (64 - (size * 8));
> >> serial_ioport_write(s, addr >> s->it_shift, value, 1);
> >> }
> >
> > Couldn't this be simply "value &= 255"? Otherwise looks good.
>
> This is not incorrect, but I don't think this is the correct fix.
> Isn't it what memory::access_with_adjusted_size() is supposed to do with
> .impl.max_access_size = 1?
The mm version doesn't have .impl.max_access_size=1.
Thanks,
Paolo
> This looks like the same issue I tried to fix here:
> http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg02255.html
>
> I'll try to find some time to respin the full series with tests.
>
> >> @@ -1014,16 +1014,22 @@ static const MemoryRegionOps serial_mm_ops[3] = {
> >> .read = serial_mm_read,
> >> .write = serial_mm_write,
> >> .endianness = DEVICE_NATIVE_ENDIAN,
> >> + .valid.max_access_size = 8,
> >> + .impl.max_access_size = 8,
> >> },
> >> [DEVICE_LITTLE_ENDIAN] = {
> >> .read = serial_mm_read,
> >> .write = serial_mm_write,
> >> .endianness = DEVICE_LITTLE_ENDIAN,
> >> + .valid.max_access_size = 8,
> >> + .impl.max_access_size = 8,
> >> },
> >> [DEVICE_BIG_ENDIAN] = {
> >> .read = serial_mm_read,
> >> .write = serial_mm_write,
> >> .endianness = DEVICE_BIG_ENDIAN,
> >> + .valid.max_access_size = 8,
> >> + .impl.max_access_size = 8,
>