[Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

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

This version of the patchset avoids breaking legacy Linux guests
by 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. If this doesn't match reality we use the broken mapping.
(Thanks to Michael S. Tsirkin for this suggestion.)

If the kernel ever writes something different to the old kernel
then QEMU will assume it is dealing with a working kernel and
will stop looking for broken values. This allows a new kernel to
put any values it likes in PCI_INTERRUPT_LINE, provided its init
code does an initial write of something other than 27 to let QEMU
know it can cope with the correct irq mapping.

Changes v3->v4:
 * rebase following recent pci changes
 * patch 7 minor tweaks as per mst review:
    added VPB_PCI prefix to enum names, drop specified values
    drop unnecessary casts of void*
Changes v2->v3:
 * patch 7: make broken guest autodetect a one-time thing rather than
   reversible, to allow more flexibility in how new kernels choose
   to use the PCI_INTERRUPT_LINE registers
Changes v1->v2:
 * autodetect broken guests rather than using user-set property
 * use pci_swizzle_map_fn
 * new patch: drop unnecessary vpb_pci_config_addr() function

Peter Maydell (11):
  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/versatile_pci: Drop unnecessary vpb_pci_config_addr()

 hw/arm/realview.c    |   22 +--
 hw/arm/versatilepb.c |   11 +-
 hw/versatile_pci.c   |  399 ++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 371 insertions(+), 61 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v4 01/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 d67ca79..04d0029 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 v4 02/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 04d0029..a50a18b 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 v4 03/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 a50a18b..ad33ce7 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, TYPE_PCI_BUS);
@@ -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 v4 04/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 ad33ce7..9e0ece0 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), TYPE_PCI_BUS);
+    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, TYPE_PCI_BUS);
+
+    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 v4 05/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 9e0ece0..ce5bdf2 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), TYPE_PCI_BUS);
     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 v4 06/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 hw/versatile_pci.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index ce5bdf2..8f8612c 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 v4 07/11] 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.

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 <peter.maydell@linaro.org>
---
 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 8f8612c..5d543a9 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 {
+    PCI_VPB_IRQMAP_ASSUME_OK,
+    PCI_VPB_IRQMAP_BROKEN,
+    PCI_VPB_IRQMAP_FORCE_OK,
+};
+
 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 = opaque;
+    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE
+        && s->irq_mapping == PCI_VPB_IRQMAP_ASSUME_OK) {
+        uint8_t devfn = addr >> 8;
+        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {
+            if (val == 27) {
+                s->irq_mapping = PCI_VPB_IRQMAP_BROKEN;
+            } else {
+                s->irq_mapping = PCI_VPB_IRQMAP_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 = 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);
     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 == PCI_VPB_IRQMAP_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 = PCI_VPB_IRQMAP_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

[Qemu-devel] [PATCH v4 08/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 hw/arm/realview.c    |    7 +--
 hw/arm/versatilepb.c |    7 +--
 hw/versatile_pci.c   |  127 ++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 130 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 5d543a9..b0132e6 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -39,6 +39,7 @@ typedef struct {
     PCIHostState parent_obj;
 
     qemu_irq irq[4];
+    MemoryRegion controlregs;
     MemoryRegion mem_config;
     MemoryRegion mem_config2;
     MemoryRegion pci_io_space;
@@ -50,9 +51,27 @@ typedef struct {
     int realview;
 
     /* Variable state: */
+    uint32_t imap[3];
+    uint32_t smap[3];
+    uint32_t selfid;
+    uint32_t flags;
     uint8_t irq_mapping;
 } 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_UINT8(irq_mapping, PCIVPBState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define TYPE_VERSATILE_PCI "versatile_pci"
 #define PCI_VPB(obj) \
     OBJECT_CHECK(PCIVPBState, (obj), TYPE_VERSATILE_PCI)
@@ -61,6 +80,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;
@@ -155,6 +261,14 @@ 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;
     s->irq_mapping = PCI_VPB_IRQMAP_ASSUME_OK;
 }
 
@@ -195,13 +309,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-vpb-selfconfig", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config);
@@ -252,6 +368,7 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pci_vpb_realize;
     dc->reset = pci_vpb_reset;
+    dc->vmsd = &pci_vpb_vmstate;
 }
 
 static const TypeInfo pci_vpb_info = {
-- 
1.7.9.5

[Qemu-devel] [PATCH v4 09/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 v4 10/11] 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>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 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 b0132e6..e99f35f 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -42,13 +42,20 @@ 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;
+    uint32_t mem_win_size[3];
 
     /* Variable state: */
     uint32_t imap[3];
@@ -58,10 +65,49 @@ typedef struct {
     uint8_t irq_mapping;
 } 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),
@@ -103,6 +149,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:
@@ -270,6 +317,8 @@ static void pci_vpb_reset(DeviceState *d)
     s->selfid = 0;
     s->flags = 0;
     s->irq_mapping = PCI_VPB_IRQMAP_ASSUME_OK;
+
+    pci_vpb_update_all_windows(s);
 }
 
 static void pci_vpb_init(Object *obj)
@@ -278,9 +327,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), TYPE_PCI_BUS);
     h->bus = &s->pci_bus;
 
