[Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Laurent Vivier]

The target endianness is not deduced anymore from
the architecture name but asked directly to the guest,
using a new qtest command: "endianness". As it can't
change (this is the value of TARGET_WORDS_BIGENDIAN),
we store it to not have to ask every time we want to
know if we have to byte-swap a value.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
CC: Greg Kurz <groug@kaod.org>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Peter Maydell <peter.maydell@linaro.org>
---
v2:
- move the "endianness" command to a function and
  don't move the qtest_init()/qtest_quit() functions

 qtest.c                   |  7 +++++
 tests/libqos/virtio-pci.c |  2 +-
 tests/libqtest.c          | 68 ++++++++++++++++-------------------------------
 tests/libqtest.h          | 16 ++++++++---
 tests/virtio-blk-test.c   |  2 +-
 5 files changed, 45 insertions(+), 50 deletions(-)

diff --git a/qtest.c b/qtest.c
index 22482cc..b53b39c 100644
--- a/qtest.c
+++ b/qtest.c
@@ -537,6 +537,13 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
 
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
+    } else if (strcmp(words[0], "endianness") == 0) {
+        qtest_send_prefix(chr);
+#if defined(TARGET_WORDS_BIGENDIAN)
+        qtest_sendf(chr, "OK big\n");
+#else
+        qtest_sendf(chr, "OK little\n");
+#endif
 #ifdef TARGET_PPC64
     } else if (strcmp(words[0], "rtas") == 0) {
         uint64_t res, args, ret;
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 18b92b9..6e005c1 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -86,7 +86,7 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t addr)
     int i;
     uint64_t u64 = 0;
 
-    if (qtest_big_endian()) {
+    if (target_big_endian()) {
         for (i = 0; i < 8; ++i) {
             u64 |= (uint64_t)qpci_io_readb(dev->pdev,
                                 (void *)(uintptr_t)addr + i) << (7 - i) * 8;
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 6f6bdf1..d4e6bff 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -37,6 +37,7 @@ struct QTestState
     bool irq_level[MAX_IRQ];
     GString *rx;
     pid_t qemu_pid;  /* our child QEMU process */
+    bool big_endian;
 };
 
 static GHookList abrt_hooks;
@@ -47,6 +48,8 @@ static struct sigaction sigact_old;
     g_assert_cmpint(ret, !=, -1); \
 } while (0)
 
+static int qtest_query_target_endianness(QTestState *s);
+
 static int init_socket(const char *socket_path)
 {
     struct sockaddr_un addr;
@@ -209,6 +212,10 @@ QTestState *qtest_init(const char *extra_args)
         kill(s->qemu_pid, SIGSTOP);
     }
 
+    /* ask endianness of the target */
+
+    s->big_endian = qtest_query_target_endianness(s);
+
     return s;
 }
 
@@ -342,6 +349,20 @@ redo:
     return words;
 }
 
+static int qtest_query_target_endianness(QTestState *s)
+{
+    gchar **args;
+    int big_endian;
+
+    qtest_sendf(s, "endianness\n");
+    args = qtest_rsp(s, 1);
+    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
+    big_endian = strcmp(args[1], "big") == 0;
+    g_strfreev(args);
+
+    return big_endian;
+}
+
 typedef struct {
     JSONMessageParser parser;
     QDict *response;
@@ -886,50 +907,7 @@ char *hmp(const char *fmt, ...)
     return ret;
 }
 
-bool qtest_big_endian(void)
+bool qtest_big_endian(QTestState *s)
 {
-    const char *arch = qtest_get_arch();
-    int i;
-
-    static const struct {
-        const char *arch;
-        bool big_endian;
-    } endianness[] = {
-        { "aarch64", false },
-        { "alpha", false },
-        { "arm", false },
-        { "cris", false },
-        { "i386", false },
-        { "lm32", true },
-        { "m68k", true },
-        { "microblaze", true },
-        { "microblazeel", false },
-        { "mips", true },
-        { "mips64", true },
-        { "mips64el", false },
-        { "mipsel", false },
-        { "moxie", true },
-        { "or32", true },
-        { "ppc", true },
-        { "ppc64", true },
-        { "ppcemb", true },
-        { "s390x", true },
-        { "sh4", false },
-        { "sh4eb", true },
-        { "sparc", true },
-        { "sparc64", true },
-        { "unicore32", false },
-        { "x86_64", false },
-        { "xtensa", false },
-        { "xtensaeb", true },
-        {},
-    };
-
-    for (i = 0; endianness[i].arch; i++) {
-        if (strcmp(endianness[i].arch, arch) == 0) {
-            return endianness[i].big_endian;
-        }
-    }
-
-    return false;
+    return s->big_endian;
 }
diff --git a/tests/libqtest.h b/tests/libqtest.h
index f7402e0..4be1f77 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -410,6 +410,14 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
 int64_t qtest_clock_set(QTestState *s, int64_t val);
 
 /**
+ * qtest_big_endian:
+ * @s: QTestState instance to operate on.
+ *
+ * Returns: True if the architecture under test has a big endian configuration.
+ */
+bool qtest_big_endian(QTestState *s);
+
+/**
  * qtest_get_arch:
  *
  * Returns: The architecture for the QEMU executable under test.
@@ -874,12 +882,14 @@ static inline int64_t clock_set(int64_t val)
 }
 
 /**
- * qtest_big_endian:
+ * target_big_endian:
  *
  * Returns: True if the architecture under test has a big endian configuration.
  */
-bool qtest_big_endian(void);
-
+static inline bool target_big_endian(void)
+{
+    return qtest_big_endian(global_qtest);
+}
 
 QDict *qmp_fd_receive(int fd);
 void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 3c4fecc..0506917 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -125,7 +125,7 @@ static inline void virtio_blk_fix_request(QVirtioBlkReq *req)
     bool host_endian = false;
 #endif
 
