From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: Laurent Vivier <lvivier@redhat.com>,
qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>
Subject: Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
Date: Fri, 7 Oct 2016 10:55:29 +1100 [thread overview]
Message-ID: <20161006235529.GI18490@umbus.fritz.box> (raw)
In-Reply-To: <20161006224622.516cee20@bahia>
[-- Attachment #1: Type: text/plain, Size: 13329 bytes --]
On Thu, Oct 06, 2016 at 10:46:22PM +0200, Greg Kurz wrote:
> On Thu, 6 Oct 2016 20:56:58 +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>
> > ---
> > 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 <groug@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.
>
> > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2016-10-07 0:13 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-06 18:56 [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init() Laurent Vivier
2016-10-06 20:45 ` Peter Maydell
2016-10-07 7:10 ` Laurent Vivier
2016-10-07 7:31 ` Greg Kurz
2016-10-07 9:36 ` Peter Maydell
2016-10-07 9:57 ` Greg Kurz
2016-10-06 20:46 ` Greg Kurz
2016-10-06 23:55 ` David Gibson [this message]
2016-10-07 7:14 ` Greg Kurz
2016-10-07 9:39 ` Peter Maydell
2016-10-07 10:10 ` Greg Kurz
2016-10-10 1:30 ` David Gibson
2016-10-10 1:28 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161006235529.GI18490@umbus.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=groug@kaod.org \
--cc=lvivier@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.