[Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Alex Williamson]

Rather than have everyone call pci_bridge_map_irq() themselves and
come up with incorrect mapping functions let's use the default PCI
defined swizzle function unless told otherwise.  Then we can also
clean out the duplicate function in pci_bridge_dev.  Tested with an
assigned device behind a PCIe switch behind a PCIe root port at
addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
option to find devices behind PCIe ports if not addr=0.0.  Windows
finds them but won't use them (code 10).

This replaces the ioh3420 & xio3130 upstream/downstream patches.
Thanks,

Alex

---

Alex Williamson (2):
      pci_bridge: Use a default map_irq function
      pci_bridge: Remove duplicate IRQ swizzle function


 hw/pci/pci_bridge.c |    2 +-
 hw/pci_bridge_dev.c |    9 ---------
 2 files changed, 1 insertion(+), 10 deletions(-)

[Qemu-devel] [PATCH 1/2] pci_bridge: Use a default map_irq function

[Alex Williamson]

The PCI bridge spec defines a default swizzle for translating INTx
IRQs from secondary bus to primary.  Use this by default for any
bridge that doesn't set a function.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci/pci_bridge.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 84e7c19..5d27fa2 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -367,7 +367,7 @@ int pci_bridge_initfn(PCIDevice *dev)
     qbus_create_inplace(&sec_bus->qbus, TYPE_PCI_BUS, &dev->qdev,
                         br->bus_name);
     sec_bus->parent_dev = dev;
-    sec_bus->map_irq = br->map_irq;
+    sec_bus->map_irq = br->map_irq ? br->map_irq : pci_swizzle_map_irq_fn;
     sec_bus->address_space_mem = &br->address_space_mem;
     memory_region_init(&br->address_space_mem, "pci_bridge_pci", INT64_MAX);
     sec_bus->address_space_io = &br->address_space_io;

[Qemu-devel] [PATCH 2/2] pci_bridge: Remove duplicate IRQ swizzle function

[Alex Williamson]

pci_bridge_dev_map_irq_fn() is identical to pci_swizzle_map_irq_fn(),
which is now the default for all PCI bridges.  We can therefore remove
this function and the pci_bridge_map_irq() call that used it.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci_bridge_dev.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/pci_bridge_dev.c b/hw/pci_bridge_dev.c
index 1124c53..710f688 100644
--- a/hw/pci_bridge_dev.c
+++ b/hw/pci_bridge_dev.c
@@ -36,21 +36,12 @@ struct PCIBridgeDev {
 };
 typedef struct PCIBridgeDev PCIBridgeDev;
 
-/* Mapping mandated by PCI-to-PCI Bridge architecture specification,
- * revision 1.2 */
-/* Table 9-1: Interrupt Binding for Devices Behind a Bridge */
-static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)
-{
-    return (irq_num + PCI_SLOT(dev->devfn)) % PCI_NUM_PINS;
-}
-
 static int pci_bridge_dev_initfn(PCIDevice *dev)
 {
     PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
     PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br);
     int err;
 
-    pci_bridge_map_irq(br, NULL, pci_bridge_dev_map_irq_fn);
     err = pci_bridge_initfn(dev);
     if (err) {
         goto bridge_error;

Re: [Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Michael S. Tsirkin]

On Thu, Mar 07, 2013 at 04:16:48PM -0700, Alex Williamson wrote:
> Rather than have everyone call pci_bridge_map_irq() themselves and
> come up with incorrect mapping functions let's use the default PCI
> defined swizzle function unless told otherwise.  Then we can also
> clean out the duplicate function in pci_bridge_dev.  Tested with an
> assigned device behind a PCIe switch behind a PCIe root port at
> addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
> option to find devices behind PCIe ports if not addr=0.0.  Windows
> finds them but won't use them (code 10).

I'm guessing this only applies to downstream ports right?
The spec IIRC says that slot is ignored.
The real way is probably by making a device an endpoint
integrated into the switch, so it's behind the upstream port.

> This replaces the ioh3420 & xio3130 upstream/downstream patches.
> Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (2):
>       pci_bridge: Use a default map_irq function
>       pci_bridge: Remove duplicate IRQ swizzle function
> 
> 
>  hw/pci/pci_bridge.c |    2 +-
>  hw/pci_bridge_dev.c |    9 ---------
>  2 files changed, 1 insertion(+), 10 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Michael S. Tsirkin]

