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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).