-    if (qtest_big_endian() != host_endian) {
+    if (target_big_endian() != host_endian) {
         req->type = bswap32(req->type);
         req->ioprio = bswap32(req->ioprio);
         req->sector = bswap64(req->sector);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Greg Kurz]

On Fri,  7 Oct 2016 12:14:27 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> The target endianness is not deduced anymore from
> the architecture name but asked directly to the guest,
> using a new qtest command: "endianness". As it can't
> change (this is the value of TARGET_WORDS_BIGENDIAN),
> we store it to not have to ask every time we want to
> know if we have to byte-swap a value.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> CC: Greg Kurz <groug@kaod.org>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Peter Maydell <peter.maydell@linaro.org>
> ---
> v2:
> - move the "endianness" command to a function and
>   don't move the qtest_init()/qtest_quit() functions
> 

Not speaking about the current discussion on TARGET_WORDS_BIGENDIAN,
I guess a consensus could be that this only makes sense when testing
legacy virtio. People should not be tempted to use this anywhere else
actually.

>  qtest.c                   |  7 +++++
>  tests/libqos/virtio-pci.c |  2 +-
>  tests/libqtest.c          | 68 ++++++++++++++++-------------------------------
>  tests/libqtest.h          | 16 ++++++++---
>  tests/virtio-blk-test.c   |  2 +-
>  5 files changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/qtest.c b/qtest.c
> index 22482cc..b53b39c 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -537,6 +537,13 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
>  
>          qtest_send_prefix(chr);
>          qtest_send(chr, "OK\n");
> +    } else if (strcmp(words[0], "endianness") == 0) {
> +        qtest_send_prefix(chr);
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +        qtest_sendf(chr, "OK big\n");
> +#else
> +        qtest_sendf(chr, "OK little\n");
> +#endif
>  #ifdef TARGET_PPC64
>      } else if (strcmp(words[0], "rtas") == 0) {
>          uint64_t res, args, ret;
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 18b92b9..6e005c1 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -86,7 +86,7 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t addr)
>      int i;
>      uint64_t u64 = 0;
>  
> -    if (qtest_big_endian()) {
> +    if (target_big_endian()) {
>          for (i = 0; i < 8; ++i) {
>              u64 |= (uint64_t)qpci_io_readb(dev->pdev,
>                                  (void *)(uintptr_t)addr + i) << (7 - i) * 8;
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 6f6bdf1..d4e6bff 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -37,6 +37,7 @@ struct QTestState
>      bool irq_level[MAX_IRQ];
>      GString *rx;
>      pid_t qemu_pid;  /* our child QEMU process */
> +    bool big_endian;
>  };
>  
>  static GHookList abrt_hooks;
> @@ -47,6 +48,8 @@ static struct sigaction sigact_old;
>      g_assert_cmpint(ret, !=, -1); \
>  } while (0)
>  
> +static int qtest_query_target_endianness(QTestState *s);
> +
>  static int init_socket(const char *socket_path)
>  {
>      struct sockaddr_un addr;
> @@ -209,6 +212,10 @@ QTestState *qtest_init(const char *extra_args)
>          kill(s->qemu_pid, SIGSTOP);
>      }
>  
> +    /* ask endianness of the target */
> +
> +    s->big_endian = qtest_query_target_endianness(s);
> +
>      return s;
>  }
>  
> @@ -342,6 +349,20 @@ redo:
>      return words;
>  }
>  
> +static int qtest_query_target_endianness(QTestState *s)
> +{
> +    gchar **args;
> +    int big_endian;
> +
> +    qtest_sendf(s, "endianness\n");
> +    args = qtest_rsp(s, 1);
> +    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> +    big_endian = strcmp(args[1], "big") == 0;
> +    g_strfreev(args);
> +
> +    return big_endian;
> +}
> +
>  typedef struct {
>      JSONMessageParser parser;
>      QDict *response;
> @@ -886,50 +907,7 @@ char *hmp(const char *fmt, ...)
>      return ret;
>  }
>  
> -bool qtest_big_endian(void)
> +bool qtest_big_endian(QTestState *s)
>  {
> -    const char *arch = qtest_get_arch();
> -    int i;
> -
> -    static const struct {
> -        const char *arch;
> -        bool big_endian;
> -    } endianness[] = {
> -        { "aarch64", false },
> -        { "alpha", false },
> -        { "arm", false },
> -        { "cris", false },
> -        { "i386", false },
> -        { "lm32", true },
> -        { "m68k", true },
> -        { "microblaze", true },
> -        { "microblazeel", false },
> -        { "mips", true },
> -        { "mips64", true },
> -        { "mips64el", false },
> -        { "mipsel", false },
> -        { "moxie", true },
> -        { "or32", true },
> -        { "ppc", true },
> -        { "ppc64", true },
> -        { "ppcemb", true },
> -        { "s390x", true },
> -        { "sh4", false },
> -        { "sh4eb", true },
> -        { "sparc", true },
> -        { "sparc64", true },
> -        { "unicore32", false },
> -        { "x86_64", false },
> -        { "xtensa", false },
> -        { "xtensaeb", true },
> -        {},
> -    };
> -
> -    for (i = 0; endianness[i].arch; i++) {
> -        if (strcmp(endianness[i].arch, arch) == 0) {
> -            return endianness[i].big_endian;
> -        }
> -    }
> -
> -    return false;
> +    return s->big_endian;
>  }
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index f7402e0..4be1f77 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -410,6 +410,14 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
>  int64_t qtest_clock_set(QTestState *s, int64_t val);
>  
>  /**
> + * qtest_big_endian:
> + * @s: QTestState instance to operate on.
> + *
> + * Returns: True if the architecture under test has a big endian configuration.
> + */
> +bool qtest_big_endian(QTestState *s);
> +
> +/**
>   * qtest_get_arch:
>   *
>   * Returns: The architecture for the QEMU executable under test.
> @@ -874,12 +882,14 @@ static inline int64_t clock_set(int64_t val)
>  }
>  
>  /**
> - * qtest_big_endian:
> + * target_big_endian:
>   *
>   * Returns: True if the architecture under test has a big endian configuration.
>   */
> -bool qtest_big_endian(void);
> -
> +static inline bool target_big_endian(void)
> +{
> +    return qtest_big_endian(global_qtest);
> +}
>  
>  QDict *qmp_fd_receive(int fd);
>  void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 3c4fecc..0506917 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -125,7 +125,7 @@ static inline void virtio_blk_fix_request(QVirtioBlkReq *req)
>      bool host_endian = false;
>  #endif
>  
> -    if (qtest_big_endian() != host_endian) {
> +    if (target_big_endian() != host_endian) {
>          req->type = bswap32(req->type);
>          req->ioprio = bswap32(req->ioprio);
>          req->sector = bswap64(req->sector);

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Laurent Vivier]

On 07/10/2016 12:48, Greg Kurz wrote:
> On Fri,  7 Oct 2016 12:14:27 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> The target endianness is not deduced anymore from
>> the architecture name but asked directly to the guest,
>> using a new qtest command: "endianness". As it can't
>> change (this is the value of TARGET_WORDS_BIGENDIAN),
>> we store it to not have to ask every time we want to
>> know if we have to byte-swap a value.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> CC: Greg Kurz <groug@kaod.org>
>> CC: David Gibson <david@gibson.dropbear.id.au>
>> CC: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> v2:
>> - move the "endianness" command to a function and
>>   don't move the qtest_init()/qtest_quit() functions
>>
> 
> Not speaking about the current discussion on TARGET_WORDS_BIGENDIAN,
> I guess a consensus could be that this only makes sense when testing
> legacy virtio. People should not be tempted to use this anywhere else
> actually.

I can rename target_big_endian() into qvirtio_is_big_endian() on the
test side (and put it in libqos/virtio.h).

I'd like to keep the qtest_big_endian() as it returns
TARGET_WORDS_BIGENDIAN, and qvirtio_is_big_endian() will depend also on
virtio-1.0 or not.

Laurent

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Greg Kurz]

