[Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling

David Gibson posted 4 patches 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170602031507.29881-1-david@gibson.dropbear.id.au
Test checkpatch failed
Test docker passed
Test s390x passed
hw/ppc/spapr.c               |   8 +++-
hw/ppc/spapr_cpu_core.c      |  61 +++++++++++++++++++++-----
hw/ppc/spapr_hcall.c         |   8 ++--
include/hw/ppc/spapr.h       |  12 +++--
qapi/string-input-visitor.c  |  11 +++++
qapi/string-output-visitor.c |  14 ++++++
target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
target/ppc/cpu.h             |   5 ++-
target/ppc/machine.c         |  69 +++++++++++++++++++++++++++--
target/ppc/translate_init.c  |  86 +++++++++++-------------------------
10 files changed, 292 insertions(+), 84 deletions(-)
[Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by David Gibson 6 years, 10 months ago
This is a rebased and revised version of my patches revising CPU
compatiblity mode handling on ppc, last posted in November.  Since
then, many of the patches have already been merged (some for 2.9, some
since).  This is what's left.

 * There was conceptual confusion about what a compatibility mode
   means, and how it interacts with the machine type.  This cleans
   that up, clarifying that a compatibility mode (as an externally set
   option) only makes sense on machine types that don't permit the
   guest hypervisor privilege (i.e. 'pseries')

 * It was previously the user's (or management layer's) responsibility
   to determine compatibility of CPUs on either end for migration.
   This uses the compatibility modes to check that properly during an
   incoming migration.

This hasn't been extensively tested yet.  There are quite a few
migration cases to consider, for example:

Basic:

1) Boot guest with -cpu host
        Should go into POWER8 compat mode after CAS
        Previously would have been raw mode

2) Boot guest with -machine pseries,max-cpu-compat=power7 -cpu host
        Should go into POWER7 compat mode

3) Boot guest with -cpu host,compat=power7
        Should act as (2), but print a warning

4) Boot guest via libvirt with power7 compat mode specified in XML
        Should act as (3), (2) once we fix libvirt

5) Hack guest to only advertise power7 compatibility, boot with -cpu host
        Should go into POWER7 compat mode after CAS

6) Hack guest to only advertise real PVRs
        Should remain in POWER8 raw mode after CAS

7) Hack guest to only advertise real PVRs
   Boot with -machine pseries,max-cpu-compat=power8
        Should fail at CAS time

8) Hack guest to only advertise power7 compatibility, boot with -cpu host
   Reboot to normal guest
        Should go to power7 compat mode after CAS of boot 1
        Should revert to raw mode on reboot
        SHould go to power8 compat mode after CAS of boot 2

Migration:

9) Boot guest with qemu-2.6 -machine pseries-2.6 -cpu host
   Migrate to qemu-2.8 -machine pseries-2.6 -cpu host
        Should work, end up running in power8 raw mode

10) Boot guest with qemu-2.7 -machine pseries-2.7 -cpu host
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host
        Should work, end up running in power8 raw mode

11) Boot guest with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host,compat=power7
        Should work, be running in POWER7 compat after, but give warning like
        (3)

12) Boot guest with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7,max-cpu-compat=power7 -cpu host
        Should work, be running in POWER7 compat after, no warning

13) Boot to SLOF with qemu-2.6 -machine pseries-2.6 -cpu host
    Migrate to qemu-2.8 -machine pseries-2.6 -cpu host

        ?

14) Boot to SLOF with qemu-2.7 -machine pseries-2.7 -cpu host
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host
        ?

15) Boot to SLOF with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host,compat=power7
        ?

16) Boot to SLOF with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7,max-cpu-compat=power7 -cpu host
        ?

17) Boot guest with qemu-2.6 -machine pseries-2.6 -cpu host
    Migrate to qemu-2.7.z -machine pseries-2.6 -cpu host
        Should work

18) Hack guest to only advertise power7 compatibility, boot with -cpu host
    Boot with qemu-2.8, migrate to qemu-2.8
        Should be in power7 compat mode after CAS on source, and still
        in power7 compat mode on destination

Changes since v4:
  * Fixed a crash bug in the smp option compatiblity mangling
  * Removed an unnecessary fallback for missing pvr_match
  * Some spelling corrections
  * Migration core patch removed (alread merged to ppc-for-2.10)

Changes since v3:
  * Backwards compatible -cpu handling now removes compat= option from
    options passed on to the cpu, so it doesn't trigger further warnings
  * Add a migration fix make cpu_synchronize_state() safe in post_load
    handlers, which in turn fixes a bug in 5/5.
  * A number of bugfixes and other tweaks suggested by feedback on v2.

