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

[Michael S. Tsirkin]

On Thu, Apr 04, 2013 at 01:58:29PM +0100, 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.
> 
> Since this change would otherwise 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), we implement a mechanism
> for automatically detecting those broken kernels and switching back
> to the old mapping. This works by looking at the values the kernel
> writes to the PCI_INTERRUPT_LINE register in the config space, which
> is effectively the interrupt number the kernel expects the device
> to be using.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Looks good a couple of suggestion on the implementation.

> ---
>  hw/versatile_pci.c |  105 
> +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index 576e619..859fbee 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -13,6 +13,28 @@
>  #include "hw/pci/pci_host.h"
>  #include "exec/address-spaces.h"
>  
> +/* Old and buggy versions of QEMU used the wrong mapping from
> + * PCI IRQs to system interrupt lines. Unfortunately the Linux
> + * kernel also had the corresponding bug in setting up interrupts
> + * (so older kernels work on QEMU and not on real hardware).
> + * We automatically detect these broken kernels and flip back
> + * to the broken irq mapping by spotting guest writes to the
> + * PCI_INTERRUPT_LINE register to see where the guest thinks
> + * interrupts are going to be routed. So we start in state
> + * ASSUME_OK on reset, and transition to either BROKEN or
> + * FORCE_OK at the first write to an INTERRUPT_LINE register for
> + * a slot where broken and correct interrupt mapping would differ.
> + * Once in either BROKEN or FORCE_OK we never transition again;
> + * this allows a newer kernel to use the INTERRUPT_LINE
> + * registers arbitrarily once it has indicated that it isn't
> + * broken in its init code somewhere.
> + */
> +enum {
> +    IRQ_MAPPING_ASSUME_OK = 0,
> +    IRQ_MAPPING_BROKEN = 1,
> +    IRQ_MAPPING_FORCE_OK = 2,

Better to prefix with PCI_VPB_ or something.
And we don't really care what the values are,
can let enum assign what it wants.

> +};
> +
>  typedef struct {
>      PCIHostState parent_obj;
>  
> @@ -26,6 +48,9 @@ typedef struct {
>  
>      /* Constant for life of device: */
>      int realview;
> +
> +    /* Variable state: */
> +    uint8_t irq_mapping;
>  } PCIVPBState;
>  
>  #define TYPE_VERSATILE_PCI "versatile_pci"
> @@ -44,14 +69,27 @@ static inline uint32_t vpb_pci_config_addr(hwaddr addr)
>  static void pci_vpb_config_write(void *opaque, hwaddr addr,
>                                   uint64_t val, unsigned size)
>  {
> -    pci_data_write(opaque, vpb_pci_config_addr(addr), val, size);
> +    PCIVPBState *s = (PCIVPBState *)opaque;

Please don't cast void *.

> +    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE

> +        && s->irq_mapping == IRQ_MAPPING_ASSUME_OK) {
> +        uint8_t devfn = addr >> 8;
> +        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {

Would it be enough to just detect this for devfn 0?
If yes this happens to be our device here, so we could
cleanly implement a config_write callback, like this:

        pci_default_write_config
        if (!ranger_cover_byte(addr, size, PCI_INTERRUPT_LINE))
                return;
        if (dev->config[PCI_INTERRUPT_LINE] == 27) {
                s->irq_mapping = IRQ_MAPPING_BROKEN;
        } else {
                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
        }
                


> +            if (val == 27) {
> +                s->irq_mapping = IRQ_MAPPING_BROKEN;
> +            } else {
> +                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
> +            }
> +        }
> +    }
> +    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);


>  }
>  
>  static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
>                                      unsigned size)
>  {
> +    PCIVPBState *s = (PCIVPBState *)opaque;
>      uint32_t val;
> -    val = pci_data_read(opaque, vpb_pci_config_addr(addr), size);
> +    val = pci_data_read(&s->pci_bus, vpb_pci_config_addr(addr), size);

Seems cleaner but don't cast void * pointer pls.

>      return val;
>  }
>  
> @@ -63,7 +101,47 @@ static const MemoryRegionOps pci_vpb_config_ops = {
>  
>  static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
>  {
> -    return irq_num;
> +    PCIVPBState *s = container_of(d->bus, PCIVPBState, pci_bus);
> +
> +    if (s->irq_mapping == IRQ_MAPPING_BROKEN) {
> +        /* Legacy broken IRQ mapping for compatibility with old and
> +         * buggy Linux guests
> +         */
> +        return 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_swizzle_map_irq_fn(d, irq_num + 2);
> +}
> +
> +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_swizzle_map_irq_fn(d, irq_num + 3);
>  }
>  
>  static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
> @@ -73,6 +151,13 @@ static void pci_vpb_set_irq(void *opaque, int irq_num, 
> int level)
>      qemu_set_irq(pic[irq_num], level);
>  }
>  
> +static void pci_vpb_reset(DeviceState *d)
> +{
> +    PCIVPBState *s = PCI_VPB(d);
> +
> +    s->irq_mapping = IRQ_MAPPING_ASSUME_OK;
> +}
> +
>  static void pci_vpb_init(Object *obj)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> @@ -95,13 +180,20 @@ 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->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.  */
>  
> @@ -110,10 +202,10 @@ static void pci_vpb_realize(DeviceState *dev, Error 
> **errp)
>       * 1 : PCI config window
>       * 2 : PCI IO window
>       */
> -    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, &s->pci_bus,
> +    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, s,
>                            "pci-vpb-selfconfig", 0x1000000);
>      sysbus_init_mmio(sbd, &s->mem_config);
> -    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, &s->pci_bus,
> +    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, s,
>                            "pci-vpb-config", 0x1000000);
>      sysbus_init_mmio(sbd, &s->mem_config2);
>  
> @@ -159,6 +251,7 @@ static void pci_vpb_class_init(ObjectClass *klass, void 
> *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pci_vpb_realize;
> +    dc->reset = pci_vpb_reset;
>  }
>  
>  static const TypeInfo pci_vpb_info = {
> -- 
> 1.7.9.5