@@ -288,6 +338,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)
@@ -314,6 +369,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);
@@ -333,6 +389,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);
 }
@@ -384,6 +450,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

[Qemu-devel] [PATCH v4 11/11] hw/versatile_pci: Drop unnecessary vpb_pci_config_addr()

[Peter Maydell]

Drop the vpb_pci_config_addr() function -- it is unnecessary since
the size of the memory regions means the hwaddr is always within
the 24 bit size. (This function was probably a leftover from when
read/write functions were called with absolute addresses rather
than relative ones.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Paul Brook <paul@codesourcery.com>
---
 hw/versatile_pci.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index e99f35f..540daf7 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -214,11 +214,6 @@ static const MemoryRegionOps pci_vpb_reg_ops = {
     },
 };
 
-static inline uint32_t vpb_pci_config_addr(hwaddr addr)
-{
-    return addr & 0xffffff;
-}
-
 static void pci_vpb_config_write(void *opaque, hwaddr addr,
                                  uint64_t val, unsigned size)
 {
@@ -234,7 +229,7 @@ static void pci_vpb_config_write(void *opaque, hwaddr addr,
             }
         }
     }
-    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);
+    pci_data_write(&s->pci_bus, addr, val, size);
 }
 
 static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
@@ -242,7 +237,7 @@ static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
 {
     PCIVPBState *s = opaque;
     uint32_t val;
-    val = pci_data_read(&s->pci_bus, vpb_pci_config_addr(addr), size);
+    val = pci_data_read(&s->pci_bus, addr, size);
     return val;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Rob Landley]

On 04/06/2013 10:44:25 AM, 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

Yes. Yes it was. However, what you were doing matched the kernel for  
many years.

I build system images that are working on versatilePB right now, with  
the 3.8 kernel.

   http://landley.net/aboriginal/bin/system-image-armv5l.tar.bz2

Grab that, ./run-emulator.sh and it should boot to a shell prompt  
within which you can compile hello world.

The kernel guys have screwed this up three consecutive times, as  
described here:

   http://landley.net/hg/aboriginal/rev/7bf850767bb8

Because as far as I can tell, nobody has any test hardware for this  
anymore. (But no other board emulation I've tried lets me stick a PCI  
bus and a scsi controller, ethernet, and serial port into an arm board  
along with enough memory to natively compile stuff.)

This past release I tried to fix it up properly:

   http://landley.net/notes.html#15-03-2013
   http://landley.net/notes.html#16-03-2013

And then I gave up and hit it with a rock:

   http://landley.net/hg/aboriginal/rev/96fb8598a446

> This version of the patchset avoids breaking legacy Linux guests
> by 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. If this doesn't match reality we use the broken mapping.
> (Thanks to Michael S. Tsirkin for this suggestion.)

My concern here was that you'd going to change qemu so it doesn't run  
the old images, and require a very new qemu to run the new images, so  
there will be an incompatible flag day.

This is especially a concern for me because I had to go back to qemu  
1.2.0 to get stable builds on all my targets, because the 1.3.0 and  
1.4.0 releases were both broken:

   http://landley.net/aboriginal/news.html

Yay improving the latest and greatest, but when I'm regression testing  
as many different platforms as I can get working, stuff tends to break.  
(The most recent target-specific one for me on the QEMU side was  
probably the powerpc video thing,
http://landley.net/notes-2012.html#17-11-2012 . Other than the 1.3/1.4  
general TCG instability which was the translation unit size being  
calculated wrong.) This is why I can't demand my users upgrade each  
time I do a release: the target they're interested in might not work  
because my images depend on things you don't regression test much.

I'm glad to see you're addressing backwards compatability, but it looks  
like I might have to patch my kernels to write the _old_ value to this  
register in order to get something that runs on old qemu? Because I  
broke my kernel to fit what qemu was doing, and I dunno when this  
register write changed. (I _do_ know that what the kernel guys have  
been doing to versatile is insane and inconsistent, and if you change  
your code to humor them they'll probably break it again. Their register  
mapping in 3.8 was so wrong even _I_ could tell.)

Rob

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Peter Maydell]

