[Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Thomas Huth]

The ppc64 postcopy test does not work with KVM-PR, and it is also
causing annoying warning messages when run on a x86 host. So let's
use KVM here only if we know that we're running with KVM-HV (which
automatically also means that we're running on a ppc64 host), and
fall back to TCG otherwise.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/postcopy-test.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index d6613c5..dafe8be 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -380,17 +380,21 @@ static void test_migrate(void)
                                   " -incoming %s",
                                   tmpfs, bootpath, uri);
     } else if (strcmp(arch, "ppc64") == 0) {
+        const char *accel;
+
+        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
+        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
         init_bootfile_ppc(bootpath);
-        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
+        cmd_src = g_strdup_printf("-machine accel=%s -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"
+                                  accel, tmpfs, bootpath);
+        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
                                   " -name pcdest,debug-threads=on"
                                   " -serial file:%s/dest_serial"
                                   " -incoming %s",
-                                  tmpfs, uri);
+                                  accel, tmpfs, uri);
     } else {
         g_assert_not_reached();
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Laurent Vivier]

On 16/11/2016 09:39, Thomas Huth wrote:
> The ppc64 postcopy test does not work with KVM-PR, and it is also
> causing annoying warning messages when run on a x86 host. So let's
> use KVM here only if we know that we're running with KVM-HV (which
> automatically also means that we're running on a ppc64 host), and
> fall back to TCG otherwise.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/postcopy-test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index d6613c5..dafe8be 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -380,17 +380,21 @@ static void test_migrate(void)
>                                    " -incoming %s",
>                                    tmpfs, bootpath, uri);
>      } else if (strcmp(arch, "ppc64") == 0) {
> +        const char *accel;
> +
> +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";

why not "kvm" instead of "kvm:tcg"?
If it doesn't work it should fail.

Laurent

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Thomas Huth]

On 16.11.2016 10:19, Laurent Vivier wrote:
> 
> 
> On 16/11/2016 09:39, Thomas Huth wrote:
>> The ppc64 postcopy test does not work with KVM-PR, and it is also
>> causing annoying warning messages when run on a x86 host. So let's
>> use KVM here only if we know that we're running with KVM-HV (which
>> automatically also means that we're running on a ppc64 host), and
>> fall back to TCG otherwise.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/postcopy-test.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
>> index d6613c5..dafe8be 100644
>> --- a/tests/postcopy-test.c
>> +++ b/tests/postcopy-test.c
>> @@ -380,17 +380,21 @@ static void test_migrate(void)
>>                                    " -incoming %s",
>>                                    tmpfs, bootpath, uri);
>>      } else if (strcmp(arch, "ppc64") == 0) {
>> +        const char *accel;
>> +
>> +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
>> +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
> 
> why not "kvm" instead of "kvm:tcg"?
> If it doesn't work it should fail.

Yes, sounds right. I'll send a v2...

 Thomas

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Laurent Vivier]

On 16/11/2016 09:39, Thomas Huth wrote:
> The ppc64 postcopy test does not work with KVM-PR, and it is also
> causing annoying warning messages when run on a x86 host. So let's
> use KVM here only if we know that we're running with KVM-HV (which
> automatically also means that we're running on a ppc64 host), and
> fall back to TCG otherwise.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  tests/postcopy-test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index d6613c5..dafe8be 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -380,17 +380,21 @@ static void test_migrate(void)
>                                    " -incoming %s",
>                                    tmpfs, bootpath, uri);
>      } else if (strcmp(arch, "ppc64") == 0) {
> +        const char *accel;
> +
> +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
>          init_bootfile_ppc(bootpath);
> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +        cmd_src = g_strdup_printf("-machine accel=%s -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"
> +                                  accel, tmpfs, bootpath);
> +        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
>                                    " -name pcdest,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
>                                    " -incoming %s",
> -                                  tmpfs, uri);
> +                                  accel, tmpfs, uri);
>      } else {
>          g_assert_not_reached();
>      }
> 

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Greg Kurz]

