[Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Peter Maydell]

This patch series fixes a number of serious bugs in our emulation of
the PCI controller found on VersatilePB and the early Realview boards:
 * our interrupt mapping was totally wrong
 * we weren't implementing the PCI memory windows
 * the I/O window wasn't mapped on VersatilePB
 * realview mapped things at the wrong addresses
 * we didn't implement the controller's registers at all
It also updates to some reasonable approximation to QOM best practice,
including subclassing pci_host rather than doing everything by hand.

I haven't implemented support for the SMAP registers (which control
how the controller converts accesses made by bus-mastering PCI
devices into system addresses). For the moment we rely on the fact
that Linux always maps things 1:1. (It wouldn't be too hard to add
SMAP support but it requires changing QEMU's pci code to allow a
controller to pass in a MemoryRegion* for DMA to use instead of
the system address space, so I prefer to leave that for another series.)

Patchset tested on both versatilepb and realview, using a set of
Linux kernel patches written by Arnd Bergmann:
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/029040.html
which were in turn tested against real PB926 and PB1176 hardware.


 * WARNING WARNING *

This patchset will break any use of PCI (including the default SCSI
card) on versatilepb with current Linux kernels, because those kernels
have the matching bug in interrupt mapping to old QEMU.

I've provided 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

(If anybody wants to suggest a better way of handling this please do.)



Peter Maydell (10):
  versatile_pci: Fix hardcoded tabs
  versatile_pci: Expose PCI I/O region on Versatile PB
  versatile_pci: Update to realize and instance init functions
  versatile_pci: Change to subclassing TYPE_PCI_HOST_BRIDGE
  versatile_pci: Use separate PCI I/O space rather than system I/O space
  versatile_pci: Put the host bridge PCI device at slot 29
  versatile_pci: Implement the correct PCI IRQ mapping
  versatile_pci: Implement the PCI controller's control registers
  arm/realview: Fix mapping of PCI regions
  versatile_pci: Expose PCI memory space to system

 hw/arm/realview.c    |   22 +--
 hw/arm/versatilepb.c |   11 +-
 hw/versatile_pci.c   |  368 ++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 344 insertions(+), 57 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 01/10] versatile_pci: Fix hardcoded tabs

[Peter Maydell]

There is just one line in this source file with a hardcoded tab
indent, so just fix it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 0b97a40..16ce5d1 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -104,7 +104,7 @@ static int pci_realview_init(SysBusDevice *dev)
 static int versatile_pci_host_init(PCIDevice *d)
 {
     pci_set_word(d->config + PCI_STATUS,
-		 PCI_STATUS_66MHZ | PCI_STATUS_DEVSEL_MEDIUM);
+                 PCI_STATUS_66MHZ | PCI_STATUS_DEVSEL_MEDIUM);
     pci_set_byte(d->config + PCI_LATENCY_TIMER, 0x10);
     return 0;
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH 02/10] versatile_pci: Expose PCI I/O region on Versatile PB

[Peter Maydell]

Comments in the QEMU source code claim that the version of the PCI
controller on the VersatilePB board doesn't support the PCI I/O
region, but this is incorrect; expose that region, map it in the
correct location, and drop the misleading comments.

This change removes the only currently implemented difference
between the realview-pci and versatile-pci models; however there
are other differences in not-yet-implemented functionality, so we
retain the distinction between the two device types.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/versatilepb.c |    3 +--
 hw/versatile_pci.c   |    8 +++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index baaa265..0d08d15 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -226,14 +226,13 @@ static void versatile_init(QEMUMachineInitArgs *args, int board_id)
     qdev_init_nofail(dev);
     sysbus_mmio_map(busdev, 0, 0x41000000); /* PCI self-config */
     sysbus_mmio_map(busdev, 1, 0x42000000); /* PCI config */
+    sysbus_mmio_map(busdev, 2, 0x43000000); /* PCI I/O */
     sysbus_connect_irq(busdev, 0, sic[27]);
     sysbus_connect_irq(busdev, 1, sic[28]);
     sysbus_connect_irq(busdev, 2, sic[29]);
     sysbus_connect_irq(busdev, 3, sic[30]);
     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
 
-    /* The Versatile PCI bridge does not provide access to PCI IO space,
-       so many of the qemu PCI devices are not useable.  */
     for(n = 0; n < nb_nics; n++) {
         nd = &nd_table[n];
 
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 16ce5d1..1312f46 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -77,7 +77,7 @@ static int pci_vpb_init(SysBusDevice *dev)
     /* Our memory regions are:
      * 0 : PCI self config window
      * 1 : PCI config window
-     * 2 : PCI IO window (realview_pci only)
+     * 2 : PCI IO window
      */
     memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, bus,
                           "pci-vpb-selfconfig", 0x1000000);
@@ -85,10 +85,8 @@ static int pci_vpb_init(SysBusDevice *dev)
     memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
                           "pci-vpb-config", 0x1000000);
     sysbus_init_mmio(dev, &s->mem_config2);
-    if (s->realview) {
-        isa_mmio_setup(&s->isa, 0x0100000);
-        sysbus_init_mmio(dev, &s->isa);
-    }
+    isa_mmio_setup(&s->isa, 0x0100000);
+    sysbus_init_mmio(dev, &s->isa);
 
     pci_create_simple(bus, -1, "versatile_pci_host");
     return 0;
-- 
1.7.9.5

[Qemu-devel] [PATCH 03/10] versatile_pci: Update to realize and instance init functions

[Peter Maydell]

Update the Versatile PCI controller to use a realize function rather
than SysBusDevice::init. To reflect the fact that the 'realview_pci'
class is taking most of its implementation from 'versatile_pci' (and
to make the QOM casts work) we make 'realview_pci' a subclass of
'versatile_pci'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |   50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 1312f46..541f6b5 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -21,6 +21,14 @@ typedef struct {
     MemoryRegion isa;
 } PCIVPBState;
 
+#define TYPE_VERSATILE_PCI "versatile_pci"
+#define PCI_VPB(obj) \
+    OBJECT_CHECK(PCIVPBState, (obj), TYPE_VERSATILE_PCI)
+
+#define TYPE_VERSATILE_PCI_HOST "versatile_pci_host"
+#define PCI_VPB_HOST(obj) \
+    OBJECT_CHECK(PCIDevice, (obj), TYPE_VERSATILE_PCIHOST)
+
 static inline uint32_t vpb_pci_config_addr(hwaddr addr)
 {
     return addr & 0xffffff;
@@ -58,16 +66,17 @@ static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num], level);
 }
 
