[Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

The libqos driver for omap_i2c currently does not work on Big Endian.
Introduce helpers for reading from and writing to 16-bit armel registers.

This fixes tmp105-test failures on ppc.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
 1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)

diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
index 9be57e9..7d50ef2 100644
--- a/tests/libi2c-omap.c
+++ b/tests/libi2c-omap.c
@@ -12,6 +12,7 @@
 #include <string.h>
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
 #include "libqtest.h"
 
 enum OMAPI2CRegisters {
@@ -48,12 +49,24 @@ typedef struct OMAPI2C {
 } OMAPI2C;
 
 
+static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
+{
+    memread(addr, data, 2);
+    *data = le16_to_cpu(*data);
+}
+
+static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
+{
+    data = cpu_to_le16(data);
+    memwrite(addr, &data, 2);
+}
+
 static void omap_i2c_set_slave_addr(OMAPI2C *s, uint8_t addr)
 {
     uint16_t data = addr;
 
-    memwrite(s->addr + OMAP_I2C_SA, &data, 2);
-    memread(s->addr + OMAP_I2C_SA, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_SA, data);
+    omap_i2c_read16(s->addr + OMAP_I2C_SA, &data);
     g_assert_cmphex(data, ==, addr);
 }
 
@@ -66,22 +79,22 @@ static void omap_i2c_send(I2CAdapter *i2c, uint8_t addr,
     omap_i2c_set_slave_addr(s, addr);
 
     data = len;
-    memwrite(s->addr + OMAP_I2C_CNT, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CNT, data);
 
     data = OMAP_I2C_CON_I2C_EN |
            OMAP_I2C_CON_TRX |
            OMAP_I2C_CON_MST |
            OMAP_I2C_CON_STT |
            OMAP_I2C_CON_STP;
-    memwrite(s->addr + OMAP_I2C_CON, &data, 2);
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CON, data);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) != 0);
 
-    memread(s->addr + OMAP_I2C_STAT, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
     g_assert((data & OMAP_I2C_STAT_NACK) == 0);
 
     while (len > 1) {
-        memread(s->addr + OMAP_I2C_STAT, &data, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
         g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
 
         memwrite(s->addr + OMAP_I2C_DATA, buf, 2);
@@ -89,13 +102,13 @@ static void omap_i2c_send(I2CAdapter *i2c, uint8_t addr,
         len -= 2;
     }
     if (len == 1) {
-        memread(s->addr + OMAP_I2C_STAT, &data, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
         g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
 
         memwrite(s->addr + OMAP_I2C_DATA, buf, 1);
     }
 
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) == 0);
 }
 
@@ -108,32 +121,32 @@ static void omap_i2c_recv(I2CAdapter *i2c, uint8_t addr,
     omap_i2c_set_slave_addr(s, addr);
 
     data = len;
-    memwrite(s->addr + OMAP_I2C_CNT, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CNT, data);
 
     data = OMAP_I2C_CON_I2C_EN |
            OMAP_I2C_CON_MST |
            OMAP_I2C_CON_STT |
            OMAP_I2C_CON_STP;
-    memwrite(s->addr + OMAP_I2C_CON, &data, 2);
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_write16(s->addr + OMAP_I2C_CON, data);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) == 0);
 
-    memread(s->addr + OMAP_I2C_STAT, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
     g_assert((data & OMAP_I2C_STAT_NACK) == 0);
 
-    memread(s->addr + OMAP_I2C_CNT, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_CNT, &data);
     g_assert_cmpuint(data, ==, len);
 
     while (len > 0) {
-        memread(s->addr + OMAP_I2C_STAT, &data, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &data);
         g_assert((data & OMAP_I2C_STAT_RRDY) != 0);
         g_assert((data & OMAP_I2C_STAT_ROVR) == 0);
 
         memread(s->addr + OMAP_I2C_DATA, &data, 2);
 
-        memread(s->addr + OMAP_I2C_STAT, &stat, 2);
+        omap_i2c_read16(s->addr + OMAP_I2C_STAT, &stat);
         if (unlikely(len == 1)) {
-            *buf = data & 0xf;
+            *buf = le16_to_cpu(data) & 0xf;
             buf++;
             len--;
         } else {
@@ -143,7 +156,7 @@ static void omap_i2c_recv(I2CAdapter *i2c, uint8_t addr,
         }
     }
 
-    memread(s->addr + OMAP_I2C_CON, &data, 2);
+    omap_i2c_read16(s->addr + OMAP_I2C_CON, &data);
     g_assert((data & OMAP_I2C_CON_STP) == 0);
 }
 
@@ -159,7 +172,7 @@ I2CAdapter *omap_i2c_create(uint64_t addr)
     i2c->recv = omap_i2c_recv;
 
     /* verify the mmio address by looking for a known signature */
-    memread(addr + OMAP_I2C_REV, &data, 2);
+    omap_i2c_read16(addr + OMAP_I2C_REV, &data);
     g_assert_cmphex(data, ==, 0x34);
 
     return i2c;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Peter Maydell]

