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

[Greg Kurz]

On Tue, 13 Jun 2017 10:00:03 +0100
"Dr. David Alan Gilbert" <address@hidden> wrote:

> * Greg Kurz (address@hidden) wrote:
> > On Tue, 13 Jun 2017 16:06:31 +0800
> > David Gibson <address@hidden> wrote:
> >   
> > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:  
> > [...]  
> > > > > > > > +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.
> > >   
> > 
> > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > 
> > Cc'ing Juan and David.  
> 
> So what's the problem with a (void *)i ?

https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa

> It's simple, as long as you're
> not actually using the opaque anywhere it's easy.
> 

but as you say, since the opaque isn't used anywhere, it is probably
okay to pass (void *) i.


> Note from a quick glance at your patch;  will this work migrating
> from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
> the 2.9 ?
> 

Yeah but I need to add some comments as David suggested.

The idea is that 2.9 used to create a bunch of objects at machine init,
that only get used when CPUs are plugged. With 2.10, these objects are
now created under the CPUs. When migrating from 2.10 to 2.9, we only need
to send the real objects. The dummy vmstate entries don't send anything
(.needed always returns false) since the corresponding objects in 2.9 aren't
being used and still have their default state.

> Dave
> 
> 
> > --
> > Greg  
> 
> 
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK

pgpqi2PNoZtop.pgp
Description: OpenPGP digital signature