-static int pci_vpb_init(SysBusDevice *dev)
+static void pci_vpb_realize(DeviceState *dev, Error **errp)
 {
-    PCIVPBState *s = FROM_SYSBUS(PCIVPBState, dev);
+    PCIVPBState *s = PCI_VPB(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     PCIBus *bus;
     int i;
 
     for (i = 0; i < 4; i++) {
-        sysbus_init_irq(dev, &s->irq[i]);
+        sysbus_init_irq(sbd, &s->irq[i]);
     }
-    bus = pci_register_bus(&dev->qdev, "pci",
+    bus = pci_register_bus(dev, "pci",
                            pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
                            get_system_memory(), get_system_io(),
                            PCI_DEVFN(11, 0), 4);
@@ -81,22 +90,14 @@ static int pci_vpb_init(SysBusDevice *dev)
      */
     memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, bus,
                           "pci-vpb-selfconfig", 0x1000000);
-    sysbus_init_mmio(dev, &s->mem_config);
+    sysbus_init_mmio(sbd, &s->mem_config);
     memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
                           "pci-vpb-config", 0x1000000);
-    sysbus_init_mmio(dev, &s->mem_config2);
+    sysbus_init_mmio(sbd, &s->mem_config2);
     isa_mmio_setup(&s->isa, 0x0100000);
-    sysbus_init_mmio(dev, &s->isa);
+    sysbus_init_mmio(sbd, &s->isa);
 
     pci_create_simple(bus, -1, "versatile_pci_host");
-    return 0;
-}
-
-static int pci_realview_init(SysBusDevice *dev)
-{
-    PCIVPBState *s = FROM_SYSBUS(PCIVPBState, dev);
-    s->realview = 1;
-    return pci_vpb_init(dev);
 }
 
 static int versatile_pci_host_init(PCIDevice *d)
@@ -118,7 +119,7 @@ static void versatile_pci_host_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo versatile_pci_host_info = {
-    .name          = "versatile_pci_host",
+    .name          = TYPE_VERSATILE_PCI_HOST,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIDevice),
     .class_init    = versatile_pci_host_class_init,
@@ -126,30 +127,29 @@ static const TypeInfo versatile_pci_host_info = {
 
 static void pci_vpb_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
-    sdc->init = pci_vpb_init;
+    dc->realize = pci_vpb_realize;
 }
 
 static const TypeInfo pci_vpb_info = {
-    .name          = "versatile_pci",
+    .name          = TYPE_VERSATILE_PCI,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(PCIVPBState),
     .class_init    = pci_vpb_class_init,
 };
 
-static void pci_realview_class_init(ObjectClass *klass, void *data)
+static void pci_realview_init(Object *obj)
 {
-    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+    PCIVPBState *s = PCI_VPB(obj);
 
-    sdc->init = pci_realview_init;
+    s->realview = 1;
 }
 
 static const TypeInfo pci_realview_info = {
     .name          = "realview_pci",
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(PCIVPBState),
-    .class_init    = pci_realview_class_init,
+    .parent        = TYPE_VERSATILE_PCI,
+    .instance_init = pci_realview_init,
 };
 
 static void versatile_pci_register_types(void)
-- 
1.7.9.5

[Qemu-devel] [PATCH 04/10] versatile_pci: Change to subclassing TYPE_PCI_HOST_BRIDGE

[Peter Maydell]

Change versatile_pci to subclass TYPE_PCI_HOST_BRIDGE and generally
handle PCI in a more QOM-like fashion.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |   41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 541f6b5..dfd3001 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -9,16 +9,22 @@
 
 #include "hw/sysbus.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "exec/address-spaces.h"
 
 typedef struct {
-    SysBusDevice busdev;
+    PCIHostState parent_obj;
+
     qemu_irq irq[4];
-    int realview;
     MemoryRegion mem_config;
     MemoryRegion mem_config2;
     MemoryRegion isa;
+    PCIBus pci_bus;
+    PCIDevice pci_dev;
+
+    /* Constant for life of device: */
+    int realview;
 } PCIVPBState;
 
 #define TYPE_VERSATILE_PCI "versatile_pci"
@@ -66,20 +72,31 @@ static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num], level);
 }
 
+static void pci_vpb_init(Object *obj)
+{
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    PCIVPBState *s = PCI_VPB(obj);
+
+    pci_bus_new_inplace(&s->pci_bus, DEVICE(obj), "pci",
+                        get_system_memory(), get_system_io(),
+                        PCI_DEVFN(11, 0));
+    h->bus = &s->pci_bus;
+
+    object_initialize(&s->pci_dev, TYPE_VERSATILE_PCI_HOST);
+    qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
+}
+
 static void pci_vpb_realize(DeviceState *dev, Error **errp)
 {
     PCIVPBState *s = PCI_VPB(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    PCIBus *bus;
     int i;
 
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(sbd, &s->irq[i]);
     }
-    bus = pci_register_bus(dev, "pci",
-                           pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
-                           get_system_memory(), get_system_io(),
-                           PCI_DEVFN(11, 0), 4);
+
+    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, pci_vpb_map_irq, s->irq, 4);
 
     /* ??? Register memory space.  */
 
@@ -88,16 +105,17 @@ 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, bus,
+    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, &s->pci_bus,
                           "pci-vpb-selfconfig", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config);
-    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
+    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, &s->pci_bus,
                           "pci-vpb-config", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config2);
     isa_mmio_setup(&s->isa, 0x0100000);
     sysbus_init_mmio(sbd, &s->isa);
 
-    pci_create_simple(bus, -1, "versatile_pci_host");
+    /* TODO Remove once realize propagates to child devices. */
+    object_property_set_bool(OBJECT(&s->pci_dev), true, "realized", errp);
 }
 
 static int versatile_pci_host_init(PCIDevice *d)
@@ -134,8 +152,9 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo pci_vpb_info = {
     .name          = TYPE_VERSATILE_PCI,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(PCIVPBState),
+    .instance_init = pci_vpb_init,
     .class_init    = pci_vpb_class_init,
 };
 
-- 
1.7.9.5

[Qemu-devel] [PATCH 05/10] versatile_pci: Use separate PCI I/O space rather than system I/O space

[Peter Maydell]

Rather than overloading the system I/O space (which doesn't even make
any sense on ARM) for PCI I/O, create an memory region in the PCI
controller and use that to represent the I/O space.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index dfd3001..777e9b1 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -19,7 +19,8 @@ typedef struct {
     qemu_irq irq[4];
     MemoryRegion mem_config;
     MemoryRegion mem_config2;
-    MemoryRegion isa;
+    MemoryRegion pci_io_space;
+    MemoryRegion pci_io_window;
     PCIBus pci_bus;
     PCIDevice pci_dev;
 
@@ -77,8 +78,10 @@ static void pci_vpb_init(Object *obj)
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     PCIVPBState *s = PCI_VPB(obj);
 
+    memory_region_init(&s->pci_io_space, "pci_io", 1ULL << 32);
+
     pci_bus_new_inplace(&s->pci_bus, DEVICE(obj), "pci",
-                        get_system_memory(), get_system_io(),
+                        get_system_memory(), &s->pci_io_space,
                         PCI_DEVFN(11, 0));
     h->bus = &s->pci_bus;
 
@@ -111,8 +114,14 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, &s->pci_bus,
                           "pci-vpb-config", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config2);
-    isa_mmio_setup(&s->isa, 0x0100000);
-    sysbus_init_mmio(sbd, &s->isa);
+
+    /* The window into I/O space is always into a fixed base address;
+     * its size is the same for both realview and versatile.
+     */
+    memory_region_init_alias(&s->pci_io_window, "pci-vbp-io-window",
+                             &s->pci_io_space, 0, 0x100000);
+
+    sysbus_init_mmio(sbd, &s->pci_io_space);
 
     /* TODO Remove once realize propagates to child devices. */
     object_property_set_bool(OBJECT(&s->pci_dev), true, "realized", errp);
