ppc/xics: fix XICSStateClass parent class

[Qemu-devel] [PATCH] ppc/xics: fix XICSStateClass parent class

Posted by Cédric Le Goater 7 years, 2 months ago

XICSState inherits from SysBusDevice and so the object class should
inherit from SysBusDeviceClass.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xics.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 3f0c31610aa4..e8794aa5cba8 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -71,7 +71,7 @@ typedef struct ICSState ICSState;
 typedef struct ICSIRQState ICSIRQState;
 
 struct XICSStateClass {
-    DeviceClass parent_class;
+    SysBusDeviceClass parent_class;
 
     void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
     void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] ppc/xics: fix XICSStateClass parent class

Posted by David Gibson 7 years, 2 months ago

On Mon, Feb 13, 2017 at 10:33:24AM +0100, Cédric Le Goater wrote:
> XICSState inherits from SysBusDevice and so the object class should
> inherit from SysBusDeviceClass.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

That's definitely a bug, but I don't think this is the right fix.

XICS was put on sysbus back when I thought that was where you put any
"system" devices that didn't belong on a more specific bus.  I've
since discovered that's not the case, and - particularly since xics
doesn't expose any mmio - it shouldn't be a sysbus device at all.

I've instead made a patch changing it to a direct child of
TYPE_DEVICE.  Since the device shouldn't ever be command line
instantiated, and it has no migratable state I believe this should be
a safe change.

This is in ppc-for-2.9, tentatively; please review:

From eae21fcf2430588f17e6a58d505aa8eaa580b507 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Tue, 14 Feb 2017 12:58:05 +1100
Subject: [PATCH] xics: XICS should not be a SysBusDevice

Currently xics - the component of the IBM POWER interrupt controller
representing the overall interrupt fabric / architecture is represented as
a descedent of SysBusDevice.  However, this is not really correct - the
xics presents nothing in MMIO space so it should be an "unattached" device
in the current QOM model.

Since this device will always be created by the machine type, not created
specifically from the command line, and because it has no migrated state
it should be safe to move it around the device composition tree.

Therefore this patch changes it to a descendent of TYPE_DEVICE, and makes
it an unattached device.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics.c | 2 +-
 hw/ppc/spapr.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 095c16a..372b831 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -260,7 +260,7 @@ static void xics_common_class_init(ObjectClass *oc, void *data)

 static const TypeInfo xics_common_info = {
     .name          = TYPE_XICS_COMMON,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_DEVICE,
     .instance_size = sizeof(XICSState),
     .class_size    = sizeof(XICSStateClass),
     .instance_init = xics_common_initfn,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6f37288..9b6ad0f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -101,7 +101,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
     Error *err = NULL;
     DeviceState *dev;

-    dev = qdev_create(NULL, type);
+    dev = DEVICE(object_new(type));
     qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
     qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
-- 
2.9.3

-- 
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] ppc/xics: fix XICSStateClass parent class

Posted by Cédric Le Goater 7 years, 2 months ago

On 02/14/2017 02:58 AM, David Gibson wrote:
> On Mon, Feb 13, 2017 at 10:33:24AM +0100, Cédric Le Goater wrote:
>> XICSState inherits from SysBusDevice and so the object class should
>> inherit from SysBusDeviceClass.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> That's definitely a bug, but I don't think this is the right fix.
> 
> XICS was put on sysbus back when I thought that was where you put any
> "system" devices that didn't belong on a more specific bus.  I've
> since discovered that's not the case, and - particularly since xics
> doesn't expose any mmio - it shouldn't be a sysbus device at all.
> 
> I've instead made a patch changing it to a direct child of
> TYPE_DEVICE.  Since the device shouldn't ever be command line
> instantiated, and it has no migratable state I believe this should be
> a safe change.
> 
> This is in ppc-for-2.9, tentatively; please review:
> 
> From eae21fcf2430588f17e6a58d505aa8eaa580b507 Mon Sep 17 00:00:00 2001
> From: David Gibson <david@gibson.dropbear.id.au>
> Date: Tue, 14 Feb 2017 12:58:05 +1100
> Subject: [PATCH] xics: XICS should not be a SysBusDevice
> 
> Currently xics - the component of the IBM POWER interrupt controller
> representing the overall interrupt fabric / architecture is represented as
> a descedent of SysBusDevice.  However, this is not really correct - the

