* [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices @ 2017-10-31 15:24 Mike Nawrocki 2017-11-02 18:09 ` Paolo Bonzini 0 siblings, 1 reply; 5+ messages in thread From: Mike Nawrocki @ 2017-10-31 15:24 UTC (permalink / raw) To: qemu-devel; +Cc: mst, pbonzini, Mike Nawrocki Some drivers for the PPMC7400 PowerPC evaluation board accesses the serial registers through the floating point unit (stfd/ldfd), which is an 8-byte wide access. This patch enables that behavior. Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu> --- hw/char/serial.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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); } @@ -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, }, }; -- 2.14.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices 2017-10-31 15:24 [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices Mike Nawrocki @ 2017-11-02 18:09 ` Paolo Bonzini 2017-11-02 19:28 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 5+ messages in thread From: Paolo Bonzini @ 2017-11-02 18:09 UTC (permalink / raw) To: Mike Nawrocki, qemu-devel; +Cc: mst On 31/10/2017 16:24, Mike Nawrocki wrote: > > 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. Paolo > @@ -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, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices 2017-11-02 18:09 ` Paolo Bonzini @ 2017-11-02 19:28 ` Philippe Mathieu-Daudé 2017-11-02 20:04 ` Paolo Bonzini 0 siblings, 1 reply; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2017-11-02 19:28 UTC (permalink / raw) To: Paolo Bonzini, Mike Nawrocki, Avi Kivity, Peter Maydell, Konrad Frederic Cc: qemu-devel, mst 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? 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, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices 2017-11-02 19:28 ` Philippe Mathieu-Daudé @ 2017-11-02 20:04 ` Paolo Bonzini 2017-11-06 16:08 ` Michael Nawrocki 0 siblings, 1 reply; 5+ messages in thread From: Paolo Bonzini @ 2017-11-02 20:04 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Mike Nawrocki, Avi Kivity, Peter Maydell, Konrad Frederic, qemu-devel, mst ----- Original Message ----- > From: "Philippe Mathieu-Daudé" <f4bug@amsat.org> > To: "Paolo Bonzini" <pbonzini@redhat.com>, "Mike Nawrocki" <michael.nawrocki@gtri.gatech.edu>, "Avi Kivity" > <avi@redhat.com>, "Peter Maydell" <peter.maydell@linaro.org>, "Konrad Frederic" <fred.konrad@greensocs.com> > Cc: qemu-devel@nongnu.org, mst@redhat.com > 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, > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices 2017-11-02 20:04 ` Paolo Bonzini @ 2017-11-06 16:08 ` Michael Nawrocki 0 siblings, 0 replies; 5+ messages in thread From: Michael Nawrocki @ 2017-11-06 16:08 UTC (permalink / raw) To: Paolo Bonzini, Philippe Mathieu-Daudé Cc: Avi Kivity, Peter Maydell, Konrad Frederic, qemu-devel, mst On 11/02/2017 04:04 PM, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Philippe Mathieu-Daudé" <f4bug@amsat.org> >> To: "Paolo Bonzini" <pbonzini@redhat.com>, "Mike Nawrocki" <michael.nawrocki@gtri.gatech.edu>, "Avi Kivity" >> <avi@redhat.com>, "Peter Maydell" <peter.maydell@linaro.org>, "Konrad Frederic" <fred.konrad@greensocs.com> >> Cc: qemu-devel@nongnu.org, mst@redhat.com >> 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, >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-06 16:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-31 15:24 [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices Mike Nawrocki 2017-11-02 18:09 ` Paolo Bonzini 2017-11-02 19:28 ` Philippe Mathieu-Daudé 2017-11-02 20:04 ` Paolo Bonzini 2017-11-06 16:08 ` Michael Nawrocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).