Re: [Qemu-devel] [PATCH 07/10] versatile_pci: Implement the correct PCI IRQ mapping

[Michael S. Tsirkin]

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 <address@hidden>
> ---
>  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