On 2 February 2013 16:45, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> The libqos driver for omap_i2c currently does not work on Big Endian.
> Introduce helpers for reading from and writing to 16-bit armel registers.
>
> This fixes tmp105-test failures on ppc.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
>  1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)
>
> diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
> index 9be57e9..7d50ef2 100644
> --- a/tests/libi2c-omap.c
> +++ b/tests/libi2c-omap.c
> @@ -12,6 +12,7 @@
>  #include <string.h>
>
>  #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>  #include "libqtest.h"
>
>  enum OMAPI2CRegisters {
> @@ -48,12 +49,24 @@ typedef struct OMAPI2C {
>  } OMAPI2C;
>
>
> +static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
> +{
> +    memread(addr, data, 2);
> +    *data = le16_to_cpu(*data);
> +}
> +
> +static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
> +{
> +    data = cpu_to_le16(data);
> +    memwrite(addr, &data, 2);
> +}

There's nothing special about the OMAP i2c device that I know of:
shouldn't the test code just be using a generic "write 16 bit value
to memory with appropriate endianness for target CPU" function ?

-- PMM

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Andreas Färber]

Am 02.02.2013 17:49, schrieb Peter Maydell:
> On 2 February 2013 16:45, Andreas Färber <afaerber@suse.de> wrote:
>> From: Andreas Färber <andreas.faerber@web.de>
>>
>> The libqos driver for omap_i2c currently does not work on Big Endian.
>> Introduce helpers for reading from and writing to 16-bit armel registers.
>>
>> This fixes tmp105-test failures on ppc.
>>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>  tests/libi2c-omap.c |   51 ++++++++++++++++++++++++++++++++-------------------
>>  1 Datei geändert, 32 Zeilen hinzugefügt(+), 19 Zeilen entfernt(-)
>>
>> diff --git a/tests/libi2c-omap.c b/tests/libi2c-omap.c
>> index 9be57e9..7d50ef2 100644
>> --- a/tests/libi2c-omap.c
>> +++ b/tests/libi2c-omap.c
>> @@ -12,6 +12,7 @@
>>  #include <string.h>
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/bswap.h"
>>  #include "libqtest.h"
>>
>>  enum OMAPI2CRegisters {
>> @@ -48,12 +49,24 @@ typedef struct OMAPI2C {
>>  } OMAPI2C;
>>
>>
>> +static inline void omap_i2c_read16(uint64_t addr, uint16_t *data)
>> +{
>> +    memread(addr, data, 2);
>> +    *data = le16_to_cpu(*data);
>> +}
>> +
>> +static inline void omap_i2c_write16(uint64_t addr, uint16_t data)
>> +{
>> +    data = cpu_to_le16(data);
>> +    memwrite(addr, &data, 2);
>> +}
> 
> There's nothing special about the OMAP i2c device that I know of:
> shouldn't the test code just be using a generic "write 16 bit value
> to memory with appropriate endianness for target CPU" function ?

I asked about where to address this issue [1], no concrete answers, and
this is the only solution I came up with.

libqtest.h has no generic endian-aware memread functions unlike Alex,
you or me expected. It reads a sequence of bytes from guest memory and
transmits them one-by-one over the text-based qtest protocol. Since they
are raw bytes I need to convert them here. tmp105-test itself was
transmitting byte arrays, so it did not need changes.

Looking into where that annoying "mipid" debug output is coming from is
a bit further down on my to-do list for 1.4.

I tested this patch to resolve the test breakage on ppc and not to
introduce a regression for x86_64. I don't want arm disabled like sparc.
Any qtest API remodelling would be 1.5 material now, I guess, and could
be hidden inside my helper functions.

Andreas

[1] http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg05081.html

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Peter Maydell]

On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.02.2013 17:49, schrieb Peter Maydell:
>> There's nothing special about the OMAP i2c device that I know of:
>> shouldn't the test code just be using a generic "write 16 bit value
>> to memory with appropriate endianness for target CPU" function ?
>
> I asked about where to address this issue [1], no concrete answers, and
> this is the only solution I came up with.
>
> libqtest.h has no generic endian-aware memread functions unlike Alex,
> you or me expected. It reads a sequence of bytes from guest memory and
> transmits them one-by-one over the text-based qtest protocol.

OK, so this is just busted for accessing devices. The protocol
has to have some way of letting you do a 32 bit / 16 bit / 8 bit
access (and maybe 64 bit as well while we're here). memread
and memwrite are OK for RAM accesses [ie anything you'd be
happy to have cached or buffered in a real system] but for
memory mapped registers we need to have an equivalent of
inb/inw/inl/outb/outw/outl that guarantee to do exactly one
access of exactly the required width.

(If you want to do a workaround for 1.4 to get the test suite
running again I don't object; this is just a discussion about
what the right long term fix should be.)

-- PMM

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Andreas Färber]

