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 <david@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,
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);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..9b34673 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
{
VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
- bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
- !pci_bus_is_root(pci_dev->bus);
+ bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
if (!kvm_has_many_ioeventfds()) {
proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..7b9a40f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
PCIBus *pci_find_primary_bus(void);
PCIBus *pci_device_root_bus(const PCIDevice *d);
+bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
const char *pci_root_bus_path(PCIDevice *dev);
PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
int pci_qdev_find_device(const char *id, PCIDevice **pdev);
--
2.9.3
On 03/28/2017 05:16 AM, 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 <david@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);
> }
>
Hi David,
> +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
I agree with the patch but I think the function name
may be better.
It actually queries if a hybrid device is pcie on this slot.
> +{
> + 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,
> 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) ||
Gerd agreed with this change, but I suppose we need a compat property since
is a behavior change.
Thanks,
Marcel
> xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
> ret = pcie_endpoint_cap_init(dev, 0xa0);
> assert(ret >= 0);
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..9b34673 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> {
> VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> - bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
> - !pci_bus_is_root(pci_dev->bus);
> + bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
>
> if (!kvm_has_many_ioeventfds()) {
> proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..7b9a40f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
>
> PCIBus *pci_find_primary_bus(void);
> PCIBus *pci_device_root_bus(const PCIDevice *d);
> +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
> const char *pci_root_bus_path(PCIDevice *dev);
> PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>
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 <david@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.
> 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?
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..9b34673 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> {
> VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> - bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
> - !pci_bus_is_root(pci_dev->bus);
> + bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
>
> if (!kvm_has_many_ioeventfds()) {
> proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
I don't actually remember why do we disable pcie on a root port.
Marcel, do you happen to know?
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..7b9a40f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
>
> PCIBus *pci_find_primary_bus(void);
> PCIBus *pci_device_root_bus(const PCIDevice *d);
> +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
> const char *pci_root_bus_path(PCIDevice *dev);
> PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> --
> 2.9.3
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 <david@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).
>
> > 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
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 <david@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".
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.
>
> >
> > > 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
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 <david@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 :(.
> 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.
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.
>
> >
> > >
> > > > 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
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 <david@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
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 <david@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()?
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
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 <david@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.
So, I agree the current name isn't great, but that suggestion doesn't
make sense to me. The function is determining if the hybrid device
should present as PCIe rather than PCI, not whether it's permitted at
all.
>
> > 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 need to fix this for the next spin.
>
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f9b7244..9b34673 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> > {
> > VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> > VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> > - bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
> > - !pci_bus_is_root(pci_dev->bus);
> > + bool pcie_port = pci_allow_hybrid_pcie(pci_dev);
> >
> > if (!kvm_has_many_ioeventfds()) {
> > proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>
> I don't actually remember why do we disable pcie on a root port.
> Marcel, do you happen to know?
I assume you mean on a root bus rather than on a root port (in the
sense of under the root port's fake P2P bridge). I was told it breaks
firmware and/or some guests on x86.
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index a37a2d5..7b9a40f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -453,6 +453,7 @@ void pci_for_each_bus(PCIBus *bus,
> >
> > PCIBus *pci_find_primary_bus(void);
> > PCIBus *pci_device_root_bus(const PCIDevice *d);
> > +bool pci_allow_hybrid_pcie(PCIDevice *pci_dev);
> > const char *pci_root_bus_path(PCIDevice *dev);
> > PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> > int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>
--
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
© 2016 - 2026 Red Hat, Inc.