From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55560) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNobi-0008Vg-PN for qemu-devel@nongnu.org; Thu, 04 Apr 2013 14:09:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UNoUs-0005IC-BA for qemu-devel@nongnu.org; Thu, 04 Apr 2013 14:03:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25896) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNoUs-0005I2-0h for qemu-devel@nongnu.org; Thu, 04 Apr 2013 14:02:50 -0400 Date: Thu, 4 Apr 2013 20:03:34 +0300 From: "Michael S. Tsirkin" Message-ID: <20130404170334.GA10837@redhat.com> References: <1365080313-20875-1-git-send-email-peter.maydell@linaro.org> <1365080313-20875-8-git-send-email-peter.maydell@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1365080313-20875-8-git-send-email-peter.maydell@linaro.org> Subject: Re: [Qemu-devel] [PATCH v3 07/11] versatile_pci: Implement the correct PCI IRQ mapping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Arnd Bergmann , patches@linaro.org, Will Deacon , qemu-devel@nongnu.org, Paul Brook , Andreas =?iso-8859-1?Q?F=E4rber?= , Aurelien Jarno 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 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