Am 02.02.2013 18:44, schrieb Peter Maydell:
> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>> There's nothing special about the OMAP i2c device that I know of:
>>> shouldn't the test code just be using a generic "write 16 bit value
>>> to memory with appropriate endianness for target CPU" function ?
>>
>> I asked about where to address this issue [1], no concrete answers, and
>> this is the only solution I came up with.
>>
>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>> you or me expected. It reads a sequence of bytes from guest memory and
>> transmits them one-by-one over the text-based qtest protocol.
> 
> OK, so this is just busted for accessing devices. The protocol
> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
> access (and maybe 64 bit as well while we're here). memread
> and memwrite are OK for RAM accesses [ie anything you'd be
> happy to have cached or buffered in a real system] but for
> memory mapped registers we need to have an equivalent of
> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
> access of exactly the required width.
> 
> (If you want to do a workaround for 1.4 to get the test suite
> running again I don't object; this is just a discussion about
> what the right long term fix should be.)

OK. As discussed with Alex in IRC, I think the issue is that libqtest
does not know about which endianness the guest uses, so the only thing
we can do on the library side is generalize my helpers (e.g.,
memread_le16u). Anything else would need to be done in qtest.c, i.e.
adding new commands to the protocol (e.g., memread16) and implementing
them on both sides.

m48t59-test, as only other memread() user, gets around this due to doing
byte access only.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Blue Swirl]

On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>> There's nothing special about the OMAP i2c device that I know of:
>>> shouldn't the test code just be using a generic "write 16 bit value
>>> to memory with appropriate endianness for target CPU" function ?
>>
>> I asked about where to address this issue [1], no concrete answers, and
>> this is the only solution I came up with.
>>
>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>> you or me expected. It reads a sequence of bytes from guest memory and
>> transmits them one-by-one over the text-based qtest protocol.
>
> OK, so this is just busted for accessing devices. The protocol
> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
> access (and maybe 64 bit as well while we're here). memread
> and memwrite are OK for RAM accesses [ie anything you'd be
> happy to have cached or buffered in a real system] but for
> memory mapped registers we need to have an equivalent of
> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
> access of exactly the required width.

I was also just making a patch (but not so nice as Andreas'). My
analysis was that qtest.c and libqtest.c just pass the result of
cpu_physical_memory_rw() as is, with no endian conversion. So the
result needs to be converted to host CPU order just like Andreas did.
I think the protocol is OK, my initial reaction was to put byte
swapping there but that's not right.

>
> (If you want to do a workaround for 1.4 to get the test suite
> running again I don't object; this is just a discussion about
> what the right long term fix should be.)
>
> -- PMM

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Peter Maydell]

On 2 February 2013 18:26, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>>> you or me expected. It reads a sequence of bytes from guest memory and
>>> transmits them one-by-one over the text-based qtest protocol.
>>
>> OK, so this is just busted for accessing devices. The protocol
>> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
>> access (and maybe 64 bit as well while we're here). memread
>> and memwrite are OK for RAM accesses [ie anything you'd be
>> happy to have cached or buffered in a real system] but for
>> memory mapped registers we need to have an equivalent of
>> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
>> access of exactly the required width.
>
> I was also just making a patch (but not so nice as Andreas'). My
> analysis was that qtest.c and libqtest.c just pass the result of
> cpu_physical_memory_rw() as is, with no endian conversion. So the
> result needs to be converted to host CPU order just like Andreas did.
> I think the protocol is OK, my initial reaction was to put byte
> swapping there but that's not right.

No, I think the protocol is broken, even if you ignore the
question of endianness. Consider that a device's MemoryRegion
can have different behaviour depending on whether you access it
as a byte, halfword or word size. cpu_physical_memory_rw() won't
let you make a word size access to an unaligned address.

-- PMM

Re: [Qemu-devel] [PATCH for-1.4] libi2c-omap: Fix endianness dependency

[Blue Swirl]

On Sat, Feb 2, 2013 at 8:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 February 2013 18:26, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sat, Feb 2, 2013 at 5:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 2 February 2013 17:37, Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 02.02.2013 17:49, schrieb Peter Maydell:
>>>> libqtest.h has no generic endian-aware memread functions unlike Alex,
>>>> you or me expected. It reads a sequence of bytes from guest memory and
>>>> transmits them one-by-one over the text-based qtest protocol.
>>>
>>> OK, so this is just busted for accessing devices. The protocol
>>> has to have some way of letting you do a 32 bit / 16 bit / 8 bit
>>> access (and maybe 64 bit as well while we're here). memread
>>> and memwrite are OK for RAM accesses [ie anything you'd be
>>> happy to have cached or buffered in a real system] but for
>>> memory mapped registers we need to have an equivalent of
>>> inb/inw/inl/outb/outw/outl that guarantee to do exactly one
>>> access of exactly the required width.
>>
>> I was also just making a patch (but not so nice as Andreas'). My
>> analysis was that qtest.c and libqtest.c just pass the result of
>> cpu_physical_memory_rw() as is, with no endian conversion. So the
>> result needs to be converted to host CPU order just like Andreas did.
>> I think the protocol is OK, my initial reaction was to put byte
>> swapping there but that's not right.
>
> No, I think the protocol is broken, even if you ignore the
> question of endianness. Consider that a device's MemoryRegion
> can have different behaviour depending on whether you access it
> as a byte, halfword or word size. cpu_physical_memory_rw() won't
> let you make a word size access to an unaligned address.

But the width is transmitted too, so any size accesses can be performed.

>
> -- PMM