On Fri, 7 Oct 2016 12:57:41 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 07/10/2016 12:48, Greg Kurz wrote:
> > On Fri,  7 Oct 2016 12:14:27 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> The target endianness is not deduced anymore from
> >> the architecture name but asked directly to the guest,
> >> using a new qtest command: "endianness". As it can't
> >> change (this is the value of TARGET_WORDS_BIGENDIAN),
> >> we store it to not have to ask every time we want to
> >> know if we have to byte-swap a value.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> CC: Greg Kurz <groug@kaod.org>
> >> CC: David Gibson <david@gibson.dropbear.id.au>
> >> CC: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >> v2:
> >> - move the "endianness" command to a function and
> >>   don't move the qtest_init()/qtest_quit() functions
> >>  
> > 
> > Not speaking about the current discussion on TARGET_WORDS_BIGENDIAN,
> > I guess a consensus could be that this only makes sense when testing
> > legacy virtio. People should not be tempted to use this anywhere else
> > actually.  
> 
> I can rename target_big_endian() into qvirtio_is_big_endian() on the
> test side (and put it in libqos/virtio.h).
> 

Yes.

> I'd like to keep the qtest_big_endian() as it returns
> TARGET_WORDS_BIGENDIAN, and qvirtio_is_big_endian() will depend also on
> virtio-1.0 or not.
> 

Indeed but my suggestion is to open code this in qvirtio_is_big_endian(),
and even rename QTestState::big_endian to virtio_big_endian to make it
really obvious it should not be used elsewhere.

I now remember this is what I was resolutely suggested to do in
include/qom/cpu.h at the time we started to support ppc64le:

    bool (*virtio_is_big_endian)(CPUState *cpu);

Cheers.

--
Greg

> Laurent

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Peter Maydell]

On 7 October 2016 at 13:27, Greg Kurz <groug@kaod.org> wrote:
> Indeed but my suggestion is to open code this in qvirtio_is_big_endian(),
> and even rename QTestState::big_endian to virtio_big_endian to make it
> really obvious it should not be used elsewhere.
>
> I now remember this is what I was resolutely suggested to do in
> include/qom/cpu.h at the time we started to support ppc64le:
>
>     bool (*virtio_is_big_endian)(CPUState *cpu);

