| Message ID | 1475780218-26393-1-git-send-email-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 6 October 2016 at 11:56, 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. > > 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; Why rename the function? We're only changing its implementation. > 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); This diff arrangement makes the patch a bit hard to read; what meant that the functions had to be moved around? > + /* 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); This would be better in its own utility function, I think. thanks -- PMM
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> > 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);
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); >
On 06/10/2016 22:45, Peter Maydell wrote: > On 6 October 2016 at 11:56, 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. >> >> 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; > > Why rename the function? We're only changing its > implementation. Because in libqtest.c, qtest_XXXX() functions take always a QTestState argument, and then in libqtest.h, we have an inline like "inline static XXXX(...) { qtest_XXXX(global_qtest ...) }". >> 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); > > This diff arrangement makes the patch a bit hard to read; > what meant that the functions had to be moved around? Yes, I know. I move this function after qtest_sendf() and qtest_rsp() to not have to declare them before. There are no circular dependencies, so we can. > >> + /* 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); > > This would be better in its own utility function, I think. Yes, I agree, but my wondering was how to name it :P , qtest_big_endian() and target_big_endian() are already in use, and as it is a 6 lines function, used once, I guessed we could inline it here. Thanks, Laurent
On Fri, 7 Oct 2016 10:55:29 +1100 David Gibson <david@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 <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. > 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); > > >
On Fri, 7 Oct 2016 09:10:14 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > On 06/10/2016 22:45, Peter Maydell wrote: > > On 6 October 2016 at 11:56, 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. > >> > >> 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; > > > > Why rename the function? We're only changing its > > implementation. > > Because in libqtest.c, qtest_XXXX() functions take always a QTestState > argument, and then in libqtest.h, we have an inline like "inline static > XXXX(...) { qtest_XXXX(global_qtest ...) }". > > >> 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); > > > > This diff arrangement makes the patch a bit hard to read; > > what meant that the functions had to be moved around? > > Yes, I know. I move this function after qtest_sendf() and qtest_rsp() to > not have to declare them before. There are no circular dependencies, so > we can. > > > > >> + /* 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); > > > > This would be better in its own utility function, I think. > > Yes, I agree, but my wondering was how to name it :P , > qtest_big_endian() and target_big_endian() are already in use, and as it > is a 6 lines function, used once, I guessed we could inline it here. > This is TARGET_WORDS_BIGENDIAN which is constant for a single QEMU run... why moving it to a function ? Unless there are plans to have dynamic target endianness in QEMU, I guess it makes more sense to open code as you did. Cheers. -- Greg > Thanks, > Laurent
On 7 October 2016 at 08:31, Greg Kurz <groug@kaod.org> wrote: > On Fri, 7 Oct 2016 09:10:14 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> On 06/10/2016 22:45, Peter Maydell wrote: >> > On 6 October 2016 at 11:56, Laurent Vivier <lvivier@redhat.com> wrote: >> >> + /* 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); >> > >> > This would be better in its own utility function, I think. >> >> Yes, I agree, but my wondering was how to name it :P , >> qtest_big_endian() and target_big_endian() are already in use, and as it >> is a 6 lines function, used once, I guessed we could inline it here. >> > > This is TARGET_WORDS_BIGENDIAN which is constant for a single QEMU > run... why moving it to a function ? Unless there are plans to > have dynamic target endianness in QEMU, I guess it makes more > sense to open code as you did. I thought it would be better in its own function simply because "query the QEMU process for the value of TARGET_WORDS_BIGENDIAN" is a simple well defined and self contained operation, which isn't very tightly coupled to the init-the-connection stuff that qtest_init() does. qtest_init() is already a fairly long function and so I think it makes for more maintainable code to have a (static, local) qtest_query_target_endianness() function rather than inlining it. thanks -- PMM
On 7 October 2016 at 00:55, David Gibson <david@gibson.dropbear.id.au> wrote: > It is an improvement. But I still think if we're relying on the > ill-defined "target endianness" we're already doing something wrong. Target endianness is not ill-defined. It's a clear and constant property of the bus the CPU is plugged into. It is a bit weird to rely on it in the test code, which is why only the virtio tests currently use qtest_big_endian(). thanks -- PMM
On Fri, 7 Oct 2016 10:36:58 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 October 2016 at 08:31, Greg Kurz <groug@kaod.org> wrote: > > On Fri, 7 Oct 2016 09:10:14 +0200 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> On 06/10/2016 22:45, Peter Maydell wrote: > >> > On 6 October 2016 at 11:56, Laurent Vivier <lvivier@redhat.com> wrote: > >> >> + /* 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); > >> > > >> > This would be better in its own utility function, I think. > >> > >> Yes, I agree, but my wondering was how to name it :P , > >> qtest_big_endian() and target_big_endian() are already in use, and as it > >> is a 6 lines function, used once, I guessed we could inline it here. > >> > > > > This is TARGET_WORDS_BIGENDIAN which is constant for a single QEMU > > run... why moving it to a function ? Unless there are plans to > > have dynamic target endianness in QEMU, I guess it makes more > > sense to open code as you did. > > I thought it would be better in its own function simply > because "query the QEMU process for the value of > TARGET_WORDS_BIGENDIAN" is a simple well defined and > self contained operation, which isn't very tightly > coupled to the init-the-connection stuff that qtest_init() > does. qtest_init() is already a fairly long function and > so I think it makes for more maintainable code to have > a (static, local) qtest_query_target_endianness() function > rather than inlining it. > > thanks > -- PMM It's a matter of taste, but your point makes sense. I'd say let the maintainer decide, but... $ ./scripts/get_maintainer.pl -f tests/libqtest.c get_maintainer.pl: No maintainers found, printing recent contributors. get_maintainer.pl: Do not blindly cc: them on patches! Use common sense. Cheers. -- Greg
On Fri, 7 Oct 2016 10:39:09 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 October 2016 at 00:55, David Gibson <david@gibson.dropbear.id.au> wrote: > > It is an improvement. But I still think if we're relying on the > > ill-defined "target endianness" we're already doing something wrong. > > Target endianness is not ill-defined. It's a clear and constant > property of the bus the CPU is plugged into. It is a bit weird > to rely on it in the test code, which is why only the virtio > tests currently use qtest_big_endian(). > And to discourage anyone to use it in a test program, maybe it could even be renamed virtio_big_endian() and put in a virtio specific header file ? This is how it is done in QEMU. > thanks > -- PMM Cheers. -- Greg
On Fri, Oct 07, 2016 at 10:39:09AM +0100, Peter Maydell wrote: > On 7 October 2016 at 00:55, David Gibson <david@gibson.dropbear.id.au> wrote: > > It is an improvement. But I still think if we're relying on the > > ill-defined "target endianness" we're already doing something wrong. > > Target endianness is not ill-defined. It's a clear and constant > property of the bus the CPU is plugged into. It's certainly not clear to me. How are you defining it? Preferably in terms of visible effects, rather than something that requires snooping into pieces of hardware that aren't actually modelled in qemu... > It is a bit weird > to rely on it in the test code, which is why only the virtio > tests currently use qtest_big_endian(). > > thanks > -- PMM >
On Fri, Oct 07, 2016 at 12:10:07PM +0200, Greg Kurz wrote: > On Fri, 7 Oct 2016 10:39:09 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > > > On 7 October 2016 at 00:55, David Gibson <david@gibson.dropbear.id.au> wrote: > > > It is an improvement. But I still think if we're relying on the > > > ill-defined "target endianness" we're already doing something wrong. > > > > Target endianness is not ill-defined. It's a clear and constant > > property of the bus the CPU is plugged into. It is a bit weird > > to rely on it in the test code, which is why only the virtio > > tests currently use qtest_big_endian(). > > > > And to discourage anyone to use it in a test program, maybe it > could even be renamed virtio_big_endian() and put in a virtio > specific header file ? This is how it is done in QEMU. I think that's a good idea.
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);
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. 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(-)