On Thu, Mar 07, 2013 at 04:16:48PM -0700, Alex Williamson wrote:
> Rather than have everyone call pci_bridge_map_irq() themselves and
> come up with incorrect mapping functions let's use the default PCI
> defined swizzle function unless told otherwise.  Then we can also
> clean out the duplicate function in pci_bridge_dev.  Tested with an
> assigned device behind a PCIe switch behind a PCIe root port at
> addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
> option to find devices behind PCIe ports if not addr=0.0.  Windows
> finds them but won't use them (code 10).
> 
> This replaces the ioh3420 & xio3130 upstream/downstream patches.
> Thanks,
> 
> Alex

Applied, thanks.

> ---
> 
> Alex Williamson (2):
>       pci_bridge: Use a default map_irq function
>       pci_bridge: Remove duplicate IRQ swizzle function
> 
> 
>  hw/pci/pci_bridge.c |    2 +-
>  hw/pci_bridge_dev.c |    9 ---------
>  2 files changed, 1 insertion(+), 10 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Alex Williamson]

On Sun, 2013-03-10 at 18:16 +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 07, 2013 at 04:16:48PM -0700, Alex Williamson wrote:
> > Rather than have everyone call pci_bridge_map_irq() themselves and
> > come up with incorrect mapping functions let's use the default PCI
> > defined swizzle function unless told otherwise.  Then we can also
> > clean out the duplicate function in pci_bridge_dev.  Tested with an
> > assigned device behind a PCIe switch behind a PCIe root port at
> > addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
> > option to find devices behind PCIe ports if not addr=0.0.  Windows
> > finds them but won't use them (code 10).
> 
> I'm guessing this only applies to downstream ports right?
> The spec IIRC says that slot is ignored.
> The real way is probably by making a device an endpoint
> integrated into the switch, so it's behind the upstream port.

I think that's wrong.  The upstream device of an endpoint behind a
switch should be the downstream port, followed by the upstream port.
That's how we model it today and I think it's accurate.  Slot is
undefined for an upstream port, but that's the PCIe slot, not the
PCI_SLOT(devfn), aka "device", slot.  So I'm not sure how that's
relevant here.

If there's something you want me to change please let me know, otherwise
I'm at a loss how to incorporate changes based on this feedback.
Thanks,

Alex

> > This replaces the ioh3420 & xio3130 upstream/downstream patches.
> > Thanks,
> > 
> > Alex
> > 
> > ---
> > 
> > Alex Williamson (2):
> >       pci_bridge: Use a default map_irq function
> >       pci_bridge: Remove duplicate IRQ swizzle function
> > 
> > 
> >  hw/pci/pci_bridge.c |    2 +-
> >  hw/pci_bridge_dev.c |    9 ---------
> >  2 files changed, 1 insertion(+), 10 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Alex Williamson]

On Sun, 2013-03-10 at 12:13 -0600, Alex Williamson wrote:
> On Sun, 2013-03-10 at 18:16 +0200, Michael S. Tsirkin wrote:
> > On Thu, Mar 07, 2013 at 04:16:48PM -0700, Alex Williamson wrote:
> > > Rather than have everyone call pci_bridge_map_irq() themselves and
> > > come up with incorrect mapping functions let's use the default PCI
> > > defined swizzle function unless told otherwise.  Then we can also
> > > clean out the duplicate function in pci_bridge_dev.  Tested with an
> > > assigned device behind a PCIe switch behind a PCIe root port at
> > > addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
> > > option to find devices behind PCIe ports if not addr=0.0.  Windows
> > > finds them but won't use them (code 10).
> > 
> > I'm guessing this only applies to downstream ports right?
> > The spec IIRC says that slot is ignored.
> > The real way is probably by making a device an endpoint
> > integrated into the switch, so it's behind the upstream port.
> 
> I think that's wrong.  The upstream device of an endpoint behind a
> switch should be the downstream port, followed by the upstream port.
> That's how we model it today and I think it's accurate.  Slot is
> undefined for an upstream port, but that's the PCIe slot, not the
> PCI_SLOT(devfn), aka "device", slot.  So I'm not sure how that's
> relevant here.
> 
> If there's something you want me to change please let me know, otherwise
> I'm at a loss how to incorporate changes based on this feedback.

D'oh, and now I see you've applied it.  If there's any follow-up you're
looking for from the above, let me know.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Michael S. Tsirkin]