descendant ?

> xics presents nothing in MMIO space so it should be an "unattached" device
> in the current QOM model.
> 
> Since this device will always be created by the machine type, not created
> specifically from the command line, and because it has no migrated state
> it should be safe to move it around the device composition tree.
> 
> Therefore this patch changes it to a descendent of TYPE_DEVICE, and makes
> it an unattached device.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

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

Thanks,

C.

> ---
>  hw/intc/xics.c | 2 +-
>  hw/ppc/spapr.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 095c16a..372b831 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -260,7 +260,7 @@ static void xics_common_class_init(ObjectClass *oc, void *data)
>  
>  static const TypeInfo xics_common_info = {
>      .name          = TYPE_XICS_COMMON,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_DEVICE,
>      .instance_size = sizeof(XICSState),
>      .class_size    = sizeof(XICSStateClass),
>      .instance_init = xics_common_initfn,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6f37288..9b6ad0f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -101,7 +101,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>      Error *err = NULL;
>      DeviceState *dev;
>  
> -    dev = qdev_create(NULL, type);
> +    dev = DEVICE(object_new(type));
>      qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
>      qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>

Re: [Qemu-devel] [PATCH] ppc/xics: fix XICSStateClass parent class

Posted by David Gibson 7 years, 2 months ago

On Tue, Feb 14, 2017 at 07:25:07AM +0100, Cédric Le Goater wrote:
> On 02/14/2017 02:58 AM, David Gibson wrote:
> > On Mon, Feb 13, 2017 at 10:33:24AM +0100, Cédric Le Goater wrote:
> >> XICSState inherits from SysBusDevice and so the object class should
> >> inherit from SysBusDeviceClass.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > 
> > That's definitely a bug, but I don't think this is the right fix.
> > 
> > XICS was put on sysbus back when I thought that was where you put any
> > "system" devices that didn't belong on a more specific bus.  I've
> > since discovered that's not the case, and - particularly since xics
> > doesn't expose any mmio - it shouldn't be a sysbus device at all.
> > 
> > I've instead made a patch changing it to a direct child of
> > TYPE_DEVICE.  Since the device shouldn't ever be command line
> > instantiated, and it has no migratable state I believe this should be
> > a safe change.
> > 
> > This is in ppc-for-2.9, tentatively; please review:
> > 
> > From eae21fcf2430588f17e6a58d505aa8eaa580b507 Mon Sep 17 00:00:00 2001
> > From: David Gibson <david@gibson.dropbear.id.au>
> > Date: Tue, 14 Feb 2017 12:58:05 +1100
> > Subject: [PATCH] xics: XICS should not be a SysBusDevice
> > 
> > Currently xics - the component of the IBM POWER interrupt controller
> > representing the overall interrupt fabric / architecture is represented as
> > a descedent of SysBusDevice.  However, this is not really correct - the
> 
> descendant ?

descendent, actually, we were both wrong.

> 
> > xics presents nothing in MMIO space so it should be an "unattached" device
> > in the current QOM model.
> > 
> > Since this device will always be created by the machine type, not created
> > specifically from the command line, and because it has no migrated state
> > it should be safe to move it around the device composition tree.
> > 
> > Therefore this patch changes it to a descendent of TYPE_DEVICE, and makes
> > it an unattached device.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> > ---
> >  hw/intc/xics.c | 2 +-
> >  hw/ppc/spapr.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 095c16a..372b831 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -260,7 +260,7 @@ static void xics_common_class_init(ObjectClass *oc, void *data)
> >  
> >  static const TypeInfo xics_common_info = {
> >      .name          = TYPE_XICS_COMMON,
> > -    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .parent        = TYPE_DEVICE,
> >      .instance_size = sizeof(XICSState),
> >      .class_size    = sizeof(XICSStateClass),
> >      .instance_init = xics_common_initfn,
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6f37288..9b6ad0f 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -101,7 +101,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
> >      Error *err = NULL;
> >      DeviceState *dev;
> >  
> > -    dev = qdev_create(NULL, type);
> > +    dev = DEVICE(object_new(type));
> >      qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
> >      qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
> >      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> > 
> 

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