* [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).