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

[David Gibson]

On Thu, Oct 06, 2016 at 10:46:22PM +0200, Greg Kurz wrote:
> On Thu,  6 Oct 2016 20:56:58 +0200
> Laurent Vivier <address@hidden> 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 <address@hidden>
> > CC: Greg Kurz <address@hidden>
> > CC: David Gibson <address@hidden>
> > CC: Peter Maydell <address@hidden>
> > ---
> > Note: this patch can be seen as a v2 of
> > "qtest: evaluate endianness of the target in qtest_init()"
> > from the patch series "tests: enable virtio tests on SPAPR"
> > in which I have added the idea from Peter to ask the endianness
> > directly to the guest using a new qtest command.
> > 
> 
> This is definitely an improvement indeed.
> 
> Reviewed-by: Greg Kurz <address@hidden>

It is an improvement.  But I still think if we're relying on the
ill-defined "target endianness" we're already doing something wrong.

> 
> >  qtest.c                   |   7 ++
> >  tests/libqos/virtio-pci.c |   2 +-
> >  tests/libqtest.c          | 224 
> > ++++++++++++++++++++--------------------------
> >  tests/libqtest.h          |  16 +++-
> >  tests/virtio-blk-test.c   |   2 +-
> >  5 files changed, 118 insertions(+), 133 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..27cf6b1 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,89 +147,6 @@ void qtest_add_abrt_handler(GHookFunc fn, const void 
> > *data)
> >      g_hook_prepend(&abrt_hooks, hook);
> >  }
> >  
> > -QTestState *qtest_init(const char *extra_args)
> > -{
> > -    QTestState *s;
> > -    int sock, qmpsock, i;
> > -    gchar *socket_path;
> > -    gchar *qmp_socket_path;
> > -    gchar *command;
> > -    const char *qemu_binary;
> > -
> > -    qemu_binary = getenv("QTEST_QEMU_BINARY");
> > -    g_assert(qemu_binary != NULL);
> > -
> > -    s = g_malloc(sizeof(*s));
> > -
> > -    socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> > -    qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> > -
> > -    sock = init_socket(socket_path);
> > -    qmpsock = init_socket(qmp_socket_path);
> > -
> > -    qtest_add_abrt_handler(kill_qemu_hook_func, s);
> > -
> > -    s->qemu_pid = fork();
> > -    if (s->qemu_pid == 0) {
> > -        setenv("QEMU_AUDIO_DRV", "none", true);
> > -        command = g_strdup_printf("exec %s "
> > -                                  "-qtest unix:%s,nowait "
> > -                                  "-qtest-log %s "
> > -                                  "-qmp unix:%s,nowait "
> > -                                  "-machine accel=qtest "
> > -                                  "-display none "
> > -                                  "%s", qemu_binary, socket_path,
> > -                                  getenv("QTEST_LOG") ? "/dev/fd/2" : 
> > "/dev/null",
> > -                                  qmp_socket_path,
> > -                                  extra_args ?: "");
> > -        execlp("/bin/sh", "sh", "-c", command, NULL);
> > -        exit(1);
> > -    }
> > -
> > -    s->fd = socket_accept(sock);
> > -    if (s->fd >= 0) {
> > -        s->qmp_fd = socket_accept(qmpsock);
> > -    }
> > -    unlink(socket_path);
> > -    unlink(qmp_socket_path);
> > -    g_free(socket_path);
> > -    g_free(qmp_socket_path);
> > -
> > -    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> > -
> > -    s->rx = g_string_new("");
> > -    for (i = 0; i < MAX_IRQ; i++) {
> > -        s->irq_level[i] = false;
> > -    }
> > -
> > -    /* Read the QMP greeting and then do the handshake */
> > -    qtest_qmp_discard_response(s, "");
> > -    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> > -
> > -    if (getenv("QTEST_STOP")) {
> > -        kill(s->qemu_pid, SIGSTOP);
> > -    }
> > -
> > -    return s;
> > -}
> > -
> > -void qtest_quit(QTestState *s)
> > -{
> > -    qtest_instances = g_list_remove(qtest_instances, s);
> > -    g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, 
> > s));
> > -
> > -    /* Uninstall SIGABRT handler on last instance */
> > -    if (!qtest_instances) {
> > -        cleanup_sigabrt_handler();
> > -    }
> > -
> > -    kill_qemu(s);
> > -    close(s->fd);
> > -    close(s->qmp_fd);
> > -    g_string_free(s->rx, true);
> > -    g_free(s);
> > -}
> > -
> >  static void socket_send(int fd, const char *buf, size_t size)
> >  {
> >      size_t offset;
> > @@ -347,6 +265,99 @@ typedef struct {
> >      QDict *response;
> >  } QMPResponseParser;
> >  
> > +QTestState *qtest_init(const char *extra_args)
> > +{
> > +    QTestState *s;
> > +    int sock, qmpsock, i;
> > +    gchar *socket_path;
> > +    gchar *qmp_socket_path;
> > +    gchar *command;
> > +    const char *qemu_binary;
> > +    gchar **args;
> > +
> > +    qemu_binary = getenv("QTEST_QEMU_BINARY");
> > +    g_assert(qemu_binary != NULL);
> > +
> > +    s = g_malloc(sizeof(*s));
> > +
> > +    socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> > +    qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> > +
> > +    sock = init_socket(socket_path);
> > +    qmpsock = init_socket(qmp_socket_path);
> > +
> > +    qtest_add_abrt_handler(kill_qemu_hook_func, s);
> > +
> > +    s->qemu_pid = fork();
> > +    if (s->qemu_pid == 0) {
> > +        setenv("QEMU_AUDIO_DRV", "none", true);
> > +        command = g_strdup_printf("exec %s "
> > +                                  "-qtest unix:%s,nowait "
> > +                                  "-qtest-log %s "
> > +                                  "-qmp unix:%s,nowait "
> > +                                  "-machine accel=qtest "
> > +                                  "-display none "
> > +                                  "%s", qemu_binary, socket_path,
> > +                                  getenv("QTEST_LOG") ? "/dev/fd/2"
> > +                                                      : "/dev/null",
> > +                                  qmp_socket_path,
> > +                                  extra_args ?: "");
> > +        execlp("/bin/sh", "sh", "-c", command, NULL);
> > +        exit(1);
> > +    }
> > +
> > +    s->fd = socket_accept(sock);
> > +    if (s->fd >= 0) {
> > +        s->qmp_fd = socket_accept(qmpsock);
> > +    }
> > +    unlink(socket_path);
> > +    unlink(qmp_socket_path);
> > +    g_free(socket_path);
> > +    g_free(qmp_socket_path);
> > +
> > +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> > +
> > +    s->rx = g_string_new("");
> > +    for (i = 0; i < MAX_IRQ; i++) {
> > +        s->irq_level[i] = false;
> > +    }
> > +
> > +    /* Read the QMP greeting and then do the handshake */
> > +    qtest_qmp_discard_response(s, "");
> > +    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> > +
> > +    if (getenv("QTEST_STOP")) {
> > +        kill(s->qemu_pid, SIGSTOP);
> > +    }
> > +
> > +    /* ask endianness of the target */
> > +
> > +    qtest_sendf(s, "endianness\n");
> > +    args = qtest_rsp(s, 1);
> > +    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 
> > 0);
> > +    s->big_endian = strcmp(args[1], "big") == 0;
> > +    g_strfreev(args);
> > +
> > +    return s;
> > +}
> > +
> > +void qtest_quit(QTestState *s)
> > +{
> > +    qtest_instances = g_list_remove(qtest_instances, s);
> > +    g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, 
> > s));
> > +
> > +    /* Uninstall SIGABRT handler on last instance */
> > +    if (!qtest_instances) {
> > +        cleanup_sigabrt_handler();
> > +    }
> > +
> > +    kill_qemu(s);
> > +    close(s->fd);
> > +    close(s->qmp_fd);
> > +    g_string_free(s->rx, true);
> > +    g_free(s);
> > +}
> > +
> >  static void qmp_response(JSONMessageParser *parser, GQueue *tokens)
> >  {
> >      QMPResponseParser *qmp = container_of(parser, QMPResponseParser, 
> > parser);
> > @@ -886,50 +897,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

signature.asc
Description: PGP signature