* [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
@ 2016-10-06 18:56 Laurent Vivier
2016-10-06 20:45 ` Peter Maydell
2016-10-06 20:46 ` Greg Kurz
0 siblings, 2 replies; 13+ messages in thread
From: Laurent Vivier @ 2016-10-06 18:56 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Greg Kurz, David Gibson, Peter Maydell
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;
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);
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
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-06 20:46 ` Greg Kurz
1 sibling, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2016-10-06 20:45 UTC (permalink / raw)
To: Laurent Vivier; +Cc: QEMU Developers, Greg Kurz, David Gibson
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-06 20:45 ` Peter Maydell
@ 2016-10-07 7:10 ` Laurent Vivier
2016-10-07 7:31 ` Greg Kurz
0 siblings, 1 reply; 13+ messages in thread
From: Laurent Vivier @ 2016-10-07 7:10 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Greg Kurz, David Gibson
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-07 7:10 ` Laurent Vivier
@ 2016-10-07 7:31 ` Greg Kurz
2016-10-07 9:36 ` Peter Maydell
0 siblings, 1 reply; 13+ messages in thread
From: Greg Kurz @ 2016-10-07 7:31 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Peter Maydell, QEMU Developers, David Gibson
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-07 7:31 ` Greg Kurz
@ 2016-10-07 9:36 ` Peter Maydell
2016-10-07 9:57 ` Greg Kurz
0 siblings, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2016-10-07 9:36 UTC (permalink / raw)
To: Greg Kurz; +Cc: Laurent Vivier, QEMU Developers, David Gibson
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-07 9:36 ` Peter Maydell
@ 2016-10-07 9:57 ` Greg Kurz
0 siblings, 0 replies; 13+ messages in thread
From: Greg Kurz @ 2016-10-07 9:57 UTC (permalink / raw)
To: Peter Maydell; +Cc: Laurent Vivier, QEMU Developers, David Gibson
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
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-06 20:46 ` Greg Kurz
2016-10-06 23:55 ` David Gibson
1 sibling, 1 reply; 13+ messages in thread
From: Greg Kurz @ 2016-10-06 20:46 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, David Gibson, Peter Maydell
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);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-06 20:46 ` Greg Kurz
@ 2016-10-06 23:55 ` David Gibson
2016-10-07 7:14 ` Greg Kurz
2016-10-07 9:39 ` Peter Maydell
0 siblings, 2 replies; 13+ messages in thread
From: David Gibson @ 2016-10-06 23:55 UTC (permalink / raw)
To: Greg Kurz; +Cc: Laurent Vivier, qemu-devel, Peter Maydell
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-06 23:55 ` David Gibson
@ 2016-10-07 7:14 ` Greg Kurz
2016-10-07 9:39 ` Peter Maydell
1 sibling, 0 replies; 13+ messages in thread
From: Greg Kurz @ 2016-10-07 7:14 UTC (permalink / raw)
To: David Gibson; +Cc: Laurent Vivier, qemu-devel, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 14113 bytes --]
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);
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-06 23:55 ` David Gibson
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:28 ` David Gibson
1 sibling, 2 replies; 13+ messages in thread
From: Peter Maydell @ 2016-10-07 9:39 UTC (permalink / raw)
To: David Gibson; +Cc: Greg Kurz, Laurent Vivier, QEMU Developers
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
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
1 sibling, 1 reply; 13+ messages in thread
From: Greg Kurz @ 2016-10-07 10:10 UTC (permalink / raw)
To: Peter Maydell; +Cc: David Gibson, Laurent Vivier, QEMU Developers
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-07 10:10 ` Greg Kurz
@ 2016-10-10 1:30 ` David Gibson
0 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2016-10-10 1:30 UTC (permalink / raw)
To: Greg Kurz; +Cc: Peter Maydell, Laurent Vivier, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]
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.
--
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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
2016-10-07 9:39 ` Peter Maydell
2016-10-07 10:10 ` Greg Kurz
@ 2016-10-10 1:28 ` David Gibson
1 sibling, 0 replies; 13+ messages in thread
From: David Gibson @ 2016-10-10 1:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: Greg Kurz, Laurent Vivier, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 978 bytes --]
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
>
--
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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-10-10 1:33 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
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).