Re: [Qemu-devel] [PATCH V6 for-2.3 12/26] hw/pci: introduce TYPE_PCI_MAIN_HOST_BRIDGE interface

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Apr 27, 2015 at 04:04:38PM +0300, Marcel Apfelbaum wrote:
> On 04/27/2015 03:48 PM, Michael S. Tsirkin wrote:
> >On Mon, Apr 27, 2015 at 03:30:01PM +0300, Marcel Apfelbaum wrote:
> >>On 04/27/2015 02:24 PM, Michael S. Tsirkin wrote:
> >>>On Thu, Mar 19, 2015 at 08:52:47PM +0200, Marcel Apfelbaum wrote:
> >>>>From: Marcel Apfelbaum <marcel.a@redhat.com>
> >>>>
> >>>>This is a marker interface used to differentiate the
> >>>>"default" host bridge on a system with multiple host bridges.
> >>>>This differentiation is required only for pc machines for now
> >>>>by the ACPI subsystem.
> >>>>
> >>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>
> >>>Fixing hotplug for pxb almost for sure means we'll need
> >>>to drop this later.
> >>Not really, I am not sure how it is related. I'll go over the pxb pci devices
> >>for this hotplug. I have no intention to drop this later.
> >>This interface denotes the main/default/bus 0 host bridge.
> >
> >It's related since you will want to describe all host bridges in acpi.
> When I'll submit the pxb hotplug support it will be more clear,
> Basically, I'll go over the pxbs leaving the default host bridge alone.

So instead, just go over all roots.

> >
> >>
> >>  How about an interface to
> >>>iterate over objects by type?
> >>>You can then blacklist pxbs for now.
> >>Less elegant in my opinion,
> >
> >Right but it will be needed in the future anyway.
> I think we should leave this for when we'll need it, this series
> is already long enough without adding a new QOM iteration type.
> 
> Thanks,
> Marcel

Do you have hotplug working?
Maybe wait until it's there, then we can discuss.

> >
> >>>
> >>>>---
> >>>>  hw/i386/acpi-build.c      | 9 ++++++---
> >>>>  hw/pci-host/piix.c        | 5 +++++
> >>>>  hw/pci-host/q35.c         | 4 ++++
> >>>>  hw/pci/pci_host.c         | 6 ++++++
> >>>>  include/hw/pci/pci_host.h | 7 +++++++
> >>>>  5 files changed, 28 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>>index d0a5c85..86e474a 100644
> >>>>--- a/hw/i386/acpi-build.c
> >>>>+++ b/hw/i386/acpi-build.c
> >>>>@@ -249,7 +249,8 @@ static void acpi_get_pci_info(PcPciInfo *info)
> >>>>      Object *pci_host;
> >>>>      bool ambiguous;
> >>>>
> >>>>-    pci_host = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
> >>>>+    pci_host = object_resolve_path_type("", TYPE_PCI_MAIN_HOST_BRIDGE,
> >>>>+                                        &ambiguous);
> >>>>      g_assert(!ambiguous);
> >>>>      g_assert(pci_host);
> >>>>
> >>>>@@ -993,7 +994,8 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>>              PCIBus *bus = NULL;
> >>>>              bool ambiguous;
> >>>>
> >>>>-            pci_host = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
> >>>>+            pci_host = object_resolve_path_type("", TYPE_PCI_MAIN_HOST_BRIDGE,
> >>>>+                                                &ambiguous);
> >>>>              if (!ambiguous && pci_host) {
> >>>>                  bus = PCI_HOST_BRIDGE(pci_host)->bus;
> >>>>              }
> >>>>@@ -1338,7 +1340,8 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> >>>>      QObject *o;
> >>>>      bool ambiguous;
> >>>>
> >>>>-    pci_host = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
> >>>>+    pci_host = object_resolve_path_type("", TYPE_PCI_MAIN_HOST_BRIDGE,
> >>>>+                                        &ambiguous);
> >>>>      g_assert(!ambiguous);
> >>>>      g_assert(pci_host);
> >>>>
> >>>>diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >>>>index 292b6e9..0033ab4 100644
> >>>>--- a/hw/pci-host/piix.c
> >>>>+++ b/hw/pci-host/piix.c
> >>>>@@ -766,6 +766,11 @@ static const TypeInfo i440fx_pcihost_info = {
> >>>>      .instance_size = sizeof(I440FXState),
> >>>>      .instance_init = i440fx_pcihost_initfn,
> >>>>      .class_init    = i440fx_pcihost_class_init,
> >>>>+    .interfaces    = (InterfaceInfo[]) {
> >>>>+        { TYPE_PCI_MAIN_HOST_BRIDGE },
> >>>>+        { }
> >>>>+    }
> >>>>+
> >>>>  };
> >>>>
> >>>>  static void i440fx_register_types(void)
> >>>>diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >>>>index 5dd559e..e35b3a2 100644
> >>>>--- a/hw/pci-host/q35.c
> >>>>+++ b/hw/pci-host/q35.c
> >>>>@@ -193,6 +193,10 @@ static const TypeInfo q35_host_info = {
> >>>>      .instance_size = sizeof(Q35PCIHost),
> >>>>      .instance_init = q35_host_initfn,
> >>>>      .class_init = q35_host_class_init,
> >>>>+    .interfaces = (InterfaceInfo[]) {
> >>>>+        { TYPE_PCI_MAIN_HOST_BRIDGE },
> >>>>+        { }
> >>>>+    }
> >>>>  };
> >>>>
> >>>>  /****************************************************************************
> >>>>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>>>index 3e26f92..87180c8 100644
> >>>>--- a/hw/pci/pci_host.c
> >>>>+++ b/hw/pci/pci_host.c
> >>>>@@ -175,6 +175,11 @@ const MemoryRegionOps pci_host_data_be_ops = {
> >>>>      .endianness = DEVICE_BIG_ENDIAN,
> >>>>  };
> >>>>
> >>>>+static const TypeInfo pci_main_host_interface_info = {
> >>>>+    .name          = TYPE_PCI_MAIN_HOST_BRIDGE,
> >>>>+    .parent        = TYPE_INTERFACE,
> >>>>+};
> >>>>+
> >>>>  static const TypeInfo pci_host_type_info = {
> >>>>      .name = TYPE_PCI_HOST_BRIDGE,
> >>>>      .parent = TYPE_SYS_BUS_DEVICE,
> >>>>@@ -185,6 +190,7 @@ static const TypeInfo pci_host_type_info = {
> >>>>
> >>>>  static void pci_host_register_types(void)
> >>>>  {
> >>>>+    type_register_static(&pci_main_host_interface_info);
> >>>>      type_register_static(&pci_host_type_info);
> >>>>  }
> >>>>
> >>>>diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> >>>>index ba31595..3c72e26 100644
> >>>>--- a/include/hw/pci/pci_host.h
> >>>>+++ b/include/hw/pci/pci_host.h
> >>>>@@ -30,6 +30,13 @@
> >>>>
> >>>>  #include "hw/sysbus.h"
> >>>>
> >>>>+/**
> >>>>+ * Marker interface for classes whose instances can
> >>>>+ * be main host bridges. It is intended to be used
> >>>>+ * when the QOM tree includes multiple host bridges.
> >>>>+ */
> >>>>+#define TYPE_PCI_MAIN_HOST_BRIDGE "pci-main-host-bridge"
> >>>>+
> >>>>  #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
> >>>>  #define PCI_HOST_BRIDGE(obj) \
> >>>>      OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
> >>>>--
> >>>>2.1.0