Re: [Qemu-devel] [PATCH v2 02/10] sysbus: Set cannot_instantiate_with_device_add_yet

[Marcel Apfelbaum]

On Tue, 2013-10-29 at 17:08 +0100, address@hidden wrote:
> From: Markus Armbruster <address@hidden>
> 
> device_add plugs devices into suitable bus.  For "real" buses, that
> actually connects the device.  For sysbus, the connections need to be
> made separately, and device_add can't do that.  The device would be
> left unconnected, and could not possibly work.
> 
> Quite a few, but not all sysbus devices already set
> cannot_instantiate_with_device_add_yet in their class init function.
> 
> Set it in their abstract base's class init function
> sysbus_device_class_init(), and remove the now redundant assignments
> from device class init functions.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

Seems OK to me.
Reviewed-by: Marcel Apfelbaum <address@hidden>

> ---
>  hw/alpha/typhoon.c         | 2 --
>  hw/arm/versatilepb.c       | 1 -
>  hw/audio/pl041.c           | 1 -
>  hw/core/sysbus.c           | 7 +++++++
>  hw/display/pl110.c         | 1 -
>  hw/dma/pl080.c             | 1 -
>  hw/i386/kvm/clock.c        | 1 -
>  hw/i386/kvmvapic.c         | 1 -
>  hw/intc/arm_gic.c          | 1 -
>  hw/intc/arm_gic_common.c   | 1 -
>  hw/intc/arm_gic_kvm.c      | 1 -
>  hw/intc/ioapic_common.c    | 1 -
>  hw/intc/pl190.c            | 1 -
>  hw/isa/isa-bus.c           | 1 -
>  hw/misc/arm_l2x0.c         | 1 -
>  hw/nvram/fw_cfg.c          | 1 -
>  hw/pci-host/bonito.c       | 2 --
>  hw/pci-host/grackle.c      | 2 --
>  hw/pci-host/piix.c         | 1 -
>  hw/pci-host/prep.c         | 1 -
>  hw/ppc/spapr_vio.c         | 2 --
>  hw/s390x/ipl.c             | 1 -
>  hw/s390x/s390-virtio-bus.c | 2 --
>  hw/s390x/virtio-ccw.c      | 2 --
>  hw/sd/pl181.c              | 1 -
>  hw/timer/arm_mptimer.c     | 1 -
>  hw/timer/hpet.c            | 1 -
>  hw/timer/pl031.c           | 1 -
>  28 files changed, 7 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
> index 60987ed..71a5a37 100644
> --- a/hw/alpha/typhoon.c
> +++ b/hw/alpha/typhoon.c
> @@ -934,11 +934,9 @@ static int typhoon_pcihost_init(SysBusDevice *dev)
>  
>  static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = typhoon_pcihost_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo typhoon_pcihost_info = {
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index bb0c0ba..aef2bde 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -390,7 +390,6 @@ static void vpb_sic_class_init(ObjectClass *klass, void 
> *data)
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = vpb_sic_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &vmstate_vpb_sic;
>  }
>  
> diff --git a/hw/audio/pl041.c b/hw/audio/pl041.c
> index 8ba661a..ed82be5 100644
> --- a/hw/audio/pl041.c
> +++ b/hw/audio/pl041.c
> @@ -632,7 +632,6 @@ static void pl041_device_class_init(ObjectClass *klass, 
> void *data)
>  
>      k->init = pl041_init;
>      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->reset = pl041_device_reset;
>      dc->vmsd = &vmstate_pl041;
>      dc->props = pl041_device_properties;
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index b84cd4a..6e880a8 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -257,6 +257,13 @@ static void sysbus_device_class_init(ObjectClass *klass, 
> void *data)
>      DeviceClass *k = DEVICE_CLASS(klass);
>      k->init = sysbus_device_init;
>      k->bus_type = TYPE_SYSTEM_BUS;
> +    /*
> +     * device_add plugs devices into suitable bus.  For "real" buses,
> +     * that actually connects the device.  For sysbus, the connections
> +     * need to be made separately, and device_add can't do that.  The
> +     * device would be left unconncected, and could not possibly work.
> +     */
> +    k->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo sysbus_device_type_info = {
> diff --git a/hw/display/pl110.c b/hw/display/pl110.c
> index 7ad5972..ab689e9 100644
> --- a/hw/display/pl110.c
> +++ b/hw/display/pl110.c
> @@ -496,7 +496,6 @@ static void pl110_class_init(ObjectClass *klass, void 
> *data)
>  
>      k->init = pl110_initfn;
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &vmstate_pl110;
>  }
>  
> diff --git a/hw/dma/pl080.c b/hw/dma/pl080.c
> index a515621..cb7bda9 100644
> --- a/hw/dma/pl080.c
> +++ b/hw/dma/pl080.c
> @@ -381,7 +381,6 @@ static void pl080_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &vmstate_pl080;
>  }
>  
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index abd2ce8..892aa02 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -114,7 +114,6 @@ static void kvmclock_class_init(ObjectClass *klass, void 
> *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = kvmclock_realize;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &kvmclock_vmsd;
>  }
>  
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index f1a0a9d..44ee62a 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -827,7 +827,6 @@ static void vapic_class_init(ObjectClass *klass, void 
> *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->reset   = vapic_reset;
>      dc->vmsd    = &vmstate_vapic;
>      dc->realize = vapic_realize;
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 24ad276..f13a927 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -704,7 +704,6 @@ static void arm_gic_class_init(ObjectClass *klass, void 
> *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      ARMGICClass *agc = ARM_GIC_CLASS(klass);
>  
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      agc->parent_realize = dc->realize;
>      dc->realize = arm_gic_realize;
>  }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 9047143..84aa7fc 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -156,7 +156,6 @@ static void arm_gic_common_class_init(ObjectClass *klass, 
> void *data)
>      dc->realize = arm_gic_common_realize;
>      dc->props = arm_gic_common_properties;
>      dc->vmsd = &vmstate_gic;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo arm_gic_common_type = {
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index a0bbf12..59a3da5 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -150,7 +150,6 @@ static void kvm_arm_gic_class_init(ObjectClass *klass, 
> void *data)
>      kgc->parent_reset = dc->reset;
>      dc->realize = kvm_arm_gic_realize;
>      dc->reset = kvm_arm_gic_reset;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo kvm_arm_gic_info = {
> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> index cc5a80d..9ba1a26 100644
> --- a/hw/intc/ioapic_common.c
> +++ b/hw/intc/ioapic_common.c
> @@ -98,7 +98,6 @@ static void ioapic_common_class_init(ObjectClass *klass, 
> void *data)
>  
>      dc->realize = ioapic_common_realize;
>      dc->vmsd = &vmstate_ioapic_common;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo ioapic_common_type = {
> diff --git a/hw/intc/pl190.c b/hw/intc/pl190.c
> index b16bc02..2bf359a 100644
> --- a/hw/intc/pl190.c
> +++ b/hw/intc/pl190.c
> @@ -273,7 +273,6 @@ static void pl190_class_init(ObjectClass *klass, void 
> *data)
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = pl190_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->reset = pl190_reset;
>      dc->vmsd = &vmstate_pl190;
>  }
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 6b2114d..55d0100 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -197,7 +197,6 @@ static void isabus_bridge_class_init(ObjectClass *klass, 
> void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->fw_name = "isa";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo isabus_bridge_info = {
> diff --git a/hw/misc/arm_l2x0.c b/hw/misc/arm_l2x0.c
> index ceea99d..9e220c9 100644
> --- a/hw/misc/arm_l2x0.c
> +++ b/hw/misc/arm_l2x0.c
> @@ -179,7 +179,6 @@ static void l2x0_class_init(ObjectClass *klass, void 
> *data)
>  
>      k->init = l2x0_priv_init;
>      dc->vmsd = &vmstate_l2x0;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->props = l2x0_properties;
>      dc->reset = l2x0_priv_reset;
>  }
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 553599f..9bcfde2 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -576,7 +576,6 @@ static void fw_cfg_class_init(ObjectClass *klass, void 
> *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = fw_cfg_realize;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->reset = fw_cfg_reset;
>      dc->vmsd = &vmstate_fw_cfg;
>      dc->props = fw_cfg_properties;
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index 2e08e9d..bfb9820 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -819,11 +819,9 @@ static const TypeInfo bonito_info = {
>  
>  static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = bonito_pcihost_initfn;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo bonito_pcihost_info = {
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index a2c5f56..c178375 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -143,10 +143,8 @@ static const TypeInfo grackle_pci_info = {
>  static void pci_grackle_class_init(ObjectClass *klass, void *data)
>  {
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init = pci_grackle_init_device;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo grackle_pci_host_info = {
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 697de65..21ffe97 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -730,7 +730,6 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
> void *data)
>      hc->root_bus_path = i440fx_pcihost_root_bus_path;
>      dc->realize = i440fx_pcihost_realize;
>      dc->fw_name = "pci";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->props = i440fx_props;
>  }
>  
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 58b8c5e..ebc40c6 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -215,7 +215,6 @@ static void raven_pcihost_class_init(ObjectClass *klass, 
> void *data)
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->realize = raven_pcihost_realizefn;
>      dc->fw_name = "pci";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo raven_pcihost_info = {
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 1e33819..9ac15b5 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -528,11 +528,9 @@ static int spapr_vio_bridge_init(SysBusDevice *dev)
>  
>  static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = spapr_vio_bridge_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo spapr_vio_bridge_info = {
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index f86a4af..cc29d8e 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -181,7 +181,6 @@ static void s390_ipl_class_init(ObjectClass *klass, void 
> *data)
>      k->init = s390_ipl_init;
>      dc->props = s390_ipl_properties;
>      dc->reset = s390_ipl_reset;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo s390_ipl_info = {
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index eccc3e7..46c5ff1 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -676,11 +676,9 @@ static int s390_virtio_bridge_init(SysBusDevice *dev)
>  
>  static void s390_virtio_bridge_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = s390_virtio_bridge_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo s390_virtio_bridge_info = {
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index df13b70..71a7e66 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1274,11 +1274,9 @@ static int virtual_css_bridge_init(SysBusDevice *dev)
>  
>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = virtual_css_bridge_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo virtual_css_bridge_info = {
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index d830188..462558b 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -506,7 +506,6 @@ static void pl181_class_init(ObjectClass *klass, void 
> *data)
>      sdc->init = pl181_init;
>      k->vmsd = &vmstate_pl181;
>      k->reset = pl181_reset;
> -    k->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>  }
>  
>  static const TypeInfo pl181_info = {
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index f9cdeea..4c59699 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -297,7 +297,6 @@ static void arm_mptimer_class_init(ObjectClass *klass, 
> void *data)
>      sbc->init = arm_mptimer_init;
>      dc->vmsd = &vmstate_arm_mptimer;
>      dc->reset = arm_mptimer_reset;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->props = arm_mptimer_properties;
>  }
>  
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 3777764..446a591 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -751,7 +751,6 @@ static void hpet_device_class_init(ObjectClass *klass, 
> void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = hpet_realize;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->reset = hpet_reset;
>      dc->vmsd = &vmstate_hpet;
>      dc->props = hpet_device_properties;
> diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c
> index 2f7360c..34d9b44 100644
> --- a/hw/timer/pl031.c
> +++ b/hw/timer/pl031.c
> @@ -251,7 +251,6 @@ static void pl031_class_init(ObjectClass *klass, void 
> *data)
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
>      k->init = pl031_init;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &vmstate_pl031;
>  }
>