Not really the same thing though -- virtio_is_big_endian
in QEMU is indeed used only in virtio, because it makes
dubious use of the internals of the CPU state. The
equivalent of this proposed qtest function is the #define
TARGET_BIG_ENDIAN, which is global to all of QEMU and
reasonably widely used (because it's not a property of
the CPU's internals).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Greg Kurz]

On Fri, 7 Oct 2016 13:31:10 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 7 October 2016 at 13:27, Greg Kurz <groug@kaod.org> wrote:
> > Indeed but my suggestion is to open code this in qvirtio_is_big_endian(),
> > and even rename QTestState::big_endian to virtio_big_endian to make it
> > really obvious it should not be used elsewhere.
> >
> > I now remember this is what I was resolutely suggested to do in
> > include/qom/cpu.h at the time we started to support ppc64le:
> >
> >     bool (*virtio_is_big_endian)(CPUState *cpu);  
> 
> Not really the same thing though -- virtio_is_big_endian
> in QEMU is indeed used only in virtio, because it makes
> dubious use of the internals of the CPU state. The
> equivalent of this proposed qtest function is the #define
> TARGET_BIG_ENDIAN, which is global to all of QEMU and
> reasonably widely used (because it's not a property of
> the CPU's internals).
> 

Indeed but is it expected to be used in other tests than
virtio ?

$ git grep qtest_big_endian tests/
tests/libqos/virtio-pci.c:    if (qtest_big_endian()) {
tests/libqtest.c:bool qtest_big_endian(void)
tests/libqtest.h: * qtest_big_endian:
tests/libqtest.h:bool qtest_big_endian(void);
tests/virtio-blk-test.c:    if (qtest_big_endian() != host_endian) {

> thanks
> -- PMM

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Peter Maydell]

On 7 October 2016 at 13:45, Greg Kurz <groug@kaod.org> wrote:
> On Fri, 7 Oct 2016 13:31:10 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 7 October 2016 at 13:27, Greg Kurz <groug@kaod.org> wrote:
>> > Indeed but my suggestion is to open code this in qvirtio_is_big_endian(),
>> > and even rename QTestState::big_endian to virtio_big_endian to make it
>> > really obvious it should not be used elsewhere.
>> >
>> > I now remember this is what I was resolutely suggested to do in
>> > include/qom/cpu.h at the time we started to support ppc64le:
>> >
>> >     bool (*virtio_is_big_endian)(CPUState *cpu);
>>
>> Not really the same thing though -- virtio_is_big_endian
>> in QEMU is indeed used only in virtio, because it makes
>> dubious use of the internals of the CPU state. The
>> equivalent of this proposed qtest function is the #define
>> TARGET_BIG_ENDIAN, which is global to all of QEMU and
>> reasonably widely used (because it's not a property of
>> the CPU's internals).
>>
>
> Indeed but is it expected to be used in other tests than
> virtio ?

Well, that's where we came in.

Personally I'd rather see this patch purely fix the current
rather dodgy implementation of the existing qtest_big_endian()
function, which seems to be non-controversial, rather than
getting bogged down too much in the questions about what the
function name should be and how widely it should be used, etc.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Laurent Vivier]

On 07/10/2016 14:52, Peter Maydell wrote:
> On 7 October 2016 at 13:45, Greg Kurz <groug@kaod.org> wrote:
>> On Fri, 7 Oct 2016 13:31:10 +0100
>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>> On 7 October 2016 at 13:27, Greg Kurz <groug@kaod.org> wrote:
>>>> Indeed but my suggestion is to open code this in qvirtio_is_big_endian(),
>>>> and even rename QTestState::big_endian to virtio_big_endian to make it
>>>> really obvious it should not be used elsewhere.
>>>>
>>>> I now remember this is what I was resolutely suggested to do in
>>>> include/qom/cpu.h at the time we started to support ppc64le:
>>>>
>>>>     bool (*virtio_is_big_endian)(CPUState *cpu);
>>>
>>> Not really the same thing though -- virtio_is_big_endian
>>> in QEMU is indeed used only in virtio, because it makes
>>> dubious use of the internals of the CPU state. The
>>> equivalent of this proposed qtest function is the #define
>>> TARGET_BIG_ENDIAN, which is global to all of QEMU and
>>> reasonably widely used (because it's not a property of
>>> the CPU's internals).
>>>
>>
>> Indeed but is it expected to be used in other tests than
>> virtio ?
> 
> Well, that's where we came in.
> 
> Personally I'd rather see this patch purely fix the current
> rather dodgy implementation of the existing qtest_big_endian()
> function, which seems to be non-controversial, rather than
> getting bogged down too much in the questions about what the
> function name should be and how widely it should be used, etc.

I'd rather too..

And I can rework this part later, as I've a series to enable virtio
tests for SPAPR.

So if v2 covers all non virtio naming space issues, is it acceptable as-is?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Greg Kurz]

On Fri, 7 Oct 2016 14:56:26 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 07/10/2016 14:52, Peter Maydell wrote:
> > On 7 October 2016 at 13:45, Greg Kurz <groug@kaod.org> wrote:  
> >> On Fri, 7 Oct 2016 13:31:10 +0100
> >> Peter Maydell <peter.maydell@linaro.org> wrote:
> >>  
> >>> On 7 October 2016 at 13:27, Greg Kurz <groug@kaod.org> wrote:  
> >>>> Indeed but my suggestion is to open code this in qvirtio_is_big_endian(),
> >>>> and even rename QTestState::big_endian to virtio_big_endian to make it
> >>>> really obvious it should not be used elsewhere.
> >>>>
> >>>> I now remember this is what I was resolutely suggested to do in
> >>>> include/qom/cpu.h at the time we started to support ppc64le:
> >>>>
> >>>>     bool (*virtio_is_big_endian)(CPUState *cpu);  
> >>>
> >>> Not really the same thing though -- virtio_is_big_endian
> >>> in QEMU is indeed used only in virtio, because it makes
> >>> dubious use of the internals of the CPU state. The
> >>> equivalent of this proposed qtest function is the #define
> >>> TARGET_BIG_ENDIAN, which is global to all of QEMU and
> >>> reasonably widely used (because it's not a property of
> >>> the CPU's internals).
> >>>  
> >>
> >> Indeed but is it expected to be used in other tests than
> >> virtio ?  
> > 
> > Well, that's where we came in.
> > 
> > Personally I'd rather see this patch purely fix the current
> > rather dodgy implementation of the existing qtest_big_endian()
> > function, which seems to be non-controversial, rather than
> > getting bogged down too much in the questions about what the
> > function name should be and how widely it should be used, etc.  
> 
> I'd rather too..
> 

Fair enough.

> And I can rework this part later, as I've a series to enable virtio
> tests for SPAPR.
> 
> So if v2 covers all non virtio naming space issues, is it acceptable as-is?
> 

What I said with v1 still stands: this is an improvement over what we currently
have.

Reviewed-by: Greg Kurz <groug@kaod.org>

> Thanks,
> Laurent

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7149 bytes --]

On Fri, Oct 07, 2016 at 12:14:27PM +0200, Laurent Vivier wrote:
> The target endianness is not deduced anymore from
> the architecture name but asked directly to the guest,
> using a new qtest command: "endianness". As it can't
> change (this is the value of TARGET_WORDS_BIGENDIAN),
> we store it to not have to ask every time we want to
> know if we have to byte-swap a value.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> CC: Greg Kurz <groug@kaod.org>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

As with Greg, I think this is an improvement over the current
situation, even though I still have concerns about other things
related to this.

> ---
> v2:
> - move the "endianness" command to a function and
>   don't move the qtest_init()/qtest_quit() functions
> 
>  qtest.c                   |  7 +++++
>  tests/libqos/virtio-pci.c |  2 +-
>  tests/libqtest.c          | 68 ++++++++++++++++-------------------------------
>  tests/libqtest.h          | 16 ++++++++---
>  tests/virtio-blk-test.c   |  2 +-
>  5 files changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/qtest.c b/qtest.c
> index 22482cc..b53b39c 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -537,6 +537,13 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
>  
>          qtest_send_prefix(chr);
>          qtest_send(chr, "OK\n");
> +    } else if (strcmp(words[0], "endianness") == 0) {
> +        qtest_send_prefix(chr);
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +        qtest_sendf(chr, "OK big\n");
> +#else
> +        qtest_sendf(chr, "OK little\n");
> +#endif
>  #ifdef TARGET_PPC64
>      } else if (strcmp(words[0], "rtas") == 0) {
>          uint64_t res, args, ret;
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 18b92b9..6e005c1 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -86,7 +86,7 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t addr)
>      int i;
>      uint64_t u64 = 0;
>  
> -    if (qtest_big_endian()) {
> +    if (target_big_endian()) {
>          for (i = 0; i < 8; ++i) {
>              u64 |= (uint64_t)qpci_io_readb(dev->pdev,
>                                  (void *)(uintptr_t)addr + i) << (7 - i) * 8;
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 6f6bdf1..d4e6bff 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -37,6 +37,7 @@ struct QTestState
>      bool irq_level[MAX_IRQ];
>      GString *rx;
>      pid_t qemu_pid;  /* our child QEMU process */
> +    bool big_endian;
>  };
>  
>  static GHookList abrt_hooks;
> @@ -47,6 +48,8 @@ static struct sigaction sigact_old;
>      g_assert_cmpint(ret, !=, -1); \
>  } while (0)
>  
> +static int qtest_query_target_endianness(QTestState *s);
> +
>  static int init_socket(const char *socket_path)
>  {
>      struct sockaddr_un addr;
> @@ -209,6 +212,10 @@ QTestState *qtest_init(const char *extra_args)
>          kill(s->qemu_pid, SIGSTOP);
>      }
>  
> +    /* ask endianness of the target */
> +
> +    s->big_endian = qtest_query_target_endianness(s);
> +
>      return s;
>  }
>  
> @@ -342,6 +349,20 @@ redo:
>      return words;
>  }
>  
> +static int qtest_query_target_endianness(QTestState *s)
> +{
> +    gchar **args;
> +    int big_endian;
> +
> +    qtest_sendf(s, "endianness\n");
> +    args = qtest_rsp(s, 1);
> +    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> +    big_endian = strcmp(args[1], "big") == 0;
> +    g_strfreev(args);
> +
> +    return big_endian;
> +}
> +
>  typedef struct {
>      JSONMessageParser parser;
>      QDict *response;
> @@ -886,50 +907,7 @@ char *hmp(const char *fmt, ...)
>      return ret;
>  }
>  
> -bool qtest_big_endian(void)
> +bool qtest_big_endian(QTestState *s)
>  {
> -    const char *arch = qtest_get_arch();
> -    int i;
> -
> -    static const struct {
> -        const char *arch;
> -        bool big_endian;
> -    } endianness[] = {
> -        { "aarch64", false },
> -        { "alpha", false },
> -        { "arm", false },
> -        { "cris", false },
> -        { "i386", false },
> -        { "lm32", true },
> -        { "m68k", true },
> -        { "microblaze", true },
> -        { "microblazeel", false },
> -        { "mips", true },
> -        { "mips64", true },
> -        { "mips64el", false },
> -        { "mipsel", false },
> -        { "moxie", true },
> -        { "or32", true },
> -        { "ppc", true },
> -        { "ppc64", true },
> -        { "ppcemb", true },
> -        { "s390x", true },
> -        { "sh4", false },
> -        { "sh4eb", true },
> -        { "sparc", true },
> -        { "sparc64", true },
> -        { "unicore32", false },
> -        { "x86_64", false },
> -        { "xtensa", false },
> -        { "xtensaeb", true },
> -        {},
> -    };
> -
> -    for (i = 0; endianness[i].arch; i++) {
> -        if (strcmp(endianness[i].arch, arch) == 0) {
> -            return endianness[i].big_endian;
> -        }
> -    }
> -
> -    return false;
> +    return s->big_endian;
>  }
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index f7402e0..4be1f77 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -410,6 +410,14 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
>  int64_t qtest_clock_set(QTestState *s, int64_t val);
>  
>  /**
> + * qtest_big_endian:
> + * @s: QTestState instance to operate on.
> + *
> + * Returns: True if the architecture under test has a big endian configuration.
> + */
> +bool qtest_big_endian(QTestState *s);
> +
> +/**
>   * qtest_get_arch:
>   *
>   * Returns: The architecture for the QEMU executable under test.
> @@ -874,12 +882,14 @@ static inline int64_t clock_set(int64_t val)
>  }
>  
>  /**
> - * qtest_big_endian:
> + * target_big_endian:
>   *
>   * Returns: True if the architecture under test has a big endian configuration.
>   */
> -bool qtest_big_endian(void);
> -
> +static inline bool target_big_endian(void)
> +{
> +    return qtest_big_endian(global_qtest);
> +}
>  
>  QDict *qmp_fd_receive(int fd);
>  void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 3c4fecc..0506917 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -125,7 +125,7 @@ static inline void virtio_blk_fix_request(QVirtioBlkReq *req)
>      bool host_endian = false;
>  #endif
>  
> -    if (qtest_big_endian() != host_endian) {
> +    if (target_big_endian() != host_endian) {
>          req->type = bswap32(req->type);
>          req->ioprio = bswap32(req->ioprio);
>          req->sector = bswap64(req->sector);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

On Fri, Oct 07, 2016 at 12:48:49PM +0200, Greg Kurz wrote:
> On Fri,  7 Oct 2016 12:14:27 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > The target endianness is not deduced anymore from
> > the architecture name but asked directly to the guest,
> > using a new qtest command: "endianness". As it can't
> > change (this is the value of TARGET_WORDS_BIGENDIAN),
> > we store it to not have to ask every time we want to
> > know if we have to byte-swap a value.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > CC: Greg Kurz <groug@kaod.org>
> > CC: David Gibson <david@gibson.dropbear.id.au>
> > CC: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > v2:
> > - move the "endianness" command to a function and
> >   don't move the qtest_init()/qtest_quit() functions
> > 
> 
> Not speaking about the current discussion on TARGET_WORDS_BIGENDIAN,
> I guess a consensus could be that this only makes sense when testing
> legacy virtio. People should not be tempted to use this anywhere else
> actually.

I agree in principle.

However right now - until we get fixed-endianness qtest accessors of
some sort - this test is more or less necessary to make most tests
cross platform correct (except PCI, see below).  That's because the
testcase will generally know the endianness of the hardware it's
testing, but has to compensate for the endianness that the
readw/writew primitives will use along the way.

For PCI we side steo this, because the PCI read/write wrappers are
already target specific, and include unconditional swaps to
compoensate for the "guest endianness".

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Peter Maydell]

On 10 October 2016 at 05:55, David Gibson <david@gibson.dropbear.id.au> wrote:
> However right now - until we get fixed-endianness qtest accessors of
> some sort - this test is more or less necessary to make most tests
> cross platform correct (except PCI, see below).  That's because the
> testcase will generally know the endianness of the hardware it's
> testing, but has to compensate for the endianness that the
> readw/writew primitives will use along the way.

The test case isn't testing the hardware (device), though, it's
testing the hardware in a system. The same device in QEMU board A
could be behind a bridge or otherwise differently wired by
the SoC from the device in QEMU board B. The qtest setup
doesn't instantiate a device and talk directly to it, it
instantiates a board and talks over the same interface the
emulated CPU would.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

On Mon, Oct 10, 2016 at 10:18:40AM +0100, Peter Maydell wrote:
> On 10 October 2016 at 05:55, David Gibson <david@gibson.dropbear.id.au> wrote:
> > However right now - until we get fixed-endianness qtest accessors of
> > some sort - this test is more or less necessary to make most tests
> > cross platform correct (except PCI, see below).  That's because the
> > testcase will generally know the endianness of the hardware it's
> > testing, but has to compensate for the endianness that the
> > readw/writew primitives will use along the way.
> 
> The test case isn't testing the hardware (device), though, it's
> testing the hardware in a system. The same device in QEMU board A
> could be behind a bridge or otherwise differently wired by
> the SoC from the device in QEMU board B. The qtest setup
> doesn't instantiate a device and talk directly to it, it
> instantiates a board and talks over the same interface the
> emulated CPU would.

If your bridges change the effective endianness of your device, then
your bridge hardware design is broken.  Such boards exist but they
are, thankfully, rare.

In the overwhelming majority of cases the endianness of the device is
known independent of the guest CPU and board.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Peter Maydell]

On 10 October 2016 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> In the overwhelming majority of cases the endianness of the device is
> known independent of the guest CPU and board.

We don't seem to model things that way:

$ git grep '.endianness = DEVICE_NATIVE' |wc -l
341
$ git grep '.endianness = DEVICE_BIG' |wc -l
18
$ git grep '.endianness = DEVICE_LITTLE' |wc -l
172

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5374 bytes --]