-- 
1.7.9.5

[Qemu-devel] [PATCH 06/10] versatile_pci: Put the host bridge PCI device at slot 29

[Peter Maydell]

On real hardware the host bridge appears as a PCI device in slot 29,
so make QEMU put its host bridge in that slot too.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 777e9b1..576e619 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -87,6 +87,8 @@ static void pci_vpb_init(Object *obj)
 
     object_initialize(&s->pci_dev, TYPE_VERSATILE_PCI_HOST);
     qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
+    object_property_set_int(OBJECT(&s->pci_dev), PCI_DEVFN(29, 0), "addr",
+                            NULL);
 }
 
 static void pci_vpb_realize(DeviceState *dev, Error **errp)
-- 
1.7.9.5

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

[Peter Maydell]

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;
+}
+
+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

[Qemu-devel] [PATCH 08/10] versatile_pci: Implement the PCI controller's control registers

[Peter Maydell]

The versatile_pci PCI controller has a set of control registers which
handle the mapping between PCI and system address spaces. Implement
these registers (though for now they have no effect since we don't
implement mapping PCI space into system memory at all).

The most natural order for our sysbus regions has the control
registers at the start, so move all the others down one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/realview.c    |    7 +--
 hw/arm/versatilepb.c |    7 +--
 hw/versatile_pci.c   |  135 ++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 138 insertions(+), 11 deletions(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 5fb490c..ba61d18 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -217,9 +217,10 @@ static void realview_init(QEMUMachineInitArgs *args,
         dev = qdev_create(NULL, "realview_pci");
         busdev = SYS_BUS_DEVICE(dev);
         qdev_init_nofail(dev);
-        sysbus_mmio_map(busdev, 0, 0x61000000); /* PCI self-config */
-        sysbus_mmio_map(busdev, 1, 0x62000000); /* PCI config */
-        sysbus_mmio_map(busdev, 2, 0x63000000); /* PCI I/O */
+        sysbus_mmio_map(busdev, 0, 0x10019000); /* PCI controller registers */
+        sysbus_mmio_map(busdev, 1, 0x61000000); /* PCI self-config */
+        sysbus_mmio_map(busdev, 2, 0x62000000); /* PCI config */
+        sysbus_mmio_map(busdev, 3, 0x63000000); /* PCI I/O */
         sysbus_connect_irq(busdev, 0, pic[48]);
         sysbus_connect_irq(busdev, 1, pic[49]);
         sysbus_connect_irq(busdev, 2, pic[50]);
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 0d08d15..9c9bfde 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -224,9 +224,10 @@ static void versatile_init(QEMUMachineInitArgs *args, int board_id)
     dev = qdev_create(NULL, "versatile_pci");
     busdev = SYS_BUS_DEVICE(dev);
     qdev_init_nofail(dev);
-    sysbus_mmio_map(busdev, 0, 0x41000000); /* PCI self-config */
-    sysbus_mmio_map(busdev, 1, 0x42000000); /* PCI config */
-    sysbus_mmio_map(busdev, 2, 0x43000000); /* PCI I/O */
+    sysbus_mmio_map(busdev, 0, 0x10001000); /* PCI controller regs */
+    sysbus_mmio_map(busdev, 1, 0x41000000); /* PCI self-config */
+    sysbus_mmio_map(busdev, 2, 0x42000000); /* PCI config */
+    sysbus_mmio_map(busdev, 3, 0x43000000); /* PCI I/O */
     sysbus_connect_irq(busdev, 0, sic[27]);
     sysbus_connect_irq(busdev, 1, sic[28]);
     sysbus_connect_irq(busdev, 2, sic[29]);
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 7739f4c..a617675 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -17,6 +17,7 @@ typedef struct {
     PCIHostState parent_obj;
 
     qemu_irq irq[4];
+    MemoryRegion controlregs;
     MemoryRegion mem_config;
     MemoryRegion mem_config2;
     MemoryRegion pci_io_space;
@@ -27,8 +28,27 @@ typedef struct {
     /* Constant for life of device: */
     int realview;
     uint8_t broken_irq_mapping;
+
+    /* Register state: */
+    uint32_t imap[3];
+    uint32_t smap[3];
+    uint32_t selfid;
+    uint32_t flags;
 } PCIVPBState;
 
+static const VMStateDescription pci_vpb_vmstate = {
+    .name = "versatile-pci",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(imap, PCIVPBState, 3),
+        VMSTATE_UINT32_ARRAY(smap, PCIVPBState, 3),
+        VMSTATE_UINT32(selfid, PCIVPBState),
+        VMSTATE_UINT32(flags, PCIVPBState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define TYPE_VERSATILE_PCI "versatile_pci"
 #define PCI_VPB(obj) \
     OBJECT_CHECK(PCIVPBState, (obj), TYPE_VERSATILE_PCI)
@@ -37,6 +57,93 @@ typedef struct {
 #define PCI_VPB_HOST(obj) \
     OBJECT_CHECK(PCIDevice, (obj), TYPE_VERSATILE_PCIHOST)
 
+typedef enum {
+    PCI_IMAP0 = 0x0,
+    PCI_IMAP1 = 0x4,
+    PCI_IMAP2 = 0x8,
+    PCI_SELFID = 0xc,
+    PCI_FLAGS = 0x10,
+    PCI_SMAP0 = 0x14,
+    PCI_SMAP1 = 0x18,
+    PCI_SMAP2 = 0x1c,
+} PCIVPBControlRegs;
+
+static void pci_vpb_reg_write(void *opaque, hwaddr addr,
+                              uint64_t val, unsigned size)
+{
+    PCIVPBState *s = opaque;
+
+    switch (addr) {
+    case PCI_IMAP0:
+    case PCI_IMAP1:
+    case PCI_IMAP2:
+    {
+        int win = (addr - PCI_IMAP0) >> 2;
+        s->imap[win] = val;
+        break;
+    }
+    case PCI_SELFID:
+        s->selfid = val;
+        break;
+    case PCI_FLAGS:
+        s->flags = val;
+        break;
+    case PCI_SMAP0:
+    case PCI_SMAP1:
+    case PCI_SMAP2:
+    {
+        int win = (addr - PCI_SMAP0) >> 2;
+        s->smap[win] = val;
+        break;
+    }
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pci_vpb_reg_write: Bad offset %x\n", (int)addr);
+        break;
+    }
+}
+
+static uint64_t pci_vpb_reg_read(void *opaque, hwaddr addr,
+                                 unsigned size)
+{
+    PCIVPBState *s = opaque;
+
+    switch (addr) {
+    case PCI_IMAP0:
+    case PCI_IMAP1:
+    case PCI_IMAP2:
+    {
+        int win = (addr - PCI_IMAP0) >> 2;
+        return s->imap[win];
+    }
+    case PCI_SELFID:
+        return s->selfid;
+    case PCI_FLAGS:
+        return s->flags;
+    case PCI_SMAP0:
+    case PCI_SMAP1:
+    case PCI_SMAP2:
+    {
+        int win = (addr - PCI_SMAP0) >> 2;
+        return s->smap[win];
+    }
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pci_vpb_reg_read: Bad offset %x\n", (int)addr);
+        return 0;
+    }
+}
+
+static const MemoryRegionOps pci_vpb_reg_ops = {
+    .read = pci_vpb_reg_read,
+    .write = pci_vpb_reg_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static inline uint32_t vpb_pci_config_addr(hwaddr addr)
 {
     return addr & 0xffffff;
@@ -115,6 +222,20 @@ 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->imap[0] = 0;
+    s->imap[1] = 0;
+    s->imap[2] = 0;
+    s->smap[0] = 0;
+    s->smap[1] = 0;
+    s->smap[2] = 0;
+    s->selfid = 0;
+    s->flags = 0;
+}
+
 static void pci_vpb_init(Object *obj)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
@@ -154,13 +275,15 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
 
     pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
 
-    /* ??? Register memory space.  */
-
     /* Our memory regions are:
-     * 0 : PCI self config window
-     * 1 : PCI config window
-     * 2 : PCI IO window
+     * 0 : our control registers
+     * 1 : PCI self config window
+     * 2 : PCI config window
+     * 3 : PCI IO window
      */
+    memory_region_init_io(&s->controlregs, &pci_vpb_reg_ops, s, "pci-vpb-regs",
+                          0x1000);
+    sysbus_init_mmio(sbd, &s->controlregs);
     memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, &s->pci_bus,
                           "pci-vpb-selfconfig", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config);
@@ -216,6 +339,8 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pci_vpb_realize;
     dc->props = pci_vpb_properties;
+    dc->reset = pci_vpb_reset;
+    dc->vmsd = &pci_vpb_vmstate;
 }
 
 static const TypeInfo pci_vpb_info = {
-- 
1.7.9.5

[Qemu-devel] [PATCH 09/10] arm/realview: Fix mapping of PCI regions

[Peter Maydell]

Fix the mapping of the PCI regions for the realview board, which were
all incorrect. (This was never noticed because the Linux kernel
doesn't actually include a PCI driver for the realview boards.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/realview.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index ba61d18..23c968b 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -218,9 +218,9 @@ static void realview_init(QEMUMachineInitArgs *args,
         busdev = SYS_BUS_DEVICE(dev);
         qdev_init_nofail(dev);
         sysbus_mmio_map(busdev, 0, 0x10019000); /* PCI controller registers */
-        sysbus_mmio_map(busdev, 1, 0x61000000); /* PCI self-config */
-        sysbus_mmio_map(busdev, 2, 0x62000000); /* PCI config */
-        sysbus_mmio_map(busdev, 3, 0x63000000); /* PCI I/O */
+        sysbus_mmio_map(busdev, 1, 0x60000000); /* PCI self-config */
+        sysbus_mmio_map(busdev, 2, 0x61000000); /* PCI config */
+        sysbus_mmio_map(busdev, 3, 0x62000000); /* PCI I/O */
         sysbus_connect_irq(busdev, 0, pic[48]);
         sysbus_connect_irq(busdev, 1, pic[49]);
         sysbus_connect_irq(busdev, 2, pic[50]);
@@ -304,12 +304,12 @@ static void realview_init(QEMUMachineInitArgs *args,
     /*  0x58000000 PISMO.  */
     /*  0x5c000000 PISMO.  */
     /* 0x60000000 PCI.  */
-    /* 0x61000000 PCI Self Config.  */
-    /* 0x62000000 PCI Config.  */
-    /* 0x63000000 PCI IO.  */
-    /* 0x64000000 PCI mem 0.  */
-    /* 0x68000000 PCI mem 1.  */
-    /* 0x6c000000 PCI mem 2.  */
+    /* 0x60000000 PCI Self Config.  */
+    /* 0x61000000 PCI Config.  */
+    /* 0x62000000 PCI IO.  */
+    /* 0x63000000 PCI mem 0.  */
+    /* 0x64000000 PCI mem 1.  */
+    /* 0x68000000 PCI mem 2.  */
 
     /* ??? Hack to map an additional page of ram for the secondary CPU
        startup code.  I guess this works on real hardware because the
-- 
1.7.9.5

[Qemu-devel] [PATCH 10/10] versatile_pci: Expose PCI memory space to system

[Peter Maydell]

The VersatilePB's PCI controller exposes the PCI memory space to the
system via three regions controlled by the mapping control registers.
Implement this so that guests can actually use MMIO-BAR PCI cards.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/realview.c    |    3 +++
 hw/arm/versatilepb.c |    3 +++
 hw/versatile_pci.c   |   72 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 23c968b..db41525 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -221,6 +221,9 @@ static void realview_init(QEMUMachineInitArgs *args,
         sysbus_mmio_map(busdev, 1, 0x60000000); /* PCI self-config */
         sysbus_mmio_map(busdev, 2, 0x61000000); /* PCI config */
         sysbus_mmio_map(busdev, 3, 0x62000000); /* PCI I/O */
+        sysbus_mmio_map(busdev, 4, 0x63000000); /* PCI memory window 1 */
+        sysbus_mmio_map(busdev, 5, 0x64000000); /* PCI memory window 2 */
+        sysbus_mmio_map(busdev, 6, 0x68000000); /* PCI memory window 3 */
         sysbus_connect_irq(busdev, 0, pic[48]);
         sysbus_connect_irq(busdev, 1, pic[49]);
         sysbus_connect_irq(busdev, 2, pic[50]);
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 9c9bfde..413f0e9 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -228,6 +228,9 @@ static void versatile_init(QEMUMachineInitArgs *args, int board_id)
     sysbus_mmio_map(busdev, 1, 0x41000000); /* PCI self-config */
     sysbus_mmio_map(busdev, 2, 0x42000000); /* PCI config */
     sysbus_mmio_map(busdev, 3, 0x43000000); /* PCI I/O */
+    sysbus_mmio_map(busdev, 4, 0x44000000); /* PCI memory window 1 */
+    sysbus_mmio_map(busdev, 5, 0x50000000); /* PCI memory window 2 */
+    sysbus_mmio_map(busdev, 6, 0x60000000); /* PCI memory window 3 */
     sysbus_connect_irq(busdev, 0, sic[27]);
     sysbus_connect_irq(busdev, 1, sic[28]);
     sysbus_connect_irq(busdev, 2, sic[29]);
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index a617675..05e1ec8 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -20,14 +20,21 @@ typedef struct {
     MemoryRegion controlregs;
     MemoryRegion mem_config;
     MemoryRegion mem_config2;
+    /* Containers representing the PCI address spaces */
     MemoryRegion pci_io_space;
+    MemoryRegion pci_mem_space;
+    /* Alias regions into PCI address spaces which we expose as sysbus regions.
+     * The offsets into pci_mem_space are controlled by the imap registers.
+     */
     MemoryRegion pci_io_window;
+    MemoryRegion pci_mem_window[3];
     PCIBus pci_bus;
     PCIDevice pci_dev;
 
     /* Constant for life of device: */
     int realview;
     uint8_t broken_irq_mapping;
+    uint32_t mem_win_size[3];
 
     /* Register state: */
     uint32_t imap[3];
@@ -36,10 +43,49 @@ typedef struct {
     uint32_t flags;
 } PCIVPBState;
 
+static void pci_vpb_update_window(PCIVPBState *s, int i)
+{
+    /* Adjust the offset of the alias region we use for
+     * the memory window i to account for a change in the
+     * value of the corresponding IMAP register.
+     * Note that the semantics of the IMAP register differ
+     * for realview and versatile variants of the controller.
+     */
+    hwaddr offset;
+    if (s->realview) {
+        /* Top bits of register (masked according to window size) provide
+         * top bits of PCI address.
+         */
+        offset = s->imap[i] & ~(s->mem_win_size[i] - 1);
+    } else {
+        /* Bottom 4 bits of register provide top 4 bits of PCI address */
+        offset = s->imap[i] << 28;
+    }
+    memory_region_set_alias_offset(&s->pci_mem_window[i], offset);
+}
+
+static void pci_vpb_update_all_windows(PCIVPBState *s)
+{
+    /* Update all alias windows based on the current register state */
+    int i;
+
+    for (i = 0; i < 3; i++) {
+        pci_vpb_update_window(s, i);
+    }
+}
+
+static int pci_vpb_post_load(void *opaque, int version_id)
+{
+    PCIVPBState *s = opaque;
+    pci_vpb_update_all_windows(s);
+    return 0;
+}
+
 static const VMStateDescription pci_vpb_vmstate = {
     .name = "versatile-pci",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = pci_vpb_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(imap, PCIVPBState, 3),
         VMSTATE_UINT32_ARRAY(smap, PCIVPBState, 3),
@@ -80,6 +126,7 @@ static void pci_vpb_reg_write(void *opaque, hwaddr addr,
     {
         int win = (addr - PCI_IMAP0) >> 2;
         s->imap[win] = val;
+        pci_vpb_update_window(s, win);
         break;
     }
     case PCI_SELFID:
@@ -234,6 +281,8 @@ static void pci_vpb_reset(DeviceState *d)
     s->smap[2] = 0;
     s->selfid = 0;
     s->flags = 0;
+
+    pci_vpb_update_all_windows(s);
 }
 
 static void pci_vpb_init(Object *obj)
@@ -242,9 +291,10 @@ static void pci_vpb_init(Object *obj)
     PCIVPBState *s = PCI_VPB(obj);
 
     memory_region_init(&s->pci_io_space, "pci_io", 1ULL << 32);
+    memory_region_init(&s->pci_mem_space, "pci_mem", 1ULL << 32);
 
     pci_bus_new_inplace(&s->pci_bus, DEVICE(obj), "pci",
-                        get_system_memory(), &s->pci_io_space,
+                        &s->pci_mem_space, &s->pci_io_space,
                         PCI_DEVFN(11, 0));
     h->bus = &s->pci_bus;
 
@@ -252,6 +302,11 @@ static void pci_vpb_init(Object *obj)
     qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
     object_property_set_int(OBJECT(&s->pci_dev), PCI_DEVFN(29, 0), "addr",
                             NULL);
+
+    /* Window sizes for VersatilePB; realview_pci's init will override */
+    s->mem_win_size[0] = 0x0c000000;
+    s->mem_win_size[1] = 0x10000000;
+    s->mem_win_size[2] = 0x10000000;
 }
 
 static void pci_vpb_realize(DeviceState *dev, Error **errp)
@@ -280,6 +335,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
      * 1 : PCI self config window
      * 2 : PCI config window
      * 3 : PCI IO window
+     * 4..6 : PCI memory windows
      */
     memory_region_init_io(&s->controlregs, &pci_vpb_reg_ops, s, "pci-vpb-regs",
                           0x1000);
@@ -299,6 +355,16 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
 
     sysbus_init_mmio(sbd, &s->pci_io_space);
 
+    /* Create the alias regions corresponding to our three windows onto
+     * PCI memory space. The sizes vary from board to board; the base
+     * offsets are guest controllable via the IMAP registers.
+     */
+    for (i = 0; i < 3; i++) {
+        memory_region_init_alias(&s->pci_mem_window[i], "pci-vbp-window",
+                                 &s->pci_mem_space, 0, s->mem_win_size[i]);
+        sysbus_init_mmio(sbd, &s->pci_mem_window[i]);
+    }
+
     /* TODO Remove once realize propagates to child devices. */
     object_property_set_bool(OBJECT(&s->pci_dev), true, "realized", errp);
 }
@@ -356,6 +422,10 @@ static void pci_realview_init(Object *obj)
     PCIVPBState *s = PCI_VPB(obj);
 
     s->realview = 1;
+    /* The PCI window sizes are different on Realview boards */
+    s->mem_win_size[0] = 0x01000000;
+    s->mem_win_size[1] = 0x04000000;
+    s->mem_win_size[2] = 0x08000000;
 }
 
 static const TypeInfo pci_realview_info = {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Aurelien Jarno]

On Sun, Mar 24, 2013 at 11:32:30AM +0000, Peter Maydell wrote:
> This patch series fixes a number of serious bugs in our emulation of
> the PCI controller found on VersatilePB and the early Realview boards:
>  * our interrupt mapping was totally wrong
>  * we weren't implementing the PCI memory windows
>  * the I/O window wasn't mapped on VersatilePB
>  * realview mapped things at the wrong addresses
>  * we didn't implement the controller's registers at all
> It also updates to some reasonable approximation to QOM best practice,
> including subclassing pci_host rather than doing everything by hand.
> 
> I haven't implemented support for the SMAP registers (which control
> how the controller converts accesses made by bus-mastering PCI
> devices into system addresses). For the moment we rely on the fact
> that Linux always maps things 1:1. (It wouldn't be too hard to add
> SMAP support but it requires changing QEMU's pci code to allow a
> controller to pass in a MemoryRegion* for DMA to use instead of
> the system address space, so I prefer to leave that for another series.)
> 
> Patchset tested on both versatilepb and realview, using a set of
> Linux kernel patches written by Arnd Bergmann:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/029040.html
> which were in turn tested against real PB926 and PB1176 hardware.
> 
> 
>  * WARNING WARNING *
> 
> This patchset will break any use of PCI (including the default SCSI
> card) on versatilepb with current Linux kernels, because those kernels

Do you mean Versatile/PB and not Versatile/AB, or actually both?

> have the matching bug in interrupt mapping to old QEMU.

How is real hardware working with this bug?

> I've provided 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
> 
> (If anybody wants to suggest a better way of handling this please do.)

Do you have a pointer to the corresponding kernel patch? Is it possible
to get the kernel to detect if it should use the correct or the broken 
IRQ mapping?


> Peter Maydell (10):
>   versatile_pci: Fix hardcoded tabs
>   versatile_pci: Expose PCI I/O region on Versatile PB
>   versatile_pci: Update to realize and instance init functions
>   versatile_pci: Change to subclassing TYPE_PCI_HOST_BRIDGE
>   versatile_pci: Use separate PCI I/O space rather than system I/O space
>   versatile_pci: Put the host bridge PCI device at slot 29
>   versatile_pci: Implement the correct PCI IRQ mapping
>   versatile_pci: Implement the PCI controller's control registers
>   arm/realview: Fix mapping of PCI regions
>   versatile_pci: Expose PCI memory space to system
> 
>  hw/arm/realview.c    |   22 +--
>  hw/arm/versatilepb.c |   11 +-
>  hw/versatile_pci.c   |  368 ++++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 344 insertions(+), 57 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Peter Maydell]

On 24 March 2013 15:58, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Sun, Mar 24, 2013 at 11:32:30AM +0000, Peter Maydell wrote:
>> This patch series fixes a number of serious bugs in our emulation of
>> the PCI controller found on VersatilePB and the early Realview boards:

>> Patchset tested on both versatilepb and realview, using a set of
>> Linux kernel patches written by Arnd Bergmann:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/029040.html
>> which were in turn tested against real PB926 and PB1176 hardware.
>>
>>
>>  * WARNING WARNING *
>>
>> This patchset will break any use of PCI (including the default SCSI
>> card) on versatilepb with current Linux kernels, because those kernels
>
> Do you mean Versatile/PB and not Versatile/AB, or actually both?

I mean PB. The AB doesn't have a PCI controller (though we incorrectly
model it as having one; I suppose we could patch QEMU to stop doing
that).

>> have the matching bug in interrupt mapping to old QEMU.
>
> How is real hardware working with this bug?

It doesn't. Unless you apply Arnd's patches, PCI support
on real hardware is flat out broken. (This was never noticed
because (a) real hardware is getting rare by now and (b) the
PCI backplane is even rarer.)

>> I've provided 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
>>
>> (If anybody wants to suggest a better way of handling this please do.)
>
> Do you have a pointer to the corresponding kernel patch? Is it possible
> to get the kernel to detect if it should use the correct or the broken
> IRQ mapping?

I linked to the kernel patches above. It might I guess be possible
to identify a fixed QEMU (if nothing else we could put something
into QEMU that was identifiable); that still leaves existing kernels
breaking, though.

-- PMM

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Michael S. Tsirkin]

On Sun, Mar 24, 2013 at 04:58:37PM +0100, Aurelien Jarno wrote:
> On Sun, Mar 24, 2013 at 11:32:30AM +0000, Peter Maydell wrote:
> > This patch series fixes a number of serious bugs in our emulation of
> > the PCI controller found on VersatilePB and the early Realview boards:
> >  * our interrupt mapping was totally wrong
> >  * we weren't implementing the PCI memory windows
> >  * the I/O window wasn't mapped on VersatilePB
> >  * realview mapped things at the wrong addresses
> >  * we didn't implement the controller's registers at all
> > It also updates to some reasonable approximation to QOM best practice,
> > including subclassing pci_host rather than doing everything by hand.
> > 
> > I haven't implemented support for the SMAP registers (which control
> > how the controller converts accesses made by bus-mastering PCI
> > devices into system addresses). For the moment we rely on the fact
> > that Linux always maps things 1:1. (It wouldn't be too hard to add
> > SMAP support but it requires changing QEMU's pci code to allow a
> > controller to pass in a MemoryRegion* for DMA to use instead of
> > the system address space, so I prefer to leave that for another series.)
> > 
> > Patchset tested on both versatilepb and realview, using a set of
> > Linux kernel patches written by Arnd Bergmann:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/029040.html
> > which were in turn tested against real PB926 and PB1176 hardware.
> > 
> > 
> >  * WARNING WARNING *
> > 
> > This patchset will break any use of PCI (including the default SCSI
> > card) on versatilepb with current Linux kernels, because those kernels
> 
> Do you mean Versatile/PB and not Versatile/AB, or actually both?
> 
> > have the matching bug in interrupt mapping to old QEMU.
> 
> How is real hardware working with this bug?
> 
> > I've provided 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
> > 
> > (If anybody wants to suggest a better way of handling this please do.)
> 
> Do you have a pointer to the corresponding kernel patch? Is it possible
> to get the kernel to detect if it should use the correct or the broken 
> IRQ mapping?

Alternatively, or additionally, how about detecting the correct or
the incorrect kernel and updating the mapping?
For example, maybe we could do this using the
IRQ value written into the device pci config register?

If we can't find anything, maybe add our own register
so the same qemu config can support old and new kernels?

> 
> > Peter Maydell (10):
> >   versatile_pci: Fix hardcoded tabs
> >   versatile_pci: Expose PCI I/O region on Versatile PB
> >   versatile_pci: Update to realize and instance init functions
> >   versatile_pci: Change to subclassing TYPE_PCI_HOST_BRIDGE
> >   versatile_pci: Use separate PCI I/O space rather than system I/O space
> >   versatile_pci: Put the host bridge PCI device at slot 29
> >   versatile_pci: Implement the correct PCI IRQ mapping
> >   versatile_pci: Implement the PCI controller's control registers
> >   arm/realview: Fix mapping of PCI regions
> >   versatile_pci: Expose PCI memory space to system
> > 
> >  hw/arm/realview.c    |   22 +--
> >  hw/arm/versatilepb.c |   11 +-
> >  hw/versatile_pci.c   |  368 ++++++++++++++++++++++++++++++++++++++++++++------
> >  3 files changed, 344 insertions(+), 57 deletions(-)
> > 
> > -- 
> > 1.7.9.5
> > 
> > 
> > 
> 
> -- 
> Aurelien Jarno	                        GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Aurelien Jarno]

On Sun, Mar 24, 2013 at 04:49:37PM +0000, Peter Maydell wrote:
> On 24 March 2013 15:58, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Sun, Mar 24, 2013 at 11:32:30AM +0000, Peter Maydell wrote:
> >> This patch series fixes a number of serious bugs in our emulation of
> >> the PCI controller found on VersatilePB and the early Realview boards:
> 
> >> Patchset tested on both versatilepb and realview, using a set of
> >> Linux kernel patches written by Arnd Bergmann:
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/029040.html
> >> which were in turn tested against real PB926 and PB1176 hardware.
> >>
> >>
> >>  * WARNING WARNING *
> >>
> >> This patchset will break any use of PCI (including the default SCSI
> >> card) on versatilepb with current Linux kernels, because those kernels
> >
> > Do you mean Versatile/PB and not Versatile/AB, or actually both?
> 
> I mean PB. The AB doesn't have a PCI controller (though we incorrectly
> model it as having one; I suppose we could patch QEMU to stop doing
> that).
> 
> >> have the matching bug in interrupt mapping to old QEMU.
> >
> > How is real hardware working with this bug?
> 
> It doesn't. Unless you apply Arnd's patches, PCI support
> on real hardware is flat out broken. (This was never noticed
> because (a) real hardware is getting rare by now and (b) the
> PCI backplane is even rarer.)
> 
> >> I've provided 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
> >>
> >> (If anybody wants to suggest a better way of handling this please do.)
> >
> > Do you have a pointer to the corresponding kernel patch? Is it possible
> > to get the kernel to detect if it should use the correct or the broken
> > IRQ mapping?
> 
> I linked to the kernel patches above. It might I guess be possible
> to identify a fixed QEMU (if nothing else we could put something
> into QEMU that was identifiable); that still leaves existing kernels
> breaking, though.
> 

