[Qemu-devel] [PATCH 0/3] Add support for for GICv2m and MSIs to arm-virt

[Qemu-devel] [PATCH 0/3] Add support for for GICv2m and MSIs to arm-virt

[Christoffer Dall]

Now when we have a host generic PCIe controller in the virt board, it
would be nice to be able to use MSIs so that we can eventually enable
VHOST with KVM.

With these patches you can use MSIs with TCG and with KVM, but you still
need some fixes for the mapping of the IRQ index to the GSI number for
IRQFD to work.  A separate patch series will follow this one to enable
that.

Tested with KVM on XGene and with TCG by configuring a virtio-pci
network adapter for the guest and verifying MSIs going through as
expected.

Christoffer Dall (3):
  target-arm: Add GIC phandle to VirtBoardInfo
  arm_gicv2m: Add GICv2m widget to support MSIs
  target-arm: Add the GICv2m to the virt board

 hw/arm/virt.c         |  69 +++++++++++++++----
 hw/intc/Makefile.objs |   1 +
 hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
 pixman                |   2 +-
 4 files changed, 237 insertions(+), 15 deletions(-)
 create mode 100644 hw/intc/arm_gicv2m.c

-- 
2.1.2.330.g565301e.dirty

[Qemu-devel] [PATCH 1/3] target-arm: Add GIC phandle to VirtBoardInfo

[Christoffer Dall]

Instead of passing the GIC phandle around between functions, add it to
the VirtBoardInfo just like we do for the clock_phandle.  We are about
to add the v2m phandle as well, and it's easier not having to pass
around a bunch of phandles, return multiple values from functions, etc.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/arm/virt.c | 26 +++++++++++---------------
 pixman        |  2 +-
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index febff22..e01676a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -87,6 +87,7 @@ typedef struct VirtBoardInfo {
     void *fdt;
     int fdt_size;
     uint32_t clock_phandle;
+    uint32_t gic_phandle;
 } VirtBoardInfo;
 
 typedef struct {
@@ -322,12 +323,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
     }
 }
 
-static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi)
+static void fdt_add_gic_node(VirtBoardInfo *vbi)
 {
-    uint32_t gic_phandle;
 
-    gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
-    qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", gic_phandle);
+    vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
+    qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", vbi->gic_phandle);
 
     qemu_fdt_add_subnode(vbi->fdt, "/intc");
     /* 'cortex-a15-gic' means 'GIC v2' */
@@ -340,12 +340,10 @@ static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi)
                                      2, vbi->memmap[VIRT_GIC_DIST].size,
                                      2, vbi->memmap[VIRT_GIC_CPU].base,
                                      2, vbi->memmap[VIRT_GIC_CPU].size);
-    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle);
-
-    return gic_phandle;
+    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
 }
 
-static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
 {
     /* We create a standalone GIC v2 */
     DeviceState *gicdev;
@@ -392,7 +390,7 @@ static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
         pic[i] = qdev_get_gpio_in(gicdev, i);
     }
 
-    return fdt_add_gic_node(vbi);
+    fdt_add_gic_node(vbi);
 }
 
 static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
@@ -639,8 +637,7 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
                            0x7           /* PCI irq */);
 }
 
-static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
-                        uint32_t gic_phandle)
+static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
 {
     hwaddr base = vbi->memmap[VIRT_PCIE].base;
     hwaddr size = vbi->memmap[VIRT_PCIE].size;
@@ -712,7 +709,7 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
                                  2, base_mmio, 2, size_mmio);
 
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
-    create_pcie_irq_map(vbi, gic_phandle, irq, nodename);
+    create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
 
     g_free(nodename);
 }
@@ -734,7 +731,6 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     const char *cpu_model = machine->cpu_model;
     VirtBoardInfo *vbi;
