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

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

On Tue, Apr 28, 2015 at 01:21:14PM +0300, Marcel Apfelbaum wrote:
> On 04/28/2015 11:43 AM, Michael S. Tsirkin wrote:
> >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 <marcel@redhat.com>
> >>>>>>---
> >>>>>>  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.
> It helps by letting us querying the interface instead of asking:
> Is this host-bridge PIIX ? look for child buses and see if there are roots
> else: is this host-bridge ...

But what does not help is all the indirection and hiding.


> >
> >>>
> >>>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.
> Maybe I didn't fully understand what you want.
> Do you want to add the new root buses as child buses of root bus 0?
> This interface is not used in this context, is used in other scenarios:
> [PATCH V6 for-2.3 21/26] hw/pci: inform bios if the system has extra pci root buses
> [PATCH V6 for-2.3 15/26] hw/acpi: add support for i440fx 'snooping' root busses
> 
> I can look for other usages, can you tell me how it will look without this interface?
> 
> Other thing, in config we have pci_data_write calling (in the end) pci_find_bus_nr that does  not support
> multiple root buses.
> Changing it could affect a lot of functionality and also looking at the function it will not be pretty or clear.
> Keeping it at host-bridge level (see  [PATCH V6 for-2.3 14/26] hw/pci-host/piix: implement TYPE_PCI_HOST_BRIDGE_SNOOPED interface)
> and having piix query its pxbs for the extra root buses seems a good fit.
> 
> 
> Thanks,
> Marce

Really ideally we would just have a P2P bridge.
This is what we wanted originally: declare a bridge as
a pci host in ACPI.
Unfortunately you tried, and said this confuses guests, so we don't make
this device look like a bridge to guests.  But it still can/should make
it look just like a bridge to qemu, and all the machinery for forwarding
config transactions will mostly work.

One thing that won't work is the funky unrolled recursion in
pci_find_bus_nr, since that assumes each bridge has
a bus range. I guess we'll just have to make it recursive.


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