Changes since RFCv2:
  * Many patches dropped, since they're already merged
  * Rebased, fixed conflicts
  * Restored support for backwards migration (wasn't as complicated as
    I thought)
  * Updated final patch's description to more accurately reflect the
    logic

Changes since RFCv1:
  * Change CAS logic to prefer compatibility modes over raw mode
  * Simplified by giving up on half-hearted attempts to maintain
    backwards migration
  * Folded migration stream changes into a single patch
  * Removed some preliminary patches which are already merged

David Gibson (3):
  pseries: Move CPU compatibility property to machine
  pseries: Reset CPU compatibility mode
  ppc: Rework CPU compatibility testing across migration

Greg Kurz (1):
  qapi: add explicit null to string input and output visitors

 hw/ppc/spapr.c               |   8 +++-
 hw/ppc/spapr_cpu_core.c      |  61 +++++++++++++++++++++-----
 hw/ppc/spapr_hcall.c         |   8 ++--
 include/hw/ppc/spapr.h       |  12 +++--
 qapi/string-input-visitor.c  |  11 +++++
 qapi/string-output-visitor.c |  14 ++++++
 target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
 target/ppc/cpu.h             |   5 ++-
 target/ppc/machine.c         |  69 +++++++++++++++++++++++++++--
 target/ppc/translate_init.c  |  86 +++++++++++-------------------------
 10 files changed, 292 insertions(+), 84 deletions(-)

-- 
2.9.4


Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by no-reply@patchew.org 6 years, 10 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170602031507.29881-1-david@gibson.dropbear.id.au
Subject: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6ccf47f ppc: Rework CPU compatibility testing across migration
2b75df3 pseries: Reset CPU compatibility mode
282a89c pseries: Move CPU compatibility property to machine
8741a73 qapi: add explicit null to string input and output visitors

=== OUTPUT BEGIN ===
Checking PATCH 1/4: qapi: add explicit null to string input and output visitors...
Checking PATCH 2/4: pseries: Move CPU compatibility property to machine...
Checking PATCH 3/4: pseries: Reset CPU compatibility mode...
Checking PATCH 4/4: ppc: Rework CPU compatibility testing across migration...
ERROR: braces {} are necessary for all arms of this statement
#94: FILE: target/ppc/machine.c:236:
+    if (cpu->compat_pvr) {
[...]
+    } else
[...]

total: 1 errors, 0 warnings, 100 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by David Gibson 6 years, 10 months ago
On Thu, Jun 01, 2017 at 08:55:19PM -0700, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 20170602031507.29881-1-david@gibson.dropbear.id.au
> Subject: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 6ccf47f ppc: Rework CPU compatibility testing across migration
> 2b75df3 pseries: Reset CPU compatibility mode
> 282a89c pseries: Move CPU compatibility property to machine
> 8741a73 qapi: add explicit null to string input and output visitors
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/4: qapi: add explicit null to string input and output visitors...
> Checking PATCH 2/4: pseries: Move CPU compatibility property to machine...
> Checking PATCH 3/4: pseries: Reset CPU compatibility mode...
> Checking PATCH 4/4: ppc: Rework CPU compatibility testing across migration...
> ERROR: braces {} are necessary for all arms of this statement
> #94: FILE: target/ppc/machine.c:236:
> +    if (cpu->compat_pvr) {
> [...]
> +    } else
> [...]
> 
> total: 1 errors, 0 warnings, 100 lines checked

This is a false positive triggered by an #ifdef intersecting the
if/else.

-- 
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
Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by David Gibson 6 years, 9 months ago
On Fri, Jun 02, 2017 at 01:15:03PM +1000, David Gibson wrote:
> This is a rebased and revised version of my patches revising CPU
> compatiblity mode handling on ppc, last posted in November.  Since
> then, many of the patches have already been merged (some for 2.9, some
> since).  This is what's left.
> 
>  * There was conceptual confusion about what a compatibility mode
>    means, and how it interacts with the machine type.  This cleans
>    that up, clarifying that a compatibility mode (as an externally set
>    option) only makes sense on machine types that don't permit the
>    guest hypervisor privilege (i.e. 'pseries')
> 
>  * It was previously the user's (or management layer's) responsibility
>    to determine compatibility of CPUs on either end for migration.
>    This uses the compatibility modes to check that properly during an
>    incoming migration.

Anyone willing to give some review and/or testing, particularly for
patch 2/4.

