From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn30g-00065p-Re for qemu-devel@nongnu.org; Tue, 28 Apr 2015 06:45:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yn30c-0004Pa-Nw for qemu-devel@nongnu.org; Tue, 28 Apr 2015 06:45:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33354) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn30c-0004PW-E2 for qemu-devel@nongnu.org; Tue, 28 Apr 2015 06:44:58 -0400 Date: Tue, 28 Apr 2015 12:44:52 +0200 From: "Michael S. Tsirkin" Message-ID: <20150428123109-mutt-send-email-mst@redhat.com> References: <1426791181-23831-1-git-send-email-marcel@redhat.com> <1426791181-23831-14-git-send-email-marcel@redhat.com> <20150427141221-mutt-send-email-mst@redhat.com> <553E331C.5070409@redhat.com> <20150427164119-mutt-send-email-mst@redhat.com> <553F474E.9000800@redhat.com> <20150428104029-mutt-send-email-mst@redhat.com> <553F5F1A.9080804@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <553F5F1A.9080804@redhat.com> Subject: Re: [Qemu-devel] [PATCH V6 for-2.3 13/26] hw/pci-host: introduce TYPE_PCI_HOST_BRIDGE_SNOOPED interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: kraxel@redhat.com, quintela@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, alex.williamson@redhat.com, kevin@koconnor.net, hare@suse.de, imammedo@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net 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 > >>>>>>--- > >>>>>> 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 { > >>>>>>+ /* */ > >>>>>>+ InterfaceClass parent_class; > >>>>>>+ > >>>>>>+ /* */ > >>>>>>+ 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