[Qemu-devel] [PATCH v2 00/11] Fix versatile_pci (now without breaking linux)

[Qemu-devel] [PATCH v2 00/11] Fix versatile_pci (now without breaking linux)

[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.)
I haven't included any mechanism for forcing this autodetection off.

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   |  386 +++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 358 insertions(+), 61 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 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>
---
 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 v2 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>
---
 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 v2 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>
---
 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 v2 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>
---
 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 v2 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>
---
 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 v2 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>
---
 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 v2 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. If this doesn't match reality we use the broken mapping.

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

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 576e619..1b56796 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -26,6 +26,9 @@ typedef struct {
 
     /* Constant for life of device: */
     int realview;
+
+    /* Variable state: */
+    bool broken_irq_mapping;
 } PCIVPBState;
 
 #define TYPE_VERSATILE_PCI "versatile_pci"
@@ -44,14 +47,37 @@ 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);
+    /* 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.
+     * We allow arbitrary flipping between broken and non broken
+     * to permit corner cases like kexec between two kernels.
+     */
+    PCIVPBState *s = (PCIVPBState *)opaque;
+    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE) {
+        uint8_t devfn = addr >> 8;
+        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {
+            if (val == 27) {
+                s->broken_irq_mapping = true;
+            } else {
+                s->broken_irq_mapping = false;
+            }
+        }
+    }
+    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);
 }
 
 static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
                                     unsigned size)
 {
+    PCIVPBState *s = (PCIVPBState *)opaque;
     uint32_t val;
-    val = pci_data_read(opaque, vpb_pci_config_addr(addr), size);
+    val = pci_data_read(&s->pci_bus, vpb_pci_config_addr(addr), size);
     return val;
 }
 
@@ -63,7 +89,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->broken_irq_mapping) {
+        /* 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)
@@ -95,13 +161,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 +183,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);
 
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 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>
---
 hw/arm/realview.c    |    7 +--
 hw/arm/versatilepb.c |    7 +--
 hw/versatile_pci.c   |  134 ++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 137 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 1b56796..a598b64 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;
@@ -28,9 +29,27 @@ typedef struct {
     int realview;
 
     /* Variable state: */
+    uint32_t imap[3];
+    uint32_t smap[3];
+    uint32_t selfid;
+    uint32_t flags;
     bool broken_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_BOOL(broken_irq_mapping, PCIVPBState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define TYPE_VERSATILE_PCI "versatile_pci"
 #define PCI_VPB(obj) \
     OBJECT_CHECK(PCIVPBState, (obj), TYPE_VERSATILE_PCI)
@@ -39,6 +58,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;
@@ -139,6 +245,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);
@@ -176,13 +296,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);
@@ -232,6 +354,8 @@ 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;
+    dc->vmsd = &pci_vpb_vmstate;
 }
 
 static const TypeInfo pci_vpb_info = {
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 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>
---
 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 v2 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>
---
 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 a598b64..1575dd7 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -20,13 +20,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];
@@ -36,10 +43,49 @@ typedef struct {
     bool broken_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),
@@ -81,6 +127,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:
@@ -257,6 +304,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)
@@ -265,9 +314,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;
 
@@ -275,6 +325,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)
@@ -301,6 +356,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);
@@ -320,6 +376,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);
 }