-    uint32_t gic_phandle;
     char **cpustr;
 
     if (!cpu_model) {
@@ -812,13 +808,13 @@ static void machvirt_init(MachineState *machine)
 
     create_flash(vbi);
 
-    gic_phandle = create_gic(vbi, pic);
+    create_gic(vbi, pic);
 
     create_uart(vbi, pic);
 
     create_rtc(vbi, pic);
 
-    create_pcie(vbi, pic, gic_phandle);
+    create_pcie(vbi, pic);
 
     /* Create mmio transports, so the user can create virtio backends
      * (which will be automatically plugged in to the transports). If
diff --git a/pixman b/pixman
index 87eea99..97336fa 160000
--- a/pixman
+++ b/pixman
@@ -1 +1 @@
-Subproject commit 87eea99e443b389c978cf37efc52788bf03a0ee0
+Subproject commit 97336fad32acf802003855cd8bd6477fa49a12e3
-- 
2.1.2.330.g565301e.dirty

[Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Christoffer Dall]

The ARM GICv2m widget is a little device that handle MSI interrupt
writes to a trigger register and ties them to a range of interrupt lines
wires to the GIC.  It has a few status/id registers and the interrupt wires,
and that's about it.

A board instantiates the device by setting the base SPI number and
number SPIs for the frame.  The base-spi parameter is indexed in the SPI
number space only, so base-spi == 0, means IRQ number 32.  When a device
(the PCI host controller) writes to the trigger register, the payload is
the GIC IRQ number, so we have to subtract 32 from that and then index
into our frame of SPIs.

When instantiating a GICv2m device, tell PCI that we have instantiated
something that can deal with MSIs.  We rely on the board actually wiring
up the GICv2m to the PCI host controller.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/intc/Makefile.objs |   1 +
 hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 181 insertions(+)
 create mode 100644 hw/intc/arm_gicv2m.c

diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 843864a..6b63dfc 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
 
 obj-$(CONFIG_APIC) += apic.o apic_common.o
 obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
+obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
 obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
 obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
 obj-$(CONFIG_GRLIB) += grlib_irqmp.o
diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
new file mode 100644
index 0000000..a80a16d
--- /dev/null
+++ b/hw/intc/arm_gicv2m.c
@@ -0,0 +1,180 @@
+/*
+ *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
+ *
+ * Copyright (C) 2015 Linaro, All rights reserved.
+ *
+ * Author: Christoffer Dall <christoffer.dall@linaro.org>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/sysbus.h"
+#include "hw/pci/msi.h"
+
+#define TYPE_GICV2M "gicv2m"
+#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
+
+#define GICV2M_NUM_SPI_MAX 128
+
+#define V2M_MSI_TYPER           0x008
+#define V2M_MSI_SETSPI_NS       0x040
+#define V2M_MSI_IIDR            0xFCC
+#define V2M_IIDR0               0xFD0
+
+#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
+#define IMPLEMENTER_ARM         0x43b
+
+typedef struct GICv2mState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq spi[GICV2M_NUM_SPI_MAX];
+
+    uint32_t base_spi;
+    uint32_t num_spi;
+} GICv2mState;
+
+static void gicv2m_set_irq(void *opaque, int irq)
+{
+    GICv2mState *s = (GICv2mState *)opaque;
+
+    qemu_irq_pulse(s->spi[irq]);
+}
+
+static uint64_t gicv2m_read(void *opaque, hwaddr offset,
+                            unsigned size)
+{
+    GICv2mState *s = (GICv2mState *)opaque;
+    uint32_t val;
+
+    if (size != 4) {
+        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
+        return 0;
+    }
+
+    switch (offset) {
+    case V2M_MSI_TYPER:
+        val = (s->base_spi + 32) << 16;
+        val |= s->num_spi;
+        return val;
+    case V2M_MSI_IIDR:
+        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
+    default:
+        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
+            return 0;
+        }
+
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "gicv2m_read: Bad offset %x\n", (int)offset);
+        return 0;
+    }
+}
+
+static void gicv2m_write(void *opaque, hwaddr offset,
+                        uint64_t value, unsigned size)
+{
+    GICv2mState *s = (GICv2mState *)opaque;
+
+    if (size != 4) {
+        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
+        return;
+    }
+
+    switch (offset) {
+    case V2M_MSI_SETSPI_NS: {
+        int spi;
+
+        spi = (value & 0x3ff) - (s->base_spi + 32);
+        if (spi < s->num_spi) {
+            gicv2m_set_irq(s, spi);
+        }
+        return;
+    }
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "gicv2m_write: Bad offset %x\n", (int)offset);
+        return;
+    }
+}
+
+static const MemoryRegionOps gicv2m_ops = {
+    .read = gicv2m_read,
+    .write = gicv2m_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void gicv2m_realize(DeviceState *dev, Error **errp)
+{
+    GICv2mState *s = GICV2M(dev);
+    int i;
+
+    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
+        error_setg(errp,
+                   "requested %u SPIs exceeds GICv2m frame maximum %d",
+                   s->num_spi, GICV2M_NUM_SPI_MAX);
+        return;
+    }
+
+    if (s->base_spi + 32 > 1020 - s->num_spi) {
+        error_setg(errp,
+                   "requested base SPI %u+%u exceeds max. number 1020",
+                   s->base_spi + 32, s->num_spi);
+        return;
+    }
+
+    for (i = 0; i < s->num_spi; i++) {
+        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
+    }
+
+    msi_supported = true;
+}
+
+static void gicv2m_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    GICv2mState *s = GICV2M(obj);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
+                          "gicv2m", 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static Property gicv2m_properties[] = {
+    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
+    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void gicv2m_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = gicv2m_properties;
+    dc->realize = gicv2m_realize;
+}
+
+static const TypeInfo gicv2m_info = {
+    .name          = TYPE_GICV2M,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(GICv2mState),
+    .instance_init = gicv2m_init,
+    .class_init    = gicv2m_class_init,
+};
+
+static void gicv2m_register_types(void)
+{
+    type_register_static(&gicv2m_info);
+}
+
+type_init(gicv2m_register_types)
-- 
2.1.2.330.g565301e.dirty

[Qemu-devel] [PATCH 3/3] target-arm: Add the GICv2m to the virt board

[Christoffer Dall]

Adding the GICv2m to the virt board should allow us to enable MSIs on
the generic PCI host controller, in theory.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/arm/virt.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e01676a..0c8dae9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -45,6 +45,7 @@
 #include "hw/pci-host/gpex.h"
 
 #define NUM_VIRTIO_TRANSPORTS 32
+#define NUM_GICV2M_SPIS 64
 
 /* Number of external interrupt lines to configure the GIC with */
 #define NUM_IRQS 128
@@ -71,6 +72,7 @@ enum {
     VIRT_RTC,
     VIRT_FW_CFG,
     VIRT_PCIE,
+    VIRT_GIC_V2M,
 };
 
 typedef struct MemMapEntry {
@@ -88,6 +90,7 @@ typedef struct VirtBoardInfo {
     int fdt_size;
     uint32_t clock_phandle;
     uint32_t gic_phandle;
+    uint32_t v2m_phandle;
 } VirtBoardInfo;
 
 typedef struct {
@@ -127,6 +130,7 @@ static const MemMapEntry a15memmap[] = {
     /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
     [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
     [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
+    [VIRT_GIC_V2M] =    { 0x08020000, 0x00001000 },
     [VIRT_UART] =       { 0x09000000, 0x00001000 },
     [VIRT_RTC] =        { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
@@ -148,6 +152,7 @@ static const int a15irqmap[] = {
     [VIRT_RTC] = 2,
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
+    [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
 };
 
 static VirtBoardInfo machines[] = {
@@ -323,9 +328,21 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
     }
 }
 
-static void fdt_add_gic_node(VirtBoardInfo *vbi)
+static void fdt_add_v2m_gic_node(VirtBoardInfo *vbi)
 {
+    vbi->v2m_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
+    qemu_fdt_add_subnode(vbi->fdt, "/intc/v2m");
+    qemu_fdt_setprop_string(vbi->fdt, "/intc/v2m", "compatible",
+                            "arm,gic-v2m-frame");
+    qemu_fdt_setprop(vbi->fdt, "/intc/v2m", "msi-controller", NULL, 0);
+    qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc/v2m", "reg",
+                                 2, vbi->memmap[VIRT_GIC_V2M].base,
+                                 2, vbi->memmap[VIRT_GIC_V2M].size);
+    qemu_fdt_setprop_cell(vbi->fdt, "/intc/v2m", "phandle", vbi->v2m_phandle);
+}
 
+static void fdt_add_gic_node(VirtBoardInfo *vbi)
+{
     vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
     qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", vbi->gic_phandle);
 
@@ -340,9 +357,31 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
                                      2, vbi->memmap[VIRT_GIC_DIST].size,
                                      2, vbi->memmap[VIRT_GIC_CPU].base,
                                      2, vbi->memmap[VIRT_GIC_CPU].size);
+    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
+    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
+    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
 }
 
+static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    int i;
+    int irq = vbi->irqmap[VIRT_GIC_V2M];
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "gicv2m");
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vbi->memmap[VIRT_GIC_V2M].base);
+    qdev_prop_set_uint32(dev, "base-spi", irq);
+    qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS);
+    qdev_init_nofail(dev);
+
+    for (i = 0; i < NUM_GICV2M_SPIS; i++) {
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]);
+    }
+
+    fdt_add_v2m_gic_node(vbi);
+}
+
 static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
 {
     /* We create a standalone GIC v2 */
@@ -391,6 +430,8 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
     }
 
     fdt_add_gic_node(vbi);
+
+    create_v2m(vbi, pic);
 }
 
 static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
@@ -699,6 +740,10 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
     qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
                            nr_pcie_buses - 1);
+    qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
+                           nr_pcie_buses - 1);
+
+    qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent", vbi->v2m_phandle);
 
     qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
                                  2, base_ecam, 2, size_ecam);
-- 
2.1.2.330.g565301e.dirty

Re: [Qemu-devel] [PATCH 0/3] Add support for for GICv2m and MSIs to arm-virt

[Peter Maydell]

On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Now when we have a host generic PCIe controller in the virt board, it
> would be nice to be able to use MSIs so that we can eventually enable
> VHOST with KVM.
>
> With these patches you can use MSIs with TCG and with KVM, but you still
> need some fixes for the mapping of the IRQ index to the GSI number for
> IRQFD to work.  A separate patch series will follow this one to enable
> that.
>
> Tested with KVM on XGene and with TCG by configuring a virtio-pci
> network adapter for the guest and verifying MSIs going through as
> expected.
>
> Christoffer Dall (3):
>   target-arm: Add GIC phandle to VirtBoardInfo
>   arm_gicv2m: Add GICv2m widget to support MSIs
>   target-arm: Add the GICv2m to the virt board
>
>  hw/arm/virt.c         |  69 +++++++++++++++----
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  pixman                |   2 +-

Will review properly later, but what's pixman doing in your diffstat? :-)

-- PMM

Re: [Qemu-devel] [PATCH 0/3] Add support for for GICv2m and MSIs to arm-virt

[Nikolay Nikolaev]

On Thu, Apr 9, 2015 at 12:20 AM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> Now when we have a host generic PCIe controller in the virt board, it
> would be nice to be able to use MSIs so that we can eventually enable
> VHOST with KVM.
>
> With these patches you can use MSIs with TCG and with KVM, but you still
> need some fixes for the mapping of the IRQ index to the GSI number for
> IRQFD to work.  A separate patch series will follow this one to enable
> that.
>
> Tested with KVM on XGene and with TCG by configuring a virtio-pci
> network adapter for the guest and verifying MSIs going through as
> expected.

Christofer, have you measured the network performance difference with KVM.
Probably the next patchseries (with IRQFD) makes bigger difference.

