[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour
Date: Wed, 19 Apr 2017 21:04:55 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

On 04/18/2017 05:33 PM, Eduardo Habkost wrote:
On Tue, Apr 18, 2017 at 12:21:51PM +1000, David Gibson wrote:
On Mon, Apr 17, 2017 at 03:30:46PM -0300, Eduardo Habkost wrote:
On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
Currently PCI/PCIe hybrid devices - that is, devices which can appear as
either plain PCI or PCIe depending on where they're attached - will only
appear in PCIe mode if they're attached to a PCIe bus via a root port or
downstream port.

This is correct for "standard" PCIe setups, but there are some platforms
which need different behaviour (notably "pseries" whose paravirtualized
PCI host bridges have some idiosyncracies).

This patch allows the host bridge to override the normal behaviour.

Signed-off-by: David Gibson <address@hidden>
 hw/pci/pci.c              | 11 +++++++++--
 include/hw/pci/pci_host.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 779787b..ac68065 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)

 bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)

Why are we asking the device about allowing hybrid pcie, and not
the bus itself? Shouldn't we be able to query this before the
device is plugged, and before pci_dev->bus is even set?

In other words, why pci_allow_hyberid_pcie() and
pci_device_get_bus() get a PCIDevice* argument instead of a
PCIBus* argument?

Ah good point.  I made it a PCIDevice simply for convenience, but
you're right we should be able to query before the device is plugged.


-    PCIBus *bus = pci_dev->bus;
+    PCIHostState *host_bridge = 

There's something I don't understand completely: what exactly
guarantees that pci_device_root_bus(...)->qbus.parent is always
going to be a PCI_HOST_BRIDGE?

Well, by definition whatever is above the root bus isn't PCI, which
pretty much means it has to be a PCI host bridge.  A machine could
break this assumption, but I think that would be a bug.  We use this
same pattern to find a PCI device's (or bus's) host bridge in other
places - there doesn't appear to be another way.

Agreed, the root bus is always exposed by (some kind of) a host bridge.

After taking a better look at the way PCI buses are created, I
agree it's OK. Maybe a
  PCIHostState *pci_host_bridge(PCIBus *bus)
helper would be useful?

For sure.

Anyway, both pci_bus_root_bus() and pci_host_bridge() could be
implemented as follow-ups if you prefer, so:

Reviewed-by: Eduardo Habkost <address@hidden>


But I still have another suggestion:

+    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
+    if (hc->allow_hybrid_pcie) {
+        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
+    } else {
+        PCIBus *bus = pci_dev->bus;

-    return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
+        return pci_bus_is_express(bus) && !pci_bus_is_root(bus);

Maybe it's a matter of personal taste, but instead of requiring
an explicit check for NULL hc->allow_hybrid_pcie, why not set a
default hc->allow_hybrid_pcie() implementation on
(pci_bus_is_express(bus) && !pci_bus_is_root(bus))?

reply via email to

[Prev in Thread] Current Thread [Next in Thread]