| Message ID | 4ab69528-5fe7-1f44-078a-04af2a1985e6@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Laurent Vivier (lvivier@redhat.com) 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 <lvivier@redhat.com> > >>>>>>> --- > >>>>>>> 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? Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 26/07/2016 11:54, Dr. David Alan Gilbert wrote: > * Laurent Vivier (lvivier@redhat.com) 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 <lvivier@redhat.com> >>>>>>>>> --- >>>>>>>>> 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. Laurent
On 26.07.2016 11:53, Laurent Vivier 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 <lvivier@redhat.com> >>>>>>>> --- >>>>>>>> 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 I think you'd need to test CONFIG_KVM, too, since it could also have been disabled on on PPC, couldn't it? > +#define QEMU_CMD_ACCEL "-machine accel=kvm:tcg" > +#else > +#define QEMU_CMD_ACCEL "-machine accel=tcg" > +#endif Alternatively, what about shutting up the message in accel.c by changing it like that: if (!qtest_enabled()) { error_report("\"%s\" accelerator not found.\n", buf); } ? Thomas
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 (lvivier@redhat.com) 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 <lvivier@redhat.com> > >>>>>>>>> --- > >>>>>>>>> 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.
On 26/07/2016 12:02, Thomas Huth wrote: > On 26.07.2016 11:53, Laurent Vivier 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 <lvivier@redhat.com> >>>>>>>>> --- >>>>>>>>> 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 > > I think you'd need to test CONFIG_KVM, too, since it could also have > been disabled on on PPC, couldn't it? Sure. >> +#define QEMU_CMD_ACCEL "-machine accel=kvm:tcg" >> +#else >> +#define QEMU_CMD_ACCEL "-machine accel=tcg" >> +#endif > > Alternatively, what about shutting up the message in accel.c by changing > it like that: > > if (!qtest_enabled()) { > error_report("\"%s\" accelerator not found.\n", buf); > } > I've tried that, and we always get the messages in the "make check" output. Laurent
On 26/07/2016 14:53, Laurent Vivier wrote: > > > On 26/07/2016 12:02, Thomas Huth wrote: >> On 26.07.2016 11:53, Laurent Vivier 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 <lvivier@redhat.com> >>>>>>>>>> --- >>>>>>>>>> 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 >> >> I think you'd need to test CONFIG_KVM, too, since it could also have >> been disabled on on PPC, couldn't it? > > Sure. > >>> +#define QEMU_CMD_ACCEL "-machine accel=kvm:tcg" >>> +#else >>> +#define QEMU_CMD_ACCEL "-machine accel=tcg" >>> +#endif >> >> Alternatively, what about shutting up the message in accel.c by changing >> it like that: >> >> if (!qtest_enabled()) { >> error_report("\"%s\" accelerator not found.\n", buf); >> } >> > > I've tried that, and we always get the messages in the "make check" output. No, I'm wrong: I didn't add the "qtest_enabled()", only replace the fprintf() by an "error_report()"... it should work. Laurent
On 26/07/2016 12:02, Thomas Huth wrote: > On 26.07.2016 11:53, Laurent Vivier 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 <lvivier@redhat.com> >>>>>>>>> --- >>>>>>>>> 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 > > I think you'd need to test CONFIG_KVM, too, since it could also have > been disabled on on PPC, couldn't it? I've tried to use CONFIG_KVM, but as it is defined per target in config-target.h we can't use it in qtest sources. I think we can only use CONFIGs defined in config-host.h (as the same binary is used for all the targets). Laurent
On 26/07/2016 12:02, Thomas Huth wrote: > On 26.07.2016 11:53, Laurent Vivier 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 <lvivier@redhat.com> >>>>>>>>> --- >>>>>>>>> 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 > > I think you'd need to test CONFIG_KVM, too, since it could also have > been disabled on on PPC, couldn't it? > >> +#define QEMU_CMD_ACCEL "-machine accel=kvm:tcg" >> +#else >> +#define QEMU_CMD_ACCEL "-machine accel=tcg" >> +#endif > > Alternatively, what about shutting up the message in accel.c by changing > it like that: > > if (!qtest_enabled()) { > error_report("\"%s\" accelerator not found.\n", buf); > } > We can't use this as we overwrite "-machine accel=qtest" with our "-machine accel=kvm:tcg": qtest accelerator is not initialized and qtest_enabled() is false. Thanks, Laurent
--- 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",