In any case, sharing some numbers will be great.
Thanks.

regards,
Nikolay Nikolaev

>
> Christoffer Dall (3):
>   target-arm: Add GIC phandle to VirtBoardInfo
>   arm_gicv2m: Add GICv2m widget to support MSIs
>   target-arm: Add the GICv2m to the virt board
>
>  hw/arm/virt.c         |  69 +++++++++++++++----
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  pixman                |   2 +-
>  4 files changed, 237 insertions(+), 15 deletions(-)
>  create mode 100644 hw/intc/arm_gicv2m.c
>
> --
> 2.1.2.330.g565301e.dirty
>
>

Re: [Qemu-devel] [PATCH 0/3] Add support for for GICv2m and MSIs to arm-virt

[Christoffer Dall]

On Thu, Apr 9, 2015 at 12:01 AM, Nikolay Nikolaev
<n.nikolaev@virtualopensystems.com> wrote:
> On Thu, Apr 9, 2015 at 12:20 AM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
>> Now when we have a host generic PCIe controller in the virt board, it
>> would be nice to be able to use MSIs so that we can eventually enable
>> VHOST with KVM.
>>
>> With these patches you can use MSIs with TCG and with KVM, but you still
>> need some fixes for the mapping of the IRQ index to the GSI number for
>> IRQFD to work.  A separate patch series will follow this one to enable
>> that.
>>
>> Tested with KVM on XGene and with TCG by configuring a virtio-pci
>> network adapter for the guest and verifying MSIs going through as
>> expected.
>
> Christofer, have you measured the network performance difference with KVM.
> Probably the next patchseries (with IRQFD) makes bigger difference.
>
I did run numbers, and it depends greatly on the workload, but for
something simple like netperf, you see a huge increase in performance
(several orders of magnitude).  Surprisingly, a large portion of that
seems to come from using ioeventfd for the mmio accesses to vhost,
possibly because it begins to use multiple queues.  For latency
measurements, irqfd also makes a big difference.

-Christoffer

Re: [Qemu-devel] [PATCH 0/3] Add support for for GICv2m and MSIs to arm-virt

[Christoffer Dall]

On Wed, Apr 08, 2015 at 10:31:53PM +0100, Peter Maydell wrote:
> On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Now when we have a host generic PCIe controller in the virt board, it
> > would be nice to be able to use MSIs so that we can eventually enable
> > VHOST with KVM.
> >
> > With these patches you can use MSIs with TCG and with KVM, but you still
> > need some fixes for the mapping of the IRQ index to the GSI number for
> > IRQFD to work.  A separate patch series will follow this one to enable
> > that.
> >
> > Tested with KVM on XGene and with TCG by configuring a virtio-pci
> > network adapter for the guest and verifying MSIs going through as
> > expected.
> >
> > Christoffer Dall (3):
> >   target-arm: Add GIC phandle to VirtBoardInfo
> >   arm_gicv2m: Add GICv2m widget to support MSIs
> >   target-arm: Add the GICv2m to the virt board
> >
> >  hw/arm/virt.c         |  69 +++++++++++++++----
> >  hw/intc/Makefile.objs |   1 +
> >  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  pixman                |   2 +-
> 
> Will review properly later, but what's pixman doing in your diffstat? :-)
> 
Whoops :)

I've removed that change locally, will adjust when re-submitting after
you've reviewed them.

Thanks,
-Christoffer

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Eric Auger]

Hi Christoffer,
On 04/08/2015 11:20 PM, Christoffer Dall wrote:
> The ARM GICv2m widget is a little device that handle MSI interrupt
> writes to a trigger register and ties them to a range of interrupt lines
> wires to the GIC.  It has a few status/id registers and the interrupt wires,
> and that's about it.
> 
> A board instantiates the device by setting the base SPI number and
> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> number space only, so base-spi == 0, means IRQ number 32.  When a device
> (the PCI host controller) writes to the trigger register, the payload is
> the GIC IRQ number, so we have to subtract 32 from that and then index
> into our frame of SPIs.
> 
> When instantiating a GICv2m device, tell PCI that we have instantiated
> something that can deal with MSIs.  We rely on the board actually wiring
> up the GICv2m to the PCI host controller.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+)
>  create mode 100644 hw/intc/arm_gicv2m.c
> 
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 843864a..6b63dfc 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>  
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
objects?
>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> new file mode 100644
> index 0000000..a80a16d
> --- /dev/null
> +++ b/hw/intc/arm_gicv2m.c
> @@ -0,0 +1,180 @@
> +/*
> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> + *
> + * Copyright (C) 2015 Linaro, All rights reserved.
> + *
> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/pci/msi.h"
> +
> +#define TYPE_GICV2M "gicv2m"
> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
> +
> +#define GICV2M_NUM_SPI_MAX 128
> +
Maybe we could add a comment that the model supports a single non secure
MSI register frame
> +#define V2M_MSI_TYPER           0x008
> +#define V2M_MSI_SETSPI_NS       0x040
> +#define V2M_MSI_IIDR            0xFCC
> +#define V2M_IIDR0               0xFD0
> +
> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> +#define IMPLEMENTER_ARM         0x43b
> +
> +typedef struct GICv2mState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> +
/* first absolute SPI index */, to avoid mixing with ID?
> +    uint32_t base_spi;
> +    uint32_t num_spi;
> +} GICv2mState;
> +
> +static void gicv2m_set_irq(void *opaque, int irq)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    qemu_irq_pulse(s->spi[irq]);
> +}
> +
> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> +                            unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +    uint32_t val;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
> +        return 0;
> +    }
> +
> +    switch (offset) {
> +    case V2M_MSI_TYPER:
> +        val = (s->base_spi + 32) << 16;
> +        val |= s->num_spi;
> +        return val;
> +    case V2M_MSI_IIDR:
> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
I guess there is a single arch revision (0?), [19:16]
> +    default:
> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
/* Peripheral ID2 reg */?
> +            return 0;
> +        }
> +
/* reserved */?
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> +        return 0;
> +    }
> +}
> +
> +static void gicv2m_write(void *opaque, hwaddr offset,
> +                        uint64_t value, unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
> +        return;
> +    }
> +
> +    switch (offset) {
> +    case V2M_MSI_SETSPI_NS: {
> +        int spi;
> +
> +        spi = (value & 0x3ff) - (s->base_spi + 32);
> +        if (spi < s->num_spi) {
> +            gicv2m_set_irq(s, spi);
> +        }
shouldn't we print an error msg in case spi is not within the frame range?
also shouldn't we check spi >= 0?

Best Regards
Eric
> +        return;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_write: Bad offset %x\n", (int)offset);
> +        return;
> +    }
> +}
> +
> +static const MemoryRegionOps gicv2m_ops = {
> +    .read = gicv2m_read,
> +    .write = gicv2m_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void gicv2m_realize(DeviceState *dev, Error **errp)
> +{
> +    GICv2mState *s = GICV2M(dev);
> +    int i;
> +
> +    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
> +        error_setg(errp,
> +                   "requested %u SPIs exceeds GICv2m frame maximum %d",
> +                   s->num_spi, GICV2M_NUM_SPI_MAX);
> +        return;
> +    }
> +
> +    if (s->base_spi + 32 > 1020 - s->num_spi) {
> +        error_setg(errp,
> +                   "requested base SPI %u+%u exceeds max. number 1020",
> +                   s->base_spi + 32, s->num_spi);
> +        return;
> +    }
> +
> +    for (i = 0; i < s->num_spi; i++) {
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> +    }
> +
> +    msi_supported = true;
> +}
> +
> +static void gicv2m_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    GICv2mState *s = GICV2M(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
> +                          "gicv2m", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static Property gicv2m_properties[] = {
> +    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
> +    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void gicv2m_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = gicv2m_properties;
> +    dc->realize = gicv2m_realize;
> +}
> +
> +static const TypeInfo gicv2m_info = {
> +    .name          = TYPE_GICV2M,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GICv2mState),
> +    .instance_init = gicv2m_init,
> +    .class_init    = gicv2m_class_init,
> +};
> +
> +static void gicv2m_register_types(void)
> +{
> +    type_register_static(&gicv2m_info);
> +}
> +
> +type_init(gicv2m_register_types)
> 

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Christoffer Dall]