Now that stable kernel releases work well and that most distribution are
basing their releases on longterm kernels, it might be possible to get
the fix included there, and from there in the distributions. I think
it's something that would be acceptable, provided it is below the 100
lines limit including context.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Peter Maydell]

On 24 March 2013 19:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Mar 24, 2013 at 04:58:37PM +0100, Aurelien Jarno wrote:
>> On Sun, Mar 24, 2013 at 11:32:30AM +0000, Peter Maydell wrote:
>> > I've provided 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
>> >
>> > (If anybody wants to suggest a better way of handling this please do.)
>>
>> Do you have a pointer to the corresponding kernel patch? Is it possible
>> to get the kernel to detect if it should use the correct or the broken
>> IRQ mapping?
>
> Alternatively, or additionally, how about detecting the correct or
> the incorrect kernel and updating the mapping?
> For example, maybe we could do this using the
> IRQ value written into the device pci config register?

Yeah, ideally being able to detect the buggy kernel would be good;
I can't see anything at the controller level that would do though,
and I don't really know enough about PCI to know about generic
PCI stuff that would work. (Why would the OS need to tell the
device anything about its IRQ if it's hardwired?)

> If we can't find anything, maybe add our own register
> so the same qemu config can support old and new kernels?

Then the new kernel wouldn't work on real hardware...

-- PMM

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Arnd Bergmann]

