On Fri, 7 Oct 2016 09:10:14 +0200
Laurent Vivier <lviv...@redhat.com> wrote:
> On 06/10/2016 22:45, Peter Maydell wrote:
> > 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.
>
> 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