On 8 April 2013 18:37, Rob Landley <rob@landley.net> wrote:
> On 04/06/2013 10:44:25 AM, 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
>
>
> Yes. Yes it was. However, what you were doing matched the kernel for many
> years.

> The kernel guys have screwed this up three consecutive times, as described
> here:
>
>   http://landley.net/hg/aboriginal/rev/7bf850767bb8
>
> Because as far as I can tell, nobody has any test hardware for this anymore.

There is some but it's pretty rare.

>> This version of the patchset avoids breaking legacy Linux guests
>> by 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. If this doesn't match reality we use the broken mapping.
>> (Thanks to Michael S. Tsirkin for this suggestion.)
>
>
> My concern here was that you'd going to change qemu so it doesn't run the
> old images, and require a very new qemu to run the new images, so there will
> be an incompatible flag day.

No, it doesn't break old images, as the paragraph you quote clearly
states. Yes, if the kernel guys fix the kernel you'll need a newer
QEMU to run that. However as the kernel patches have been floating around
unapplied since 2011 you can draw your own conclusions about how quickly
the kernel will get fixed.

> Yay improving the latest and greatest, but when I'm regression testing as
> many different platforms as I can get working, stuff tends to break. (The
> most recent target-specific one for me on the QEMU side was probably the
> powerpc video thing,
> http://landley.net/notes-2012.html#17-11-2012 . Other than the 1.3/1.4
> general TCG instability which was the translation unit size being calculated
> wrong.) This is why I can't demand my users upgrade each time I do a
> release: the target they're interested in might not work because my images
> depend on things you don't regression test much.

Yes, TCG and minor platform emulation is to some extent on a 'best
effort' basis. Unfortunately we have neither the developer resources
nor the automated test infrastructure to do better; if you can help
on either front do say.

> I'm glad to see you're addressing backwards compatability, but it looks like
> I might have to patch my kernels to write the _old_ value to this register
> in order to get something that runs on old qemu?

If you want to run on a QEMU predating this patchset then you need
to have a kernel which expects the old and broken behaviour, yes.
This patchset is to some extent futureproofing in that it ensures
that if the kernel guys actually fix the versatilepb PCI code at
some future date the QEMU that is out in the wild will still cope.

> Because I broke my kernel
> to fit what qemu was doing, and I dunno when this register write changed. (I
> _do_ know that what the kernel guys have been doing to versatile is insane
> and inconsistent, and if you change your code to humor them they'll probably
> break it again. Their register mapping in 3.8 was so wrong even _I_ could
> tell.)

I don't have any interest in adding workarounds to QEMU for kernel
versions which didn't work on any QEMU version or on real hardware.
I do care about making QEMU work like the hardware, and about keeping
it working with kernels that assume the old longstanding QEMU
behaviour. This patchset satisfies both those goals.

If you have issues with the kernel breaking things then you need to
take it up with the kernel developers.

-- PMM

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Rob Landley]

On 04/08/2013 03:16:18 PM, Peter Maydell wrote:
> On 8 April 2013 18:37, Rob Landley <rob@landley.net> wrote:
> > On 04/06/2013 10:44:25 AM, 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
> >
> >
> > Yes. Yes it was. However, what you were doing matched the kernel  
> for many
> > years.
> 
> > The kernel guys have screwed this up three consecutive times, as  
> described
> > here:
> >
> >   http://landley.net/hg/aboriginal/rev/7bf850767bb8
> >
> > Because as far as I can tell, nobody has any test hardware for this  
> anymore.
> 
> There is some but it's pretty rare.

Now that the next kernel's about to come out, I'm trying to get my arm  
versatile image to work under qemu 1.5.0. The old kernel doesn't work,  
and the current vanilla kernel doesn't work. This change broke it.

I'm testing current linux-git both with and without this patch:

--- linux/arch/arm/mach-versatile/pci.c 2013-04-28 19:36:01.000000000  
-0500
+++ linux.bak/arch/arm/mach-versatile/pci.c     2013-04-29  
19:09:44.857097553 -0500
@@ -333,7 +333,7 @@
          *  26     1     IRQ_SIC_PCI2
          *  27     1     IRQ_SIC_PCI3
          */
-       irq = IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
+       irq = 59; //IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);

         return irq;
  }

With the patch, qemu 1.2.0 works, but qemu 1.5 versatile does not work  
with or without the patch.

Here's a test image to demonstrate the problem: it works fine under  
qemu 1.2.0, but doesn't under 1.5.0:

   http://landley.net/aboriginal/bin/system-image-armv5l.tar.bz2

