Re: [Qemu-devel] [PATCH v3 0/2] Fix migration of old pseries

[Greg Kurz]

On Tue, 23 Feb 2016 11:01:00 +0100
Greg Kurz <address@hidden> wrote:

> On Tue, 23 Feb 2016 09:31:10 +0100
> Juan Quintela <address@hidden> wrote:
> > Greg Kurz <address@hidden> wrote:  
> > > On Fri, 19 Feb 2016 11:11:47 +1100
> > > David Gibson <address@hidden> wrote:
> > >    
> > >> On Thu, Feb 18, 2016 at 12:32:11PM +0100, Greg Kurz wrote:    
> > >> > QEMU 2.4 broke the migration of old pseries machine with the addition
> > >> > of configuration sections, which are sent unconditionally.
> > >> > 
> > >> > We assume that QEMU 2.3 is more deployed than any newer release (based 
> > >> > on
> > >> > the versions currently shipped by most distros). This v3 series hence
> > >> > reverses the logic from v2: it now fully fixes migration of old pseries
> > >> > from/to QEMU 2.3 and provides a manual workaround for the QEMU 
> > >> > 2.4/2.4.1/2.5
> > >> > case.
> > >> > 
> > >> > With this series, I could migrate the same pseries-2.3 instance in a 
> > >> > full
> > >> > 2.3->2.6->2.5->2.6->2.4->2.6->2.3 cycle.      
> > >> 
> > >> Sorry, I've lost track slightly here.  Does this series apply on top
> > >> of, or instead of your earlier series that peeks for the config
> > >> section?
> > >>     
> > >
> > > This v3 series applies instead of the v2 that peeks for the config 
> > > section.
> > >
> > > It was suggested by Laurent during review, and motivated by your decision
> > > to favor fixing 2.3 over 2.4.
> > >
> > > As shown in Laurent's detailed test report, migration from/to 2.3.x now 
> > > works
> > > out of the box and 2.4.x/2.5 requires qom-set.
> > >
> > > I was also feeling a bit uncomfortable with all these machine properties 
> > > to
> > > disable the configuration section, which was explicitly coded to be 
> > > non-optional
> > > according to the changelog of commit 61964c23. The logic inversion in v3 
> > > seem
> > > to be friendlier with the configuration section design.
> > >
> > > Juan, could you share your thoughts ?    
> > 
> > As said previously, we don't have any better fix.  We could try to make
> > the "enforce" thing only for some machine types, but I guess that it
> > would make the mess even bigger :-(
> > 
> > And on an unrelated topic, I guess we need to "agree" in a way to add
> > new features to migration for all machine types.  Right now we have:
> > - two x86 boards
> > - now s390
> > - and now ppc
> > - and I guess arm is on the way
> > 
> > So, I guess we would need something like of what we have on
> > hw/i386/pc_piix.c
> > 
> > 
> > static void pc_compat_2_3(MachineState *machine)
> > {
> >     PCMachineState *pcms = PC_MACHINE(machine);
> >     savevm_skip_section_footers();
> >     if (kvm_enabled()) {
> >         pcms->smm = ON_OFF_AUTO_OFF;
> >     }
> >     global_state_set_optional();
> >     savevm_skip_configuration();
> > }
> > 
> > 
> > Really, we need something like this
> > 
> > static void migration_compat_2_3(MachineState *machine)
> > {
> >     savevm_skip_section_footers();
> >     global_state_set_optional();
> >     savevm_skip_configuration();
> > }
> >   
> 
> Indeed, there is code duplication here... and it is not the first time:
> pseries is also missing a compat property for the vmdesc section. In fact,
> people change migration and often fix x86 only in a followup patch.
> 
> > to be included in every machine that cares about migration.  Is there
> > ane easy way/place where to do this globaly?
> >   
> 
> There has been a first round of compat stuff refactoring with
> commit 68a27b208a, which introduced include/hw/compat.h.
> 
> Maybe we can declare the migration_compat_* helpers in this header
> and put the code in a new hw/core/compat.c file ? Or add this to
> hw/core/machine.c ?
> 
> I'll give a try and send a series.
> 

Hmm... looking at the pc code, I've stumbled accross this commit:

commit 798595075bf51c7e3125d260a19d860b9aa63e69
Author: Eduardo Habkost <address@hidden>
Date:   Mon Sep 28 15:07:21 2015 -0300

pc: Add a comment explaining why pc_compat_2_4() doesn't exist

which explains that these compat helpers are deprecated for new pc machine
types... but I assume (maybe wrongly?) that it goes the same for all
machine types. And indeed, I could not find recent pc, pseries or s390-ccw 
machine
using this construct.

In the end, I'm not sure it is worth refactoring the code for older
machines if new machines are expected to handle compat issues differently.

> > Later, Juan.
> >   
> 
> Thanks !
> 
> --
> Greg
> 
>