On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
> Hi Christoffer,
> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
> > The ARM GICv2m widget is a little device that handle MSI interrupt
> > writes to a trigger register and ties them to a range of interrupt lines
> > wires to the GIC.  It has a few status/id registers and the interrupt wires,
> > and that's about it.
> > 
> > A board instantiates the device by setting the base SPI number and
> > number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> > number space only, so base-spi == 0, means IRQ number 32.  When a device
> > (the PCI host controller) writes to the trigger register, the payload is
> > the GIC IRQ number, so we have to subtract 32 from that and then index
> > into our frame of SPIs.
> > 
> > When instantiating a GICv2m device, tell PCI that we have instantiated
> > something that can deal with MSIs.  We rely on the board actually wiring
> > up the GICv2m to the PCI host controller.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  hw/intc/Makefile.objs |   1 +
> >  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 181 insertions(+)
> >  create mode 100644 hw/intc/arm_gicv2m.c
> > 
> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> > index 843864a..6b63dfc 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
> >  
> >  obj-$(CONFIG_APIC) += apic.o apic_common.o
> >  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> > +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
> objects?

I'm honestly not quite sure what the difference between common-obj-y and
obj-y is?

> >  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
> >  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
> >  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> > diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> > new file mode 100644
> > index 0000000..a80a16d
> > --- /dev/null
> > +++ b/hw/intc/arm_gicv2m.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> > + *
> > + * Copyright (C) 2015 Linaro, All rights reserved.
> > + *
> > + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "hw/sysbus.h"
> > +#include "hw/pci/msi.h"
> > +
> > +#define TYPE_GICV2M "gicv2m"
> > +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
> > +
> > +#define GICV2M_NUM_SPI_MAX 128
> > +
> Maybe we could add a comment that the model supports a single non secure
> MSI register frame

Isn't that already part of the GICv2m specification and hence implied
for emulating a gicv2m?

> > +#define V2M_MSI_TYPER           0x008
> > +#define V2M_MSI_SETSPI_NS       0x040
> > +#define V2M_MSI_IIDR            0xFCC
> > +#define V2M_IIDR0               0xFD0
> > +
> > +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> > +#define IMPLEMENTER_ARM         0x43b
> > +
> > +typedef struct GICv2mState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion iomem;
> > +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> > +
> /* first absolute SPI index */, to avoid mixing with ID?

not sure this comment helps, I think reading the code is actually more
clear, unless you can think of an even more clear wording for the
comment?

> > +    uint32_t base_spi;
> > +    uint32_t num_spi;
> > +} GICv2mState;
> > +
> > +static void gicv2m_set_irq(void *opaque, int irq)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    qemu_irq_pulse(s->spi[irq]);
> > +}
> > +
> > +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> > +                            unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +    uint32_t val;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
> > +        return 0;
> > +    }
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_TYPER:
> > +        val = (s->base_spi + 32) << 16;
> > +        val |= s->num_spi;
> > +        return val;
> > +    case V2M_MSI_IIDR:
> > +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
> I guess there is a single arch revision (0?), [19:16]

yes, see the spec.

> > +    default:
> > +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
> /* Peripheral ID2 reg */?
> > +            return 0;
> > +        }
> > +
> /* reserved */?
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> > +        return 0;
> > +    }
> > +}
> > +
> > +static void gicv2m_write(void *opaque, hwaddr offset,
> > +                        uint64_t value, unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
> > +        return;
> > +    }
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_SETSPI_NS: {
> > +        int spi;
> > +
> > +        spi = (value & 0x3ff) - (s->base_spi + 32);
> > +        if (spi < s->num_spi) {
> > +            gicv2m_set_irq(s, spi);
> > +        }
> shouldn't we print an error msg in case spi is not within the frame range?
> also shouldn't we check spi >= 0?

no, we should just emulate the behavior of the device, which clearly
states: "If the resulting value does not identify an SPI that is
allocated to this frame then the write has no effect." - so no effect is
what I'm going for :)


Thanks for the review!

-Christoffer

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Eric Auger]

