On 6 October 2016 at 11:56, Laurent Vivier <lviv...@redhat.com> wrote:
> The target endianness is not deduced anymore from
> the architecture name but asked directly to the guest,
> using a new qtest command: "endianness". As it can't
> change (this is the value of TARGET_WORDS_BIGENDIAN),
> we store it to not have to ask every time we want to
> know if we have to byte-swap a value.
>
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
> CC: Greg Kurz <gr...@kaod.org>
> CC: David Gibson <da...@gibson.dropbear.id.au>
> CC: Peter Maydell <peter.mayd...@linaro.org>
> ---
> Note: this patch can be seen as a v2 of
> "qtest: evaluate endianness of the target in qtest_init()"
> from the patch series "tests: enable virtio tests on SPAPR"
> in which I have added the idea from Peter to ask the endianness
> directly to the guest using a new qtest command.
>
> 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
