Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] qtest: evaluate endianness of the target in qtest_init()

[David Gibson]

On Thu, Sep 29, 2016 at 07:15:06PM +0200, Laurent Vivier wrote:
> This allows to store it and not have to rescan the list
> each time we need it.
> 
> Signed-off-by: Laurent Vivier <address@hidden>
> Reviewed-by: Greg Kurz <address@hidden>

Reviewed-by: David Gibson <address@hidden>

In that it looks technically correct.  The whole notion of guest
endianness is kind of bogus, doubly so when we're essentially
replacing the cpu with the qtest proxy.  It'd be better to make all
the io operations have explicit endianness.

Still, it looks reasonable in the short term.

> ---
>  tests/libqos/virtio-pci.c |  2 +-
>  tests/libqtest.c          | 96 
> +++++++++++++++++++++++++----------------------
>  tests/libqtest.h          | 16 ++++++--
>  tests/virtio-blk-test.c   |  2 +-
>  4 files changed, 66 insertions(+), 50 deletions(-)
> 
> 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..aa4bc9e 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;
> @@ -146,6 +147,52 @@ void qtest_add_abrt_handler(GHookFunc fn, const void 
> *data)
>      g_hook_prepend(&abrt_hooks, hook);
>  }
>  
> +static bool arch_is_big_endian(const char *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 },
> +        { "tricore", false },
> +        {},
> +    };
> +
> +    for (i = 0; endianness[i].arch; i++) {
> +        if (strcmp(endianness[i].arch, arch) == 0) {
> +            return endianness[i].big_endian;
> +        }
> +    }
> +    g_assert_not_reached();
> +}
> +
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s;
> @@ -209,6 +256,8 @@ QTestState *qtest_init(const char *extra_args)
>          kill(s->qemu_pid, SIGSTOP);
>      }
>  
> +    s->big_endian = arch_is_big_endian(qtest_get_arch());
> +
>      return s;
>  }
>  
> @@ -886,50 +935,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 8cf62f6..a4de3e4 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -120,7 +120,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

signature.asc
Description: PGP signature