On Wed, Aug 30, 2017 at 03:54:32PM +1000, David Gibson wrote:
> On Tue, Aug 29, 2017 at 11:12:36AM -0300, Eduardo Habkost wrote:
> > On Tue, Aug 29, 2017 at 09:42:39PM +1000, David Gibson wrote:
> > > On Wed, Apr 26, 2017 at 06:23:58PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 28, 2017 at 01:16:49PM +1100, David Gibson wrote:
> > > > > virtio-pci and XHCI are "hybrid" devices in the sense that they can
> > > > > present
> > > > > themselves as either PCIe or plain PCI devices depending on the
> > > > > machine
> > > > > and bus they're connected to.
> > > > >
> > > > > For virtio-pci to present as PCIe it requires that it's connected to
> > > > > a PCIe
> > > > > bus and that it's not a root bus - this is to ensure that the device
> > > > > is
> > > > > connected via a PCIe root port or downstream port rather than being a
> > > > > integrated endpoint. Some guests (Windows in particular AIUI) don't
> > > > > really
> > > > > cope with PCIe integrated endpoints.
> > > > >
> > > > > For XHCI it only checks that the bus is PCIe, but that probably means
> > > > > it
> > > > > would cause problems if attached as an integrated devices directly to
> > > > > a
> > > > > PCIe root bus.
> > > > >
> > > > > This patch makes the test consistent between XHCI and virtio-pci, and
> > > > > clarifies things by having them both use a new
> > > > > 'pci_allow_hybrid_pcie()'
> > > > > helper which performs the same check as virtio-pci.
> > > > >
> > > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> > > > > ---
> > > > > hw/pci/pci.c | 7 +++++++
> > > > > hw/usb/hcd-xhci.c | 2 +-
> > > > > hw/virtio/virtio-pci.c | 3 +--
> > > > > include/hw/pci/pci.h | 1 +
> > > > > 4 files changed, 10 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index bd8043c..779787b 100644
> > > > > --- a/hw/pci/pci.c
> > > > > +++ b/hw/pci/pci.c
> > > > > @@ -390,6 +390,13 @@ bool pci_bus_is_root(PCIBus *bus)
> > > > > return PCI_BUS_GET_CLASS(bus)->is_root(bus);
> > > > > }
> > > > >
> > > > > +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> > > > > +{
> > > > > + PCIBus *bus = pci_dev->bus;
> > > > > +
> > > > > + return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
> > > > > +}
> > > > > +
> > > > > void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState
> > > > > *parent,
> > > > > const char *name,
> > > > > MemoryRegion *address_space_mem,
> > > >
> > > > I'd prefer pci_allow_hybrid_pci_pcie.
> > >
> > > Ok, I've made that change for the next spin (aimed at 2.11, obviously).
> >
> > I'm a bit confused by the naming: by looking at the function
> > name, I don't know if "allow hybrid" means "this bus+device can
> > (also) work as Conventional PCI" or "this bus+device can (also)
> > work as PCI Express".
>
> Neither, actually. It means "should this device, which is capable of
> both PCI and PCIe operation, operate as PCIe in this context". It's
> only expected to be called by devices which support both modes of
> operation.
>
> I have yet to think of a succinct name which conveys that :(.
Based on this description, maybe pci_hybrid_allow_pcie() would be
clearer.
But based on the comments below, I have another suggestion:
>
> > What about just naming it pci_allow_pcie() or
> > pci_bus_allow_pcie()? It looks like the function doesn't care if
> > the device is hybrid or PCIe-only: it's only checking if the
> > device can work as PCIe on that bus. It's up to the device to
> > decide if it should switch to Conventional PCI or report an error
> > if the function returns false.
>
> Hmm.. that would mean changing *every* existing PCIe device to call
> this, which I don't think I want to do.
Maybe calling it from the common PCI realize function won't be a
bad idea. But let's discuss that after we clean up the existing
hybrid devices.
>
> Also it's _not* really saying if a device can operate as PCIe. AIUI,
> a device _can_ operate as PCIe on a root bus (without a port) although
> it's unusual. Integrated PCIe devices would do so, IIUC. For that
> matter I believe current devices which only support PCIe mode will
> also operate in PCIe mode on a root bus right now; but doing so
> without inserting a root port might make guests unhappy, at least on
> PC.
If that's the case, I would change the name and documentation to
say "defaults to", "should", "recommend", or "prefer".
What about pci_bus_prefers_pcie() or pci_hybrid_prefers_pcie()?
In either case, we really need a doc comment clearly explaining
the function purpose and semantics.
>
> >
> > >
> > > >
> > > > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > > > > index f0af852..a7ff4fd 100644
> > > > > --- a/hw/usb/hcd-xhci.c
> > > > > +++ b/hw/usb/hcd-xhci.c
> > > > > @@ -3619,7 +3619,7 @@ static void usb_xhci_realize(struct PCIDevice
> > > > > *dev, Error **errp)
> > > > >
> > > > > PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > > > &xhci->mem);
> > > > >
> > > > > - if (pci_bus_is_express(dev->bus) ||
> > > > > + if (pci_allow_hybrid_pcie(dev) ||
> > > > > xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
> > > > > ret = pcie_endpoint_cap_init(dev, 0xa0);
> > > > > assert(ret >= 0);
> > > >
> > > > This seems to change the behaviour for xhci on a root bus - what
> > > > am I missing?
> > >
> > > Nothing. I didn't consider the backwards compat implications; I'll
> > > fix it for the next spin.
> > >
> >
> >
> >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
--
Eduardo