On 04/10/2015 11:58 AM, Christoffer Dall wrote:
> On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
>> Hi Christoffer,
>> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
>>> The ARM GICv2m widget is a little device that handle MSI interrupt
>>> writes to a trigger register and ties them to a range of interrupt lines
>>> wires to the GIC.  It has a few status/id registers and the interrupt wires,
>>> and that's about it.
>>>
>>> A board instantiates the device by setting the base SPI number and
>>> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
>>> number space only, so base-spi == 0, means IRQ number 32.  When a device
>>> (the PCI host controller) writes to the trigger register, the payload is
>>> the GIC IRQ number, so we have to subtract 32 from that and then index
>>> into our frame of SPIs.
>>>
>>> When instantiating a GICv2m device, tell PCI that we have instantiated
>>> something that can deal with MSIs.  We rely on the board actually wiring
>>> up the GICv2m to the PCI host controller.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  hw/intc/Makefile.objs |   1 +
>>>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 181 insertions(+)
>>>  create mode 100644 hw/intc/arm_gicv2m.c
>>>
>>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>>> index 843864a..6b63dfc 100644
>>> --- a/hw/intc/Makefile.objs
>>> +++ b/hw/intc/Makefile.objs
>>> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>>>  
>>>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>>>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
>>> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
>> objects?
> 
> I'm honestly not quite sure what the difference between common-obj-y and
> obj-y is?
> 
>>>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>>>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>>>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
>>> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
>>> new file mode 100644
>>> index 0000000..a80a16d
>>> --- /dev/null
>>> +++ b/hw/intc/arm_gicv2m.c
>>> @@ -0,0 +1,180 @@
>>> +/*
>>> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
>>> + *
>>> + * Copyright (C) 2015 Linaro, All rights reserved.
>>> + *
>>> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "hw/sysbus.h"
>>> +#include "hw/pci/msi.h"
>>> +
>>> +#define TYPE_GICV2M "gicv2m"
>>> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
>>> +
>>> +#define GICV2M_NUM_SPI_MAX 128
>>> +
>> Maybe we could add a comment that the model supports a single non secure
>> MSI register frame
> 
> Isn't that already part of the GICv2m specification and hence implied
> for emulating a gicv2m?
> 
>>> +#define V2M_MSI_TYPER           0x008
>>> +#define V2M_MSI_SETSPI_NS       0x040
>>> +#define V2M_MSI_IIDR            0xFCC
>>> +#define V2M_IIDR0               0xFD0
>>> +
>>> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
>>> +#define IMPLEMENTER_ARM         0x43b
>>> +
>>> +typedef struct GICv2mState {
>>> +    SysBusDevice parent_obj;
>>> +
>>> +    MemoryRegion iomem;
>>> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
>>> +
>> /* first absolute SPI index */, to avoid mixing with ID?
> 
> not sure this comment helps, I think reading the code is actually more
> clear, unless you can think of an even more clear wording for the
> comment?
> 
>>> +    uint32_t base_spi;
>>> +    uint32_t num_spi;
>>> +} GICv2mState;
>>> +
>>> +static void gicv2m_set_irq(void *opaque, int irq)
>>> +{
>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>> +
>>> +    qemu_irq_pulse(s->spi[irq]);
>>> +}
>>> +
>>> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
>>> +                            unsigned size)
>>> +{
>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>> +    uint32_t val;
>>> +
>>> +    if (size != 4) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
>>> +        return 0;
>>> +    }
>>> +
>>> +    switch (offset) {
>>> +    case V2M_MSI_TYPER:
>>> +        val = (s->base_spi + 32) << 16;
>>> +        val |= s->num_spi;
>>> +        return val;
>>> +    case V2M_MSI_IIDR:
>>> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
>> I guess there is a single arch revision (0?), [19:16]
> 
> yes, see the spec.
> 
>>> +    default:
>>> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
>> /* Peripheral ID2 reg */?
>>> +            return 0;
>>> +        }
>>> +
>> /* reserved */?
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>> +static void gicv2m_write(void *opaque, hwaddr offset,
>>> +                        uint64_t value, unsigned size)
>>> +{
>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>> +
>>> +    if (size != 4) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
>>> +        return;
>>> +    }
>>> +
>>> +    switch (offset) {
>>> +    case V2M_MSI_SETSPI_NS: {
>>> +        int spi;
>>> +
>>> +        spi = (value & 0x3ff) - (s->base_spi + 32);
>>> +        if (spi < s->num_spi) {
>>> +            gicv2m_set_irq(s, spi);
>>> +        }
>> shouldn't we print an error msg in case spi is not within the frame range?
>> also shouldn't we check spi >= 0?
> 
> no, we should just emulate the behavior of the device, which clearly
> states: "If the resulting value does not identify an SPI that is
> allocated to this frame then the write has no effect." - so no effect is
> what I'm going for :)
OK,

and about spi>= 0?

Eric
> 
> 
> Thanks for the review!
> 
> -Christoffer
> 

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Christoffer Dall]

On Fri, Apr 10, 2015 at 12:34 PM, Eric Auger <eric.auger@linaro.org> wrote:
> On 04/10/2015 11:58 AM, Christoffer Dall wrote:
>> On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
>>> Hi Christoffer,
>>> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
>>>> The ARM GICv2m widget is a little device that handle MSI interrupt
>>>> writes to a trigger register and ties them to a range of interrupt lines
>>>> wires to the GIC.  It has a few status/id registers and the interrupt wires,
>>>> and that's about it.
>>>>
>>>> A board instantiates the device by setting the base SPI number and
>>>> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
>>>> number space only, so base-spi == 0, means IRQ number 32.  When a device
>>>> (the PCI host controller) writes to the trigger register, the payload is
>>>> the GIC IRQ number, so we have to subtract 32 from that and then index
>>>> into our frame of SPIs.
>>>>
>>>> When instantiating a GICv2m device, tell PCI that we have instantiated
>>>> something that can deal with MSIs.  We rely on the board actually wiring
>>>> up the GICv2m to the PCI host controller.
>>>>
>>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>> ---
>>>>  hw/intc/Makefile.objs |   1 +
>>>>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 181 insertions(+)
>>>>  create mode 100644 hw/intc/arm_gicv2m.c
>>>>
>>>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>>>> index 843864a..6b63dfc 100644
>>>> --- a/hw/intc/Makefile.objs
>>>> +++ b/hw/intc/Makefile.objs
>>>> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>>>>
>>>>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>>>>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
>>>> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>>> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
>>> objects?
>>
>> I'm honestly not quite sure what the difference between common-obj-y and
>> obj-y is?
>>
>>>>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>>>>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>>>>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
>>>> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
>>>> new file mode 100644
>>>> index 0000000..a80a16d
>>>> --- /dev/null
>>>> +++ b/hw/intc/arm_gicv2m.c
>>>> @@ -0,0 +1,180 @@
>>>> +/*
>>>> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
>>>> + *
>>>> + * Copyright (C) 2015 Linaro, All rights reserved.
>>>> + *
>>>> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
>>>> + *
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include "hw/sysbus.h"
>>>> +#include "hw/pci/msi.h"
>>>> +
>>>> +#define TYPE_GICV2M "gicv2m"
>>>> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
>>>> +
>>>> +#define GICV2M_NUM_SPI_MAX 128
>>>> +
>>> Maybe we could add a comment that the model supports a single non secure
>>> MSI register frame
>>
>> Isn't that already part of the GICv2m specification and hence implied
>> for emulating a gicv2m?
>>
>>>> +#define V2M_MSI_TYPER           0x008
>>>> +#define V2M_MSI_SETSPI_NS       0x040
>>>> +#define V2M_MSI_IIDR            0xFCC
>>>> +#define V2M_IIDR0               0xFD0
>>>> +
>>>> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
>>>> +#define IMPLEMENTER_ARM         0x43b
>>>> +
>>>> +typedef struct GICv2mState {
>>>> +    SysBusDevice parent_obj;
>>>> +
>>>> +    MemoryRegion iomem;
>>>> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
>>>> +
>>> /* first absolute SPI index */, to avoid mixing with ID?
>>
>> not sure this comment helps, I think reading the code is actually more
>> clear, unless you can think of an even more clear wording for the
>> comment?
>>
>>>> +    uint32_t base_spi;
>>>> +    uint32_t num_spi;
>>>> +} GICv2mState;
>>>> +
>>>> +static void gicv2m_set_irq(void *opaque, int irq)
>>>> +{
>>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>>> +
>>>> +    qemu_irq_pulse(s->spi[irq]);
>>>> +}
>>>> +
>>>> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
>>>> +                            unsigned size)
>>>> +{
>>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>>> +    uint32_t val;
>>>> +
>>>> +    if (size != 4) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    switch (offset) {
>>>> +    case V2M_MSI_TYPER:
>>>> +        val = (s->base_spi + 32) << 16;
>>>> +        val |= s->num_spi;
>>>> +        return val;
>>>> +    case V2M_MSI_IIDR:
>>>> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
>>> I guess there is a single arch revision (0?), [19:16]
>>
>> yes, see the spec.
>>
>>>> +    default:
>>>> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
>>> /* Peripheral ID2 reg */?
>>>> +            return 0;
>>>> +        }
>>>> +
>>> /* reserved */?
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
>>>> +        return 0;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void gicv2m_write(void *opaque, hwaddr offset,
>>>> +                        uint64_t value, unsigned size)
>>>> +{
>>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>>> +
>>>> +    if (size != 4) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    switch (offset) {
>>>> +    case V2M_MSI_SETSPI_NS: {
>>>> +        int spi;
>>>> +
>>>> +        spi = (value & 0x3ff) - (s->base_spi + 32);
>>>> +        if (spi < s->num_spi) {
>>>> +            gicv2m_set_irq(s, spi);
>>>> +        }
>>> shouldn't we print an error msg in case spi is not within the frame range?
>>> also shouldn't we check spi >= 0?
>>
>> no, we should just emulate the behavior of the device, which clearly
>> states: "If the resulting value does not identify an SPI that is
>> allocated to this frame then the write has no effect." - so no effect is
>> what I'm going for :)
> OK,
>
> and about spi>= 0?
>
oh, right, good point, I need to check the lower bound too.  Will address in v2.

Waiting for Peter's review and I will incorporate your fixes.

-Christoffer

Re: [Qemu-devel] [PATCH 1/3] target-arm: Add GIC phandle to VirtBoardInfo

[Peter Maydell]

On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Instead of passing the GIC phandle around between functions, add it to
> the VirtBoardInfo just like we do for the clock_phandle.  We are about
> to add the v2m phandle as well, and it's easier not having to pass
> around a bunch of phandles, return multiple values from functions, etc.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/arm/virt.c | 26 +++++++++++---------------
>  pixman        |  2 +-
>  2 files changed, 12 insertions(+), 16 deletions(-)

Apart from the stray pixman submodule bump,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Peter Maydell]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On 10 April 2015 at 10:58, Christoffer Dall <christoffer.dall@linaro.org>
wrote:
> On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
>> Hi Christoffer,
>> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
>> > The ARM GICv2m widget is a little device that handle MSI interrupt
>> > writes to a trigger register and ties them to a range of interrupt
lines
>> > wires to the GIC.  It has a few status/id registers and the interrupt
wires,
>> > and that's about it.
>> >
>> > A board instantiates the device by setting the base SPI number and
>> > number SPIs for the frame.  The base-spi parameter is indexed in the
SPI
>> > number space only, so base-spi == 0, means IRQ number 32.  When a
device
>> > (the PCI host controller) writes to the trigger register, the payload
is
>> > the GIC IRQ number, so we have to subtract 32 from that and then index
>> > into our frame of SPIs.
>> >
>> > When instantiating a GICv2m device, tell PCI that we have instantiated
>> > something that can deal with MSIs.  We rely on the board actually
wiring
>> > up the GICv2m to the PCI host controller.
>> >
>> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> > ---
>> >  hw/intc/Makefile.objs |   1 +
>> >  hw/intc/arm_gicv2m.c  | 180
++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 181 insertions(+)
>> >  create mode 100644 hw/intc/arm_gicv2m.c
>> >
>> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>> > index 843864a..6b63dfc 100644
>> > --- a/hw/intc/Makefile.objs
>> > +++ b/hw/intc/Makefile.objs
>> > @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>> >
>> >  obj-$(CONFIG_APIC) += apic.o apic_common.o
>> >  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
>> > +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
>> objects?
>
> I'm honestly not quite sure what the difference between common-obj-y and
> obj-y is?

common-obj-y is for object files which can be built once and
then linked into any qemu-*-softmmu binary, ie they have no
dependencies on details of the target CPU. obj-y is for
objects which get built for every target CPU they're going
to be used for (in this case that would mean separate .o files
for arm-softmmu and aarch64-softmmu).

common-obj-y is preferred unless there's something that
absolutely requires the code to be built per-cpu. In this
case there probably isn't anything like that.

-- PMM

[-- Attachment #2: Type: text/html, Size: 3110 bytes --]

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Peter Maydell]

[-- Attachment #1: Type: text/plain, Size: 8132 bytes --]

On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org>
wrote:

> The ARM GICv2m widget is a little device that handle MSI interrupt
>

"handles"


> writes to a trigger register and ties them to a range of interrupt lines
> wires to the GIC.  It has a few status/id registers and the interrupt
> wires,
> and that's about it.
>
> A board instantiates the device by setting the base SPI number and
> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> number space only, so base-spi == 0, means IRQ number 32.  When a device
> (the PCI host controller) writes to the trigger register, the payload is
> the GIC IRQ number, so we have to subtract 32 from that and then index
> into our frame of SPIs.
>
> When instantiating a GICv2m device, tell PCI that we have instantiated
> something that can deal with MSIs.  We rely on the board actually wiring
> up the GICv2m to the PCI host controller.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 180
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+)
>  create mode 100644 hw/intc/arm_gicv2m.c
>
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 843864a..6b63dfc 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>

Should be common-obj-, as per other email.


>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> new file mode 100644
> index 0000000..a80a16d
> --- /dev/null
> +++ b/hw/intc/arm_gicv2m.c
> @@ -0,0 +1,180 @@
> +/*
> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> + *
> + * Copyright (C) 2015 Linaro, All rights reserved.
> + *
> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <
> http://www.gnu.org/licenses/>.
> + */
>

A reference to the document defining the GICv2M would be nice here
(it's the SBSA unless I'm confused).



> +
> +#include "hw/sysbus.h"
> +#include "hw/pci/msi.h"
> +
> +#define TYPE_GICV2M "gicv2m"
>

I think this should be
#define TYPE_ARM_GICV2M "arm-gicv2m"


> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
>

and  ARM_GICV2M(obj)


> +
> +#define GICV2M_NUM_SPI_MAX 128
> +
> +#define V2M_MSI_TYPER           0x008
> +#define V2M_MSI_SETSPI_NS       0x040
> +#define V2M_MSI_IIDR            0xFCC
> +#define V2M_IIDR0               0xFD0
> +
> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> +#define IMPLEMENTER_ARM         0x43b
> +
> +typedef struct GICv2mState {
>

ARMGICv2mState

+    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> +
> +    uint32_t base_spi;
> +    uint32_t num_spi;
> +} GICv2mState;
> +
> +static void gicv2m_set_irq(void *opaque, int irq)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    qemu_irq_pulse(s->spi[irq]);
> +}
> +
> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> +                            unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +    uint32_t val;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n",
> size);
> +        return 0;
> +    }
> +
> +    switch (offset) {
> +    case V2M_MSI_TYPER:
> +        val = (s->base_spi + 32) << 16;
> +        val |= s->num_spi;
> +        return val;
> +    case V2M_MSI_IIDR:
> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
>

This choice of implementer field value is a little hard to justify :-)