Extract it and ./run-emulator.sh. You get a shell prompt from 1.2.0,  
from 1.5.0 it never gets a scsi interrupt, and after a timeout goes  
into an abort/reset loop.

> >> This version of the patchset avoids breaking legacy Linux guests
> >> by automatically detecting those broken kernels and switching back
> >> to the old mapping.

As testing with the above image confirms: it does not.

> >> 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. If this doesn't match reality we use the broken  
> mapping.
> >> (Thanks to Michael S. Tsirkin for this suggestion.)

Define "reality".

The kernel changed what irqs it was expecting _three_times_ last year,  
and as far as I can tell none of them were what they were _trying_ to  
do.

Here's my blog entry and the source control comments where I diagnosed  
this stuff to show that the kernel guys have no flipping CLUE how an  
arm versatile board actually works:

1) http://landley.net/notes-2013.html#15-03-2013
2) http://landley.net/hg/aboriginal/rev/1588
3) http://landley.net/hg/aboriginal/rev/1589

Here's the text from #2:

=====
The arm guys have now screwed up the ARM versatile board's IRQ routing  
three consecutive times. I'm impressed.

The ARM versatile scsi IRQ is position 27 on the IRQ controller. That's  
what
QEMU implemented, that's what worked for years. In commit 1bc39ac5dab2  
the
kernel guys screwed up their math (among other things doing -24 and  
then &3
on the result.. can we say NOP?) so it was now trying to use IRQ 28 once
the unnecessary "swizzle" function got done with it. (I started  
reverting
that 6 months ago in aboriginal changeset 1534.) Then in commit  
f5565295892e
they incremented the IRQ controller start by 32... and didn't adjust  
map_irq()
so it was still requesting 28, now before the start of the IRQ
controller's range. Then in commit e3e92a7be693 they noticed it was
broken, and added 64 to it. (So now it's requesting 64+28=92, when it
_should_ be requesting 32+27=59. Their own description of what changed  
does
not support what the patch did, and yet...)
=====

All that was about the SCSI IRQ. Entry #3 above was about the  
_ethernet_ IRQ, which they screwed up in a different way (they did an  
&3 in a way that wrapped)about how they did the math for the ethernet  
irq wrong in a _different_ way than they did the scsi irq math wrong.

The line that's actually calculating the IRQ is:

         irq = 27 + ((slot - 24 + pin - 1) & 3);

Since 24 is divisible by 3, subtracting 24 and then & 3 on the result  
is a NOP so this math can't POSSIBLY do what they think it's doing.

The kernel code in this area is CRAP, has changed multiple times in  
semi-random ways, has obviously NEVER been tested on real hardware, and  
if you've implemented what the actual versatile documentation says it's  
clearly not what the kernel is doing.

> > My concern here was that you'd going to change qemu so it doesn't  
> run the
> > old images, and require a very new qemu to run the new images, so  
> there will
> > be an incompatible flag day.
> 
> No, it doesn't break old images, as the paragraph you quote clearly
> states. Yes, if the kernel guys fix the kernel you'll need a newer
> QEMU to run that. However as the kernel patches have been floating  
> around
> unapplied since 2011 you can draw your own conclusions about how  
> quickly
> the kernel will get fixed.

I have an old image that used to work with qemu 1.2. it does not work  
with qemu 1.5. (I skip 1.3 and 1.4 because tcg had random failures.)

I removed my irq fix patch and just let the kernel do its own math, and  
that doesn't work with qemu 1.5 either.

> > Yay improving the latest and greatest, but when I'm regression  
> testing as
> > many different platforms as I can get working, stuff tends to  
> break. (The
> > most recent target-specific one for me on the QEMU side was  
> probably the
> > powerpc video thing,
> > http://landley.net/notes-2012.html#17-11-2012 . Other than the  
> 1.3/1.4
> > general TCG instability which was the translation unit size being  
> calculated
> > wrong.) This is why I can't demand my users upgrade each time I do a
> > release: the target they're interested in might not work because my  
> images
> > depend on things you don't regression test much.
> 
> Yes, TCG and minor platform emulation is to some extent on a 'best
> effort' basis. Unfortunately we have neither the developer resources
> nor the automated test infrastructure to do better; if you can help
> on either front do say.

I have images that boot to a shell prompt, and I can build linux from  
scratch in an automated manner under the result. (That's how I found  
tcg instability: it would die in random places during the various  
native package builds.)

> > I'm glad to see you're addressing backwards compatability, but it  
> looks like
> > I might have to patch my kernels to write the _old_ value to this  
> register
> > in order to get something that runs on old qemu?
> 
> If you want to run on a QEMU predating this patchset then you need
> to have a kernel which expects the old and broken behaviour, yes.
> This patchset is to some extent futureproofing in that it ensures
> that if the kernel guys actually fix the versatilepb PCI code at
> some future date the QEMU that is out in the wild will still cope.