On Wed, 16 Nov 2016 09:39:31 +0100
Thomas Huth <thuth@redhat.com> wrote:

> The ppc64 postcopy test does not work with KVM-PR, and it is also
> causing annoying warning messages when run on a x86 host. So let's
> use KVM here only if we know that we're running with KVM-HV (which
> automatically also means that we're running on a ppc64 host), and
> fall back to TCG otherwise.
> 

This patch addresses two issues actually:
- the annoying warning when running on a ppc64 guest on a non-ppc64 host
- the fact that KVM-PR seems to be currently broken

I agree that the former makes sense, but what about the case of running
a x86 guest on a non-x86 host ?

I'm still feeling uncomfortable with the KVM-PR case... is this a workaround
we want to keep until we find out what's going on or are we starting to
partially deprecate KVM PR ? In any case, I guess we should document this
and probably print some meaningful error message.

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/postcopy-test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index d6613c5..dafe8be 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -380,17 +380,21 @@ static void test_migrate(void)
>                                    " -incoming %s",
>                                    tmpfs, bootpath, uri);
>      } else if (strcmp(arch, "ppc64") == 0) {
> +        const char *accel;
> +
> +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
>          init_bootfile_ppc(bootpath);
> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +        cmd_src = g_strdup_printf("-machine accel=%s -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"
> +                                  accel, tmpfs, bootpath);
> +        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
>                                    " -name pcdest,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
>                                    " -incoming %s",
> -                                  tmpfs, uri);
> +                                  accel, tmpfs, uri);
>      } else {
>          g_assert_not_reached();
>      }

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Wed, 16 Nov 2016 09:39:31 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > The ppc64 postcopy test does not work with KVM-PR, and it is also
> > causing annoying warning messages when run on a x86 host. So let's
> > use KVM here only if we know that we're running with KVM-HV (which
> > automatically also means that we're running on a ppc64 host), and
> > fall back to TCG otherwise.
> > 
> 
> This patch addresses two issues actually:
> - the annoying warning when running on a ppc64 guest on a non-ppc64 host
> - the fact that KVM-PR seems to be currently broken
> 
> I agree that the former makes sense, but what about the case of running
> a x86 guest on a non-x86 host ?
> 
> I'm still feeling uncomfortable with the KVM-PR case... is this a workaround
> we want to keep until we find out what's going on or are we starting to
> partially deprecate KVM PR ? In any case, I guess we should document this
> and probably print some meaningful error message.

This is certainly a work around for now, it doesn't suggest anything about
deprecation.

Dave

> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  tests/postcopy-test.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> > index d6613c5..dafe8be 100644
> > --- a/tests/postcopy-test.c
> > +++ b/tests/postcopy-test.c
> > @@ -380,17 +380,21 @@ static void test_migrate(void)
> >                                    " -incoming %s",
> >                                    tmpfs, bootpath, uri);
> >      } else if (strcmp(arch, "ppc64") == 0) {
> > +        const char *accel;
> > +
> > +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> > +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
> >          init_bootfile_ppc(bootpath);
> > -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> > +        cmd_src = g_strdup_printf("-machine accel=%s -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"
> > +                                  accel, tmpfs, bootpath);
> > +        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
> >                                    " -name pcdest,debug-threads=on"
> >                                    " -serial file:%s/dest_serial"
> >                                    " -incoming %s",
> > -                                  tmpfs, uri);
> > +                                  accel, tmpfs, uri);
> >      } else {
> >          g_assert_not_reached();
> >      }
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Greg Kurz]