On Sunday 24 March 2013, Peter Maydell wrote:
> Yeah, ideally being able to detect the buggy kernel would be good;
> I can't see anything at the controller level that would do though,
> and I don't really know enough about PCI to know about generic
> PCI stuff that would work. (Why would the OS need to tell the
> device anything about its IRQ if it's hardwired?)

I think it actually does on versatile and other platforms on which
the kernel probes the PCI bus itself, rather than relying on firmware
to have resources assigned in advance.

IIRC, the PCI_INTERRUPT_LINE pci config space byte (0x3c) is purely
informational and used as a way to communicate the interrupt number
from the bus scan code (assumed to be a PC BIOS in the PCI spec,
but drivers/pci/setup-irq.c in case of versatile+linux) to a device
driver.

So the kernel should actually write the proper interrupt number in
there. In future kernels, this may not necessarily be the hardware
number, but today it is. Can you try out what the kernel writes into
that register in qemu, with and without my patches?

I would expect the numbers to be (64+27) to (64+30), since we
linearize the interrupt numbers so that VIC gets 32 through
63 and SIC gets 64 through 95.

	Arnd

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Peter Maydell]

On 24 March 2013 21:16, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 24 March 2013, Peter Maydell wrote:
>> Yeah, ideally being able to detect the buggy kernel would be good;
>> I can't see anything at the controller level that would do though,
>> and I don't really know enough about PCI to know about generic
>> PCI stuff that would work. (Why would the OS need to tell the
>> device anything about its IRQ if it's hardwired?)
>
> I think it actually does on versatile and other platforms on which
> the kernel probes the PCI bus itself, rather than relying on firmware
> to have resources assigned in advance.
>
> IIRC, the PCI_INTERRUPT_LINE pci config space byte (0x3c) is purely
> informational and used as a way to communicate the interrupt number
> from the bus scan code (assumed to be a PC BIOS in the PCI spec,
> but drivers/pci/setup-irq.c in case of versatile+linux) to a device
> driver.
>
> So the kernel should actually write the proper interrupt number in
> there. In future kernels, this may not necessarily be the hardware
> number, but today it is. Can you try out what the kernel writes into
> that register in qemu, with and without my patches?

OK, I'll give that a go tomorrow.

While you're here, do you know what the point of the PCI_SELFID
register is? The h/w docs are clear that the OS has to write
the slot number of the board itself in to this register, but
again I don't see why the OS has to tell the hardware something
it already knows. On QEMU I just implemented this register as
a no-op and everything seems to still function...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Michael S. Tsirkin]

