Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Fri, Oct 07, 2016 at 10:39:09AM +0100, Peter Maydell wrote: > On 7 October 2016 at 00:55, David Gibson 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Fri, Oct 07, 2016 at 12:10:07PM +0200, Greg Kurz wrote: > On Fri, 7 Oct 2016 10:39:09 +0100 > Peter Maydell wrote: > > > On 7 October 2016 at 00:55, David Gibson > > 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Fri, 7 Oct 2016 10:39:09 +0100 Peter Maydell wrote: > On 7 October 2016 at 00:55, David Gibson 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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Fri, 7 Oct 2016 10:36:58 +0100 Peter Maydell wrote: > On 7 October 2016 at 08:31, Greg Kurz wrote: > > On Fri, 7 Oct 2016 09:10:14 +0200 > > Laurent Vivier wrote: > > > >> On 06/10/2016 22:45, Peter Maydell wrote: > >> > On 6 October 2016 at 11:56, Laurent Vivier 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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On 7 October 2016 at 00:55, David Gibson 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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On 7 October 2016 at 08:31, Greg Kurz wrote: > On Fri, 7 Oct 2016 09:10:14 +0200 > Laurent Vivier wrote: > >> On 06/10/2016 22:45, Peter Maydell wrote: >> > On 6 October 2016 at 11:56, Laurent Vivier 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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Fri, 7 Oct 2016 09:10:14 +0200
Laurent Vivier wrote:
> 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_() functions take always a QTestState
> argument, and then in libqtest.h, we have an inline like "inline static
> (...) { qtest_(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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Fri, 7 Oct 2016 10:55:29 +1100
David Gibson 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 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.
> > >
> >
> > This is definitely an improvement indeed.
> >
> > Reviewed-by: Greg Kurz
>
> 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_
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
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_() functions take always a QTestState
argument, and then in libqtest.h, we have an inline like "inline static
(...) { qtest_(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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Thu, Oct 06, 2016 at 10:46:22PM +0200, Greg Kurz wrote:
> On Thu, 6 Oct 2016 20:56:58 +0200
> 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.
> >
>
> This is definitely an improvement indeed.
>
> Reviewed-by: Greg Kurz
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-
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
On Thu, 6 Oct 2016 20:56:58 +0200
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.
>
This is definitely an improvement indeed.
Reviewed-by: Greg Kurz
> 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
Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
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.
> 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
[Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()
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;
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)
