| Message ID | 1408614466-17596-3-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > If ARI Forwarding is disabled, according to PCIe spec > section 7.3.1, only slot 0 with the device attached to > logic bus representing the link from downstream > ports and root ports. > > So, adding check for PCIe downstream ports and root ports, > which avoid useless operation, both hotplug and coldplug. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/pci/pci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index daeaeac..aa0af0c 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev) > return 0; > } > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) Hi, This function is specific to pcie, I think it should be placed in hw/pci/pcie.c. hw/pci/pci.c is for general pci functionality. Other thing, I think the function name does not completely match the functionality, maybe "pcie_check_slot" or "pcie_validate_slot"? Thanks, Marcel > +{ > + Object *obj = OBJECT(bus); > + > + if (pci_bus_is_root(bus)) { > + return 0; > + } > + > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > + DeviceState *parent = qbus_get_parent(BUS(obj)); > + PCIDevice *pci_dev = PCI_DEVICE(parent); > + uint8_t port_type; > + /* > + * Root ports and downstream ports of switches are the hot > + * pluggable ports in a PCI Express hierarchy. > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > + * > + * 7.3. Configuration Transaction Rules (PCI Express specification 3.0) > + * 7.3.1. Device Number > + * > + * Downstream Ports that do not have ARI Forwarding enabled must > + * associate only Device 0 with the device attached to the Logical Bus > + * representing the Link from the Port. > + * > + * If ARI Forwarding is not enabled on root ports and downstream > + * ports, only support the devices with slot non-0, regardless of > + * hotplug or coldplug. > + */ > + port_type = pcie_cap_get_type(pci_dev); > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > + if (PCI_SLOT(dev->devfn) != 0) { > + error_report("PCIe: Port's ARI Forwarding is disabled, " > + "device can't be populated in slot %d", > + PCI_SLOT(dev->devfn)); > + return -1; > + } > + } > + } > + } > + > + return 0; > +} > + > static void pci_config_alloc(PCIDevice *pci_dev) > { > int config_size = pci_config_size(pci_dev); > @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > pci_dev->bus = bus; > pci_dev->devfn = devfn; > + > + if (pci_check_pcie_port(bus, pci_dev)) { > + return NULL; > + } > + > dma_as = pci_device_iommu_address_space(pci_dev); > > memory_region_init_alias(&pci_dev->bus_master_enable_region,
> From: Marcel Apfelbaum [mailto:marcel.a@redhat.com] > Sent: Friday, August 22, 2014 2:34 PM > Subject: Re: [PATCH v2 2/2] pci: add check for pcie root ports and downstream > ports > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > If ARI Forwarding is disabled, according to PCIe spec > > section 7.3.1, only slot 0 with the device attached to > > logic bus representing the link from downstream > > ports and root ports. > > > > So, adding check for PCIe downstream ports and root ports, > > which avoid useless operation, both hotplug and coldplug. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/pci/pci.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index daeaeac..aa0af0c 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, > PCIDevice *dev) > > return 0; > > } > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > Hi, > > This function is specific to pcie, I think it should be placed > in hw/pci/pcie.c. hw/pci/pci.c is for general pci functionality. > Good idea. > Other thing, I think the function name does not completely match > the functionality, maybe "pcie_check_slot" or "pcie_validate_slot"? > OK, agreed. Thanks! Best regards, -Gonglei > Thanks, > Marcel > > > +{ > > + Object *obj = OBJECT(bus); > > + > > + if (pci_bus_is_root(bus)) { > > + return 0; > > + } > > + > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > + uint8_t port_type; > > + /* > > + * Root ports and downstream ports of switches are the hot > > + * pluggable ports in a PCI Express hierarchy. > > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > > + * > > + * 7.3. Configuration Transaction Rules (PCI Express specification > 3.0) > > + * 7.3.1. Device Number > > + * > > + * Downstream Ports that do not have ARI Forwarding enabled > must > > + * associate only Device 0 with the device attached to the Logical > Bus > > + * representing the Link from the Port. > > + * > > + * If ARI Forwarding is not enabled on root ports and downstream > > + * ports, only support the devices with slot non-0, regardless of > > + * hotplug or coldplug. > > + */ > > + port_type = pcie_cap_get_type(pci_dev); > > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > > + if (PCI_SLOT(dev->devfn) != 0) { > > + error_report("PCIe: Port's ARI Forwarding is > disabled, " > > + "device can't be populated in > slot %d", > > + PCI_SLOT(dev->devfn)); > > + return -1; > > + } > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > static void pci_config_alloc(PCIDevice *pci_dev) > > { > > int config_size = pci_config_size(pci_dev); > > @@ -827,6 +873,11 @@ static PCIDevice > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > > > pci_dev->bus = bus; > > pci_dev->devfn = devfn; > > + > > + if (pci_check_pcie_port(bus, pci_dev)) { > > + return NULL; > > + } > > + > > dma_as = pci_device_iommu_address_space(pci_dev); > > > > memory_region_init_alias(&pci_dev->bus_master_enable_region, > >
On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > If ARI Forwarding is disabled, according to PCIe spec > section 7.3.1, only slot 0 with the device attached to > logic bus representing the link from downstream > ports and root ports. > > So, adding check for PCIe downstream ports and root ports, > which avoid useless operation, both hotplug and coldplug. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/pci/pci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index daeaeac..aa0af0c 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev) > return 0; > } > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > +{ > + Object *obj = OBJECT(bus); > + > + if (pci_bus_is_root(bus)) { > + return 0; > + } > + > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > + DeviceState *parent = qbus_get_parent(BUS(obj)); > + PCIDevice *pci_dev = PCI_DEVICE(parent); > + uint8_t port_type; > + /* > + * Root ports and downstream ports of switches are the hot > + * pluggable ports in a PCI Express hierarchy. > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > + * > + * 7.3. Configuration Transaction Rules (PCI Express specification 3.0) > + * 7.3.1. Device Number > + * > + * Downstream Ports that do not have ARI Forwarding enabled must > + * associate only Device 0 with the device attached to the Logical Bus > + * representing the Link from the Port. > + * > + * If ARI Forwarding is not enabled on root ports and downstream > + * ports, only support the devices with slot non-0, regardless of > + * hotplug or coldplug. > + */ My interpretation of this section of the spec is that if ARI forwarding is not available, only the normal 8 functions can be accessed for each device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 functions, it will need the second device's namespace, eg. devfn 1.0++, which would not be routed correctly in a non-ari forward capable device. As far as I understand, with this fix you restrict an non-ARI capable switch to only expose one device? Knut > + port_type = pcie_cap_get_type(pci_dev); > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > + if (PCI_SLOT(dev->devfn) != 0) { > + error_report("PCIe: Port's ARI Forwarding is disabled, " > + "device can't be populated in slot %d", > + PCI_SLOT(dev->devfn)); > + return -1; > + } > + } > + } > + } > + > + return 0; > +} > + > static void pci_config_alloc(PCIDevice *pci_dev) > { > int config_size = pci_config_size(pci_dev); > @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > pci_dev->bus = bus; > pci_dev->devfn = devfn; > + > + if (pci_check_pcie_port(bus, pci_dev)) { > + return NULL; > + } > + > dma_as = pci_device_iommu_address_space(pci_dev); > > memory_region_init_alias(&pci_dev->bus_master_enable_region,
> -----Original Message----- > From: Knut Omang [mailto:knut.omang@oracle.com] > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and > downstream ports > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > If ARI Forwarding is disabled, according to PCIe spec > > section 7.3.1, only slot 0 with the device attached to > > logic bus representing the link from downstream > > ports and root ports. > > > > So, adding check for PCIe downstream ports and root ports, > > which avoid useless operation, both hotplug and coldplug. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/pci/pci.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index daeaeac..aa0af0c 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, > PCIDevice *dev) > > return 0; > > } > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > +{ > > + Object *obj = OBJECT(bus); > > + > > + if (pci_bus_is_root(bus)) { > > + return 0; > > + } > > + > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > + uint8_t port_type; > > + /* > > + * Root ports and downstream ports of switches are the hot > > + * pluggable ports in a PCI Express hierarchy. > > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > > + * > > + * 7.3. Configuration Transaction Rules (PCI Express specification > 3.0) > > + * 7.3.1. Device Number > > + * > > + * Downstream Ports that do not have ARI Forwarding enabled > must > > + * associate only Device 0 with the device attached to the Logical > Bus > > + * representing the Link from the Port. > > + * > > + * If ARI Forwarding is not enabled on root ports and downstream > > + * ports, only support the devices with slot non-0, regardless of > > + * hotplug or coldplug. > > + */ > > My interpretation of this section of the spec is that if ARI forwarding > is not available, only the normal 8 functions can be accessed for each > device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 > functions, it will need the second device's namespace, eg. devfn 1.0++, > which would not be routed correctly in a non-ari forward capable device. > Yes. > As far as I understand, with this fix you restrict an non-ARI capable > switch to only expose one device? > Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ', and the interface return OK, but the guest os report errors as below: [ 159.035250] Pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4) [ 159.035274] Pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4) [ 159.036517] Pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on due to button press. [ 159.188049] Pciehp 0000:05:00.0:pcie24: Failed to check link status [ 159.201968] Pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4) [ 159.202529] Pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4) Best regards, -Gonglei > Knut > > > + port_type = pcie_cap_get_type(pci_dev); > > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > > + if (PCI_SLOT(dev->devfn) != 0) { > > + error_report("PCIe: Port's ARI Forwarding is > disabled, " > > + "device can't be populated in > slot %d", > > + PCI_SLOT(dev->devfn)); > > + return -1; > > + } > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > static void pci_config_alloc(PCIDevice *pci_dev) > > { > > int config_size = pci_config_size(pci_dev); > > @@ -827,6 +873,11 @@ static PCIDevice > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > > > pci_dev->bus = bus; > > pci_dev->devfn = devfn; > > + > > + if (pci_check_pcie_port(bus, pci_dev)) { > > + return NULL; > > + } > > + > > dma_as = pci_device_iommu_address_space(pci_dev); > > > > memory_region_init_alias(&pci_dev->bus_master_enable_region, >
On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote: > > -----Original Message----- > > From: Knut Omang [mailto:knut.omang@oracle.com] > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and > > downstream ports > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > If ARI Forwarding is disabled, according to PCIe spec > > > section 7.3.1, only slot 0 with the device attached to > > > logic bus representing the link from downstream > > > ports and root ports. > > > > > > So, adding check for PCIe downstream ports and root ports, > > > which avoid useless operation, both hotplug and coldplug. > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > --- > > > hw/pci/pci.c | 51 > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 51 insertions(+) > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > index daeaeac..aa0af0c 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, > > PCIDevice *dev) > > > return 0; > > > } > > > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > > +{ > > > + Object *obj = OBJECT(bus); > > > + > > > + if (pci_bus_is_root(bus)) { > > > + return 0; > > > + } > > > + > > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > > + uint8_t port_type; > > > + /* > > > + * Root ports and downstream ports of switches are the hot > > > + * pluggable ports in a PCI Express hierarchy. > > > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > > > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > > > + * > > > + * 7.3. Configuration Transaction Rules (PCI Express specification > > 3.0) > > > + * 7.3.1. Device Number > > > + * > > > + * Downstream Ports that do not have ARI Forwarding enabled > > must > > > + * associate only Device 0 with the device attached to the Logical > > Bus > > > + * representing the Link from the Port. > > > + * > > > + * If ARI Forwarding is not enabled on root ports and downstream > > > + * ports, only support the devices with slot non-0, regardless of > > > + * hotplug or coldplug. > > > + */ > > > > My interpretation of this section of the spec is that if ARI forwarding > > is not available, only the normal 8 functions can be accessed for each > > device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 > > functions, it will need the second device's namespace, eg. devfn 1.0++, > > which would not be routed correctly in a non-ari forward capable device. > > > Yes. > > > As far as I understand, with this fix you restrict an non-ARI capable > > switch to only expose one device? > > > Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ', > and the interface return OK, but the guest os report errors as below: > > [ 159.035250] Pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4) > [ 159.035274] Pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4) > [ 159.036517] Pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on due to button press. > [ 159.188049] Pciehp 0000:05:00.0:pcie24: Failed to check link status > [ 159.201968] Pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4) > [ 159.202529] Pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4) Ah - I see! I think this also explains why I have been seeing this error and failure to hotplug recently (with an ARIfwd enabled root port and an ARI capable device)... It seems your patch is preventing this from happening in the non-arifwd case, but will still be a problem with a single ARI capable device if ARIfwd is enabled, even if no more than one function is exposed by the device? Best regards, Knut > Best regards, > -Gonglei > > > Knut > > > > > + port_type = pcie_cap_get_type(pci_dev); > > > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > > > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > > > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > > > + if (PCI_SLOT(dev->devfn) != 0) { > > > + error_report("PCIe: Port's ARI Forwarding is > > disabled, " > > > + "device can't be populated in > > slot %d", > > > + PCI_SLOT(dev->devfn)); > > > + return -1; > > > + } > > > + } > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static void pci_config_alloc(PCIDevice *pci_dev) > > > { > > > int config_size = pci_config_size(pci_dev); > > > @@ -827,6 +873,11 @@ static PCIDevice > > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > > > > > pci_dev->bus = bus; > > > pci_dev->devfn = devfn; > > > + > > > + if (pci_check_pcie_port(bus, pci_dev)) { > > > + return NULL; > > > + } > > > + > > > dma_as = pci_device_iommu_address_space(pci_dev); > > > > > > memory_region_init_alias(&pci_dev->bus_master_enable_region, > > >
> From: Knut Omang [mailto:knut.omang@oracle.com] > Sent: Monday, August 25, 2014 5:51 PM > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and > downstream ports > > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote: > > > -----Original Message----- > > > From: Knut Omang [mailto:knut.omang@oracle.com] > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports > and > > > downstream ports > > > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > If ARI Forwarding is disabled, according to PCIe spec > > > > section 7.3.1, only slot 0 with the device attached to > > > > logic bus representing the link from downstream > > > > ports and root ports. > > > > > > > > So, adding check for PCIe downstream ports and root ports, > > > > which avoid useless operation, both hotplug and coldplug. > > > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > > --- > > > > hw/pci/pci.c | 51 > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 51 insertions(+) > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > index daeaeac..aa0af0c 100644 > > > > --- a/hw/pci/pci.c > > > > +++ b/hw/pci/pci.c > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, > > > PCIDevice *dev) > > > > return 0; > > > > } > > > > > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > > > +{ > > > > + Object *obj = OBJECT(bus); > > > > + > > > > + if (pci_bus_is_root(bus)) { > > > > + return 0; > > > > + } > > > > + > > > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > > > + uint8_t port_type; > > > > + /* > > > > + * Root ports and downstream ports of switches are the hot > > > > + * pluggable ports in a PCI Express hierarchy. > > > > + * PCI Express supports chip-to-chip interconnect, a PCIe link > can > > > > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > > > > + * > > > > + * 7.3. Configuration Transaction Rules (PCI Express > specification > > > 3.0) > > > > + * 7.3.1. Device Number > > > > + * > > > > + * Downstream Ports that do not have ARI Forwarding > enabled > > > must > > > > + * associate only Device 0 with the device attached to the > Logical > > > Bus > > > > + * representing the Link from the Port. > > > > + * > > > > + * If ARI Forwarding is not enabled on root ports and > downstream > > > > + * ports, only support the devices with slot non-0, regardless > of > > > > + * hotplug or coldplug. > > > > + */ > > > > > > My interpretation of this section of the spec is that if ARI forwarding > > > is not available, only the normal 8 functions can be accessed for each > > > device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 > > > functions, it will need the second device's namespace, eg. devfn 1.0++, > > > which would not be routed correctly in a non-ari forward capable device. > > > > > Yes. > > > > > As far as I understand, with this fix you restrict an non-ARI capable > > > switch to only expose one device? > > > > > Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ', > > and the interface return OK, but the guest os report errors as below: > > > > [ 159.035250] Pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4) > > [ 159.035274] Pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4) > > [ 159.036517] Pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on due > to button press. > > [ 159.188049] Pciehp 0000:05:00.0:pcie24: Failed to check link status > > [ 159.201968] Pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4) > > [ 159.202529] Pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4) > > Ah - I see! > I think this also explains why I have been seeing this error and failure > to hotplug recently (with an ARIfwd enabled root port and an ARI capable > device)... > > It seems your patch is preventing this from happening in the non-arifwd > case, but will still be a problem with a single ARI capable device if > ARIfwd is enabled, even if no more than one function is exposed by the > device? > Sorry for my poor English. What's your mean about " a single ARI capable device..." ? Best regards, -Gonglei
On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote: > > From: Knut Omang [mailto:knut.omang@oracle.com] > > Sent: Monday, August 25, 2014 5:51 PM > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and > > downstream ports > > > > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote: > > > > -----Original Message----- > > > > From: Knut Omang [mailto:knut.omang@oracle.com] > > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports > > and > > > > downstream ports > > > > > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > > > If ARI Forwarding is disabled, according to PCIe spec > > > > > section 7.3.1, only slot 0 with the device attached to > > > > > logic bus representing the link from downstream > > > > > ports and root ports. > > > > > > > > > > So, adding check for PCIe downstream ports and root ports, > > > > > which avoid useless operation, both hotplug and coldplug. > > > > > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > > > --- > > > > > hw/pci/pci.c | 51 > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 51 insertions(+) > > > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > > index daeaeac..aa0af0c 100644 > > > > > --- a/hw/pci/pci.c > > > > > +++ b/hw/pci/pci.c > > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, > > > > PCIDevice *dev) > > > > > return 0; > > > > > } > > > > > > > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > > > > +{ > > > > > + Object *obj = OBJECT(bus); > > > > > + > > > > > + if (pci_bus_is_root(bus)) { > > > > > + return 0; > > > > > + } > > > > > + > > > > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > > > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > > > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > > > > + uint8_t port_type; > > > > > + /* > > > > > + * Root ports and downstream ports of switches are the hot > > > > > + * pluggable ports in a PCI Express hierarchy. > > > > > + * PCI Express supports chip-to-chip interconnect, a PCIe link > > can > > > > > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > > > > > + * > > > > > + * 7.3. Configuration Transaction Rules (PCI Express > > specification > > > > 3.0) > > > > > + * 7.3.1. Device Number > > > > > + * > > > > > + * Downstream Ports that do not have ARI Forwarding > > enabled > > > > must > > > > > + * associate only Device 0 with the device attached to the > > Logical > > > > Bus > > > > > + * representing the Link from the Port. > > > > > + * > > > > > + * If ARI Forwarding is not enabled on root ports and > > downstream > > > > > + * ports, only support the devices with slot non-0, regardless > > of > > > > > + * hotplug or coldplug. > > > > > + */ > > > > > > > > My interpretation of this section of the spec is that if ARI forwarding > > > > is not available, only the normal 8 functions can be accessed for each > > > > device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 > > > > functions, it will need the second device's namespace, eg. devfn 1.0++, > > > > which would not be routed correctly in a non-ari forward capable device. > > > > > > > Yes. > > > > > > > As far as I understand, with this fix you restrict an non-ARI capable > > > > switch to only expose one device? > > > > > > > Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ', > > > and the interface return OK, but the guest os report errors as below: > > > > > > [ 159.035250] Pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4) > > > [ 159.035274] Pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4) > > > [ 159.036517] Pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on due > > to button press. > > > [ 159.188049] Pciehp 0000:05:00.0:pcie24: Failed to check link status > > > [ 159.201968] Pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4) > > > [ 159.202529] Pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4) > > > > Ah - I see! > > I think this also explains why I have been seeing this error and failure > > to hotplug recently (with an ARIfwd enabled root port and an ARI capable > > device)... > > > > It seems your patch is preventing this from happening in the non-arifwd > > case, but will still be a problem with a single ARI capable device if > > ARIfwd is enabled, even if no more than one function is exposed by the > > device? > > > Sorry for my poor English. > What's your mean about " a single ARI capable device..." ? I just meant a single device with an ARI PCI express extended capability, as opposed to the situation you describe (if I understand right) where there are multiple PCIe devices involved, each in different slots on the same switch. This can happen if a device provides a potential for more than 8 functions, and as such "cross into" the function space of the next "slot", in my case on a root port which physically speaking has only a single slot, but where the device nonetheless are able to respond to and send requests on devfn 1.x. Knut > Best regards, > -Gonglei
> -----Original Message----- > From: Knut Omang [mailto:knut.omang@oracle.com] > Sent: Monday, August 25, 2014 7:02 PM > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and > downstream ports > > On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote: > > > From: Knut Omang [mailto:knut.omang@oracle.com] > > > Sent: Monday, August 25, 2014 5:51 PM > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports > and > > > downstream ports > > > > > > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote: > > > > > -----Original Message----- > > > > > From: Knut Omang [mailto:knut.omang@oracle.com] > > > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root > ports > > > and > > > > > downstream ports > > > > > > > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > > > > > If ARI Forwarding is disabled, according to PCIe spec > > > > > > section 7.3.1, only slot 0 with the device attached to > > > > > > logic bus representing the link from downstream > > > > > > ports and root ports. > > > > > > > > > > > > So, adding check for PCIe downstream ports and root ports, > > > > > > which avoid useless operation, both hotplug and coldplug. > > > > > > > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > > > > --- > > > > > > hw/pci/pci.c | 51 > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 51 insertions(+) > > > > > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > > > index daeaeac..aa0af0c 100644 > > > > > > --- a/hw/pci/pci.c > > > > > > +++ b/hw/pci/pci.c > > > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus > *bus, > > > > > PCIDevice *dev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > > > > > +{ > > > > > > + Object *obj = OBJECT(bus); > > > > > > + > > > > > > + if (pci_bus_is_root(bus)) { > > > > > > + return 0; > > > > > > + } > > > > > > + > > > > > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > > > > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > > > > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > > > > > + uint8_t port_type; > > > > > > + /* > > > > > > + * Root ports and downstream ports of switches are the > hot > > > > > > + * pluggable ports in a PCI Express hierarchy. > > > > > > + * PCI Express supports chip-to-chip interconnect, a PCIe > link > > > can > > > > > > + * only connect one pci device/Switch/EndPoint or > PCI-bridge. > > > > > > + * > > > > > > + * 7.3. Configuration Transaction Rules (PCI Express > > > specification > > > > > 3.0) > > > > > > + * 7.3.1. Device Number > > > > > > + * > > > > > > + * Downstream Ports that do not have ARI Forwarding > > > enabled > > > > > must > > > > > > + * associate only Device 0 with the device attached to the > > > Logical > > > > > Bus > > > > > > + * representing the Link from the Port. > > > > > > + * > > > > > > + * If ARI Forwarding is not enabled on root ports and > > > downstream > > > > > > + * ports, only support the devices with slot non-0, > regardless > > > of > > > > > > + * hotplug or coldplug. > > > > > > + */ > > > > > > > > > > My interpretation of this section of the spec is that if ARI forwarding > > > > > is not available, only the normal 8 functions can be accessed for each > > > > > device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 > > > > > functions, it will need the second device's namespace, eg. devfn 1.0++, > > > > > which would not be routed correctly in a non-ari forward capable device. > > > > > > > > > Yes. > > > > > > > > > As far as I understand, with this fix you restrict an non-ARI capable > > > > > switch to only expose one device? > > > > > > > > > Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ', > > > > and the interface return OK, but the guest os report errors as below: > > > > > > > > [ 159.035250] Pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4) > > > > [ 159.035274] Pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4) > > > > [ 159.036517] Pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on > due > > > to button press. > > > > [ 159.188049] Pciehp 0000:05:00.0:pcie24: Failed to check link status > > > > [ 159.201968] Pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4) > > > > [ 159.202529] Pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4) > > > > > > Ah - I see! > > > I think this also explains why I have been seeing this error and failure > > > to hotplug recently (with an ARIfwd enabled root port and an ARI capable > > > device)... > > > > > > It seems your patch is preventing this from happening in the non-arifwd > > > case, but will still be a problem with a single ARI capable device if > > > ARIfwd is enabled, even if no more than one function is exposed by the > > > device? > > > > > Sorry for my poor English. > > What's your mean about " a single ARI capable device..." ? > > I just meant a single device with an ARI PCI express extended > capability, as opposed to the situation you describe (if I understand > right) where there are multiple PCIe devices involved, each in different > slots on the same switch. > OK. Thanks for your explanation. > This can happen if a device provides a potential for more than 8 > functions, and as such "cross into" the function space of the next > "slot", in my case on a root port which physically speaking has only a > single slot, but where the device nonetheless are able to respond to and > send requests on devfn 1.x. > I think you are right. But I haven't tested this case, TBH. Knut, could you tell me how to enable the ARI Forwarding for a PCIe device? Just call the pcie_ari_init function? Which kind of device do you use? Thanks! Best regards, -Gonglei > Knut > > > Best regards, > > -Gonglei >
On Thu, Aug 21, 2014 at 05:47:46PM +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > If ARI Forwarding is disabled, according to PCIe spec > section 7.3.1, only slot 0 with the device attached to > logic bus representing the link from downstream > ports and root ports. > > So, adding check for PCIe downstream ports and root ports, > which avoid useless operation, both hotplug and coldplug. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/pci/pci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index daeaeac..aa0af0c 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev) > return 0; > } > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > +{ > + Object *obj = OBJECT(bus); > + > + if (pci_bus_is_root(bus)) { > + return 0; > + } > + > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > + DeviceState *parent = qbus_get_parent(BUS(obj)); > + PCIDevice *pci_dev = PCI_DEVICE(parent); > + uint8_t port_type; > + /* > + * Root ports and downstream ports of switches are the hot > + * pluggable ports in a PCI Express hierarchy. > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > + * > + * 7.3. Configuration Transaction Rules (PCI Express specification 3.0) > + * 7.3.1. Device Number > + * > + * Downstream Ports that do not have ARI Forwarding enabled must > + * associate only Device 0 with the device attached to the Logical Bus > + * representing the Link from the Port. > + * > + * If ARI Forwarding is not enabled on root ports and downstream > + * ports, only support the devices with slot non-0, regardless of > + * hotplug or coldplug. > + */ > + port_type = pcie_cap_get_type(pci_dev); > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > + if (!pcie_cap_is_ari_enabled(pci_dev)) { Won't this mean cold-plugging devices is broken? I guess you could check for ARI capability instead. > + if (PCI_SLOT(dev->devfn) != 0) { > + error_report("PCIe: Port's ARI Forwarding is disabled, " > + "device can't be populated in slot %d", > + PCI_SLOT(dev->devfn)); > + return -1; > + } > + } > + } > + } > + > + return 0; > +} > + > static void pci_config_alloc(PCIDevice *pci_dev) > { > int config_size = pci_config_size(pci_dev); > @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > pci_dev->bus = bus; > pci_dev->devfn = devfn; > + > + if (pci_check_pcie_port(bus, pci_dev)) { > + return NULL; > + } > + > dma_as = pci_device_iommu_address_space(pci_dev); > > memory_region_init_alias(&pci_dev->bus_master_enable_region, > -- > 1.7.12.4 >
> From: Michael S. Tsirkin [mailto:mst@redhat.com] > Subject: Re: [PATCH v2 2/2] pci: add check for pcie root ports and downstream > ports > > On Thu, Aug 21, 2014 at 05:47:46PM +0800, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > If ARI Forwarding is disabled, according to PCIe spec > > section 7.3.1, only slot 0 with the device attached to > > logic bus representing the link from downstream > > ports and root ports. > > > > So, adding check for PCIe downstream ports and root ports, > > which avoid useless operation, both hotplug and coldplug. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/pci/pci.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index daeaeac..aa0af0c 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, > PCIDevice *dev) > > return 0; > > } > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > +{ > > + Object *obj = OBJECT(bus); > > + > > + if (pci_bus_is_root(bus)) { > > + return 0; > > + } > > + > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > + uint8_t port_type; > > + /* > > + * Root ports and downstream ports of switches are the hot > > + * pluggable ports in a PCI Express hierarchy. > > + * PCI Express supports chip-to-chip interconnect, a PCIe link can > > + * only connect one pci device/Switch/EndPoint or PCI-bridge. > > + * > > + * 7.3. Configuration Transaction Rules (PCI Express specification > 3.0) > > + * 7.3.1. Device Number > > + * > > + * Downstream Ports that do not have ARI Forwarding enabled > must > > + * associate only Device 0 with the device attached to the Logical > Bus > > + * representing the Link from the Port. > > + * > > + * If ARI Forwarding is not enabled on root ports and downstream > > + * ports, only support the devices with slot non-0, regardless of > > + * hotplug or coldplug. > > + */ > > + port_type = pcie_cap_get_type(pci_dev); > > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > > Won't this mean cold-plugging devices is broken? Yes. If root ports and downstream ports' ARP forwarding is disabled. The device with 'slot > 0' will not recognized by guest os. > I guess you could check for ARI capability instead. > You mean that we check the PCIe devices with ARI capability, but not root ports and downstream ports? If switch ports don't enable ARI Forwarding, we should not permit a device with 'slot > 0'. Of course, you said the scenario is also should be checked too. Maybe I can add a if condition like: If (!pcie_cap_is_arifwd_enabled(pci_dev) || !pcie_device_ari_cap_set(dev)) { error_report(); return; } Any comments? Thanks! Best regards, -Gonglei > > + if (PCI_SLOT(dev->devfn) != 0) { > > + error_report("PCIe: Port's ARI Forwarding is > disabled, " > > + "device can't be populated in > slot %d", > > + PCI_SLOT(dev->devfn)); > > + return -1; > > + } > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > static void pci_config_alloc(PCIDevice *pci_dev) > > { > > int config_size = pci_config_size(pci_dev); > > @@ -827,6 +873,11 @@ static PCIDevice > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > > > pci_dev->bus = bus; > > pci_dev->devfn = devfn; > > + > > + if (pci_check_pcie_port(bus, pci_dev)) { > > + return NULL; > > + } > > + > > dma_as = pci_device_iommu_address_space(pci_dev); > > > > memory_region_init_alias(&pci_dev->bus_master_enable_region, > > -- > > 1.7.12.4 > >
On Mon, 2014-08-25 at 11:23 +0000, Gonglei (Arei) wrote: > > -----Original Message----- > > From: Knut Omang [mailto:knut.omang@oracle.com] > > Sent: Monday, August 25, 2014 7:02 PM > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and > > downstream ports > > > > On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote: > > > > From: Knut Omang [mailto:knut.omang@oracle.com] > > > > Sent: Monday, August 25, 2014 5:51 PM > > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports > > and > > > > downstream ports > > > > > > > > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote: > > > > > > -----Original Message----- > > > > > > From: Knut Omang [mailto:knut.omang@oracle.com] > > > > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root > > ports > > > > and > > > > > > downstream ports > > > > > > > > > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote: > > > > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > > > > > > > If ARI Forwarding is disabled, according to PCIe spec > > > > > > > section 7.3.1, only slot 0 with the device attached to > > > > > > > logic bus representing the link from downstream > > > > > > > ports and root ports. > > > > > > > > > > > > > > So, adding check for PCIe downstream ports and root ports, > > > > > > > which avoid useless operation, both hotplug and coldplug. > > > > > > > > > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > > > > > --- > > > > > > > hw/pci/pci.c | 51 > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > 1 file changed, 51 insertions(+) > > > > > > > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > > > > index daeaeac..aa0af0c 100644 > > > > > > > --- a/hw/pci/pci.c > > > > > > > +++ b/hw/pci/pci.c > > > > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus > > *bus, > > > > > > PCIDevice *dev) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > > > > > > > +{ > > > > > > > + Object *obj = OBJECT(bus); > > > > > > > + > > > > > > > + if (pci_bus_is_root(bus)) { > > > > > > > + return 0; > > > > > > > + } > > > > > > > + > > > > > > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > > > > > > + DeviceState *parent = qbus_get_parent(BUS(obj)); > > > > > > > + PCIDevice *pci_dev = PCI_DEVICE(parent); > > > > > > > + uint8_t port_type; > > > > > > > + /* > > > > > > > + * Root ports and downstream ports of switches are the > > hot > > > > > > > + * pluggable ports in a PCI Express hierarchy. > > > > > > > + * PCI Express supports chip-to-chip interconnect, a PCIe > > link > > > > can > > > > > > > + * only connect one pci device/Switch/EndPoint or > > PCI-bridge. > > > > > > > + * > > > > > > > + * 7.3. Configuration Transaction Rules (PCI Express > > > > specification > > > > > > 3.0) > > > > > > > + * 7.3.1. Device Number > > > > > > > + * > > > > > > > + * Downstream Ports that do not have ARI Forwarding > > > > enabled > > > > > > must > > > > > > > + * associate only Device 0 with the device attached to the > > > > Logical > > > > > > Bus > > > > > > > + * representing the Link from the Port. > > > > > > > + * > > > > > > > + * If ARI Forwarding is not enabled on root ports and > > > > downstream > > > > > > > + * ports, only support the devices with slot non-0, > > regardless > > > > of > > > > > > > + * hotplug or coldplug. > > > > > > > + */ > > > > > > > > > > > > My interpretation of this section of the spec is that if ARI forwarding > > > > > > is not available, only the normal 8 functions can be accessed for each > > > > > > device, eg. device/functions 0.0 -> 0.7 - if a device has more than 8 > > > > > > functions, it will need the second device's namespace, eg. devfn 1.0++, > > > > > > which would not be routed correctly in a non-ari forward capable device. > > > > > > > > > > > Yes. > > > > > > > > > > > As far as I understand, with this fix you restrict an non-ARI capable > > > > > > switch to only expose one device? > > > > > > > > > > > Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ', > > > > > and the interface return OK, but the guest os report errors as below: > > > > > > > > > > [ 159.035250] Pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4) > > > > > [ 159.035274] Pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4) > > > > > [ 159.036517] Pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on > > due > > > > to button press. > > > > > [ 159.188049] Pciehp 0000:05:00.0:pcie24: Failed to check link status > > > > > [ 159.201968] Pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4) > > > > > [ 159.202529] Pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4) > > > > > > > > Ah - I see! > > > > I think this also explains why I have been seeing this error and failure > > > > to hotplug recently (with an ARIfwd enabled root port and an ARI capable > > > > device)... > > > > > > > > It seems your patch is preventing this from happening in the non-arifwd > > > > case, but will still be a problem with a single ARI capable device if > > > > ARIfwd is enabled, even if no more than one function is exposed by the > > > > device? > > > > > > > Sorry for my poor English. > > > What's your mean about " a single ARI capable device..." ? > > > > I just meant a single device with an ARI PCI express extended > > capability, as opposed to the situation you describe (if I understand > > right) where there are multiple PCIe devices involved, each in different > > slots on the same switch. > > > OK. Thanks for your explanation. > > > This can happen if a device provides a potential for more than 8 > > functions, and as such "cross into" the function space of the next > > "slot", in my case on a root port which physically speaking has only a > > single slot, but where the device nonetheless are able to respond to and > > send requests on devfn 1.x. > > > I think you are right. But I haven't tested this case, TBH. > Knut, could you tell me how to enable the ARI Forwarding for a PCIe device? > Just call the pcie_ari_init function? That would add the ARI extended capability to the list of PCIe capabilities in config space, but then you need to have a device with enough functions to actually need ARI, eg. more than 8. > Which kind of device do you use? Thanks! This is where the SR/IOV patch comes in, working on it,... Knut > > Best regards, > -Gonglei > > > Knut > > > > > Best regards, > > > -Gonglei > > >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index daeaeac..aa0af0c 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev) return 0; } +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) +{ + Object *obj = OBJECT(bus); + + if (pci_bus_is_root(bus)) { + return 0; + } + + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { + DeviceState *parent = qbus_get_parent(BUS(obj)); + PCIDevice *pci_dev = PCI_DEVICE(parent); + uint8_t port_type; + /* + * Root ports and downstream ports of switches are the hot + * pluggable ports in a PCI Express hierarchy. + * PCI Express supports chip-to-chip interconnect, a PCIe link can + * only connect one pci device/Switch/EndPoint or PCI-bridge. + * + * 7.3. Configuration Transaction Rules (PCI Express specification 3.0) + * 7.3.1. Device Number + * + * Downstream Ports that do not have ARI Forwarding enabled must + * associate only Device 0 with the device attached to the Logical Bus + * representing the Link from the Port. + * + * If ARI Forwarding is not enabled on root ports and downstream + * ports, only support the devices with slot non-0, regardless of + * hotplug or coldplug. + */ + port_type = pcie_cap_get_type(pci_dev); + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || + port_type == PCI_EXP_TYPE_ROOT_PORT) { + if (!pcie_cap_is_ari_enabled(pci_dev)) { + if (PCI_SLOT(dev->devfn) != 0) { + error_report("PCIe: Port's ARI Forwarding is disabled, " + "device can't be populated in slot %d", + PCI_SLOT(dev->devfn)); + return -1; + } + } + } + } + + return 0; +} + static void pci_config_alloc(PCIDevice *pci_dev) { int config_size = pci_config_size(pci_dev); @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, pci_dev->bus = bus; pci_dev->devfn = devfn; + + if (pci_check_pcie_port(bus, pci_dev)) { + return NULL; + } + dma_as = pci_device_iommu_address_space(pci_dev); memory_region_init_alias(&pci_dev->bus_master_enable_region,