On Wed, 16 Nov 2016 12:24:50 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Greg Kurz (groug@kaod.org) wrote:
> > On Wed, 16 Nov 2016 09:39:31 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> > > The ppc64 postcopy test does not work with KVM-PR, and it is also
> > > causing annoying warning messages when run on a x86 host. So let's
> > > use KVM here only if we know that we're running with KVM-HV (which
> > > automatically also means that we're running on a ppc64 host), and
> > > fall back to TCG otherwise.
> > >   
> > 
> > This patch addresses two issues actually:
> > - the annoying warning when running on a ppc64 guest on a non-ppc64 host
> > - the fact that KVM-PR seems to be currently broken
> > 
> > I agree that the former makes sense, but what about the case of running
> > a x86 guest on a non-x86 host ?
> > 
> > I'm still feeling uncomfortable with the KVM-PR case... is this a workaround
> > we want to keep until we find out what's going on or are we starting to
> > partially deprecate KVM PR ? In any case, I guess we should document this
> > and probably print some meaningful error message.  
> 
> This is certainly a work around for now, it doesn't suggest anything about
> deprecation.
> 

Well it doesn't suggest anything actually, it just silently skips KVM PR...
I would at least expect a comment in the code mentioning this is a
workaround and maybe an explicit warning for the user. If the user really
wants to run this test with KVM on ppc64, then she should ensure it is
KVM HV.

Cheers.

--
Greg

> Dave
> 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >  tests/postcopy-test.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> > > index d6613c5..dafe8be 100644
> > > --- a/tests/postcopy-test.c
> > > +++ b/tests/postcopy-test.c
> > > @@ -380,17 +380,21 @@ static void test_migrate(void)
> > >                                    " -incoming %s",
> > >                                    tmpfs, bootpath, uri);
> > >      } else if (strcmp(arch, "ppc64") == 0) {
> > > +        const char *accel;
> > > +
> > > +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> > > +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
> > >          init_bootfile_ppc(bootpath);
> > > -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> > > +        cmd_src = g_strdup_printf("-machine accel=%s -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"
> > > +                                  accel, tmpfs, bootpath);
> > > +        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
> > >                                    " -name pcdest,debug-threads=on"
> > >                                    " -serial file:%s/dest_serial"
> > >                                    " -incoming %s",
> > > -                                  tmpfs, uri);
> > > +                                  accel, tmpfs, uri);
> > >      } else {
> > >          g_assert_not_reached();
> > >      }  
> >   
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Thomas Huth]

On 16.11.2016 13:37, Greg Kurz wrote:
> On Wed, 16 Nov 2016 12:24:50 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
>> * Greg Kurz (groug@kaod.org) wrote:
>>> On Wed, 16 Nov 2016 09:39:31 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> The ppc64 postcopy test does not work with KVM-PR, and it is also
>>>> causing annoying warning messages when run on a x86 host. So let's
>>>> use KVM here only if we know that we're running with KVM-HV (which
>>>> automatically also means that we're running on a ppc64 host), and
>>>> fall back to TCG otherwise.
>>>>   
>>>
>>> This patch addresses two issues actually:
>>> - the annoying warning when running on a ppc64 guest on a non-ppc64 host
>>> - the fact that KVM-PR seems to be currently broken
>>>
>>> I agree that the former makes sense, but what about the case of running
>>> a x86 guest on a non-x86 host ?

Of course you also get these '"kvm" accelerator not found' messages
there. But so far, I think nobody complained about that yet (only for
ppc64 running on x86). And at least the test succeeds there - unlike
with KVM-PR, where the test fails completely.

>>> I'm still feeling uncomfortable with the KVM-PR case... is this a workaround
>>> we want to keep until we find out what's going on or are we starting to
>>> partially deprecate KVM PR ? In any case, I guess we should document this
>>> and probably print some meaningful error message.  
>>
>> This is certainly a work around for now, it doesn't suggest anything about
>> deprecation.
> 
> Well it doesn't suggest anything actually, it just silently skips KVM PR...
> I would at least expect a comment in the code mentioning this is a
> workaround and maybe an explicit warning for the user. If the user really
> wants to run this test with KVM on ppc64, then she should ensure it is
> KVM HV.

Honestly, also considering the number of patches that Laurent already
wrote here and never have been accepted, all this has become quite an
ugly bike-shed painting discussion.

