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

[Thomas Huth]

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

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