1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v3 0/5] spapr/xics: fix migration of older machine types
  4. View patch

[Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate

Greg Kurz posted 5 patches 8 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
Posted by Greg Kurz 8 years, 8 months ago 
The ICPState objects are currently registered to vmstate as qdev objects.
Their instance ids are hence computed automatically in the migration code,
and thus depends on the order the CPU cores were plugged.

If the destination had its CPU cores plugged in a different order than the
source, then ICPState objects will have different instance_ids and load
the wrong state.

Since CPU objects have a reliable cpu_index which is already used as
instance_id in vmstate, let's use it for ICPState as well.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 330441ff7fe8..3d76b43876c5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_register_reset(icp_reset, dev);
+    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
 
 static void icp_unrealize(DeviceState *dev, Error **errp)
 {
+    ICPState *icp = ICP(dev);
+
+    vmstate_unregister(NULL, &vmstate_icp_server, icp);
     qemu_unregister_reset(icp_reset, dev);
 }
 
@@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &vmstate_icp_server;
     dc->realize = icp_realize;
     dc->unrealize = icp_unrealize;
 }
Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
Posted by Cédric Le Goater 8 years, 8 months ago 
On 06/07/2017 07:17 PM, Greg Kurz wrote:
> The ICPState objects are currently registered to vmstate as qdev objects.
> Their instance ids are hence computed automatically in the migration code,
> and thus depends on the order the CPU cores were plugged.
> 
> If the destination had its CPU cores plugged in a different order than the
> source, then ICPState objects will have different instance_ids and load
> the wrong state.
> 
> Since CPU objects have a reliable cpu_index which is already used as
> instance_id in vmstate, let's use it for ICPState as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 330441ff7fe8..3d76b43876c5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_register_reset(icp_reset, dev);
> +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);

so, what I just said in the previous email regarding ->cs would break this 
patch. Forget about it. 


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

>  }
>  
>  static void icp_unrealize(DeviceState *dev, Error **errp)
>  {
> +    ICPState *icp = ICP(dev);
> +
> +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
>      qemu_unregister_reset(icp_reset, dev);
>  }
>  
> @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->vmsd = &vmstate_icp_server;
>      dc->realize = icp_realize;
>      dc->unrealize = icp_unrealize;
>  }
>
Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
Posted by Greg Kurz 8 years, 8 months ago 
On Wed, 7 Jun 2017 20:14:01 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > The ICPState objects are currently registered to vmstate as qdev objects.
> > Their instance ids are hence computed automatically in the migration code,
> > and thus depends on the order the CPU cores were plugged.
> > 
> > If the destination had its CPU cores plugged in a different order than the
> > source, then ICPState objects will have different instance_ids and load
> > the wrong state.
> > 
> > Since CPU objects have a reliable cpu_index which is already used as
> > instance_id in vmstate, let's use it for ICPState as well.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 330441ff7fe8..3d76b43876c5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      qemu_register_reset(icp_reset, dev);
> > +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);  
> 
> so, what I just said in the previous email regarding ->cs would break this 
> patch. Forget about it. 
> 

Unless we have an icp->cpu_index like I mentioned in the other mail.

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> >  }
> >  
> >  static void icp_unrealize(DeviceState *dev, Error **errp)
> >  {
> > +    ICPState *icp = ICP(dev);
> > +
> > +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
> >      qemu_unregister_reset(icp_reset, dev);
> >  }
> >  
> > @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> > -    dc->vmsd = &vmstate_icp_server;
> >      dc->realize = icp_realize;
> >      dc->unrealize = icp_unrealize;
> >  }
> >   
>
Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
Posted by David Gibson 8 years, 8 months ago 
On Wed, Jun 07, 2017 at 07:17:17PM +0200, Greg Kurz wrote:
> The ICPState objects are currently registered to vmstate as qdev objects.
> Their instance ids are hence computed automatically in the migration code,
> and thus depends on the order the CPU cores were plugged.
> 
> If the destination had its CPU cores plugged in a different order than the
> source, then ICPState objects will have different instance_ids and load
> the wrong state.
> 
> Since CPU objects have a reliable cpu_index which is already used as
> instance_id in vmstate, let's use it for ICPState as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

So, this is definitely a good idea w.r.t. future compatibility.  But,
won't it break migration compat as a once off, since the devices will
be found under their new id instead of the qdev id?

> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 330441ff7fe8..3d76b43876c5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_register_reset(icp_reset, dev);
> +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
>  static void icp_unrealize(DeviceState *dev, Error **errp)
>  {
> +    ICPState *icp = ICP(dev);
> +
> +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
>      qemu_unregister_reset(icp_reset, dev);
>  }
>  
> @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->vmsd = &vmstate_icp_server;
>      dc->realize = icp_realize;
>      dc->unrealize = icp_unrealize;
>  }
> 

-- 
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
Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
Posted by Greg Kurz 8 years, 8 months ago 
On Thu, 8 Jun 2017 13:59:21 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 07, 2017 at 07:17:17PM +0200, Greg Kurz wrote:
> > The ICPState objects are currently registered to vmstate as qdev objects.
> > Their instance ids are hence computed automatically in the migration code,
> > and thus depends on the order the CPU cores were plugged.
> > 
> > If the destination had its CPU cores plugged in a different order than the
> > source, then ICPState objects will have different instance_ids and load
> > the wrong state.
> > 
> > Since CPU objects have a reliable cpu_index which is already used as
> > instance_id in vmstate, let's use it for ICPState as well.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)  
> 
> So, this is definitely a good idea w.r.t. future compatibility.  But,
> won't it break migration compat as a once off, since the devices will
> be found under their new id instead of the qdev id?
> 

Older machine types used to allocate/realize all ICPState objects
at machine init time and never release them. The qdev ids are thus
0,1,2... nr_servers and happen to map to cpu_index.

> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 330441ff7fe8..3d76b43876c5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      qemu_register_reset(icp_reset, dev);
> > +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> >  }
> >  
> >  static void icp_unrealize(DeviceState *dev, Error **errp)
> >  {
> > +    ICPState *icp = ICP(dev);
> > +
> > +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
> >      qemu_unregister_reset(icp_reset, dev);
> >  }
> >  
> > @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> > -    dc->vmsd = &vmstate_icp_server;
> >      dc->realize = icp_realize;
> >      dc->unrealize = icp_unrealize;
> >  }
> >   
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.