Patchwork [07/10] versatile_pci: Implement the correct PCI IRQ mapping

login
register
mail settings
Submitter Peter Maydell
Date March 24, 2013, 11:32 a.m.
Message ID <1364124760-5794-8-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/230432/
State New
Headers show

Comments

Peter Maydell - March 24, 2013, 11:32 a.m.
Implement the correct IRQ mapping for the Versatile PCI controller; it
differs between realview and versatile boards, but the previous QEMU
implementation was correct only for the first PCI card on a versatile
board, since we weren't swizzling IRQs based on the slot number.

Note that this change will break any uses of PCI on Linux kernels which
have an equivalent bug (since they have effectively only been tested
against QEMU, not real hardware). Unfortunately this currently means
"all Linux kernels" and "all uses of versatilepb with a hard disk"
since we default to a PCI SCSI controller.

We therefore provide a property for enabling the old broken IRQ mapping;
this can be enabled with the command line option:
  -global versatile_pci.broken-irq-mapping=1

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)
Michael S. Tsirkin - March 25, 2013, 12:12 p.m.
On Sun, Mar 24, 2013 at 11:32:37AM +0000, Peter Maydell wrote:
> Implement the correct IRQ mapping for the Versatile PCI controller; it
> differs between realview and versatile boards, but the previous QEMU
> implementation was correct only for the first PCI card on a versatile
> board, since we weren't swizzling IRQs based on the slot number.
> 
> Note that this change will break any uses of PCI on Linux kernels which
> have an equivalent bug (since they have effectively only been tested
> against QEMU, not real hardware). Unfortunately this currently means
> "all Linux kernels" and "all uses of versatilepb with a hard disk"
> since we default to a PCI SCSI controller.
> 
> We therefore provide a property for enabling the old broken IRQ mapping;
> this can be enabled with the command line option:
>   -global versatile_pci.broken-irq-mapping=1
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/versatile_pci.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index 576e619..7739f4c 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -26,6 +26,7 @@ typedef struct {
>  
>      /* Constant for life of device: */
>      int realview;
> +    uint8_t broken_irq_mapping;
>  } PCIVPBState;
>  
>  #define TYPE_VERSATILE_PCI "versatile_pci"
> @@ -61,11 +62,52 @@ static const MemoryRegionOps pci_vpb_config_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
> +static int pci_vpb_broken_map_irq(PCIDevice *d, int irq_num)
>  {
> +    /* Map IRQs as old and buggy versions of QEMU have done in the past;
> +     * this is not how hardware behaves, and it will not work with guests
> +     * which drive the hardware correctly, but it allows us to work with
> +     * buggy Linux kernels which were written against the buggy QEMU.
> +     */
>      return irq_num;
>  }
>  
> +static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
> +{
> +    /* Slot to IRQ mapping for RealView Platform Baseboard 926 backplane
> +     *      name    slot    IntA    IntB    IntC    IntD
> +     *      A       31      IRQ28   IRQ29   IRQ30   IRQ27
> +     *      B       30      IRQ27   IRQ28   IRQ29   IRQ30
> +     *      C       29      IRQ30   IRQ27   IRQ28   IRQ29
> +     * Slot C is for the host bridge; A and B the peripherals.
> +     * Our output irqs 0..3 correspond to the baseboard's 27..30.
> +     *
> +     * This mapping function takes account of an oddity in the PB926
> +     * board wiring, where the FPGA's P_nINTA input is connected to
> +     * the INTB connection on the board PCI edge connector, P_nINTB
> +     * is connected to INTC, and so on, so everything is one number
> +     * further round from where you might expect.
> +     */
> +    return (PCI_SLOT(d->devfn) + irq_num - 2) % PCI_NUM_PINS;

It seems this can be a bit shorter:
	pci_swizzle_map_irq_fn(d, irq_num - 2)
and below irq_num - 1 ?

> +}
> +
> +static int pci_vpb_rv_map_irq(PCIDevice *d, int irq_num)
> +{
> +    /* Slot to IRQ mapping for RealView EB and PB1176 backplane
> +     *      name    slot    IntA    IntB    IntC    IntD
> +     *      A       31      IRQ50   IRQ51   IRQ48   IRQ49
> +     *      B       30      IRQ49   IRQ50   IRQ51   IRQ48
> +     *      C       29      IRQ48   IRQ49   IRQ50   IRQ51
> +     * Slot C is for the host bridge; A and B the peripherals.
> +     * Our output irqs 0..3 correspond to the baseboard's 48..51.
> +     *
> +     * The PB1176 and EB boards don't have the PB926 wiring oddity
> +     * described above; P_nINTA connects to INTA, P_nINTB to INTB
> +     * and so on, which is why this mapping function is different.
> +     */
> +    return (PCI_SLOT(d->devfn) + irq_num - 1) % PCI_NUM_PINS;
> +}
> +
>  static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
>  {
>      qemu_irq *pic = opaque;
> @@ -95,13 +137,22 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
>  {
>      PCIVPBState *s = PCI_VPB(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    pci_map_irq_fn mapfn;
>      int i;
>  
>      for (i = 0; i < 4; i++) {
>          sysbus_init_irq(sbd, &s->irq[i]);
>      }
>  
> -    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, pci_vpb_map_irq, s->irq, 4);
> +    if (s->broken_irq_mapping) {
> +        mapfn = pci_vpb_broken_map_irq;
> +    } else if (s->realview) {
> +        mapfn = pci_vpb_rv_map_irq;
> +    } else {
> +        mapfn = pci_vpb_map_irq;
> +    }
> +
> +    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
>  
>      /* ??? Register memory space.  */
>  
> @@ -154,11 +205,17 @@ static const TypeInfo versatile_pci_host_info = {
>      .class_init    = versatile_pci_host_class_init,
>  };
>  
> +static Property pci_vpb_properties[] = {
> +    DEFINE_PROP_UINT8("broken-irq-mapping", PCIVPBState, broken_irq_mapping, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  static void pci_vpb_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pci_vpb_realize;
> +    dc->props = pci_vpb_properties;
>  }
>  
>  static const TypeInfo pci_vpb_info = {
> -- 
> 1.7.9.5
Peter Maydell - March 25, 2013, 12:17 p.m.
On 25 March 2013 12:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Mar 24, 2013 at 11:32:37AM +0000, Peter Maydell wrote:
>> +    return (PCI_SLOT(d->devfn) + irq_num - 2) % PCI_NUM_PINS;
>
> It seems this can be a bit shorter:
>         pci_swizzle_map_irq_fn(d, irq_num - 2)
> and below irq_num - 1 ?

Yes (though does pci_swizzle_map_irq_fn() accept negative
pin values deliberately or by fluke? it might be better to
use irq_num + 2 / + 3 , maybe.)

-- PMM
Michael S. Tsirkin - March 25, 2013, 12:28 p.m.
On Mon, Mar 25, 2013 at 12:17:39PM +0000, Peter Maydell wrote:
> On 25 March 2013 12:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Mar 24, 2013 at 11:32:37AM +0000, Peter Maydell wrote:
> >> +    return (PCI_SLOT(d->devfn) + irq_num - 2) % PCI_NUM_PINS;
> >
> > It seems this can be a bit shorter:
> >         pci_swizzle_map_irq_fn(d, irq_num - 2)
> > and below irq_num - 1 ?
> 
> Yes (though does pci_swizzle_map_irq_fn() accept negative
> pin values deliberately or by fluke? it might be better to
> use irq_num + 2 / + 3 , maybe.)
> 
> -- PMM

Yes, I prefer + too. The use of - here gave me pause though I figured
out it's all right in the end.

Patch

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 576e619..7739f4c 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -26,6 +26,7 @@  typedef struct {
 
     /* Constant for life of device: */
     int realview;
+    uint8_t broken_irq_mapping;
 } PCIVPBState;
 
 #define TYPE_VERSATILE_PCI "versatile_pci"
@@ -61,11 +62,52 @@  static const MemoryRegionOps pci_vpb_config_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
+static int pci_vpb_broken_map_irq(PCIDevice *d, int irq_num)
 {
+    /* Map IRQs as old and buggy versions of QEMU have done in the past;
+     * this is not how hardware behaves, and it will not work with guests
+     * which drive the hardware correctly, but it allows us to work with
+     * buggy Linux kernels which were written against the buggy QEMU.
+     */
     return irq_num;
 }
 
+static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
+{
+    /* Slot to IRQ mapping for RealView Platform Baseboard 926 backplane
+     *      name    slot    IntA    IntB    IntC    IntD
+     *      A       31      IRQ28   IRQ29   IRQ30   IRQ27
+     *      B       30      IRQ27   IRQ28   IRQ29   IRQ30
+     *      C       29      IRQ30   IRQ27   IRQ28   IRQ29
+     * Slot C is for the host bridge; A and B the peripherals.
+     * Our output irqs 0..3 correspond to the baseboard's 27..30.
+     *
+     * This mapping function takes account of an oddity in the PB926
+     * board wiring, where the FPGA's P_nINTA input is connected to
+     * the INTB connection on the board PCI edge connector, P_nINTB
+     * is connected to INTC, and so on, so everything is one number
+     * further round from where you might expect.
+     */
+    return (PCI_SLOT(d->devfn) + irq_num - 2) % PCI_NUM_PINS;
+}
+
+static int pci_vpb_rv_map_irq(PCIDevice *d, int irq_num)
+{
+    /* Slot to IRQ mapping for RealView EB and PB1176 backplane
+     *      name    slot    IntA    IntB    IntC    IntD
+     *      A       31      IRQ50   IRQ51   IRQ48   IRQ49
+     *      B       30      IRQ49   IRQ50   IRQ51   IRQ48
+     *      C       29      IRQ48   IRQ49   IRQ50   IRQ51
+     * Slot C is for the host bridge; A and B the peripherals.
+     * Our output irqs 0..3 correspond to the baseboard's 48..51.
+     *
+     * The PB1176 and EB boards don't have the PB926 wiring oddity
+     * described above; P_nINTA connects to INTA, P_nINTB to INTB
+     * and so on, which is why this mapping function is different.
+     */
+    return (PCI_SLOT(d->devfn) + irq_num - 1) % PCI_NUM_PINS;
+}
+
 static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
 {
     qemu_irq *pic = opaque;
@@ -95,13 +137,22 @@  static void pci_vpb_realize(DeviceState *dev, Error **errp)
 {
     PCIVPBState *s = PCI_VPB(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    pci_map_irq_fn mapfn;
     int i;
 
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(sbd, &s->irq[i]);
     }
 
-    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, pci_vpb_map_irq, s->irq, 4);
+    if (s->broken_irq_mapping) {
+        mapfn = pci_vpb_broken_map_irq;
+    } else if (s->realview) {
+        mapfn = pci_vpb_rv_map_irq;
+    } else {
+        mapfn = pci_vpb_map_irq;
+    }
+
+    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
 
     /* ??? Register memory space.  */
 
@@ -154,11 +205,17 @@  static const TypeInfo versatile_pci_host_info = {
     .class_init    = versatile_pci_host_class_init,
 };
 
+static Property pci_vpb_properties[] = {
+    DEFINE_PROP_UINT8("broken-irq-mapping", PCIVPBState, broken_irq_mapping, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void pci_vpb_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pci_vpb_realize;
+    dc->props = pci_vpb_properties;
 }
 
 static const TypeInfo pci_vpb_info = {