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

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 11:39:42AM +0300, Marcel Apfelbaum wrote:
> On 04/27/2015 05:45 PM, Michael S. Tsirkin wrote:
> >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.
> It will provide us with enough elasticity when we'll try the same for Q35.
> Also this interface is used in several places and it will serve us when we'll 
> go for Q35.

I don't see how it helps, we don't just stick a virtual function
in front of all new code we write on the assumption it
helps elasticity.

> >
> >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.
> Well, this will be a problem, since the whole concept of the PXB is to snoop
> on the configuration cycles and this interface gives us a more easy
> way to understand the implementation.
> 
> We already discussed this approach and was accepted.
> The code is small and concise and the use of QOM makes sense in my opinion.
> I prefer not to re-open it now.
> 
> Thanks,
> Marcel

I'm talking about the implementation here.
We already scan the child bus list on config
transactions. Why not add your entry there,
then you can snoop to your heart's content.



> >
> >
> >>>
> >>>>+} 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