Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

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

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

On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:
> On Mon, 12 Jun 2017 22:24:56 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 08, 2017 at 11:54:10AM +0200, Greg Kurz wrote:
> > > On Thu, 8 Jun 2017 14:08:57 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:  
> > > > > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > > > > sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> > > > > This is an improvement since we no longer allocate ICPState objects
> > > > > that will never be used. But it has the side-effect of breaking
> > > > > migration of older machine types from older QEMU versions.
> > > > > 
> > > > > This patch allows spapr to register dummy "icp/server" entries to vmstate.
> > > > > These entries use a dedicated VMStateDescription that can swallow and
> > > > > discard state of an incoming migration stream, and that don't send anything
> > > > > on outgoing migration.
> > > > > 
> > > > > As for real ICPState objects, the instance_id is the cpu_index of the
> > > > > corresponding vCPU, which happens to be equal to the generated instance_id
> > > > > of older machine types.
> > > > > 
> > > > > The machine can unregister/register these entries when CPUs are dynamically
> > > > > plugged/unplugged.
> > > > > 
> > > > > This is only available for pseries-2.9 and older machines, thanks to a
> > > > > compat property.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > > v3: - new logic entirely implemented in hw/ppc/spapr.c
> > > > > ---
> > > > >  hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  include/hw/ppc/spapr.h |    2 +
> > > > >  2 files changed, 88 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 9b7ae28939a8..c15b604978f0 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -124,9 +124,52 @@ error:
> > > > >      return NULL;
> > > > >  }
> > > > >  
> > > > > +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > > > > +{
> > > > > +    return false;
> > > > > +}    
> > > > 
> > > > Uh.. the needed function always returns false, how can that work?
> > > >   
> > > 
> > > The needed function is used for outgoing migration only:
> > > 
> > > bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> > > {
> > >     if (vmsd->needed && !vmsd->needed(opaque)) {
> > >         /* optional section not needed */
> > >         return false;
> > >     }
> > >     return true;
> > > }
> > > 
> > > The idea is that all ICPState objects that were created but not associated
> > > to a vCPU by pre-2.10 machine types don't need to be migrated at all, as
> > > their state hasn't changed.
> > > 
> > > We don't even create these unneeded ICPState objects here, but simply
> > > register their ids to vmstate.
> > >   
> > > > > +
> > > > > +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > > > +    .name = "icp/server",
> > > > > +    .version_id = 1,
> > > > > +    .minimum_version_id = 1,
> > > > > +    .needed = pre_2_10_vmstate_dummy_icp_needed,  
> > > 
> > > Outgoing migration:
> > > - machine in older QEMU have unused ICPState objects (default state)
> > > - machine in QEMU 2.10 doesn't even have extra ICPState objects
> > >   
> > > => don't send anything  
> > >   
> > > > > +    .fields = (VMStateField[]) {
> > > > > +        VMSTATE_UNUSED(4), /* uint32_t xirr */
> > > > > +        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> > > > > +        VMSTATE_UNUSED(1), /* uint8_t mfrr */  
> > > 
> > > Incoming migration from older QEMU: we don't have the extra ICPState objects.
> > >   
> > > => accept the state and discard it  
> > >   
> > > > > +        VMSTATE_END_OF_LIST()
> > > > > +    },
> > > > > +};
> > > > > +
> > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > +{
> > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > +
> > > > > +    g_assert(!*flag);    
> > > > 
> > > > Apart from this assert(), you never seem to test the values in the
> > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > >   
> > > 
> > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > But I agree it isn't really useful... but more because of paranoia :)  
> > 
> > I'm all for paranoid assert()s if they can be made using data readily
> > to hand.  Adding a data structure just for the purpose of making an
> > assert() later, not so much.
> > 
> 
> It is also passed as opaque argument to vmstate_register(), where it is
> used as a key when calling vmstate_unregister(). I could possibly pass
> (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> to pass numbers.

Ah, I see your point.  Creating an array, purely to generate arbitrary
pointers is also kind of ugly, though.  Really the cpu_index / XICS
server number makes sense to identify the vmstate, but it looks like
vmstate_unregister() doesn't take that.

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