On Mon, Oct 10, 2016 at 03:10:33PM +0100, Peter Maydell wrote:
> On 10 October 2016 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> > In the overwhelming majority of cases the endianness of the device is
> > known independent of the guest CPU and board.
> 
> We don't seem to model things that way:
> 
> $ git grep '.endianness = DEVICE_NATIVE' |wc -l
> 341
> $ git grep '.endianness = DEVICE_BIG' |wc -l
> 18
> $ git grep '.endianness = DEVICE_LITTLE' |wc -l
> 172

Sigh.  So, most of these are themselves mistakes.

A lot of these are target specific devices that should be tagged with
their target's (notional) endianness, rather than NATIVE.  That
covers:

ARM
	hw/arm						 68
	hw/*/bcm283*					  8
	hw/*/imx*					 13
	hw/*/omap*					 27
	hw/*/pl*					 10
	hw/*/exynos*					  8
	hw/net/lan9118.c				  2
	hw/*/pxa2xx*					  8
	hw/gpio/zaurus.c				  1
	hw/*/versatile*					  3
	hw/*/allwinner*					  3
	hw/*/arm*					 15
	hw/*/a9*					  3
	hw/char/cadence*				  2
	hw/*/digic*					  2
	hw/*/stm32f2xx*					  5
	hw/usb/tusb6010.c				  1
	hw/misc/mst_fpga.c				  1
	hw/net/smc91c111.c				  1
	hw/net/stellaris_enet.c				  1
	hw/misc/zynq*					  2
	hw/dma/xlnx_dpdma.c				  1
	hw/display/xlnx_dp.c				  4
	hw/ssi/xilinx_spips.c				  1
	hw/display/tc6393xb.c				  1
	hw/audio/marvell_88w8618.c			  1
	hw/block/onenand.c				  1
							---
		TOTAL					193					