> +    default:
> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
>

Why not just have a "case 0xFD0..0xFFC" ?
Could do with a comment about what the ID regs are
(ie that they're mostly impdef and we choose to read as zero).

+            return 0;
> +        }
> +
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> +        return 0;
> +    }
> +}
> +
> +static void gicv2m_write(void *opaque, hwaddr offset,
> +                        uint64_t value, unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n",
> size);
> +        return;
> +    }
>

"The MSI_SETSPI_S and MSI_SETSPI_NS registers must also support
16 bit writes to bits [15:0]", so you can't insist on 32 bit
accesses only here.


> +
> +    switch (offset) {
> +    case V2M_MSI_SETSPI_NS: {
> +        int spi;
> +
> +        spi = (value & 0x3ff) - (s->base_spi + 32);
> +        if (spi < s->num_spi) {
> +            gicv2m_set_irq(s, spi);
> +        }
> +        return;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_write: Bad offset %x\n", (int)offset);
> +        return;
> +    }
> +}
> +
> +static const MemoryRegionOps gicv2m_ops = {
> +    .read = gicv2m_read,
> +    .write = gicv2m_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void gicv2m_realize(DeviceState *dev, Error **errp)
> +{
> +    GICv2mState *s = GICV2M(dev);
> +    int i;
> +
> +    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
> +        error_setg(errp,
> +                   "requested %u SPIs exceeds GICv2m frame maximum %d",
> +                   s->num_spi, GICV2M_NUM_SPI_MAX);
> +        return;
> +    }
> +
> +    if (s->base_spi + 32 > 1020 - s->num_spi) {
> +        error_setg(errp,
> +                   "requested base SPI %u+%u exceeds max. number 1020",
> +                   s->base_spi + 32, s->num_spi);
> +        return;
> +    }
> +
> +    for (i = 0; i < s->num_spi; i++) {
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> +    }
> +
> +    msi_supported = true;
> +}
> +
> +static void gicv2m_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    GICv2mState *s = GICV2M(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
> +                          "gicv2m", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static Property gicv2m_properties[] = {
> +    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
> +    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void gicv2m_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = gicv2m_properties;
> +    dc->realize = gicv2m_realize;
> +}
> +
> +static const TypeInfo gicv2m_info = {
> +    .name          = TYPE_GICV2M,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GICv2mState),
> +    .instance_init = gicv2m_init,
> +    .class_init    = gicv2m_class_init,
> +};
> +
> +static void gicv2m_register_types(void)
> +{
> +    type_register_static(&gicv2m_info);
> +}
> +
> +type_init(gicv2m_register_types)
> --
> 2.1.2.330.g565301e.dirty


Are we ever going to want to support the security extensions for
gicv2m? If we do can we backwards-compatibly add it later?
(I think the answer to that is probably 'yes' but I haven't
thought through the details...)

thanks
-- PMM