On Sun, Mar 24, 2013 at 08:53:33PM +0000, Peter Maydell wrote:
> On 24 March 2013 19:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Mar 24, 2013 at 04:58:37PM +0100, Aurelien Jarno wrote:
> >> On Sun, Mar 24, 2013 at 11:32:30AM +0000, Peter Maydell wrote:
> >> > I've provided 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
> >> >
> >> > (If anybody wants to suggest a better way of handling this please do.)
> >>
> >> Do you have a pointer to the corresponding kernel patch? Is it possible
> >> to get the kernel to detect if it should use the correct or the broken
> >> IRQ mapping?
> >
> > Alternatively, or additionally, how about detecting the correct or
> > the incorrect kernel and updating the mapping?
> > For example, maybe we could do this using the
> > IRQ value written into the device pci config register?
> 
> Yeah, ideally being able to detect the buggy kernel would be good;
> I can't see anything at the controller level that would do though,
> and I don't really know enough about PCI to know about generic
> PCI stuff that would work. (Why would the OS need to tell the
> device anything about its IRQ if it's hardwired?)

Each pci device has a bit of memory where the OS stores
the IRQ#. I think it was invented as a simple way to pass
data from BIOS to the OS on a PC.
There's no special reason to store it on the device
but everyone does it.

> > If we can't find anything, maybe add our own register
> > so the same qemu config can support old and new kernels?
> 
> Then the new kernel wouldn't work on real hardware...
> 
> -- PMM

Not if our register is read-only in real hardware.

-- 
MST

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Michael S. Tsirkin]