@@ -371,6 +437,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 v2 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>
---
 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 1575dd7..20092fe 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -192,11 +192,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)
 {
@@ -222,7 +217,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,
@@ -230,7 +225,7 @@ static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
 {
     PCIVPBState *s = (PCIVPBState *)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 v2 07/11] versatile_pci: Implement the correct PCI IRQ mapping

[Arnd Bergmann]

On Tuesday 26 March 2013, Peter Maydell wrote:
> 
> Implement the correct IRQ mapping for the Versatile PCI controller; it
> differs between realview and versatile boards, but the previous QEMU
> implementation was correct only for the first PCI card on a versatile
> board, since we weren't swizzling IRQs based on the slot number.
> 
> Since this change would otherwise break any uses of PCI on Linux kernels
> which have an equivalent bug (since they have effectively only been
> tested against QEMU, not real hardware), we implement a mechanism
> for automatically detecting those broken kernels and switching back
> to the old mapping. This works by looking at the values the kernel
> writes to the PCI_INTERRUPT_LINE register in the config space, which
> is effectively the interrupt number the kernel expects the device
> to be using. If this doesn't match reality we use the broken mapping.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Yes, very good.  We will probably introduce sparse irq support on
versatile in the near future, and then the value we write into the
PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
of view, but I will make sure that we fix the interrupt mapping
in the kernel at the same time so we always fall into the
"s->broken_irq_mapping = false;" case.

We also need to find a way to make the new kernel work with
an old qemu, and I think we can do that by using the versatile-dt
board type with a PCI device node that sets all four lines to
27, while using the actual interrupt lines for the default
versatile device tree.

	Arnd

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

[Peter Maydell]

On 26 March 2013 10:54, Arnd Bergmann <arnd@arndb.de> wrote:
> Yes, very good.  We will probably introduce sparse irq support on
> versatile in the near future, and then the value we write into the
> PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
> of view, but I will make sure that we fix the interrupt mapping
> in the kernel at the same time so we always fall into the
> "s->broken_irq_mapping = false;" case.

Yeah, as long as you avoid the number 27 you're ok :-)

> We also need to find a way to make the new kernel work with
> an old qemu, and I think we can do that by using the versatile-dt
> board type with a PCI device node that sets all four lines to
> 27, while using the actual interrupt lines for the default
> versatile device tree.

Personally I'd be happy for you to just say "needs a new QEMU".
The broken QEMU is missing so much (including working memory
windows) that I think it would be a pain to get the kernel to
cope with it.

-- PMM

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

[Arnd Bergmann]

On Tuesday 26 March 2013, Peter Maydell wrote:
> 
> On 26 March 2013 10:54, Arnd Bergmann <arnd@arndb.de> wrote:
> > Yes, very good.  We will probably introduce sparse irq support on
> > versatile in the near future, and then the value we write into the
> > PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
> > of view, but I will make sure that we fix the interrupt mapping
> > in the kernel at the same time so we always fall into the
> > "s->broken_irq_mapping = false;" case.
> 
> Yeah, as long as you avoid the number 27 you're ok :-)

Good point. I guess we'll have to keep using a legacy domain for
versatile then.

> > We also need to find a way to make the new kernel work with
> > an old qemu, and I think we can do that by using the versatile-dt
> > board type with a PCI device node that sets all four lines to
> > 27, while using the actual interrupt lines for the default
> > versatile device tree.
> 
> Personally I'd be happy for you to just say "needs a new QEMU".
> The broken QEMU is missing so much (including working memory
> windows) that I think it would be a pain to get the kernel to
> cope with it.

But it was working earlier, so I'd definitely try not to break
if at all possible. A lot of people use the verstatile qemu
model to run kernels and I would not want to deal with the
complaints I'd get if we break those. Using a separate dts
file seems easy enough.

	Arnd

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

[Peter Maydell]

On 26 March 2013 11:08, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 26 March 2013, Peter Maydell wrote:
>> On 26 March 2013 10:54, Arnd Bergmann <arnd@arndb.de> wrote:
>> > Yes, very good.  We will probably introduce sparse irq support on
>> > versatile in the near future, and then the value we write into the
>> > PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
>> > of view, but I will make sure that we fix the interrupt mapping
>> > in the kernel at the same time so we always fall into the
>> > "s->broken_irq_mapping = false;" case.
>>
>> Yeah, as long as you avoid the number 27 you're ok :-)
>
> Good point. I guess we'll have to keep using a legacy domain for
> versatile then.

I'm happy to provide some other way for QEMU to detect a
new working kernel if you want to implement one in the
kernel, if that's cleaner.

>> > We also need to find a way to make the new kernel work with
>> > an old qemu, and I think we can do that by using the versatile-dt
>> > board type with a PCI device node that sets all four lines to
>> > 27, while using the actual interrupt lines for the default
>> > versatile device tree.
>>
>> Personally I'd be happy for you to just say "needs a new QEMU".
>> The broken QEMU is missing so much (including working memory
>> windows) that I think it would be a pain to get the kernel to
>> cope with it.
>
> But it was working earlier, so I'd definitely try not to break
> if at all possible. A lot of people use the verstatile qemu
> model to run kernels and I would not want to deal with the
> complaints I'd get if we break those. Using a separate dts
> file seems easy enough.

Yeah, but it only worked earlier because the kernel PCI controller
support was so broken and limited, I think. If you run a new
kernel with the old QEMU it won't work even if you avoid the
irq-mapping issues.

Also I really don't want to get people into the habit of
using a QEMU-specific dts file. The result will be that
nobody will ever move on from that dts file to the one that
gets used with real hardware.

Plus, you guys make kernel changes that break running on
QEMU all the time, so I don't see that as a big deal really.
(Most recently, vexpress needed a pile of support for the
config registers that define the voltage regulators and clocks.)

-- PMM

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

[Michael S. Tsirkin]

On Tue, Mar 26, 2013 at 11:17:55AM +0000, Peter Maydell wrote:
> On 26 March 2013 11:08, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 26 March 2013, Peter Maydell wrote:
> >> On 26 March 2013 10:54, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > Yes, very good.  We will probably introduce sparse irq support on
> >> > versatile in the near future, and then the value we write into the
> >> > PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
> >> > of view, but I will make sure that we fix the interrupt mapping
> >> > in the kernel at the same time so we always fall into the
> >> > "s->broken_irq_mapping = false;" case.
> >>
> >> Yeah, as long as you avoid the number 27 you're ok :-)
> >
> > Good point. I guess we'll have to keep using a legacy domain for
> > versatile then.
> 
> I'm happy to provide some other way for QEMU to detect a
> new working kernel if you want to implement one in the
> kernel, if that's cleaner.

That's why I suggested writing some number at offset 0x08
(that's revision ID) in configuration space of device 0 on bus 0.
It's guaranteed harmless on real hardware and QEMU could detect this
write and say "aha new kernel, even though it uses #27".

> >> > We also need to find a way to make the new kernel work with
> >> > an old qemu, and I think we can do that by using the versatile-dt
> >> > board type with a PCI device node that sets all four lines to
> >> > 27, while using the actual interrupt lines for the default
> >> > versatile device tree.
> >>
> >> Personally I'd be happy for you to just say "needs a new QEMU".
> >> The broken QEMU is missing so much (including working memory
> >> windows) that I think it would be a pain to get the kernel to
> >> cope with it.
> >
> > But it was working earlier, so I'd definitely try not to break
> > if at all possible. A lot of people use the verstatile qemu
> > model to run kernels and I would not want to deal with the
> > complaints I'd get if we break those. Using a separate dts
> > file seems easy enough.
> 
> Yeah, but it only worked earlier because the kernel PCI controller
> support was so broken and limited, I think. If you run a new
> kernel with the old QEMU it won't work even if you avoid the
> irq-mapping issues.
> 
> Also I really don't want to get people into the habit of
> using a QEMU-specific dts file. The result will be that
> nobody will ever move on from that dts file to the one that
> gets used with real hardware.
> 
> Plus, you guys make kernel changes that break running on
> QEMU all the time, so I don't see that as a big deal really.
> (Most recently, vexpress needed a pile of support for the
> config registers that define the voltage regulators and clocks.)
> 
> -- PMM

I have to agree with that.

-- 
MST

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

[Peter Maydell]

On 26 March 2013 21:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Mar 26, 2013 at 11:17:55AM +0000, Peter Maydell wrote:
>> On 26 March 2013 11:08, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 26 March 2013, Peter Maydell wrote:
>> >> On 26 March 2013 10:54, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > Yes, very good.  We will probably introduce sparse irq support on
>> >> > versatile in the near future, and then the value we write into the
>> >> > PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
>> >> > of view, but I will make sure that we fix the interrupt mapping
>> >> > in the kernel at the same time so we always fall into the
>> >> > "s->broken_irq_mapping = false;" case.
>> >>
>> >> Yeah, as long as you avoid the number 27 you're ok :-)
>> >
>> > Good point. I guess we'll have to keep using a legacy domain for
>> > versatile then.
>>
>> I'm happy to provide some other way for QEMU to detect a
>> new working kernel if you want to implement one in the
>> kernel, if that's cleaner.
>
> That's why I suggested writing some number at offset 0x08
> (that's revision ID) in configuration space of device 0 on bus 0.
> It's guaranteed harmless on real hardware and QEMU could detect this
> write and say "aha new kernel, even though it uses #27".

OK, that makes sense. Would somebody like to provide a kernel
patch (preferably against 2.6.35+Arnd's patchset :-)) which
just prods that ID space, so I have something to test the
QEMU end against?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 00/11] Fix versatile_pci (now without breaking linux)

[Paul Brook]

> 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
>  * the I/O window wasn't mapped on VersatilePB

FWIW the documentation avaiable at the time I implemented the VersatilePB did 
not include the IO region. The PCI interrupt routing still seems to be missing 
from the docs.

Acked-by: Paul Brook <paul@codesourcery.com>
[Ignoring any issues with the backwards comaptibility hacks]

Re: [Qemu-devel] [PATCH v2 00/11] Fix versatile_pci (now without breaking linux)

[Peter Maydell]

On 28 March 2013 13:20, Paul Brook <paul@codesourcery.com> 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
>>  * the I/O window wasn't mapped on VersatilePB
>
> FWIW the documentation avaiable at the time I implemented the VersatilePB did
> not include the IO region. The PCI interrupt routing still seems to be missing
> from the docs.

The PCI interrupt routing is "documented" in the schematic of the PCI
backplane which is shipped on the CD with the board ;-)

-- PMM

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

[Michael S. Tsirkin]

On Thu, Apr 04, 2013 at 12:05:51PM +0100, Peter Maydell wrote:
> On 28 March 2013 10:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 26 March 2013 21:12, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Tue, Mar 26, 2013 at 11:17:55AM +0000, Peter Maydell wrote:
> >>> I'm happy to provide some other way for QEMU to detect a
> >>> new working kernel if you want to implement one in the
> >>> kernel, if that's cleaner.
> >>
> >> That's why I suggested writing some number at offset 0x08
> >> (that's revision ID) in configuration space of device 0 on bus 0.
> >> It's guaranteed harmless on real hardware and QEMU could detect this
> >> write and say "aha new kernel, even though it uses #27".
> >
> > OK, that makes sense.
> 
> In fact I've just realised that "allow new kernels to use #27
> without getting the back-compat broken irq mapping" conflicts
> with "allow new kernels to kexec old broken kernels".

Could you clarify why, pls?  Also - does this really matter so much?
kexec between version is always a risky affair...

> So the
> easiest approach is just to drop the code which allows us to
> switch back to broken mode again. Then the new kernel's init
> code can force non-broken mode by writing something other than
> #27 to some device's PCI_INTERRUPT_LINE register, and then
> is free to use #27 if it wants to.
> 
> -- PMM

I'm fine with this too.

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

[Peter Maydell]

On 28 March 2013 10:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 March 2013 21:12, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, Mar 26, 2013 at 11:17:55AM +0000, Peter Maydell wrote:
>>> I'm happy to provide some other way for QEMU to detect a
>>> new working kernel if you want to implement one in the
>>> kernel, if that's cleaner.
>>
>> That's why I suggested writing some number at offset 0x08
>> (that's revision ID) in configuration space of device 0 on bus 0.
>> It's guaranteed harmless on real hardware and QEMU could detect this
>> write and say "aha new kernel, even though it uses #27".
>
> OK, that makes sense.

In fact I've just realised that "allow new kernels to use #27
without getting the back-compat broken irq mapping" conflicts
with "allow new kernels to kexec old broken kernels". So the
easiest approach is just to drop the code which allows us to
switch back to broken mode again. Then the new kernel's init
code can force non-broken mode by writing something other than
#27 to some device's PCI_INTERRUPT_LINE register, and then
is free to use #27 if it wants to.

-- PMM

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

[Peter Maydell]

On 4 April 2013 11:58, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Apr 04, 2013 at 12:05:51PM +0100, Peter Maydell wrote:
>> On 28 March 2013 10:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > On 26 March 2013 21:12, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> On Tue, Mar 26, 2013 at 11:17:55AM +0000, Peter Maydell wrote:
>> >>> I'm happy to provide some other way for QEMU to detect a
>> >>> new working kernel if you want to implement one in the
>> >>> kernel, if that's cleaner.
>> >>
>> >> That's why I suggested writing some number at offset 0x08
>> >> (that's revision ID) in configuration space of device 0 on bus 0.
>> >> It's guaranteed harmless on real hardware and QEMU could detect this
>> >> write and say "aha new kernel, even though it uses #27".
>> >
>> > OK, that makes sense.
>>
>> In fact I've just realised that "allow new kernels to use #27
>> without getting the back-compat broken irq mapping" conflicts
>> with "allow new kernels to kexec old broken kernels".
>
> Could you clarify why, pls?

Either we provide a path for QEMU's state to go from "this
is a new kernel and we act like hardware" to "this is an old
kernel and we do not", or we don't. If we provide a way for
that state transition to occur, then new kernels can't use
#27; if we don't, then the kexec'd old kernel wouldn't see
the old broken mapping it expects. I only stuck the "allow
us to go back to the old mapping" code in there because I
didn't see why it would hurt at the time.

>  Also - does this really matter so much?
> kexec between version is always a risky affair...

I agree, which is why I'm dropping that idea.

-- PMM

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

[Peter Maydell]

On 26 March 2013 11:08, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 26 March 2013, Peter Maydell wrote:
>>
>> On 26 March 2013 10:54, Arnd Bergmann <arnd@arndb.de> wrote:
>> > Yes, very good.  We will probably introduce sparse irq support on
>> > versatile in the near future, and then the value we write into the
>> > PCI_INTERRUPT_LINE field will become arbitrary from qemu's point
>> > of view, but I will make sure that we fix the interrupt mapping
>> > in the kernel at the same time so we always fall into the
>> > "s->broken_irq_mapping = false;" case.
>>
>> Yeah, as long as you avoid the number 27 you're ok :-)
>
> Good point. I guess we'll have to keep using a legacy domain for
> versatile then.

So there turns out to be a problem with this. Newer
kernels (I'm looking at current mainline master) write
a number other than 27 (in this case 94) to the
PCI_INTERRUPT_LINE register, but they still assume the
old broken IRQ mapping. So our "autodetect busted kernels"
code doesn't work.

I've also discovered that there are some kernels (including
current master) which don't like it when the host PCI bridge
goes at slot 29 like it does on real hardware; so I think
I'll just have to revert that patch.

-- PMM