Re: [Qemu-devel] [PATCH V6 for-2.3 13/26] hw/pci-host: introduce TYPE_PCI_HOST_BRIDGE_SNOOPED interface

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 04:01:16PM +0300, Marcel Apfelbaum wrote:
> On 04/27/2015 03:14 PM, Michael S. Tsirkin wrote:
> >On Thu, Mar 19, 2015 at 08:52:48PM +0200, Marcel Apfelbaum wrote:
> >>TYPE_PCI_HOST_BRIDGE_SNOOPED is a special case of host bridge
> >>whose configuration registers are snooped by other host bridges
> >>to complete their configuration cycles.
> >>
> >>The interface exposes a list of snooping host bridges that
> >>shall be used by the hosts implementing this interface
> >>in order to emulate a snooping mechanism.
> >>
> >>The way that the snooping hosts are registered or how
> >>the snooping is implemented are out of the interface scope,
> >>it only provides a way to determine if a host bridge has
> >>snooping hosts and list them.
> >
> >
> >
> >>Signed-off-by: Marcel Apfelbaum <address@hidden>
> >>---
> >>  hw/pci/pci_host.c         |  8 ++++++++
> >>  include/hw/pci/pci_host.h | 24 ++++++++++++++++++++++++
> >>  2 files changed, 32 insertions(+)
> >>
> >>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>index 87180c8..288e74c 100644
> >>--- a/hw/pci/pci_host.c
> >>+++ b/hw/pci/pci_host.c
> >>@@ -180,6 +180,12 @@ static const TypeInfo pci_main_host_interface_info = {
> >>      .parent        = TYPE_INTERFACE,
> >>  };
> >>
> >>+static const TypeInfo pci_host_bridge_snooped_interface_info = {
> >>+    .name = TYPE_PCI_HOST_BRIDGE_SNOOPED,
> >>+    .parent = TYPE_INTERFACE,
> >>+    .class_size = sizeof(PCIHostBridgeSnoopedClass),
> >>+};
> >>+
> >>  static const TypeInfo pci_host_type_info = {
> >>      .name = TYPE_PCI_HOST_BRIDGE,
> >>      .parent = TYPE_SYS_BUS_DEVICE,
> >>@@ -191,7 +197,9 @@ 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_bridge_snooped_interface_info);
> >>      type_register_static(&pci_host_type_info);
> >>+
> >
> >extra empty line
> Will take care of it, thanks.
> 
> >
> >>  }
> >>
> >>  type_init(pci_host_register_types)
> >>diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> >>index 3c72e26..a041919 100644
> >>--- a/include/hw/pci/pci_host.h
> >>+++ b/include/hw/pci/pci_host.h
> >>@@ -63,6 +63,30 @@ typedef struct PCIHostBridgeClass {
> >>      const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> >>  } PCIHostBridgeClass;
> >>
> >>+/**
> >>+ * A special case of host bridge whose configuration registers
> >>+ * are snooped by other host bridges to complete their
> >>+ * configuration cycles.
> >>+ */
> >>+#define TYPE_PCI_HOST_BRIDGE_SNOOPED "pci-host-bridge-snooped"
> >>+#define TYPE_PCI_HOST_BRIDGE_SNOOPED_CLASS(klass) \
> >>+     OBJECT_CLASS_CHECK(PCIHostBridgeSnoopedClass, (klass), \
> >>+                        TYPE_PCI_HOST_BRIDGE_SNOOPED)
> >>+#define PCI_HOST_BRIDGE_SNOOPED_GET_CLASS(obj) \
> >>+     OBJECT_GET_CLASS(PCIHostBridgeSnoopedClass, (obj), \
> >>+                      TYPE_PCI_HOST_BRIDGE_SNOOPED)
> >>+#define PCI_HOST_BRIDGE_SNOOPED(obj) \
> >>+     INTERFACE_CHECK(PCIHostState, (obj), \
> >>+                     TYPE_PCI_HOST_BRIDGE_SNOOPED)
> >>+
> >>+typedef struct PCIHostBridgeSnoopedClass {
> >>+    /* <private> */
> >>+    InterfaceClass parent_class;
> >>+
> >>+    /* <public> */
> >>+    GPtrArray *(*snooping_hosts)(PCIHostState *);
> >
> >Why not just add GPtrArray here directly, and add an API to
> >register/deregister?
> The interface concentrates on usage, letting the way to fill the snooping
> hosts to the specific implementation.
> 
> Thanks,
> Marcel

IMO this is overdoing abstractions.

But all this might be moot, it's probably better to just
add everyone to the child bus list, and drop the
concept of snooping config cycles.


> >
> >>+} PCIHostBridgeSnoopedClass;
> >>+
> >>  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> >>  void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
> >>                                    uint32_t limit, uint32_t val, uint32_t 
> >> len);
> >>--
> >>2.1.0