From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44047) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsPIT-0005ru-Fx for qemu-devel@nongnu.org; Fri, 07 Oct 2016 03:10:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsPIQ-0004az-Lk for qemu-devel@nongnu.org; Fri, 07 Oct 2016 03:10:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44844) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsPIQ-0004aL-BK for qemu-devel@nongnu.org; Fri, 07 Oct 2016 03:10:18 -0400 References: <1475780218-26393-1-git-send-email-lvivier@redhat.com> From: Laurent Vivier Message-ID: <568268e9-678e-3301-d66e-001d5d1a3d4a@redhat.com> Date: Fri, 7 Oct 2016 09:10:14 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 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 >> CC: Greg Kurz >> CC: David Gibson >> CC: Peter Maydell >> --- >> 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