1. Patchew
  2. QEMU
  3. View series

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

Mike Nawrocki posted 1 patch 6 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171031152443.3670-1-michael.nawrocki@gtri.gatech.edu
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/char/serial.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices
Posted by Mike Nawrocki 6 years, 5 months ago 
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
Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices
Posted by Paolo Bonzini 6 years, 5 months ago 
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,
Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices
Posted by Philippe Mathieu-Daudé 6 years, 5 months ago 
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,
Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices
Posted by Paolo Bonzini 6 years, 5 months ago 

----- 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,
>
Re: [Qemu-devel] [PATCH] Enable 8-byte wide MMIO for 16550 serial devices
Posted by Michael Nawrocki 6 years, 5 months ago 
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,
>>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.