Do you have a kernel that runs under current qemu arm versatile  
emulation? I can poke around and figure out which irqs it expects  
_now_, but it's not "right" and presumably you're just going to change  
it again when you realize what qemu is doing and what the unpatched  
kernel is doing don't match.

> > Because I broke my kernel
> > to fit what qemu was doing, and I dunno when this register write  
> changed. (I
> > _do_ know that what the kernel guys have been doing to versatile is  
> insane
> > and inconsistent, and if you change your code to humor them they'll  
> probably
> > break it again. Their register mapping in 3.8 was so wrong even _I_  
> could
> > tell.)
> 
> I don't have any interest in adding workarounds to QEMU for kernel
> versions which didn't work on any QEMU version or on real hardware.

They worked fine on qemu 1.2 and several versions before that.

> I do care about making QEMU work like the hardware, and about keeping
> it working with kernels that assume the old longstanding QEMU
> behaviour. This patchset satisfies both those goals.

If it did, I wouldn't be emailing you. Try the system image above, on  
1.2 and on current.

> If you have issues with the kernel breaking things then you need to
> take it up with the kernel developers.

I did. They're only interested in new boards, none of which are both  
supported by qemu and emulate a PCI bus.

I'm happy to patch the kernel. But the same binary that ran under qemu  
1.2 does not run under 1.5, and that's a regression. It would be an  
acceptable regression if an unpatched kernel ran under 1.5, but it  
doesn't.

> -- PMM

Rob

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Peter Maydell]

On 28 June 2013 08:01, Rob Landley <rob@landley.net> wrote:
> Now that the next kernel's about to come out, I'm trying to get my arm
> versatile image to work under qemu 1.5.0. The old kernel doesn't work, and
> the current vanilla kernel doesn't work. This change broke it.
>
> I'm testing current linux-git both with and without this patch:
>
> --- linux/arch/arm/mach-versatile/pci.c 2013-04-28 19:36:01.000000000 -0500
> +++ linux.bak/arch/arm/mach-versatile/pci.c     2013-04-29
> 19:09:44.857097553 -0500
> @@ -333,7 +333,7 @@
>          *  26     1     IRQ_SIC_PCI2
>          *  27     1     IRQ_SIC_PCI3
>          */
> -       irq = IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
> +       irq = 59; //IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
>
>         return irq;
>  }
>
> With the patch, qemu 1.2.0 works, but qemu 1.5 versatile does not work with
> or without the patch.

The "with the patch" case is uninteresting, because it's not
actually a fix for anything, it's just a change that happened
to work with old QEMU, and it's not a change that is in upstream
Linux.

> Here's a test image to demonstrate the problem: it works fine under qemu
> 1.2.0, but doesn't under 1.5.0:
>
>   http://landley.net/aboriginal/bin/system-image-armv5l.tar.bz2
>
> Extract it and ./run-emulator.sh. You get a shell prompt from 1.2.0, from
> 1.5.0 it never gets a scsi interrupt, and after a timeout goes into an
> abort/reset loop.

Is this an image with your patch or without it? Does it work
if you use the -global option to force legacy IRQ mapping?

>> >> This version of the patchset avoids breaking legacy Linux guests
>> >> by automatically detecting those broken kernels and switching back
>> >> to the old mapping.
>
>
> As testing with the above image confirms: it does not.

I tested with several separate variants of the Linux kernel.

>> >> 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. If this doesn't match reality we use the broken mapping.
>> >> (Thanks to Michael S. Tsirkin for this suggestion.)
>
>
> Define "reality".

"would work on real hardware".

> The kernel changed what irqs it was expecting _three_times_ last year, and
> as far as I can tell none of them were what they were _trying_ to do.
>
> Here's my blog entry and the source control comments where I diagnosed this
> stuff to show that the kernel guys have no flipping CLUE how an arm
> versatile board actually works:

I agree that the kernel has made a bit of a mess in this area,
so we don't need to have that argument again.

> The kernel code in this area is CRAP, has changed multiple times in
> semi-random ways, has obviously NEVER been tested on real hardware, and if
> you've implemented what the actual versatile documentation says it's clearly
> not what the kernel is doing.

I tested these changes against a patchset which Arnd wrote which
I can guarantee was tested on real hardware because I did the
testing myself.

> Do you have a kernel that runs under current qemu arm versatile emulation? I
> can poke around and figure out which irqs it expects _now_, but it's not
> "right" and presumably you're just going to change it again when you realize
> what qemu is doing and what the unpatched kernel is doing don't match.