CRIS
	hw/cris						  2
	hw/*/etraxfs*					  3
							---
		TOTAL					  5

LM32
	hw/*/milkymist*					 10
	hw/*/lm32*					  2
							---
		TOTAL					 12							 

M68K
	hw/m68k						  4
	hw/*/mcf*					  2
							---
		TOTAL					  6

MICROBLAZE
	hw/dma/xilinx_axidma.c				  1
	hw/char/xilinx_uartlite.c			  1
	hw/intc/xilinx_intc.c				  1
	hw/net/xilinx_ethlite.c				  1
	hw/timer/xilinx_timer.c				  1
	hw/ssi/xilinx_spi.c				  1
							---
		TOTAL					  6

MIPS
	hw/mips						  8
	hw/*/mips*					  5
	hw/display/jazz_led.c				  1
	hw/dma/rc4030.c					  2
	hw/net/dp8393x.c				  1
	hw/pci-host/bonito.c				  5
							---
		TOTAL					 22

OPENRISC
	hw/openrisc					  1

PPC
	hw/ppc						  7
	hw/char/escc.c					  1
	hw/ide/macio.c					  1
	hw/misc/macio/cuda.c				  1
	hw/misc/applesmc.c				  2
	hw/net/fsl_etsec/etsec.c			  1
							---
		TOTAL					 13

SH
	hw/sh4						  4
	hw/*/sh_*					  3
	hw/display/sm501.c				  4
							---
		TOTAL					 11