[-- Attachment #2: Type: text/html, Size: 11577 bytes --]

Re: [Qemu-devel] [PATCH 3/3] target-arm: Add the GICv2m to the virt board

[Peter Maydell]

[-- Attachment #1: Type: text/plain, Size: 5300 bytes --]

On 8 April 2015 at 22:21, Christoffer Dall <christoffer.dall@linaro.org>
wrote:

> Adding the GICv2m to the virt board should allow us to enable MSIs on
> the generic PCI host controller, in theory.
>

So is this commit message just saying "I haven't tested this
patchset" :-), or are we still missing some functionality to get
things working? (kernel or qemu?)


> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/arm/virt.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e01676a..0c8dae9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -45,6 +45,7 @@
>  #include "hw/pci-host/gpex.h"
>
>  #define NUM_VIRTIO_TRANSPORTS 32
> +#define NUM_GICV2M_SPIS 64
>
>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 128
> @@ -71,6 +72,7 @@ enum {
>      VIRT_RTC,
>      VIRT_FW_CFG,
>      VIRT_PCIE,
> +    VIRT_GIC_V2M,
>  };
>
>  typedef struct MemMapEntry {
> @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo {
>      int fdt_size;
>      uint32_t clock_phandle;
>      uint32_t gic_phandle;
> +    uint32_t v2m_phandle;
>  } VirtBoardInfo;
>
>  typedef struct {
> @@ -127,6 +130,7 @@ static const MemMapEntry a15memmap[] = {
>      /* GIC distributor and CPU interfaces sit inside the CPU peripheral
> space */
>      [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
> +    [VIRT_GIC_V2M] =    { 0x08020000, 0x00001000 },
>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>      [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
> @@ -148,6 +152,7 @@ static const int a15irqmap[] = {
>      [VIRT_RTC] = 2,
>      [VIRT_PCIE] = 3, /* ... to 6 */
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
> +    [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>  };
>
>  static VirtBoardInfo machines[] = {
> @@ -323,9 +328,21 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo
> *vbi)
>      }
>  }
>
> -static void fdt_add_gic_node(VirtBoardInfo *vbi)
> +static void fdt_add_v2m_gic_node(VirtBoardInfo *vbi)
>  {
>
+    vbi->v2m_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
> +    qemu_fdt_add_subnode(vbi->fdt, "/intc/v2m");
> +    qemu_fdt_setprop_string(vbi->fdt, "/intc/v2m", "compatible",
> +                            "arm,gic-v2m-frame");
> +    qemu_fdt_setprop(vbi->fdt, "/intc/v2m", "msi-controller", NULL, 0);
> +    qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc/v2m", "reg",
> +                                 2, vbi->memmap[VIRT_GIC_V2M].base,
> +                                 2, vbi->memmap[VIRT_GIC_V2M].size);
> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc/v2m", "phandle",
> vbi->v2m_phandle);
> +}
>
> +static void fdt_add_gic_node(VirtBoardInfo *vbi)
> +{
>      vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
>      qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent",
> vbi->gic_phandle);
>
> @@ -340,9 +357,31 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
>                                       2, vbi->memmap[VIRT_GIC_DIST].size,
>                                       2, vbi->memmap[VIRT_GIC_CPU].base,
>                                       2, vbi->memmap[VIRT_GIC_CPU].size);
> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
> +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
> +    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
>

Why do we need an empty ranges attribute?

     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
>  }
>
> +static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    int i;
> +    int irq = vbi->irqmap[VIRT_GIC_V2M];
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "gicv2m");
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> vbi->memmap[VIRT_GIC_V2M].base);
> +    qdev_prop_set_uint32(dev, "base-spi", irq);
> +    qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS);
> +    qdev_init_nofail(dev);
> +
> +    for (i = 0; i < NUM_GICV2M_SPIS; i++) {
> +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]);
> +    }
> +
> +    fdt_add_v2m_gic_node(vbi);
> +}
> +
>  static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
>  {
>      /* We create a standalone GIC v2 */
> @@ -391,6 +430,8 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq
> *pic)
>      }
>
>      fdt_add_gic_node(vbi);
> +
> +    create_v2m(vbi, pic);
>  }
>
>  static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
> @@ -699,6 +740,10 @@ static void create_pcie(const VirtBoardInfo *vbi,
> qemu_irq *pic)
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
>      qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
>                             nr_pcie_buses - 1);
> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
> +                           nr_pcie_buses - 1);
>

Merge conflict misresolution?

+
> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
> vbi->v2m_phandle);
>
>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>                                   2, base_ecam, 2, size_ecam);
> --
> 2.1.2.330.g565301e.dirty
>
>
>
thanks
-- PMM

[-- Attachment #2: Type: text/html, Size: 7495 bytes --]

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Christoffer Dall]

On Tue, Apr 21, 2015 at 03:40:51PM +0100, Peter Maydell wrote:
> On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org>
> wrote:
> 
> > The ARM GICv2m widget is a little device that handle MSI interrupt
> >
> 
> "handles"
> 
> 
> > writes to a trigger register and ties them to a range of interrupt lines
> > wires to the GIC.  It has a few status/id registers and the interrupt
> > wires,
> > and that's about it.
> >
> > A board instantiates the device by setting the base SPI number and
> > number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> > number space only, so base-spi == 0, means IRQ number 32.  When a device
> > (the PCI host controller) writes to the trigger register, the payload is
> > the GIC IRQ number, so we have to subtract 32 from that and then index
> > into our frame of SPIs.
> >
> > When instantiating a GICv2m device, tell PCI that we have instantiated
> > something that can deal with MSIs.  We rely on the board actually wiring
> > up the GICv2m to the PCI host controller.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  hw/intc/Makefile.objs |   1 +
> >  hw/intc/arm_gicv2m.c  | 180
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 181 insertions(+)
> >  create mode 100644 hw/intc/arm_gicv2m.c
> >
> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> > index 843864a..6b63dfc 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
> >
> >  obj-$(CONFIG_APIC) += apic.o apic_common.o
> >  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> > +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
> >
> 
> Should be common-obj-, as per other email.
> 
> 
> >  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
> >  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
> >  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> > diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> > new file mode 100644
> > index 0000000..a80a16d
> > --- /dev/null
> > +++ b/hw/intc/arm_gicv2m.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> > + *
> > + * Copyright (C) 2015 Linaro, All rights reserved.
> > + *
> > + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <
> > http://www.gnu.org/licenses/>.
> > + */
> >
> 
> A reference to the document defining the GICv2M would be nice here
> (it's the SBSA unless I'm confused).
> 
> 
> 
> > +
> > +#include "hw/sysbus.h"
> > +#include "hw/pci/msi.h"
> > +
> > +#define TYPE_GICV2M "gicv2m"
> >
> 
> I think this should be
> #define TYPE_ARM_GICV2M "arm-gicv2m"
> 
> 
> > +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
> >
> 
> and  ARM_GICV2M(obj)
> 
> 
> > +
> > +#define GICV2M_NUM_SPI_MAX 128
> > +
> > +#define V2M_MSI_TYPER           0x008
> > +#define V2M_MSI_SETSPI_NS       0x040
> > +#define V2M_MSI_IIDR            0xFCC
> > +#define V2M_IIDR0               0xFD0
> > +
> > +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> > +#define IMPLEMENTER_ARM         0x43b
> > +
> > +typedef struct GICv2mState {
> >
> 
> ARMGICv2mState
> 
> +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion iomem;
> > +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> > +
> > +    uint32_t base_spi;
> > +    uint32_t num_spi;
> > +} GICv2mState;
> > +
> > +static void gicv2m_set_irq(void *opaque, int irq)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    qemu_irq_pulse(s->spi[irq]);
> > +}
> > +
> > +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> > +                            unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +    uint32_t val;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n",
> > size);
> > +        return 0;
> > +    }
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_TYPER:
> > +        val = (s->base_spi + 32) << 16;
> > +        val |= s->num_spi;
> > +        return val;
> > +    case V2M_MSI_IIDR:
> > +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
> >
> 
> This choice of implementer field value is a little hard to justify :-)
> 
> 
> > +    default:
> > +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
> >
> 
> Why not just have a "case 0xFD0..0xFFC" ?
> Could do with a comment about what the ID regs are
> (ie that they're mostly impdef and we choose to read as zero).
> 
> +            return 0;
> > +        }
> > +
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> > +        return 0;
> > +    }
> > +}
> > +
> > +static void gicv2m_write(void *opaque, hwaddr offset,
> > +                        uint64_t value, unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n",
> > size);
> > +        return;
> > +    }
> >
> 
> "The MSI_SETSPI_S and MSI_SETSPI_NS registers must also support
> 16 bit writes to bits [15:0]", so you can't insist on 32 bit
> accesses only here.
> 
> 
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_SETSPI_NS: {
> > +        int spi;
> > +
> > +        spi = (value & 0x3ff) - (s->base_spi + 32);
> > +        if (spi < s->num_spi) {
> > +            gicv2m_set_irq(s, spi);
> > +        }
> > +        return;
> > +    }
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "gicv2m_write: Bad offset %x\n", (int)offset);
> > +        return;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps gicv2m_ops = {
> > +    .read = gicv2m_read,
> > +    .write = gicv2m_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > +static void gicv2m_realize(DeviceState *dev, Error **errp)
> > +{
> > +    GICv2mState *s = GICV2M(dev);
> > +    int i;
> > +
> > +    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
> > +        error_setg(errp,
> > +                   "requested %u SPIs exceeds GICv2m frame maximum %d",
> > +                   s->num_spi, GICV2M_NUM_SPI_MAX);
> > +        return;
> > +    }
> > +
> > +    if (s->base_spi + 32 > 1020 - s->num_spi) {
> > +        error_setg(errp,
> > +                   "requested base SPI %u+%u exceeds max. number 1020",
> > +                   s->base_spi + 32, s->num_spi);
> > +        return;
> > +    }
> > +
> > +    for (i = 0; i < s->num_spi; i++) {
> > +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> > +    }
> > +
> > +    msi_supported = true;
> > +}
> > +
> > +static void gicv2m_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    GICv2mState *s = GICV2M(obj);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
> > +                          "gicv2m", 0x1000);
> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +}
> > +
> > +static Property gicv2m_properties[] = {
> > +    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
> > +    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void gicv2m_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->props = gicv2m_properties;
> > +    dc->realize = gicv2m_realize;
> > +}
> > +
> > +static const TypeInfo gicv2m_info = {
> > +    .name          = TYPE_GICV2M,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(GICv2mState),
> > +    .instance_init = gicv2m_init,
> > +    .class_init    = gicv2m_class_init,
> > +};
> > +
> > +static void gicv2m_register_types(void)
> > +{
> > +    type_register_static(&gicv2m_info);
> > +}
> > +
> > +type_init(gicv2m_register_types)
> > --
> > 2.1.2.330.g565301e.dirty
> 
> 
> Are we ever going to want to support the security extensions for
> gicv2m? If we do can we backwards-compatibly add it later?
> (I think the answer to that is probably 'yes' but I haven't
> thought through the details...)
> 