The ones on http://people.debian.org/~aurel32/qemu/armel/
should work.

-- PMM

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Peter Maydell]

On 28 June 2013 08:01, Rob Landley <rob@landley.net> wrote:
> Now that the next kernel's about to come out, I'm trying to get my arm
> versatile image to work under qemu 1.5.0. The old kernel doesn't work, and
> the current vanilla kernel doesn't work. This change broke it.

Nope. Current vanilla is busted under QEMU-1.4-and-earlier
behaviour too: the first PCI device will work but second
and subsequent won't. The behaviour with QEMU 1.5 is the same.
This is simply and straightforwardly a kernel bug and you need
to get the kernel folk to fix it, preferably by actually writing
PCI code which works on the hardware.

-- PMM

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Rob Landley]

On 06/29/2013 06:03:26 AM, Peter Maydell wrote:
> On 28 June 2013 08:01, Rob Landley <rob@landley.net> wrote:
> > Now that the next kernel's about to come out, I'm trying to get my  
> arm
> > versatile image to work under qemu 1.5.0. The old kernel doesn't  
> work, and
> > the current vanilla kernel doesn't work. This change broke it.
> >
> > I'm testing current linux-git both with and without this patch:
> >
> > --- linux/arch/arm/mach-versatile/pci.c 2013-04-28  
> 19:36:01.000000000 -0500
> > +++ linux.bak/arch/arm/mach-versatile/pci.c     2013-04-29
> > 19:09:44.857097553 -0500
> > @@ -333,7 +333,7 @@
> >          *  26     1     IRQ_SIC_PCI2
> >          *  27     1     IRQ_SIC_PCI3
> >          */
> > -       irq = IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
> > +       irq = 59; //IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
> >
> >         return irq;
> >  }
> >
> > With the patch, qemu 1.2.0 works, but qemu 1.5 versatile does not  
> work with
> > or without the patch.
> 
> The "with the patch" case is uninteresting, because it's not
> actually a fix for anything, it's just a change that happened
> to work with old QEMU, and it's not a change that is in upstream
> Linux.

I want to create a kernel image that works with the existing QEMU image  
people are likely to have installed on their systems, _and_ with  
current versions of QEMU.

Is that what you consider "uninteresting"?

> > Here's a test image to demonstrate the problem: it works fine under  
> qemu
> > 1.2.0, but doesn't under 1.5.0:
> >
> >   http://landley.net/aboriginal/bin/system-image-armv5l.tar.bz2
> >
> > Extract it and ./run-emulator.sh. You get a shell prompt from  
> 1.2.0, from
> > 1.5.0 it never gets a scsi interrupt, and after a timeout goes into  
> an
> > abort/reset loop.
> 
> Is this an image with your patch or without it? Does it work
> if you use the -global option to force legacy IRQ mapping?

Under qemu 1.2? It goes:

   qemu-system-arm: Property '.broken-irq-mapping' not found

See "backwards compatability", above.

I am trying to produce "an image that runs under qemu". That level of  
genericity worked for several years, at least ever since 1.0. When it  
stopped, I complained. You seem to be expressing incredulity at the  
concept.

> >> >> This version of the patchset avoids breaking legacy Linux guests
> >> >> by automatically detecting those broken kernels and switching  
> back
> >> >> to the old mapping.
> >
> >
> > As testing with the above image confirms: it does not.
> 
> I tested with several separate variants of the Linux kernel.

I tried building a vanilla linux 3.10 kernel, and it doesn't work on  
qemu 1.5. I confirmed their current interrupt mapping does not match  
any known hardware or emulator, and that it changed more than once in  
incompatible ways as they did wild-ass-guess du jour about what it  
should be now.

The fact it was obviously untested (or it wouldn't keep changing, since  
there presumably is just one right answer) says to me that this  
hardware is difficult ot come by. (The fact the IRQ spent over 5 years  
in the old state before anyone noticed would also imply a lack of test  
hardware.) However, it's useful to emulate because you can stick a PCI  
bus in it, meaning you can stick arbitrary devices (hard drive and  
network card) into said bus.