On Sun, Mar 10, 2013 at 12:13:27PM -0600, Alex Williamson wrote:
> On Sun, 2013-03-10 at 18:16 +0200, Michael S. Tsirkin wrote:
> > On Thu, Mar 07, 2013 at 04:16:48PM -0700, Alex Williamson wrote:
> > > Rather than have everyone call pci_bridge_map_irq() themselves and
> > > come up with incorrect mapping functions let's use the default PCI
> > > defined swizzle function unless told otherwise.  Then we can also
> > > clean out the duplicate function in pci_bridge_dev.  Tested with an
> > > assigned device behind a PCIe switch behind a PCIe root port at
> > > addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
> > > option to find devices behind PCIe ports if not addr=0.0.  Windows
> > > finds them but won't use them (code 10).
> > 
> > I'm guessing this only applies to downstream ports right?
> > The spec IIRC says that slot is ignored.
> > The real way is probably by making a device an endpoint
> > integrated into the switch, so it's behind the upstream port.
> 
> I think that's wrong.  The upstream device of an endpoint behind a
> switch should be the downstream port, followed by the upstream port.
> That's how we model it today and I think it's accurate.  Slot is
> undefined for an upstream port, but that's the PCIe slot, not the
> PCI_SLOT(devfn), aka "device", slot.  So I'm not sure how that's
> relevant here.
> 
> If there's something you want me to change please let me know, otherwise
> I'm at a loss how to incorporate changes based on this feedback.
> Thanks,
> 
> Alex

I applied the patch, no need to change anything.

I was only talking about testing this: while the spec says it's illegal,
you might be able to test this code by making an endpoint appear on the
switch's internal bus.  This isn't very useful except for testing
because it's out of spec.

> > > This replaces the ioh3420 & xio3130 upstream/downstream patches.
> > > Thanks,
> > > 
> > > Alex
> > > 
> > > ---
> > > 
> > > Alex Williamson (2):
> > >       pci_bridge: Use a default map_irq function
> > >       pci_bridge: Remove duplicate IRQ swizzle function
> > > 
> > > 
> > >  hw/pci/pci_bridge.c |    2 +-
> > >  hw/pci_bridge_dev.c |    9 ---------
> > >  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH 0/2] pci_bridge: Fixup/Cleanup bridge map_irq functions

[Michael S. Tsirkin]

On Sun, Mar 10, 2013 at 12:15:11PM -0600, Alex Williamson wrote:
> On Sun, 2013-03-10 at 12:13 -0600, Alex Williamson wrote:
> > On Sun, 2013-03-10 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > On Thu, Mar 07, 2013 at 04:16:48PM -0700, Alex Williamson wrote:
> > > > Rather than have everyone call pci_bridge_map_irq() themselves and
> > > > come up with incorrect mapping functions let's use the default PCI
> > > > defined swizzle function unless told otherwise.  Then we can also
> > > > clean out the duplicate function in pci_bridge_dev.  Tested with an
> > > > assigned device behind a PCIe switch behind a PCIe root port at
> > > > addresses 0-3.  Note that Linux requires the pci=pcie_scan_all boot
> > > > option to find devices behind PCIe ports if not addr=0.0.  Windows
> > > > finds them but won't use them (code 10).
> > > 
> > > I'm guessing this only applies to downstream ports right?
> > > The spec IIRC says that slot is ignored.
> > > The real way is probably by making a device an endpoint
> > > integrated into the switch, so it's behind the upstream port.
> > 
> > I think that's wrong.  The upstream device of an endpoint behind a
> > switch should be the downstream port, followed by the upstream port.
> > That's how we model it today and I think it's accurate.  Slot is
> > undefined for an upstream port, but that's the PCIe slot, not the
> > PCI_SLOT(devfn), aka "device", slot.  So I'm not sure how that's
> > relevant here.
> > 
> > If there's something you want me to change please let me know, otherwise
> > I'm at a loss how to incorporate changes based on this feedback.
> 
> D'oh, and now I see you've applied it.  If there's any follow-up you're
> looking for from the above, let me know.  Thanks,
> 
> Alex

No, not as such. We should look at PCI bridges now and see what map
function they have and whether it can be removed.  For example,
hw/dec_pci.c has a different one, and I'm guessing it's just a bug.
Need to test and if true, remove it.

-- 
MST