On Sun, Mar 24, 2013 at 09:16:28PM +0000, Arnd Bergmann wrote:
> On Sunday 24 March 2013, Peter Maydell wrote:
> > Yeah, ideally being able to detect the buggy kernel would be good;
> > I can't see anything at the controller level that would do though,
> > and I don't really know enough about PCI to know about generic
> > PCI stuff that would work. (Why would the OS need to tell the
> > device anything about its IRQ if it's hardwired?)
> 
> I think it actually does on versatile and other platforms on which
> the kernel probes the PCI bus itself, rather than relying on firmware
> to have resources assigned in advance.
> 
> IIRC, the PCI_INTERRUPT_LINE pci config space byte (0x3c) is purely
> informational and used as a way to communicate the interrupt number
> from the bus scan code (assumed to be a PC BIOS in the PCI spec,
> but drivers/pci/setup-irq.c in case of versatile+linux) to a device
> driver.
> 
> So the kernel should actually write the proper interrupt number in
> there. In future kernels, this may not necessarily be the hardware
> number, but today it is.

For future kernels, let's build in some hook that let
qemu detect a non broken guest. How about writing
some magic value into revision ID or some other
readonly field?

> Can you try out what the kernel writes into
> that register in qemu, with and without my patches?
> 
> I would expect the numbers to be (64+27) to (64+30), since we
> linearize the interrupt numbers so that VIC gets 32 through
> 63 and SIC gets 64 through 95.
> 
> 	Arnd

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Arnd Bergmann]

On Sunday 24 March 2013, Peter Maydell wrote:
> OK, I'll give that a go tomorrow.
> 
> While you're here, do you know what the point of the PCI_SELFID
> register is? The h/w docs are clear that the OS has to write
> the slot number of the board itself in to this register, but
> again I don't see why the OS has to tell the hardware something
> it already knows. On QEMU I just implemented this register as
> a no-op and everything seems to still function...
> 

