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

[Greg Kurz]

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

> * Greg Kurz (address@hidden) wrote:
> > 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.  
> 
> Yes, I don't think we're ever casting back from the (void *) to an int
> so it feels pretty safe to me.
> 

Just one nit, gcc doesn't like it on 64-bit hosts:

hw/ppc/spapr.c: In function ‘pre_2_10_vmstate_register_dummy_icp’:
hw/ppc/spapr.c:148:60: error: cast to pointer from integer of different
 size [-Werror=int-to-pointer-cast]
     vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, (void *) i);

I need to cast to (long) first.

> >   
> > > 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.
> >   
> 
> OK, that'll probably work.
> 

Thanks for this confirmation.

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

pgpCzICgI7T7C.pgp
Description: OpenPGP digital signature