diff mbox

[05/10] pci-host: Consistently set cannot_instantiate_with_device_add_yet

Message ID 1382018101-6102-6-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Oct. 17, 2013, 1:54 p.m. UTC 
From: Markus Armbruster <armbru@redhat.com>

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,
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 <armbru@redhat.com>
---
 hw/mips/gt64xxx_pci.c   |  5 +++++
 hw/pci-bridge/dec.c     |  5 +++++
 hw/pci-host/apb.c       |  5 +++++
 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  | 20 ++++++++++++++++++++
 hw/pci-host/versatile.c |  5 +++++
 hw/ppc/ppc4xx_pci.c     |  5 +++++
 hw/sh4/sh_pci.c         |  5 +++++
 13 files changed, 80 insertions(+), 4 deletions(-)

Comments

Peter Maydell Oct. 23, 2013, 9:51 a.m. UTC | #1 
On 17 October 2013 14:54,  <armbru@redhat.com> wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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.

I disagree here -- we should be using the modularity that our
device model provides, not arbitrarily squashing things together
into single objects just because we've foolishly exposed to the
end user direct command line access to "create any random object
whatsoever even if it doesn't make sense".

> 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 doesn't make sense to allow the user to create the on-PCI-bus
representation of the host controller anyway even if they could
device_add the host controller proper: creating the host controller
will always automatically create the on-PCI-bus part.

> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1157,6 +1157,11 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
>      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.
> +     */
> +    k->parent_class.cannot_instantiate_with_device_add_yet = true;

Please don't directly access parent_class -- you should be using
the proper QOM cast macros to get the DeviceClass pointer.

thanks
-- PMM
Markus Armbruster Oct. 24, 2013, 9:16 a.m. UTC | #2 
Peter Maydell <peter.maydell@linaro.org> writes:

> On 17 October 2013 14:54,  <armbru@redhat.com> wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> 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.
>
> I disagree here -- we should be using the modularity that our
> device model provides, not arbitrarily squashing things together
> into single objects just because we've foolishly exposed to the
> end user direct command line access to "create any random object
> whatsoever even if it doesn't make sense".

I'm afraid I didn't express myself clearly.  I'm not advocating
*squashing* these components together.  I'm saying that if A and B can
only be used wired together, there should be a C composed of A, B and
the necessary wiring, and that C is what actually gets put on the board
by configuration.

>> 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 doesn't make sense to allow the user to create the on-PCI-bus
> representation of the host controller anyway even if they could
> device_add the host controller proper: creating the host controller
> will always automatically create the on-PCI-bus part.

Technically, a device_add i440FX-pcihost doesn't automatically create
i440FX *now*.

I suspect we're arguing only about what exact kind of crazy device_add
of the PCI-facing part of the PCI host bridge is.  Assuming we actually
agree it's crazy in *today's* state of things, does it matter what kind
of crazy it is?  If it doesn't matter, perhaps you could give me a hint
on how to rephrase the commit message.

>> --- a/hw/mips/gt64xxx_pci.c
>> +++ b/hw/mips/gt64xxx_pci.c
>> @@ -1157,6 +1157,11 @@ static void
>> gt64120_pci_class_init(ObjectClass *klass, void *data)
>>      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.
>> +     */
>> +    k->parent_class.cannot_instantiate_with_device_add_yet = true;
>
> Please don't directly access parent_class -- you should be using
> the proper QOM cast macros to get the DeviceClass pointer.

Will fix, thanks!
diff mbox

Patch

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 3da2e67..4756bf1 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1157,6 +1157,11 @@  static void gt64120_pci_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo gt64120_pci_info = {
diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index e5e3be8..e27ecfc 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -123,6 +123,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.
+     */
+    k->parent_class.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..b8df0a6 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -521,6 +521,11 @@  static void pbm_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.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..2c20d06 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -357,6 +357,11 @@  static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo unin_main_pci_host_info = {
@@ -375,6 +380,11 @@  static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo u3_agp_pci_host_info = {
@@ -393,6 +403,11 @@  static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo unin_agp_pci_host_info = {
@@ -411,6 +426,11 @@  static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.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..de22417 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -472,6 +472,11 @@  static void versatile_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.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..72fbe61 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -166,6 +166,11 @@  static void sh_pci_host_class_init(ObjectClass *klass, void *data)
     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.
+     */
+    k->parent_class.cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sh_pci_host_info = {