Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices

[Michael Nawrocki]

On 11/02/2017 04:04 PM, Paolo Bonzini wrote:
> ----- 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

Sorry for the late reply on this. I implemented it with the shifting 
mask initially to mirror the original code, but it should definitely 
work with "value &= 255". Looking at serial_ioport_write, I think this 
might also fix a potential issue where the upper bits of the divider 
variable could be set if any bits 8-15 of "val" are set (and addr = 0).

I'll go ahead and push up a v2 patch with this fix.

Thanks,
Mike

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