Thanks for the review!

I've tried to address all your comments.

Regarding adding support for the security extensions later, I assume the
QEMU-specifics will be to add a flag to the device instantiation from
the containing board activating security support, which would grow the
IO region size of this device from 4K to 8K for an adjacent secure
register frame and adjust the offsets.  I cannot think of a reason why
that wouldn't work backwards-compatibly?

I've added a comment declaring the scope of the emulation (single
non-secure MSI register frame as Eric also suggested for the next
iteration).

-Christoffer

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Peter Maydell]

On 27 April 2015 at 14:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Regarding adding support for the security extensions later, I assume the
> QEMU-specifics will be to add a flag to the device instantiation from
> the containing board activating security support, which would grow the
> IO region size of this device from 4K to 8K for an adjacent secure
> register frame and adjust the offsets.  I cannot think of a reason why
> that wouldn't work backwards-compatibly?

I think you'd probably want to add a second MMIO region rather
than making the first one double-size. I don't think there's anything
in the spec that mandates them being adjacent. You also need to allocate
more interrupt lines.

I think this should all be backwards-compatible, yes.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs

[Christoffer Dall]

On Mon, Apr 27, 2015 at 02:43:17PM +0100, Peter Maydell wrote:
> On 27 April 2015 at 14:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Regarding adding support for the security extensions later, I assume the
> > QEMU-specifics will be to add a flag to the device instantiation from
> > the containing board activating security support, which would grow the
> > IO region size of this device from 4K to 8K for an adjacent secure
> > register frame and adjust the offsets.  I cannot think of a reason why
> > that wouldn't work backwards-compatibly?
> 
> I think you'd probably want to add a second MMIO region rather
> than making the first one double-size. I don't think there's anything
> in the spec that mandates them being adjacent. You also need to allocate
> more interrupt lines.

Right; the spec actually says they are NOT required to be adjacent.

> 
> I think this should all be backwards-compatible, yes.
> 

ok - thanks,
-Christoffer

Re: [Qemu-devel] [PATCH 3/3] target-arm: Add the GICv2m to the virt board

[Christoffer Dall]

On Tue, Apr 21, 2015 at 03:47:13PM +0100, Peter Maydell wrote:
> On 8 April 2015 at 22:21, Christoffer Dall <christoffer.dall@linaro.org>
> wrote:
> 
> > Adding the GICv2m to the virt board should allow us to enable MSIs on
> > the generic PCI host controller, in theory.
> >
> 
> So is this commit message just saying "I haven't tested this
> patchset" :-), or are we still missing some functionality to get
> things working? (kernel or qemu?)

I basically forgot to revise the commit text after the initial prototype
work.  We still need Eric's patches for this to work with IRQFD and
VHOST, but MSIs actually do work with this code on their own.  And I did
test the patch set.  I will revise the commit text.

-Christoffer

Re: [Qemu-devel] [PATCH 3/3] target-arm: Add the GICv2m to the virt board

[Christoffer Dall]

On Tue, Apr 21, 2015 at 03:47:13PM +0100, Peter Maydell wrote:

[...]

> > @@ -340,9 +357,31 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
> >                                       2, vbi->memmap[VIRT_GIC_DIST].size,
> >                                       2, vbi->memmap[VIRT_GIC_CPU].base,
> >                                       2, vbi->memmap[VIRT_GIC_CPU].size);
> > +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
> > +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
> > +    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
> >
> 
> Why do we need an empty ranges attribute?
> 

Without it, Linux fails to make things work.  I suspect this is related
to specifically setting the #address-cells and #size-cells.

I forgot by now, but I think when I originally wrote this patch I traced
through the Linux code and found out that it was required somewhere.

I also think the AMD GICv2m FDT has the empty ranges property.

-Christoffer

Re: [Qemu-devel] [PATCH 3/3] target-arm: Add the GICv2m to the virt board

[Peter Maydell]

On 27 April 2015 at 17:06, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Apr 21, 2015 at 03:47:13PM +0100, Peter Maydell wrote:
>
> [...]
>
>> > @@ -340,9 +357,31 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
>> >                                       2, vbi->memmap[VIRT_GIC_DIST].size,
>> >                                       2, vbi->memmap[VIRT_GIC_CPU].base,
>> >                                       2, vbi->memmap[VIRT_GIC_CPU].size);
>> > +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
>> > +    qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
>> > +    qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
>> >
>>
>> Why do we need an empty ranges attribute?
>>
>
> Without it, Linux fails to make things work.  I suspect this is related
> to specifically setting the #address-cells and #size-cells.

Looking at the DT spec I think it's a requirement for any node
which has children, to specify the address translation.
Empty ranges means "1:1 translation" (and more complicated
transformations are possible).

-- PMM