Re: [Qemu-devel] [PATCH v2 05/10] pci-host: Consistently 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>
> 
> Many PCI host bridges consist of a sysbus device and a PCI device.
> You need both for the thing to work.  Arguably, these bridges should
> be modelled as a single, composite devices instead of pairs of
> seemingly independent devices you can only use together, but we're not
> there, yet.
> 
> Since the sysbus part can't be instantiated with device_add, yet,
> permitting it with the PCI part is useless.  We shouldn't offer
> useless options to the user, so let's set
> cannot_instantiate_with_device_add_yet for them.
> 
> It's already set for Bonito, grackle, i440FX, and raven.  Document
> why.
> 
> Set it for the others: dec-21154, e500-host-bridge, gt64120_pci, mch,
> pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp,
> uni-north-internal-pci, uni-north-pci, and versatile_pci_host.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hw/mips/gt64xxx_pci.c   |  6 ++++++
>  hw/pci-bridge/dec.c     |  6 ++++++
>  hw/pci-host/apb.c       |  6 ++++++
>  hw/pci-host/bonito.c    |  6 +++++-
>  hw/pci-host/grackle.c   |  6 +++++-
>  hw/pci-host/piix.c      |  6 +++++-
>  hw/pci-host/ppce500.c   |  5 +++++
>  hw/pci-host/prep.c      |  6 +++++-
>  hw/pci-host/q35.c       |  5 +++++
>  hw/pci-host/uninorth.c  | 24 ++++++++++++++++++++++++
>  hw/pci-host/versatile.c |  6 ++++++
>  hw/ppc/ppc4xx_pci.c     |  5 +++++
>  hw/sh4/sh_pci.c         |  6 ++++++
>  13 files changed, 89 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 3da2e67..6398514 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d)
>  static void gt64120_pci_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init = gt64120_pci_init;
>      k->vendor_id = PCI_VENDOR_ID_MARVELL;
>      k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
>      k->revision = 0x10;
>      k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST.
What do you think about a different approach: check class_id
in parent class init func and set the flag according to it?
It corresponds to your idea of changing only sysbus base class.
Here we don't have a "natural" base class, but we can use class_id.
What do you think?
Marcel