My opinion:

- If we want to properly test KVM (be it KVM-HV or KVM-PR), write
  a proper kvm-unit-test instead. I.e. I personally don't care if this
  test in QEMU is only run with TCG or with KVM.

- The current status of "make check" is broken, since it does not
  work on KVM-PR. We've got to fix that before the release.

That means I currently really don't care if we've spill out a warning
message for KVM-PR here or not - sure, somebody just got to look at
KVM-PR later, but that's IMHO off-topic for the test here in the QEMU
context.

So if you think that the patch for fixing this issue here with the QEMU
test should look differently, please propose a different patch instead.
I'm fine with every other approach as long as we get this fixed in time
for QEMU 2.8.

 Thomas

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Greg Kurz]

On Wed, 16 Nov 2016 14:17:47 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 16.11.2016 13:37, Greg Kurz wrote:
> > On Wed, 16 Nov 2016 12:24:50 +0000
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> >> * Greg Kurz (groug@kaod.org) wrote:  
> >>> On Wed, 16 Nov 2016 09:39:31 +0100
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>     
> >>>> The ppc64 postcopy test does not work with KVM-PR, and it is also
> >>>> causing annoying warning messages when run on a x86 host. So let's
> >>>> use KVM here only if we know that we're running with KVM-HV (which
> >>>> automatically also means that we're running on a ppc64 host), and
> >>>> fall back to TCG otherwise.
> >>>>     
> >>>
> >>> This patch addresses two issues actually:
> >>> - the annoying warning when running on a ppc64 guest on a non-ppc64 host
> >>> - the fact that KVM-PR seems to be currently broken
> >>>
> >>> I agree that the former makes sense, but what about the case of running
> >>> a x86 guest on a non-x86 host ?  
> 
> Of course you also get these '"kvm" accelerator not found' messages
> there. But so far, I think nobody complained about that yet (only for
> ppc64 running on x86). And at least the test succeeds there - unlike
> with KVM-PR, where the test fails completely.
> 
> >>> I'm still feeling uncomfortable with the KVM-PR case... is this a workaround
> >>> we want to keep until we find out what's going on or are we starting to
> >>> partially deprecate KVM PR ? In any case, I guess we should document this
> >>> and probably print some meaningful error message.    
> >>
> >> This is certainly a work around for now, it doesn't suggest anything about
> >> deprecation.  
> > 
> > Well it doesn't suggest anything actually, it just silently skips KVM PR...
> > I would at least expect a comment in the code mentioning this is a
> > workaround and maybe an explicit warning for the user. If the user really
> > wants to run this test with KVM on ppc64, then she should ensure it is
> > KVM HV.  
> 
> Honestly, also considering the number of patches that Laurent already
> wrote here and never have been accepted, all this has become quite an
> ugly bike-shed painting discussion.
> 

Understood. I'm done with the trivial details ;)

> My opinion:
> 
> - If we want to properly test KVM (be it KVM-HV or KVM-PR), write
>   a proper kvm-unit-test instead. I.e. I personally don't care if this
>   test in QEMU is only run with TCG or with KVM.
> 

Agreed.

> - The current status of "make check" is broken, since it does not
>   work on KVM-PR. We've got to fix that before the release.
> 
> That means I currently really don't care if we've spill out a warning
> message for KVM-PR here or not - sure, somebody just got to look at
> KVM-PR later, but that's IMHO off-topic for the test here in the QEMU
> context.
> 
> So if you think that the patch for fixing this issue here with the QEMU
> test should look differently, please propose a different patch instead.
> I'm fine with every other approach as long as we get this fixed in time
> for QEMU 2.8.
> 

The changes to the code look ok and I prefer to spend time chasing the
KVM PR issue rather than arguing on a comment...

Cheers.

--
Greg

>  Thomas
> 

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Laurent Vivier]

