Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, May 30, 2017 at 10:01:36AM +0200, Greg Kurz wrote:
> On Tue, 30 May 2017 16:18:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, May 30, 2017 at 01:14:16AM +0200, Greg Kurz wrote:
> > > On Fri, 26 May 2017 15:23:14 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > [...]  
> > > > 
> > > > 
> > > > 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  
> > > 
> > > This seems to also have another interesting effect.
> > > 
> > > getset_compat_deprecated() could be called either during CPU realization from:
> > > 
> > > object_property_parse()
> > > {
> > >     Visitor *v = string_input_visitor_new(string);
> > >     object_property_set(obj, v, name, errp);
> > >     ...
> > > }
> > > 
> > > or during a QOM set operation from:
> > > 
> > > void object_property_set_qobject(Object *obj, QObject *value,
> > >                                  const char *name, Error **errp)
> > > {
> > >     Visitor *v;
> > > 
> > >     v = qobject_input_visitor_new(value);
> > >     object_property_set(obj, v, name, errp);
> > >     ...
> > > }
> > > 
> > > or similarly during a QOM get operation with a QObject output visitor.
> > > 
> > > The realization path no longer exists with patch 2, so you don't need
> > > to implement a null string input visitor anymore.  
> > 
> > s/patch 2/patch 3/?
> > 
> 
> Yes indeed, sorry :)
> 
> > Is that true though?  It shouldn't get called through that path in
> > practice, because we strip the compat property from the cpu object
> > properties.  But it could get called if either a) the user explicitly
> > creates a cpu object with -device CPU,compat=whatever or b) if the
> 
> Unless I'm missing something, properties of the CPU objects aren't
> exposed by CPU devices:
> 
> qemu-system-ppc64 -machine pseries \
>                   -device POWER8_v2.0-spapr-cpu-core,help
> POWER8_v2.0-spapr-cpu-core.core-id=int
> POWER8_v2.0-spapr-cpu-core.node-id=int32
> POWER8_v2.0-spapr-cpu-core.nr-threads=int
> 
> and creation of CPU objects with -object isn't possible either since
> they don't inherit from the TYPE_USER_CREATABLE class.

Ah, true, I hadn't considered that.

> > user uses the compat= property with a machine type other than pseries.
> > 
> 
> But yes, it is still possible to go through the object_property_parse()
> path with another machine type indeed.

> > Of course the user *shouldn't* do either of those things, but
> > providing a meaningful error if they do is pretty much the whole
> > purpose of this getter/setter method.
> > 
> 
> All old non-pseries machine types already complain when started with
> a POWER7 or newer CPU. Providing the extra error message looks weird:
> 
> qemu-system-ppc64 -machine ppce500 \
>                   -cpu POWER7,compat=power6
> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
>  use max-cpu-compat machine property instead
> MMU model 983043 not supported by this machine.
> 
> but I guess it's better than crashing. :)

Well, sure POWER7 doesn't make sense for an e500 machine for other
reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
compat= doesn't.

> 
> > > 
> > > This means that patch 1 is no longer needed if I get things right but
> > > you probably want Markus to second that.
> > >   
> > > >   * 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 (4):
> > > >   migration: Mark CPU states dirty before incoming migration/loadvm
> > > >   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
> > > > 
> > > >  cpus.c                       |   9 ++++
> > > >  hw/ppc/spapr.c               |   8 +++-
> > > >  hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
> > > >  hw/ppc/spapr_hcall.c         |   8 ++--
> > > >  include/hw/ppc/spapr.h       |  12 +++--
> > > >  include/sysemu/cpus.h        |   1 +
> > > >  include/sysemu/hax.h         |   1 +
> > > >  include/sysemu/hw_accel.h    |  10 +++++
> > > >  include/sysemu/kvm.h         |   1 +
> > > >  kvm-all.c                    |  10 +++++
> > > >  migration/savevm.c           |   2 +
> > > >  qapi/string-input-visitor.c  |  11 +++++
> > > >  qapi/string-output-visitor.c |  14 ++++++
> > > >  target/i386/hax-all.c        |  10 +++++
> > > >  target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
> > > >  target/ppc/cpu.h             |   5 ++-
> > > >  target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
> > > >  target/ppc/translate_init.c  |  86 +++++++++++-------------------------
> > > >  18 files changed, 340 insertions(+), 84 deletions(-)
> > > >   
> > >   
> > 
> > 
> > 
> 



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