Re: [Qemu-devel] [PATCH v2] test: port postcopy test to ppc64

[David Gibson]

On Tue, Jul 26, 2016 at 11:58:17AM +0200, Laurent Vivier wrote:
> 
> 
> On 26/07/2016 11:54, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (address@hidden) wrote:
> >>
> >>
> >> On 26/07/2016 11:39, Laurent Vivier wrote:
> >>>
> >>>
> >>> On 26/07/2016 11:28, Thomas Huth wrote:
> >>>> On 26.07.2016 11:23, Laurent Vivier wrote:
> >>>>>
> >>>>>
> >>>>> On 23/07/2016 08:30, David Gibson wrote:
> >>>>>> On Fri, Jul 22, 2016 at 09:28:58AM +0200, Laurent Vivier wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 22/07/2016 08:43, David Gibson wrote:
> >>>>>>>> On Thu, Jul 21, 2016 at 06:47:56PM +0200, Laurent Vivier wrote:
> >>>>>>>>> As userfaultfd syscall is available on powerpc, migration
> >>>>>>>>> postcopy can be used.
> >>>>>>>>>
> >>>>>>>>> This patch adds the support needed to test this on powerpc,
> >>>>>>>>> instead of using a bootsector to run code to modify memory,
> >>>>>>>>> we use a FORTH script in "boot-command" property.
> >>>>>>>>>
> >>>>>>>>> As spapr machine doesn't support "-prom-env" argument
> >>>>>>>>> (the nvram is initialized by SLOF and not by QEMU),
> >>>>>>>>> "boot-command" is provided to SLOF via a file mapped nvram
> >>>>>>>>> (with "-drive file=...,if=pflash")
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Laurent Vivier <address@hidden>
> >>>>>>>>> ---
> >>>>>>>>> v2: move FORTH script directly in sprintf()
> >>>>>>>>>     use openbios_firmware_abi.h
> >>>>>>>>>     remove useless "default" case
> >>>>>>>>>
> >>>>>>>>>  tests/Makefile.include |   1 +
> >>>>>>>>>  tests/postcopy-test.c  | 116 
> >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++--------
> >>>>>>>>>  2 files changed, 98 insertions(+), 19 deletions(-)
> >>>>>>>>
> >>>>>>>> There's a mostly cosmetic problem with this.  If you run make check
> >>>>>>>> for a ppc64 target on an x86 machine, you get:
> >>>>>>>>
> >>>>>>>> GTESTER check-qtest-ppc64
> >>>>>>>> "kvm" accelerator not found.
> >>>>>>>> "kvm" accelerator not found.
> >>>>>>>
> >>>>>>> I think this is because of "-machine accel=kvm:tcg", it tries to use 
> >>>>>>> kvm
> >>>>>>> and fall back to tcg.
> >>>>>>>
> >>>>>>> accel.c:
> >>>>>>>
> >>>>>>>      80 void configure_accelerator(MachineState *ms)
> >>>>>>>      81 {
> >>>>>>> ...
> >>>>>>>     100         acc = accel_find(buf);
> >>>>>>>     101         if (!acc) {
> >>>>>>>     102             fprintf(stderr, "\"%s\" accelerator not 
> >>>>>>> found.\n", buf);
> >>>>>>>     103             continue;
> >>>>>>>     104         }
> >>>>>>>
> >>>>>>> We can remove the "-machine" argument to use the default instead (tcg 
> >>>>>>> or
> >>>>>>> kvm).
> >>>>>>
> >>>>>> That sounds like a good option for a general test.
> >>>>>
> >>>>> In fact, we can't: we need to add a "-machine accel=XXXX" to our command
> >>>>> line to override the "-machine accel=qtest" provided by the qtest
> >>>>> framework. If we don't override it, the machine doesn't start.
> >>>>
> >>>> Would it work if you'd added some magic with "#ifdef CONFIG_KVM" here?
> >>>
> >>> I think it needs to be dynamic as the same binary test is used on x86 to
> >>> test x86 and ppc64, and vice-versa. I'm going to check if we have
> >>> something like "qtest_get_accel()"...
> >>
> >> Something like that should work:
> >>
> >> --- a/tests/postcopy-test.c
> >> +++ b/tests/postcopy-test.c
> >> @@ -380,12 +380,17 @@ static void test_migrate(void)
> >>                                    tmpfs, bootpath, uri);
> >>      } else if (strcmp(arch, "ppc64") == 0) {
> >>          init_bootfile_ppc(bootpath);
> >> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> >> +#ifdef _ARCH_PPC64
> >> +#define QEMU_CMD_ACCEL            "-machine accel=kvm:tcg"
> >> +#else
> >> +#define QEMU_CMD_ACCEL            "-machine accel=tcg"
> >> +#endif
> >> +        cmd_src = g_strdup_printf(QEMU_CMD_ACCEL " -m 256M"
> >>                                    " -name pcsource,debug-threads=on"
> >>                                    " -serial file:%s/src_serial"
> >>                                    " -drive file=%s,if=pflash,format=raw",
> >>                                    tmpfs, bootpath);
> >> -        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> >> +        cmd_dst = g_strdup_printf(QEMU_CMD_ACCEL " -m 256M"
> >>                                    " -name pcdest,debug-threads=on"
> >>                                    " -serial file:%s/dest_serial"
> >>                                    " -incoming %s",
> >>
> >> Laurent
> > 
> > Is it worth the hastle to just get rid of the two warnings?
> 
> I don't know, it's why I'd like to have the opinion of David.

I'm not really sure either.  I do dislike leaving warnings as a rule,
because for someone not familiar with the details of the test it may
not be obvious whether a warning is harmless or not.

-- 
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