On 16/11/2016 15:17, Greg Kurz wrote:
> On Wed, 16 Nov 2016 14:17:47 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 16.11.2016 13:37, Greg Kurz wrote:
>>> On Wed, 16 Nov 2016 12:24:50 +0000
>>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>>>   
>>>> * Greg Kurz (groug@kaod.org) wrote:  
>>>>> On Wed, 16 Nov 2016 09:39:31 +0100
>>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>>     
>>>>>> The ppc64 postcopy test does not work with KVM-PR, and it is also
>>>>>> causing annoying warning messages when run on a x86 host. So let's
>>>>>> use KVM here only if we know that we're running with KVM-HV (which
>>>>>> automatically also means that we're running on a ppc64 host), and
>>>>>> fall back to TCG otherwise.
>>>>>>     
[..]
> The changes to the code look ok and I prefer to spend time chasing the
> KVM PR issue rather than arguing on a comment...

For the problem itself, it seems to appear only after a
BOOK3S_INTERRUPT_SYSCALL interrupt for an KVM_EXIT_PAPR_HCALL
(H_PUT_TERM_CHAR). In this case, KVM has to exit to QEMU to manage the
output. The following interrupt is always an BOOK3S_INTERRUPT_PROGRAM
with an emulation failure.

Laurent

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Greg Kurz]

Hi Laurent,

On Thu, 17 Nov 2016 21:22:33 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 16/11/2016 15:17, Greg Kurz wrote:
> > On Wed, 16 Nov 2016 14:17:47 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 16.11.2016 13:37, Greg Kurz wrote:  
> >>> On Wed, 16 Nov 2016 12:24:50 +0000
> >>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >>>     
> >>>> * Greg Kurz (groug@kaod.org) wrote:    
> >>>>> On Wed, 16 Nov 2016 09:39:31 +0100
> >>>>> Thomas Huth <thuth@redhat.com> wrote:
> >>>>>       
> >>>>>> The ppc64 postcopy test does not work with KVM-PR, and it is also
> >>>>>> causing annoying warning messages when run on a x86 host. So let's
> >>>>>> use KVM here only if we know that we're running with KVM-HV (which
> >>>>>> automatically also means that we're running on a ppc64 host), and
> >>>>>> fall back to TCG otherwise.
> >>>>>>       
> [..]
> > The changes to the code look ok and I prefer to spend time chasing the
> > KVM PR issue rather than arguing on a comment...  
> 
> For the problem itself, it seems to appear only after a
> BOOK3S_INTERRUPT_SYSCALL interrupt for an KVM_EXIT_PAPR_HCALL
> (H_PUT_TERM_CHAR). In this case, KVM has to exit to QEMU to manage the
> output. The following interrupt is always an BOOK3S_INTERRUPT_PROGRAM
> with an emulation failure.
> 

Which specific problem are you referring to ? 

On my side, when running postcopy-test in a nested guest, I hit either one of the
three following issues (in decreasing order of probability of occurence):

1) "Memory content inconsistency at ..." like Stefan

2) "Unexpected 32 on dest_serial serial" accompanied by the following in dmesg

[131613.428616] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
[131613.503515] kvmppc_handle_exit_pr: emulation at d8 failed (00000000)

3) hang because the destination QEMU is looping on:

ioctl(19, KVM_RUN, 0)       = 2 (RESUME_HOST)


Host runs OpenPower HostOS (kernel 4.9, QEMU 2.7) and guest runs fedora25.

Cheers.

--
Greg

> Laurent

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Laurent Vivier]

On 18/11/2016 13:53, Greg Kurz wrote:
> Hi Laurent,

Hi Greg,