SPARC
	hw/*/grlib*					  3
	hw/*/slavio*					 11
	hw/dma/sparc32_dma.c				  1
	hw/dma/sun4m_iommu.c				  1
	hw/display/tcx.c				  7
	hw/pci-host/apb.c				  2
	hw/misc/eccmemctl.c				  2
	hw/display/cg3.c				  1
	hw/audio/cs4231.c				  1
							---
		TOTAL					 29

UNICORE32
	hw/*/puv3*					  5

X86
	hw/i386						  9
	hw/pci-host/q35.c				  1
	hw/timer/hpet.c					  1
							---
		TOTAL					 11
	
XTENSA
	hw/xtensa					  3


							===
			OVERALL TOTAL			317

There are some PCI devices, where AFAICT, the NATIVE_ENDIAN tag is
Just Plain Wrong (i.e. I believe these devices will be broken on BE
targets):
	hw/scsi/lsi53c895a.c				 3
	hw/ipack/tpci200.c				 5
	hw/misc/edu.c					 1
	hw/audio/intel-hda.c				 1
	hw/pci/pcie_host.c				 1

And there are some devices which only accept 1 byte accesses, so the
endianness is irrelevant anyway:
	hw/block/fdc.c					 2
	hw/dma/i8257.c					 2
	hw/isa/vt82c686.c				 1

There are ISA devices.  I'd expect these to be always-LE, but I'm not
sure exactly how they're handled.  In wonder if they've been tested on
BE.
	hw/char/parallel.c				 1
	hw/input/pckbd.c				 1
	hw/display/vga-isa-mm.c				 1

Then there are some which are part of the MR infrastructure, rather
than a device itself:
	exec.c						 8
	ioport.c					 1
	memory.c					 1
	include/exec/cpu-common.h			 1
	hw/core/empty_slot.c				 1


What does that leave..

	hw/block/pflash_cfi01.c				1
	hw/block/pflash_cfi02.c				2

These appear to have its own internal endianness handling

	hw/char/serial.c				2
	hw/timer/m48t59.c				1

These are "raw" MMIO devices which are common enough that they do
exist in the wild with weird wiring variations.  These were always
going to require some special casing.

	hw/intc/apic.c					1
	hw/intc/ioapic.c				1

All the platforms which have APIC (x86 & arm) are notionally LE.  I
imagine it would break if you tried to attach it to a BE platform.

	hw/net/lance.c					1

This is the backend of the old PC-Net driver.  I suspect the "wrapper"
layers by which it's usually used may fix the endianness.  Or possibly
it's just broken on BE.

	hw/scsi/esp.c					1

The platforms it appears on (MIPS Jazz & sun4m) are both notionally
BE. I imagine it would break if you tried to attach it to an LE
platform.

And finally
	hw/misc/ivshmem.c				1
	hw/virtio/virtio-mmio.c				1
	hw/xen/xen_pt*					2

virtio has been discussed before.  The others are also purely virtual
devices, which I suspect have made the same design error as old
virtio.  However I don't think any BE platforms use ivshmem or xen in
practice, so they could be considered always-LE.


So, yeah, I still think "overwhelming majority" is accurate.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On Tue, Oct 11, 2016 at 12:24:29PM +1100, David Gibson wrote:
> On Mon, Oct 10, 2016 at 03:10:33PM +0100, Peter Maydell wrote:
> > On 10 October 2016 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > In the overwhelming majority of cases the endianness of the device is
> > > known independent of the guest CPU and board.
> > 
> > We don't seem to model things that way:
> > 
> > $ git grep '.endianness = DEVICE_NATIVE' |wc -l
> > 341
> > $ git grep '.endianness = DEVICE_BIG' |wc -l
> > 18
> > $ git grep '.endianness = DEVICE_LITTLE' |wc -l
> > 172
> 
> Sigh.  So, most of these are themselves mistakes.
> 
> A lot of these are target specific devices that should be tagged with
> their target's (notional) endianness, rather than NATIVE.  That
> covers:

A couple of points of clarification here.

When I say the "device" endianness is known, that's a little sloppy.
I mean that the endianness of each specific register (or DMA field)
has a known endianness.  The device as a whole does not have well
defined endianness except insofar as one device will generally use the
same endianness for all its >8bit registers and/or DMA fields.

There are a few exceptions of course.  virtio-balloon (pre 1.0) on a
notionally BE platform is a horrible example: its PCI config space is
LE, the generic part of the virtio config space is BE ("native") and
the balloon specific part is LE again (due to a screwup in the spec).

Intermediate bridges in the system (on or off chip) won't affect this,
*as long as* they preserve byte address invariance.  If they *don't*
preserve byte address invariance, then:
	1) Shoot your HW designer
	2) Grudgingly work around with special cases in your tests


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[Peter Maydell]

On 11 October 2016 at 02:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Oct 10, 2016 at 03:10:33PM +0100, Peter Maydell wrote:
>> On 10 October 2016 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
>> > In the overwhelming majority of cases the endianness of the device is
>> > known independent of the guest CPU and board.
>>
>> We don't seem to model things that way:
>>
>> $ git grep '.endianness = DEVICE_NATIVE' |wc -l
>> 341
>> $ git grep '.endianness = DEVICE_BIG' |wc -l
>> 18
>> $ git grep '.endianness = DEVICE_LITTLE' |wc -l
>> 172
>
> Sigh.  So, most of these are themselves mistakes.
>
> A lot of these are target specific devices that should be tagged with
> their target's (notional) endianness, rather than NATIVE.  That
> covers:

> MICROBLAZE
>         hw/dma/xilinx_axidma.c                            1
>         hw/char/xilinx_uartlite.c                         1
>         hw/intc/xilinx_intc.c                             1
>         hw/net/xilinx_ethlite.c                           1
>         hw/timer/xilinx_timer.c                           1
>         hw/ssi/xilinx_spi.c                               1
>                                                         ---
>                 TOTAL                                     6

We build microblaze in both big and little endian configs...

> MIPS
>         hw/mips                                           8
>         hw/*/mips*                                        5
>         hw/display/jazz_led.c                             1
>         hw/dma/rc4030.c                                   2
>         hw/net/dp8393x.c                                  1
>         hw/pci-host/bonito.c                              5
>                                                         ---
>                 TOTAL                                    22