-- 
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
Re: [Qemu-devel] [Qemu-ppc] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by Suraj Jitindar Singh 6 years, 9 months ago
On Thu, 2017-06-08 at 14:17 +1000, David Gibson wrote:
> On Fri, Jun 02, 2017 at 01:15:03PM +1000, David Gibson wrote:
> > This is a rebased and revised version of my patches revising CPU
> > compatiblity mode handling on ppc, last posted in November.  Since
> > then, many of the patches have already been merged (some for 2.9,
> > some
> > since).  This is what's left.
> > 
> >  * There was conceptual confusion about what a compatibility mode
> >    means, and how it interacts with the machine type.  This cleans
> >    that up, clarifying that a compatibility mode (as an externally
> > set
> >    option) only makes sense on machine types that don't permit the
> >    guest hypervisor privilege (i.e. 'pseries')
> > 
> >  * It was previously the user's (or management layer's)
> > responsibility
> >    to determine compatibility of CPUs on either end for migration.
> >    This uses the compatibility modes to check that properly during
> > an
> >    incoming migration.
> 
> Anyone willing to give some review and/or testing, particularly for
> patch 2/4.
> 

Sorry I had been playing around with this and meant to give it a
Tested-by

Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by Greg Kurz 6 years, 9 months ago
On Thu, 8 Jun 2017 14:17:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 02, 2017 at 01:15:03PM +1000, David Gibson wrote:
> > This is a rebased and revised version of my patches revising CPU
> > compatiblity mode handling on ppc, last posted in November.  Since
> > then, many of the patches have already been merged (some for 2.9, some
> > since).  This is what's left.
> > 
> >  * There was conceptual confusion about what a compatibility mode
> >    means, and how it interacts with the machine type.  This cleans
> >    that up, clarifying that a compatibility mode (as an externally set
> >    option) only makes sense on machine types that don't permit the
> >    guest hypervisor privilege (i.e. 'pseries')
> > 
> >  * It was previously the user's (or management layer's) responsibility
> >    to determine compatibility of CPUs on either end for migration.
> >    This uses the compatibility modes to check that properly during an
> >    incoming migration.  
> 
> Anyone willing to give some review and/or testing, particularly for
> patch 2/4.
> 

I'm planning to re-run all the tests when migration of pre-2.10 machine
types is fixed.
Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by Andrea Bolognani 6 years, 9 months ago
On Fri, 2017-06-02 at 13:15 +1000, David Gibson wrote:
> This is a rebased and revised version of my patches revising CPU
> compatiblity mode handling on ppc, last posted in November.  Since
> then, many of the patches have already been merged (some for 2.9, some
> since).  This is what's left.

I've tested this the same way I had tested one of the
previous respins, eg. for basic usage and general sanity
of the interface. All the issues I pointed out last time
seems to have been addressed.


Tested-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by David Gibson 6 years, 9 months ago
On Sat, Jun 10, 2017 at 05:42:45PM +0200, Andrea Bolognani wrote:
> On Fri, 2017-06-02 at 13:15 +1000, David Gibson wrote:
> > This is a rebased and revised version of my patches revising CPU
> > compatiblity mode handling on ppc, last posted in November.  Since
> > then, many of the patches have already been merged (some for 2.9, some
> > since).  This is what's left.
> 
> I've tested this the same way I had tested one of the
> previous respins, eg. for basic usage and general sanity
> of the interface. All the issues I pointed out last time
> seems to have been addressed.
> 
> 
> Tested-by: Andrea Bolognani <abologna@redhat.com>

Ok, I think have enough acks of various sorts now.  I've merged this
into ppc-for-2.10.

-- 
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
Re: [Qemu-devel] [Qemu-ppc] [PATCHv5 0/4] Clean up compatibility mode handling
Posted by Andrea Bolognani 6 years, 9 months ago
On Fri, 2017-06-02 at 13:15 +1000, David Gibson wrote:
> This is a rebased and revised version of my patches revising CPU
> compatiblity mode handling on ppc, last posted in November.  Since
> then, many of the patches have already been merged (some for 2.9, some
> since).  This is what's left.

As discussed yesterday on libvir-list, the current
implementation will abort() when the user attempts to use
a compat mode that's not valid for the host CPU, eg. on
a POWER8 host:

  $ qemu-system-ppc64 \
    -nodefaults \
    -M pseries,accel=kvm \
    -cpu host,compat=power9
  Unexpected error in ppc_set_compat() at target/ppc/compat.c:135:
  qemu-system-ppc64: Compatibility PVR 0x0f000005 not valid for CPU
  Aborted

We should probably report the error to the user in a slightly
less destructive fashion :)


[1] https://www.redhat.com/archives/libvir-list/2017-June/msg00476.html
-- 
Andrea Bolognani / Red Hat / Virtualization