> On Thu, 17 Nov 2016 21:22:33 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> On 16/11/2016 15:17, Greg Kurz wrote:
>>> On Wed, 16 Nov 2016 14:17:47 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> On 16.11.2016 13:37, Greg Kurz wrote:  
>>>>> On Wed, 16 Nov 2016 12:24:50 +0000
>>>>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>>>>>     
>>>>>> * Greg Kurz (groug@kaod.org) wrote:    
>>>>>>> On Wed, 16 Nov 2016 09:39:31 +0100
>>>>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>>>>       
>>>>>>>> The ppc64 postcopy test does not work with KVM-PR, and it is also
>>>>>>>> causing annoying warning messages when run on a x86 host. So let's
>>>>>>>> use KVM here only if we know that we're running with KVM-HV (which
>>>>>>>> automatically also means that we're running on a ppc64 host), and
>>>>>>>> fall back to TCG otherwise.
>>>>>>>>       
>> [..]
>>> The changes to the code look ok and I prefer to spend time chasing the
>>> KVM PR issue rather than arguing on a comment...  
>>
>> For the problem itself, it seems to appear only after a
>> BOOK3S_INTERRUPT_SYSCALL interrupt for an KVM_EXIT_PAPR_HCALL
>> (H_PUT_TERM_CHAR). In this case, KVM has to exit to QEMU to manage the
>> output. The following interrupt is always an BOOK3S_INTERRUPT_PROGRAM
>> with an emulation failure.
>>
> 
> Which specific problem are you referring to ? 
..
> 2) "Unexpected 32 on dest_serial serial" accompanied by the following in dmesg
> 
> [131613.428616] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
> [131613.503515] kvmppc_handle_exit_pr: emulation at d8 failed (00000000)

This one, test running on a bare metal PowerMac G5 (F24), 4.9.0-rc5
kernel. And I have only and everytime this error.

Laurent

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On Wed, Nov 16, 2016 at 02:17:47PM +0100, Thomas Huth wrote:
> On 16.11.2016 13:37, Greg Kurz wrote:
> > On Wed, 16 Nov 2016 12:24:50 +0000
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > 
> >> * Greg Kurz (groug@kaod.org) wrote:
> >>> On Wed, 16 Nov 2016 09:39:31 +0100
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>   
> >>>> The ppc64 postcopy test does not work with KVM-PR, and it is also
> >>>> causing annoying warning messages when run on a x86 host. So let's
> >>>> use KVM here only if we know that we're running with KVM-HV (which
> >>>> automatically also means that we're running on a ppc64 host), and
> >>>> fall back to TCG otherwise.
> >>>>   
> >>>
> >>> This patch addresses two issues actually:
> >>> - the annoying warning when running on a ppc64 guest on a non-ppc64 host
> >>> - the fact that KVM-PR seems to be currently broken
> >>>
> >>> I agree that the former makes sense, but what about the case of running
> >>> a x86 guest on a non-x86 host ?
> 
> Of course you also get these '"kvm" accelerator not found' messages
> there. But so far, I think nobody complained about that yet (only for
> ppc64 running on x86). And at least the test succeeds there - unlike
> with KVM-PR, where the test fails completely.

Well, I guess I should complain about them then.  It is slightly
irritating when doing my pre-pull tests on a ppc64 host, although I'm
more or less used to it now.

> >>> I'm still feeling uncomfortable with the KVM-PR case... is this a workaround
> >>> we want to keep until we find out what's going on or are we starting to
> >>> partially deprecate KVM PR ? In any case, I guess we should document this
> >>> and probably print some meaningful error message.  
> >>
> >> This is certainly a work around for now, it doesn't suggest anything about
> >> deprecation.
> > 
> > Well it doesn't suggest anything actually, it just silently skips KVM PR...
> > I would at least expect a comment in the code mentioning this is a
> > workaround and maybe an explicit warning for the user. If the user really
> > wants to run this test with KVM on ppc64, then she should ensure it is
> > KVM HV.
> 
> Honestly, also considering the number of patches that Laurent already
> wrote here and never have been accepted, all this has become quite an
> ugly bike-shed painting discussion.
> 
> My opinion:
> 
> - If we want to properly test KVM (be it KVM-HV or KVM-PR), write
>   a proper kvm-unit-test instead. I.e. I personally don't care if this
>   test in QEMU is only run with TCG or with KVM.
> 
> - The current status of "make check" is broken, since it does not
>   work on KVM-PR. We've got to fix that before the release.
> 
> That means I currently really don't care if we've spill out a warning
> message for KVM-PR here or not - sure, somebody just got to look at
> KVM-PR later, but that's IMHO off-topic for the test here in the QEMU
> context.
> 
> So if you think that the patch for fixing this issue here with the QEMU
> test should look differently, please propose a different patch instead.
> I'm fine with every other approach as long as we get this fixed in time
> for QEMU 2.8.