I don't know really, but I think it has something to do with the
versatile board being plugged into a backplane that has multiple
slots. Apparently there is something the hardware cannot figure
out itself, e.g. whether to route the interrupts in or out of the
slot.

	Arnd

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Arnd Bergmann]

On Sunday 24 March 2013, Michael S. Tsirkin wrote:
> For future kernels, let's build in some hook that let
> qemu detect a non broken guest. How about writing
> some magic value into revision ID or some other
> readonly field?

Yes, makes sense.

Re: [Qemu-devel] [PATCH 02/10] versatile_pci: Expose PCI I/O region on Versatile PB

[Peter Crosthwaite]

Hi Peter,

On Sun, Mar 24, 2013 at 9:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Comments in the QEMU source code claim that the version of the PCI
> controller on the VersatilePB board doesn't support the PCI I/O
> region, but this is incorrect; expose that region, map it in the
> correct location, and drop the misleading comments.
>
> This change removes the only currently implemented difference
> between the realview-pci and versatile-pci models; however there
> are other differences in not-yet-implemented functionality, so we
> retain the distinction between the two device types.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/versatilepb.c |    3 +--
>  hw/versatile_pci.c   |    8 +++-----
>  2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index baaa265..0d08d15 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -226,14 +226,13 @@ static void versatile_init(QEMUMachineInitArgs *args, int board_id)
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(busdev, 0, 0x41000000); /* PCI self-config */
>      sysbus_mmio_map(busdev, 1, 0x42000000); /* PCI config */
> +    sysbus_mmio_map(busdev, 2, 0x43000000); /* PCI I/O */
>      sysbus_connect_irq(busdev, 0, sic[27]);
>      sysbus_connect_irq(busdev, 1, sic[28]);
>      sysbus_connect_irq(busdev, 2, sic[29]);
>      sysbus_connect_irq(busdev, 3, sic[30]);
>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>
> -    /* The Versatile PCI bridge does not provide access to PCI IO space,
> -       so many of the qemu PCI devices are not useable.  */
>      for(n = 0; n < nb_nics; n++) {
>          nd = &nd_table[n];
>
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index 16ce5d1..1312f46 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -77,7 +77,7 @@ static int pci_vpb_init(SysBusDevice *dev)
>      /* Our memory regions are:
>       * 0 : PCI self config window
>       * 1 : PCI config window
> -     * 2 : PCI IO window (realview_pci only)
> +     * 2 : PCI IO window
>       */
>      memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, bus,
>                            "pci-vpb-selfconfig", 0x1000000);
> @@ -85,10 +85,8 @@ static int pci_vpb_init(SysBusDevice *dev)
>      memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
>                            "pci-vpb-config", 0x1000000);
>      sysbus_init_mmio(dev, &s->mem_config2);
> -    if (s->realview) {

This is the one and only usage of ->realview. I wonder if this
argument is flawed - in real hardware is there any functional
difference between realview and versatile PCI requiring the level of
heirachy defined here? Could we take the oppurtunity to knock out this
realview flag and the "realview_pci" class altogether and save on all
the QOM boilerplate for the now functionally identical subclass?

Regards,
Peter

> -        isa_mmio_setup(&s->isa, 0x0100000);
> -        sysbus_init_mmio(dev, &s->isa);
> -    }
> +    isa_mmio_setup(&s->isa, 0x0100000);
> +    sysbus_init_mmio(dev, &s->isa);
>
>      pci_create_simple(bus, -1, "versatile_pci_host");
>      return 0;
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [PATCH 02/10] versatile_pci: Expose PCI I/O region on Versatile PB

[Peter Maydell]

On 25 March 2013 01:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Sun, Mar 24, 2013 at 9:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Comments in the QEMU source code claim that the version of the PCI
>> controller on the VersatilePB board doesn't support the PCI I/O
>> region, but this is incorrect; expose that region, map it in the
>> correct location, and drop the misleading comments.
>>
>> This change removes the only currently implemented difference
>> between the realview-pci and versatile-pci models; however there
>> are other differences in not-yet-implemented functionality, so we
>> retain the distinction between the two device types.

>> @@ -85,10 +85,8 @@ static int pci_vpb_init(SysBusDevice *dev)
>>      memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
>>                            "pci-vpb-config", 0x1000000);
>>      sysbus_init_mmio(dev, &s->mem_config2);
>> -    if (s->realview) {
>
> This is the one and only usage of ->realview. I wonder if this
> argument is flawed - in real hardware is there any functional
> difference between realview and versatile PCI requiring the level of
> heirachy defined here?

Please read the commit message and the later patches in the series :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/10] versatile_pci: Expose PCI I/O region on Versatile PB

[Peter Maydell]

On 25 March 2013 09:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 March 2013 01:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> This is the one and only usage of ->realview. I wonder if this
>> argument is flawed - in real hardware is there any functional
>> difference between realview and versatile PCI requiring the level of
>> heirachy defined here?
>
> Please read the commit message and the later patches in the series :-)

Er, and to be slightly more helpful, the following things differ
between versatilepb and realview:
 * behaviour of IMAP registers
 * memory window sizes
 * IRQ mapping

and some things we don't yet implement:
 * SMAP register behaviour
 * PCI_FLAGS register bits seem to be a little different

-- PMM

Re: [Qemu-devel] [PATCH 00/10] Fix versatile_pci (and break versatilepb linux guests!)

[Peter Maydell]

On 24 March 2013 21:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Mar 24, 2013 at 09:16:28PM +0000, Arnd Bergmann wrote:
>> I think it actually does on versatile and other platforms on which
>> the kernel probes the PCI bus itself, rather than relying on firmware
>> to have resources assigned in advance.
>>
>> IIRC, the PCI_INTERRUPT_LINE pci config space byte (0x3c) is purely
>> informational and used as a way to communicate the interrupt number
>> from the bus scan code (assumed to be a PC BIOS in the PCI spec,
>> but drivers/pci/setup-irq.c in case of versatile+linux) to a device
>> driver.
>>
>> So the kernel should actually write the proper interrupt number in
>> there. In future kernels, this may not necessarily be the hardware
>> number, but today it is.

OK, so I've now tested, and the kernel writes the interrupt
number (27..30) into PCI_INTERRUPT_LINE, so we can successfully
use this to detect broken kernels and switch to the old irq
mapping. (Broken kernels write 27 regardless of slot number,
good ones write 27..30 depending on slot number.)
[patches in progress, will send later.]

> For future kernels, let's build in some hook that let
> qemu detect a non broken guest. How about writing
> some magic value into revision ID or some other
> readonly field?

So do we still need the non-broken-guest detection hook too?
We could maybe steal a bit in PCI_FLAGS for this, but somebody
would need to check the behaviour of the hardware if you try
to write to the reserved bits on real hardware. (Usually it's
RAZ/WI or reads-as-written, either of which would be OK here.)

thanks
-- PMM

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 <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

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

[Peter Maydell]

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

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

[Michael S. Tsirkin]

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.