Re: [Qemu-devel] [RFC for-2.10 1/3] pci/pcie: Make a consistent helper for switching PCI/PCIe "hybrid" devices

[David Gibson]

On Wed, Aug 30, 2017 at 09:23:59AM -0300, Eduardo Habkost wrote:
> 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 <address@hidden>
> > > > > > ---
> > > > > >  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()?

Ok, I'm going with pci_hybrid_prefer_pcie() in the next spin.

> In either case, we really need a doc comment clearly explaining
> the function purpose and semantics.

I've put something on the function itself.  Possibly it should move to
the header.  Anyway, you can comment again on the next spin.

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

signature.asc
Description: PGP signature