On Fri, 7 Oct 2016 10:55:29 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Thu, Oct 06, 2016 at 10:46:22PM +0200, Greg Kurz wrote:
> > On Thu, 6 Oct 2016 20:56:58 +0200
> > Laurent Vivier <lviv...@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 <lviv...@redhat.com>
> > > CC: Greg Kurz <gr...@kaod.org>
> > > CC: David Gibson <da...@gibson.dropbear.id.au>
> > > CC: Peter Maydell <peter.mayd...@linaro.org>
> > > ---
> > > 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 <gr...@kaod.org>
>
> It is an improvement. But I still think if we're relying on the
> ill-defined "target endianness" we're already doing something wrong.
>
Just to be sure, you're talking about the qtest case only or about the
use of TARGET_WORDS_BIGENDIAN in QEMU itself ?
> >
> > > 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);
> >
>
pgp4ckHDosM8A.pgp
Description: OpenPGP digital signature