Ditto MIPS.

> SH
>         hw/sh4                                            4
>         hw/*/sh_*                                         3
>         hw/display/sm501.c                                4
>                                                         ---
>                 TOTAL                                    11

And SH.

> XTENSA
>         hw/xtensa                                         3

And Xtensa.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] qtest: ask endianness of the target in qtest_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]

On Tue, Oct 11, 2016 at 09:55:54AM +0100, Peter Maydell wrote:
> On 11 October 2016 at 02:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Mon, Oct 10, 2016 at 03:10:33PM +0100, Peter Maydell wrote:
> >> On 10 October 2016 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > In the overwhelming majority of cases the endianness of the device is
> >> > known independent of the guest CPU and board.
> >>
> >> We don't seem to model things that way:
> >>
> >> $ git grep '.endianness = DEVICE_NATIVE' |wc -l
> >> 341
> >> $ git grep '.endianness = DEVICE_BIG' |wc -l
> >> 18
> >> $ git grep '.endianness = DEVICE_LITTLE' |wc -l
> >> 172
> >
> > Sigh.  So, most of these are themselves mistakes.
> >
> > A lot of these are target specific devices that should be tagged with
> > their target's (notional) endianness, rather than NATIVE.  That
> > covers:
> 
> > MICROBLAZE
> >         hw/dma/xilinx_axidma.c                            1
> >         hw/char/xilinx_uartlite.c                         1
> >         hw/intc/xilinx_intc.c                             1
> >         hw/net/xilinx_ethlite.c                           1
> >         hw/timer/xilinx_timer.c                           1
> >         hw/ssi/xilinx_spi.c                               1
> >                                                         ---
> >                 TOTAL                                     6
> 
> We build microblaze in both big and little endian configs...

And the on-board devices really have different endianness in those
cases?  Yuck.  I guess it's kind of plausible here since microblaze is
a kind of weird arch built around FPGA devi

> 
> > MIPS
> >         hw/mips                                           8
> >         hw/*/mips*                                        5
> >         hw/display/jazz_led.c                             1
> >         hw/dma/rc4030.c                                   2
> >         hw/net/dp8393x.c                                  1
> >         hw/pci-host/bonito.c                              5
> >                                                         ---
> >                 TOTAL                                    22
> 
> Ditto MIPS.

Ok, some of those devices (e.g. bonito and the other fulong bits) are
only for specific machines types which are BE or LE, not both.

> > SH
> >         hw/sh4                                            4
> >         hw/*/sh_*                                         3
> >         hw/display/sm501.c                                4
> >                                                         ---
> >                 TOTAL                                    11
> 
> And SH.

Hrm.. the kernel driver appears to treat sm501 on SH as LE always.
Has it actually been tested with sh4eb?

> > XTENSA
> >         hw/xtensa                                         3
> 
> And Xtensa.

Ok, so there are a few more "native endian" devices out there than I
thought.  For the FPGA-centric platforms like Microblaze, I guess that
means you can compile the peripherals, like the core, as LE or BE.
Kind of pointless, but whatever.  For the rest (assuming the real
hardware really truly exists in both endian variants) that almost
certainly means some of the boards have bridges which don't preserve
byte-order invariance.  That is horrifically broken.

Even so, it's still a small fraction of all devices.  We should be
designing our interfaces around the modern sane case, and having extra
workarounds for horrifically broken bridges, not the other way around.

I'm happy enough to retain "native endian" readw/writew operations in
qtest for such devices.  I just want there to be fixed-endian variants
*as well* - and I'm pretty sure those will be the predominantly useful
ones for any vaguely modern device.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]