| Message ID | 20181119120820.29878-23-maozhongyi@cmss.chinamobile.com |
|---|---|
| State | New |
| Headers | show |
| Series | QOM'ify SysBusDeviceClass->init | expand |
On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote: > Currently, all sysbus devices have been converted to realize(), > so remove this path. > > Cc: ehabkost@redhat.com > Cc: thuth@redhat.com > Cc: pbonzini@redhat.com > Cc: armbru@redhat.com > Cc: peter.maydell@linaro.org > Cc: richard.henderson@linaro.org > Cc: alistair.francis@wdc.com > > Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > --- > hw/core/sysbus.c | 15 --------------- > include/hw/sysbus.h | 3 --- > 2 files changed, 18 deletions(-) > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c > index 7ac36ad3e7..030ad426c1 100644 > --- a/hw/core/sysbus.c > +++ b/hw/core/sysbus.c > @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size) > } > } > > -/* TODO remove once all sysbus devices have been converted to realize */ > -static void sysbus_realize(DeviceState *dev, Error **errp) > -{ > - SysBusDevice *sd = SYS_BUS_DEVICE(dev); > - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); > - > - if (!sbc->init) { > - return; > - } > - if (sbc->init(sd) < 0) { > - error_setg(errp, "Device initialization failed"); > - } > -} Nice. :) > - > DeviceState *sysbus_create_varargs(const char *name, > hwaddr addr, ...) > { > @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev) > static void sysbus_device_class_init(ObjectClass *klass, void *data) > { > DeviceClass *k = DEVICE_CLASS(klass); > - k->realize = sysbus_realize; Have you ensured this won't break any subclasses that saved the original realize function on a parent_realize field? Now they will have parent_realize set to NULL. Most of them use device_class_set_parent_realize() to implement that. > k->bus_type = TYPE_SYSTEM_BUS; > /* > * device_add plugs devices into a suitable bus. For "real" buses, > diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h > index 0b59a3b8d6..1aedcf05c9 100644 > --- a/include/hw/sysbus.h > +++ b/include/hw/sysbus.h > @@ -38,9 +38,6 @@ typedef struct SysBusDevice SysBusDevice; > typedef struct SysBusDeviceClass { > /*< private >*/ > DeviceClass parent_class; > - /*< public >*/ > - > - int (*init)(SysBusDevice *dev); > > /* > * Let the sysbus device format its own non-PIO, non-MMIO unit address. > -- > 2.17.1 > > >
On Mon, Nov 19, 2018 at 09:31:39PM -0200, Eduardo Habkost wrote: > On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote: > > Currently, all sysbus devices have been converted to realize(), > > so remove this path. > > > > Cc: ehabkost@redhat.com > > Cc: thuth@redhat.com > > Cc: pbonzini@redhat.com > > Cc: armbru@redhat.com > > Cc: peter.maydell@linaro.org > > Cc: richard.henderson@linaro.org > > Cc: alistair.francis@wdc.com > > > > Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > > --- > > hw/core/sysbus.c | 15 --------------- > > include/hw/sysbus.h | 3 --- > > 2 files changed, 18 deletions(-) > > > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c > > index 7ac36ad3e7..030ad426c1 100644 > > --- a/hw/core/sysbus.c > > +++ b/hw/core/sysbus.c > > @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size) > > } > > } > > > > -/* TODO remove once all sysbus devices have been converted to realize */ > > -static void sysbus_realize(DeviceState *dev, Error **errp) > > -{ > > - SysBusDevice *sd = SYS_BUS_DEVICE(dev); > > - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); > > - > > - if (!sbc->init) { > > - return; > > - } > > - if (sbc->init(sd) < 0) { > > - error_setg(errp, "Device initialization failed"); > > - } > > -} > > Nice. :) > > > > - > > DeviceState *sysbus_create_varargs(const char *name, > > hwaddr addr, ...) > > { > > @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev) > > static void sysbus_device_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *k = DEVICE_CLASS(klass); > > - k->realize = sysbus_realize; > > Have you ensured this won't break any subclasses that > saved the original realize function on a parent_realize field? > Now they will have parent_realize set to NULL. > > Most of them use device_class_set_parent_realize() to implement > that. Found one: $ ./aarch64-softmmu/qemu-system-aarch64 -machine virt,iommu=smmuv3 Segmentation fault (core dumped) (gdb) bt #0 0x0000000000000000 in () #1 0x000055555596eabe in smmu_base_realize (dev=0x555556ac0cb0, errp=0x7fffffffc450) at /home/ehabkost/rh/proj/virt/qemu/hw/arm/smmu-common.c:428 #2 0x0000555555970322 in smmu_realize (d=0x555556ac0cb0, errp=0x7fffffffc4c0) at /home/ehabkost/rh/proj/virt/qemu/hw/arm/smmuv3.c:1390 #3 0x0000555555ac8f00 in device_set_realized (obj=<optimized out>, value=<optimized out>, errp=0x7fffffffc5b0) at hw/core/qdev.c:826 #4 0x0000555555c424a7 in property_set_bool (obj=0x555556ac0cb0, v=<optimized out>, name=<optimized out>, opaque=0x555556a05b70, errp=0x7fffffffc5b0) at qom/object.c:1991 #5 0x0000555555c468df in object_property_set_qobject (obj=obj@entry=0x555556ac0cb0, value=value@entry=0x555556ac2520, name=name@entry=0x555555de03f9 "realized", errp=errp@entry=0x7fffffffc5b0) at qom/qom-qobject.c:27 #6 0x0000555555c44355 in object_property_set_bool (obj=0x555556ac0cb0, value=<optimized out>, name=0x555555de03f9 "realized", errp=0x7fffffffc5b0) at qom/object.c:1249 #7 0x0000555555ac7e22 in qdev_init_nofail (dev=dev@entry=0x555556ac0cb0) at hw/core/qdev.c:313 #8 0x000055555592ef97 in create_smmu (bus=0x5555569970a0, pic=0x7fffffffc900, vms=<optimized out>) at /home/ehabkost/rh/proj/virt/qemu/hw/arm/virt.c:1058 #9 0x000055555592ef97 in create_pcie (pic=0x7fffffffc900, vms=<optimized out>) at /home/ehabkost/rh/proj/virt/qemu/hw/arm/virt.c:1208 #10 0x000055555592ef97 in machvirt_init (machine=0x55555663e9c0) at /home/ehabkost/rh/proj/virt/qemu/hw/arm/virt.c:1549 #11 0x0000555555acf013 in machine_run_board_init (machine=0x55555663e9c0) at hw/core/machine.c:834 #12 0x000055555581dab8 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4518 > > > k->bus_type = TYPE_SYSTEM_BUS; > > /* > > * device_add plugs devices into a suitable bus. For "real" buses, > > diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h > > index 0b59a3b8d6..1aedcf05c9 100644 > > --- a/include/hw/sysbus.h > > +++ b/include/hw/sysbus.h > > @@ -38,9 +38,6 @@ typedef struct SysBusDevice SysBusDevice; > > typedef struct SysBusDeviceClass { > > /*< private >*/ > > DeviceClass parent_class; > > - /*< public >*/ > > - > > - int (*init)(SysBusDevice *dev); > > > > /* > > * Let the sysbus device format its own non-PIO, non-MMIO unit address. > > -- > > 2.17.1 > > > > > > > > -- > Eduardo
Hi, Eduardo On 11/20/18 7:31 AM, Eduardo Habkost wrote: > On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote: >> Currently, all sysbus devices have been converted to realize(), >> so remove this path. >> >> Cc: ehabkost@redhat.com >> Cc: thuth@redhat.com >> Cc: pbonzini@redhat.com >> Cc: armbru@redhat.com >> Cc: peter.maydell@linaro.org >> Cc: richard.henderson@linaro.org >> Cc: alistair.francis@wdc.com >> >> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> --- >> hw/core/sysbus.c | 15 --------------- >> include/hw/sysbus.h | 3 --- >> 2 files changed, 18 deletions(-) >> >> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c >> index 7ac36ad3e7..030ad426c1 100644 >> --- a/hw/core/sysbus.c >> +++ b/hw/core/sysbus.c >> @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size) >> } >> } >> >> -/* TODO remove once all sysbus devices have been converted to realize */ >> -static void sysbus_realize(DeviceState *dev, Error **errp) >> -{ >> - SysBusDevice *sd = SYS_BUS_DEVICE(dev); >> - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); >> - >> - if (!sbc->init) { >> - return; >> - } >> - if (sbc->init(sd) < 0) { >> - error_setg(errp, "Device initialization failed"); >> - } >> -} > > Nice. :) > > >> - >> DeviceState *sysbus_create_varargs(const char *name, >> hwaddr addr, ...) >> { >> @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev) >> static void sysbus_device_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *k = DEVICE_CLASS(klass); >> - k->realize = sysbus_realize; > > Have you ensured this won't break any subclasses that > saved the original realize function on a parent_realize field? Thanks for the catch. > Now they will have parent_realize set to NULL. In order to void the subclasses whose parent_realize field is set to NULL, the k->realize function must be retained even though it doesn't do anything practical. Just like this: -/* TODO remove once all sysbus devices have been converted to realize*/ static void sysbus_realize(DeviceState *dev, Error **errp) { - SysBusDevice *sd = SYS_BUS_DEVICE(dev); - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); - - if (!sbc->init) { - return; - } - if (sbc->init(sd) < 0) { - error_setg(errp, "Device initialization failed"); - } } it doesn't look elegant, but I didn't think of a better way, if you can give me some hints, I really appreciate it. :) Thanks, Mao > > Most of them use device_class_set_parent_realize() to implement > that. > >> k->bus_type = TYPE_SYSTEM_BUS; >> /* >> * device_add plugs devices into a suitable bus. For "real" buses, >> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h >> index 0b59a3b8d6..1aedcf05c9 100644 >> --- a/include/hw/sysbus.h >> +++ b/include/hw/sysbus.h >> @@ -38,9 +38,6 @@ typedef struct SysBusDevice SysBusDevice; >> typedef struct SysBusDeviceClass { >> /*< private >*/ >> DeviceClass parent_class; >> - /*< public >*/ >> - >> - int (*init)(SysBusDevice *dev); >> >> /* >> * Let the sysbus device format its own non-PIO, non-MMIO unit address. >> -- >> 2.17.1 >> >> >> >
On 23 November 2018 at 03:10, maozy <maozhongyi@cmss.chinamobile.com> wrote: > In order to void the subclasses whose parent_realize field is > set to NULL, the k->realize function must be retained even > though it doesn't do anything practical. Just like this: > > > -/* TODO remove once all sysbus devices have been converted to realize*/ > static void sysbus_realize(DeviceState *dev, Error **errp) > { > - SysBusDevice *sd = SYS_BUS_DEVICE(dev); > - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); > - > - if (!sbc->init) { > - return; > - } > - if (sbc->init(sd) < 0) { > - error_setg(errp, "Device initialization failed"); > - } > } > > it doesn't look elegant, but I didn't think of a better way, if you > can give me some hints, I really appreciate it. :) If we do take this approach, we should have a comment which says why we have an empty realize function, so that we don't in future forget and delete the apparently unnecessary code... thanks -- PMM
On 11/23/18 5:02 PM, Peter Maydell wrote: > On 23 November 2018 at 03:10, maozy <maozhongyi@cmss.chinamobile.com> wrote: >> In order to void the subclasses whose parent_realize field is >> set to NULL, the k->realize function must be retained even >> though it doesn't do anything practical. Just like this: >> >> >> -/* TODO remove once all sysbus devices have been converted to realize*/ >> static void sysbus_realize(DeviceState *dev, Error **errp) >> { >> - SysBusDevice *sd = SYS_BUS_DEVICE(dev); >> - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); >> - >> - if (!sbc->init) { >> - return; >> - } >> - if (sbc->init(sd) < 0) { >> - error_setg(errp, "Device initialization failed"); >> - } >> } >> >> it doesn't look elegant, but I didn't think of a better way, if you >> can give me some hints, I really appreciate it. :) > > If we do take this approach, we should have a comment which > says why we have an empty realize function, so that we don't > in future forget and delete the apparently unnecessary code... > OK, I got you, will do it. Thanks, Mao > thanks > -- PMM >
On Fri, Nov 23, 2018 at 11:10:40AM +0800, maozy wrote: > Hi, Eduardo > > On 11/20/18 7:31 AM, Eduardo Habkost wrote: > > On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote: > > > Currently, all sysbus devices have been converted to realize(), > > > so remove this path. > > > > > > Cc: ehabkost@redhat.com > > > Cc: thuth@redhat.com > > > Cc: pbonzini@redhat.com > > > Cc: armbru@redhat.com > > > Cc: peter.maydell@linaro.org > > > Cc: richard.henderson@linaro.org > > > Cc: alistair.francis@wdc.com > > > > > > Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> > > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > > > --- > > > hw/core/sysbus.c | 15 --------------- > > > include/hw/sysbus.h | 3 --- > > > 2 files changed, 18 deletions(-) > > > > > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c > > > index 7ac36ad3e7..030ad426c1 100644 > > > --- a/hw/core/sysbus.c > > > +++ b/hw/core/sysbus.c > > > @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size) > > > } > > > } > > > -/* TODO remove once all sysbus devices have been converted to realize */ > > > -static void sysbus_realize(DeviceState *dev, Error **errp) > > > -{ > > > - SysBusDevice *sd = SYS_BUS_DEVICE(dev); > > > - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); > > > - > > > - if (!sbc->init) { > > > - return; > > > - } > > > - if (sbc->init(sd) < 0) { > > > - error_setg(errp, "Device initialization failed"); > > > - } > > > -} > > > > Nice. :) > > > > > > > - > > > DeviceState *sysbus_create_varargs(const char *name, > > > hwaddr addr, ...) > > > { > > > @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev) > > > static void sysbus_device_class_init(ObjectClass *klass, void *data) > > > { > > > DeviceClass *k = DEVICE_CLASS(klass); > > > - k->realize = sysbus_realize; > > > > Have you ensured this won't break any subclasses that > > saved the original realize function on a parent_realize field? > > Thanks for the catch. > > > Now they will have parent_realize set to NULL. > > In order to void the subclasses whose parent_realize field is > set to NULL, the k->realize function must be retained even > though it doesn't do anything practical. Just like this: > > > -/* TODO remove once all sysbus devices have been converted to realize*/ > static void sysbus_realize(DeviceState *dev, Error **errp) > { > - SysBusDevice *sd = SYS_BUS_DEVICE(dev); > - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); > - > - if (!sbc->init) { > - return; > - } > - if (sbc->init(sd) < 0) { > - error_setg(errp, "Device initialization failed"); > - } > } > > it doesn't look elegant, but I didn't think of a better way, if you > can give me some hints, I really appreciate it. :) I think this is good enough for now (as long as there's a comment like Peter suggested). Allowing parent_realize to be NULL would be inconvenient to all code that uses parent_realize today. Personally, I would love to get rid of parent_realize entirely. We could simply provide a helper to let device subclasses call the parent's realize function without the need to copy function pointers around.
On Fri, 23 Nov 2018 at 18:16, Eduardo Habkost <ehabkost@redhat.com> wrote: > I think this is good enough for now (as long as there's a comment > like Peter suggested). Allowing parent_realize to be NULL would > be inconvenient to all code that uses parent_realize today. > > Personally, I would love to get rid of parent_realize entirely. > We could simply provide a helper to let device subclasses call > the parent's realize function without the need to copy function > pointers around. Agreed -- parent_realize is a hack that is working around a deficiency in our object model, and it would be nice to deal with that. But let's do our cleanups one at a time :-) thanks -- PMM
On 11/24/18 2:19 AM, Peter Maydell wrote: > On Fri, 23 Nov 2018 at 18:16, Eduardo Habkost <ehabkost@redhat.com> wrote: >> I think this is good enough for now (as long as there's a comment >> like Peter suggested). Allowing parent_realize to be NULL would >> be inconvenient to all code that uses parent_realize today. It was done in v2, please review. >> >> Personally, I would love to get rid of parent_realize entirely. >> We could simply provide a helper to let device subclasses call >> the parent's realize function without the need to copy function >> pointers around. well, I will do it later. > > Agreed -- parent_realize is a hack that is working around > a deficiency in our object model, and it would be nice to > deal with that. But let's do our cleanups one at a time :-) OK, I see. Thanks, Mao > > thanks > -- PMM >
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 7ac36ad3e7..030ad426c1 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size) } } -/* TODO remove once all sysbus devices have been converted to realize */ -static void sysbus_realize(DeviceState *dev, Error **errp) -{ - SysBusDevice *sd = SYS_BUS_DEVICE(dev); - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd); - - if (!sbc->init) { - return; - } - if (sbc->init(sd) < 0) { - error_setg(errp, "Device initialization failed"); - } -} - DeviceState *sysbus_create_varargs(const char *name, hwaddr addr, ...) { @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev) static void sysbus_device_class_init(ObjectClass *klass, void *data) { DeviceClass *k = DEVICE_CLASS(klass); - k->realize = sysbus_realize; k->bus_type = TYPE_SYSTEM_BUS; /* * device_add plugs devices into a suitable bus. For "real" buses, diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h index 0b59a3b8d6..1aedcf05c9 100644 --- a/include/hw/sysbus.h +++ b/include/hw/sysbus.h @@ -38,9 +38,6 @@ typedef struct SysBusDevice SysBusDevice; typedef struct SysBusDeviceClass { /*< private >*/ DeviceClass parent_class; - /*< public >*/ - - int (*init)(SysBusDevice *dev); /* * Let the sysbus device format its own non-PIO, non-MMIO unit address.