Hm, yeah, I concur.x

> 
>  Thomas
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Greg Kurz]

On Wed, 16 Nov 2016 09:39:31 +0100
Thomas Huth <thuth@redhat.com> wrote:

> The ppc64 postcopy test does not work with KVM-PR, and it is also
> causing annoying warning messages when run on a x86 host. So let's
> use KVM here only if we know that we're running with KVM-HV (which
> automatically also means that we're running on a ppc64 host), and
> fall back to TCG otherwise.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

FWIW

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/postcopy-test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index d6613c5..dafe8be 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -380,17 +380,21 @@ static void test_migrate(void)
>                                    " -incoming %s",
>                                    tmpfs, bootpath, uri);
>      } else if (strcmp(arch, "ppc64") == 0) {
> +        const char *accel;
> +
> +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
>          init_bootfile_ppc(bootpath);
> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +        cmd_src = g_strdup_printf("-machine accel=%s -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"
> +                                  accel, tmpfs, bootpath);
> +        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
>                                    " -name pcdest,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
>                                    " -incoming %s",
> -                                  tmpfs, uri);
> +                                  accel, tmpfs, uri);
>      } else {
>          g_assert_not_reached();
>      }

Re: [Qemu-devel] [Qemu-ppc] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

On Wed, Nov 16, 2016 at 09:39:31AM +0100, Thomas Huth wrote:
> The ppc64 postcopy test does not work with KVM-PR, and it is also
> causing annoying warning messages when run on a x86 host. So let's
> use KVM here only if we know that we're running with KVM-HV (which
> automatically also means that we're running on a ppc64 host), and
> fall back to TCG otherwise.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Applied to ppc-for-2.8.

Longer term, I think we should default to tcg for all these tests - on
x86 as well - then run KVM *as well* when available.  But in the short
term we should fix make check for the 2.8 release.

> ---
>  tests/postcopy-test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index d6613c5..dafe8be 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -380,17 +380,21 @@ static void test_migrate(void)
>                                    " -incoming %s",
>                                    tmpfs, bootpath, uri);
>      } else if (strcmp(arch, "ppc64") == 0) {
> +        const char *accel;
> +
> +        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> +        accel = access("/sys/module/kvm_hv", F_OK) ? "tcg" : "kvm:tcg";
>          init_bootfile_ppc(bootpath);
> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +        cmd_src = g_strdup_printf("-machine accel=%s -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"
> +                                  accel, tmpfs, bootpath);
> +        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
>                                    " -name pcdest,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
>                                    " -incoming %s",
> -                                  tmpfs, uri);
> +                                  accel, tmpfs, uri);
>      } else {
>          g_assert_not_reached();
>      }

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] tests/postcopy: Use KVM on ppc64 only if it is KVM-HV

[Laurent Vivier]

On 17/11/2016 04:18, David Gibson wrote:
> On Wed, Nov 16, 2016 at 09:39:31AM +0100, Thomas Huth wrote:
>> The ppc64 postcopy test does not work with KVM-PR, and it is also
>> causing annoying warning messages when run on a x86 host. So let's
>> use KVM here only if we know that we're running with KVM-HV (which
>> automatically also means that we're running on a ppc64 host), and
>> fall back to TCG otherwise.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Applied to ppc-for-2.8.
> 
> Longer term, I think we should default to tcg for all these tests - on
> x86 as well - then run KVM *as well* when available.  But in the short
> term we should fix make check for the 2.8 release.

I agree with that.

Laurent