So I did the obvious-to-me thing and patched it back to how it _was_,  
which is the only thing that works with previous versions of QEMU. You  
seem to find this approach shocking and unexpected. Do you have a  
better way to achieve compatability with older versions of qemu _and_  
current versions? (I'm open to suggestions...)

> >> >> 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. If this doesn't match reality we use the broken  
> mapping.
> >> >> (Thanks to Michael S. Tsirkin for this suggestion.)
> >
> > Define "reality".
> 
> "would work on real hardware".

The stuff that's so rare the kernel guys couldn't find any to test on?

Maybe I haven't been clear: I want something that works with current  
QEMU _and_ old QEMU.

Your paragraph above implies that there's smoething _different_ I can  
write to PCI_INTERRUPT_LINE that will trigger the old behavior. So my  
patch to do the old IRQ (which worked fine with qemu 1.2) needs to be  
bigger to humor QEMU's heuristic. I'll try to figure out how to extend  
my patch to avoid triggering your new "does not work" mode.

Let's see, stick a printf into qemu, and:

   =====Write 60=12,59=====

So it's requesting IRQ 59, and QEMU's reponse is to _not_ give it IRQ  
59 unless we trigger the "broken" flag.

So... real hardware doesn't get the IRQ it requests? Using the reqested  
IRQ is "broken"? (Apparently your patch for a real hardware kernel  
doesn't request the IRQ it's going to use? I'm confused...)

Right, easy enough to fix:

--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -333,7 +333,13 @@ static int __init versatile_map_irq(const struct  
pci_dev *dev, u8 slot, u8 pin)
          *  26     1     IRQ_SIC_PCI2
          *  27     1     IRQ_SIC_PCI3
          */
-       irq = IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
+       // Hit QEMU 1.5.0 and later with a brick so it uses the IRQ we  
say.
+       dev->bus->ops->write(dev->bus, dev->devfn, PCI_INTERRUPT_LINE,  
1, 27);
+
+       // The kernel has no clue where IRQs are, and its current  
assignments
+       // match neither the hardware nor historic QEMU. Use historic  
QEMU
+       // for compatability with old versions.
+       irq = 59; //IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);

         return irq;
  }

That should work with new _and_ old QEMU.

> > The kernel changed what irqs it was expecting _three_times_ last  
> year, and
> > as far as I can tell none of them were what they were _trying_ to  
> do.
> >
> > Here's my blog entry and the source control comments where I  
> diagnosed this
> > stuff to show that the kernel guys have no flipping CLUE how an arm
> > versatile board actually works:
> 
> I agree that the kernel has made a bit of a mess in this area,
> so we don't need to have that argument again.

Can we have the backwards compatability argument?

> > The kernel code in this area is CRAP, has changed multiple times in
> > semi-random ways, has obviously NEVER been tested on real hardware,  
> and if
> > you've implemented what the actual versatile documentation says  
> it's clearly
> > not what the kernel is doing.
> 
> I tested these changes against a patchset which Arnd wrote which
> I can guarantee was tested on real hardware because I did the
> testing myself.

Where is this patch? (Was it submitted upstream to linux-kernel?)

> > Do you have a kernel that runs under current qemu arm versatile  
> emulation? I
> > can poke around and figure out which irqs it expects _now_, but  
> it's not
> > "right" and presumably you're just going to change it again when  
> you realize
> > what qemu is doing and what the unpatched kernel is doing don't  
> match.
> 
> The ones on http://people.debian.org/~aurel32/qemu/armel/
> should work.

I'm not seeing a patch there.

Rob

Re: [Qemu-devel] [PATCH v4 00/11] Fix versatile_pci

[Peter Maydell]

On 8 July 2013 07:43, Rob Landley <rob@landley.net> wrote:
> On 06/29/2013 06:03:26 AM, Peter Maydell wrote:
>> The "with the patch" case is uninteresting, because it's not
>> actually a fix for anything, it's just a change that happened
>> to work with old QEMU, and it's not a change that is in upstream
>> Linux.
>
>
> I want to create a kernel image that works with the existing QEMU image
> people are likely to have installed on their systems, _and_ with current
> versions of QEMU.
>
> Is that what you consider "uninteresting"?

Interesting:
 * kernels which actually work on real hardware
 * mainline kernels which worked on old QEMU, because they're
   widely used
Uninteresting:
 * local hacks which worked by fluke on old QEMU but won't
   work on the hardware
 * making mainline kernels which didn't work on old QEMU work
   by adapting to their bugginess

(current vanilla mainline falls into the last of these categories)

> I am trying to produce "an image that runs under qemu". That level of
> genericity worked for several years, at least ever since 1.0. When it
> stopped, I complained. You seem to be expressing incredulity at the concept.

QEMU has always been free to improve its emulation to be
closer to what the hardware does. If your guest doesn't
work on the hardware it is always going to be liable to
break.

It's really common for new kernel changes to require new QEMU,
because they tickle some bit of hardware emulation we had omitted
or hadn't got right. If you want a kernel image that runs on
older QEMU, stick with an older kernel.

> I tried building a vanilla linux 3.10 kernel, and it doesn't work on qemu
> 1.5. I confirmed their current interrupt mapping does not match any known
> hardware or emulator, and that it changed more than once in incompatible
> ways as they did wild-ass-guess du jour about what it should be now.

Current vanilla works for the first PCI card, not for others.
It behaves like this on both QEMU 1.4 and 1.5, ie these changes
have not broken it further than it is already.

> So I did the obvious-to-me thing and patched it back to how it _was_, which
> is the only thing that works with previous versions of QEMU. You seem to
> find this approach shocking and unexpected.

You haven't patched it back like how it was, or it would work;
old 2.x kernels work fine. Old kernels wrote 27 to PCI_INTERRUPT_LINE,
not 59.

>> "would work on real hardware".
>
> The stuff that's so rare the kernel guys couldn't find any to test on?

They could, if they asked, if they cared. I actually went
around and tested Arnd's patchset back in 2011. Nobody's
ever asked me since. The point is that this patchset is a
line in the sand -- we now have a defensible set of behaviour,
because it's what the hardware does. Any future "hey, the kernel
changed and broke things" can be directed straight at the kernel
guys, because we're not changing any further unless it can be
demonstrated that we don't run a kernel that's been tested on h/w.

> Maybe I haven't been clear: I want something that works with current QEMU
> _and_ old QEMU.
>
> Your paragraph above implies that there's smoething _different_ I can write
> to PCI_INTERRUPT_LINE that will trigger the old behavior. So my patch to do
> the old IRQ (which worked fine with qemu 1.2) needs to be bigger to humor
> QEMU's heuristic. I'll try to figure out how to extend my patch to avoid
> triggering your new "does not work" mode.

The old mode is the broken one...

> Let's see, stick a printf into qemu, and:
>
>   =====Write 60=12,59=====
>
> So it's requesting IRQ 59, and QEMU's reponse is to _not_ give it IRQ 59
> unless we trigger the "broken" flag.
>
> So... real hardware doesn't get the IRQ it requests? Using the reqested IRQ
> is "broken"? (Apparently your patch for a real hardware kernel doesn't
> request the IRQ it's going to use? I'm confused...)

On real hardware, PCI_INTERRUPT_LINE has no effect -- all it
does is store a value for the kernel to read back. It has
no effect on the interrupt routing. (This is true for any
PCI controller.) However, the kernel happens to write a value
which corresponds to what it *thinks* the hardware is wired up to.
So we can use this to identify some of the mainline cases which
expect old broken behaviour.

> Right, easy enough to fix:
>
> --- a/arch/arm/mach-versatile/pci.c
> +++ b/arch/arm/mach-versatile/pci.c
> @@ -333,7 +333,13 @@ static int __init versatile_map_irq(const struct
> pci_dev *dev, u8 slot, u8 pin)
>
>          *  26     1     IRQ_SIC_PCI2
>          *  27     1     IRQ_SIC_PCI3
>          */
> -       irq = IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
> +       // Hit QEMU 1.5.0 and later with a brick so it uses the IRQ we say.

This comment is totally wrong. More accurate would be:
    // This code won't work on hardware because we still assume
    // the broken interrupt mapping used by old (pre 1.5.0)
    // versions of QEMU. Ensure that QEMU 1.5 is in its backward
    // compatibility mode by writing a definitely wrong number
    // to PCI_INTERRUPT_LINE.

Also you're probably better off doing this write in the
initialization code, rather than in versatile_map_irq().

> +       dev->bus->ops->write(dev->bus, dev->devfn, PCI_INTERRUPT_LINE, 1,
> 27);
> +
> +       // The kernel has no clue where IRQs are, and its current
> assignments
> +       // match neither the hardware nor historic QEMU. Use historic QEMU
> +       // for compatability with old versions.
>
> +       irq = 59; //IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
>
>         return irq;
>  }
>
> That should work with new _and_ old QEMU.

>> I tested these changes against a patchset which Arnd wrote which
>> I can guarantee was tested on real hardware because I did the
>> testing myself.
>
>
> Where is this patch? (Was it submitted upstream to linux-kernel?)

Yes. http://comments.gmane.org/gmane.linux.ports.arm.kernel/93393

>> > Do you have a kernel that runs under current qemu arm versatile
>> > emulation? I
>> > can poke around and figure out which irqs it expects _now_, but it's not
>> > "right" and presumably you're just going to change it again when you
>> > realize
>> > what qemu is doing and what the unpatched kernel is doing don't match.
>>
>> The ones on http://people.debian.org/~aurel32/qemu/armel/
>> should work.
>
>
> I'm not seeing a patch there.

You asked for a kernel, not a patch. They're Debian's standard
2.6.32-5.

-- PMM