>  }
>  
>  static const TypeInfo gt64120_pci_info = {
> diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
> index e5e3be8..a6ca940 100644
> --- a/hw/pci-bridge/dec.c
> +++ b/hw/pci-bridge/dec.c
> @@ -116,6 +116,7 @@ static int dec_21154_pci_host_init(PCIDevice *d)
>  static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init = dec_21154_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_DEC;
> @@ -123,6 +124,11 @@ static void dec_21154_pci_host_class_init(ObjectClass 
> *klass, void *data)
>      k->revision = 0x02;
>      k->class_id = PCI_CLASS_BRIDGE_PCI;
>      k->is_bridge = 1;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo dec_21154_pci_host_info = {
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 92f289f..1b399dd 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -516,11 +516,17 @@ static int pbm_pci_host_init(PCIDevice *d)
>  static void pbm_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init = pbm_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_SUN;
>      k->device_id = PCI_DEVICE_ID_SUN_SABRE;
>      k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo pbm_pci_host_info = {
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index bfb9820..902441f 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -806,8 +806,12 @@ static void bonito_class_init(ObjectClass *klass, void 
> *data)
>      k->revision = 0x01;
>      k->class_id = PCI_CLASS_BRIDGE_HOST;
>      dc->desc = "Host bridge";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &vmstate_bonito;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo bonito_info = {
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index c178375..7d95821 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -130,7 +130,11 @@ static void grackle_pci_class_init(ObjectClass *klass, 
> void *data)
>      k->device_id = PCI_DEVICE_ID_MOTOROLA_MPC106;
>      k->revision  = 0x00;
>      k->class_id  = PCI_CLASS_BRIDGE_HOST;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo grackle_pci_info = {
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 21ffe97..8089fd6 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -698,8 +698,12 @@ static void i440fx_class_init(ObjectClass *klass, void 
> *data)
>      k->revision = 0x02;
>      k->class_id = PCI_CLASS_BRIDGE_HOST;
>      dc->desc = "Host bridge";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>      dc->vmsd = &vmstate_i440fx;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo i440fx_info = {
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index f00793d..c80b7cb 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -387,6 +387,11 @@ static void e500_host_bridge_class_init(ObjectClass 
> *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_MPC8533E;
>      k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
>      dc->desc = "Host bridge";
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo e500_host_bridge_info = {
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index ebc40c6..042dc8f 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -198,7 +198,11 @@ static void raven_class_init(ObjectClass *klass, void 
> *data)
>      k->class_id = PCI_CLASS_BRIDGE_HOST;
>      dc->desc = "PReP Host Bridge - Motorola Raven";
>      dc->vmsd = &vmstate_raven;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo raven_info = {
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index ad703a4..4dd75c6 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -390,6 +390,11 @@ static void mch_class_init(ObjectClass *klass, void 
> *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_Q35_MCH;
>      k->revision = MCH_HOST_BRIDGE_REVISION_DEFAULT;
>      k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo mch_info = {
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index 91530cd..ae04be5 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -351,12 +351,18 @@ static int unin_internal_pci_host_init(PCIDevice *d)
>  static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init      = unin_main_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_APPLE;
>      k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI;
>      k->revision  = 0x00;
>      k->class_id  = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo unin_main_pci_host_info = {
> @@ -369,12 +375,18 @@ static const TypeInfo unin_main_pci_host_info = {
>  static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init      = u3_agp_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_APPLE;
>      k->device_id = PCI_DEVICE_ID_APPLE_U3_AGP;
>      k->revision  = 0x00;
>      k->class_id  = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo u3_agp_pci_host_info = {
> @@ -387,12 +399,18 @@ static const TypeInfo u3_agp_pci_host_info = {
>  static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init      = unin_agp_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_APPLE;
>      k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_AGP;
>      k->revision  = 0x00;
>      k->class_id  = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo unin_agp_pci_host_info = {
> @@ -405,12 +423,18 @@ static const TypeInfo unin_agp_pci_host_info = {
>  static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init      = unin_internal_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_APPLE;
>      k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_I_PCI;
>      k->revision  = 0x00;
>      k->class_id  = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo unin_internal_pci_host_info = {
> diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
> index 6b28929..71ff0de 100644
> --- a/hw/pci-host/versatile.c
> +++ b/hw/pci-host/versatile.c
> @@ -467,11 +467,17 @@ static int versatile_pci_host_init(PCIDevice *d)
>  static void versatile_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init = versatile_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_XILINX;
>      k->device_id = PCI_DEVICE_ID_XILINX_XC2VP30;
>      k->class_id = PCI_CLASS_PROCESSOR_CO;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo versatile_pci_host_info = {
> diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
> index d2d6f65..4cb7851 100644
> --- a/hw/ppc/ppc4xx_pci.c
> +++ b/hw/ppc/ppc4xx_pci.c
> @@ -380,6 +380,11 @@ static void ppc4xx_host_bridge_class_init(ObjectClass 
> *klass, void *data)
>      k->vendor_id    = PCI_VENDOR_ID_IBM;
>      k->device_id    = PCI_DEVICE_ID_IBM_440GX;
>      k->class_id     = PCI_CLASS_BRIDGE_OTHER;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo ppc4xx_host_bridge_info = {
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index e81176a..a2f6d9e 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -162,10 +162,16 @@ static int sh_pci_host_init(PCIDevice *d)
>  static void sh_pci_host_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      k->init = sh_pci_host_init;
>      k->vendor_id = PCI_VENDOR_ID_HITACHI;
>      k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo sh_pci_host_info = {