|
From: | Marcel Apfelbaum |
Subject: | Re: [Qemu-ppc] [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.Understood.{ - PCIBus *bus = pci_dev->bus; + PCIHostState *host_bridge = PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);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>
Thanks, Marcel
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 TYPE_PCI_HOST_BRIDGE that returns (pci_bus_is_express(bus) && !pci_bus_is_root(bus))?
[Prev in Thread] | Current Thread | [Next in Thread] |