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

[Qemu-devel] [PATCH v3 0/4] 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 series that enables IRQFD and vhost
is available: "ARM adaptations for vhost irqfd setup"
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01054.html)

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.

See the individual patches for changelogs.

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

Shanker Donthineni (1):
  target-arm: Extend the gic node properties

 hw/arm/virt.c         |  81 +++++++++++++++------
 hw/intc/Makefile.objs |   1 +
 hw/intc/arm_gicv2m.c  | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 252 insertions(+), 20 deletions(-)
 create mode 100644 hw/intc/arm_gicv2m.c

-- 
2.1.2.330.g565301e.dirty

[Qemu-devel] [PATCH v3 1/4] 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.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes since v2:
 - None
Changes since v1:
 - Added reviewed-by tag

 hw/arm/virt.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a7f9a10..f9f7482 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;
@@ -394,7 +392,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)
@@ -641,8 +639,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;
@@ -714,7 +711,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);
 }
@@ -736,7 +733,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) {
@@ -813,13 +809,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
-- 
2.1.2.330.g565301e.dirty

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

[Eric Auger]

Reviewed-by: Eric Auger <eric.auger@linaro.org>

On 05/24/2015 12:51 PM, Christoffer Dall 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.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes since v2:
>  - None
> Changes since v1:
>  - Added reviewed-by tag
> 
>  hw/arm/virt.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a7f9a10..f9f7482 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;
> @@ -394,7 +392,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)
> @@ -641,8 +639,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;
> @@ -714,7 +711,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);
>  }
> @@ -736,7 +733,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) {
> @@ -813,13 +809,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
> 

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

[Christoffer Dall]

The ARM GICv2m widget is a little device that handles 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>
---
Changes since v2:
 - Renamed QOM type to "arm-gicv2m"
Changes since v1:
 - Check that writes to MSI_SETSPI are within the lower boundary as well
 - Move gicv2m to common-obj in Makefile
 - Separate switch case and comment for impdef regs
 - Clearly document what is emulated
 - Allow 16 bit lower accesses to MSI_SETSPI regs
 - Fix commit grammar error
 - Remove stray pixman commit

 hw/intc/Makefile.objs |   1 +
 hw/intc/arm_gicv2m.c  | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 191 insertions(+)
 create mode 100644 hw/intc/arm_gicv2m.c

diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 843864a..092d8a8 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -11,6 +11,7 @@ common-obj-$(CONFIG_SLAVIO) += slavio_intctl.o
 common-obj-$(CONFIG_IOAPIC) += ioapic_common.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gic_common.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
+common-obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
 common-obj-$(CONFIG_OPENPIC) += openpic.o
 
 obj-$(CONFIG_APIC) += apic.o apic_common.o
diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
new file mode 100644
index 0000000..9f84f72
--- /dev/null
+++ b/hw/intc/arm_gicv2m.c
@@ -0,0 +1,190 @@
+/*
+ *  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/>.
+ */
+
+/* This file implements an emulated GICv2m widget as described in the ARM
+ * Server Base System Architecture (SBSA) specification Version 2.2
+ * (ARM-DEN-0029 v2.2) pages 35-39 without any optional implementation defined
+ * identification registers and with a single non-secure MSI register frame.
+ */
+
+#include "hw/sysbus.h"
+#include "hw/pci/msi.h"
+
+#define TYPE_ARM_GICV2M "arm-gicv2m"
+#define ARM_GICV2M(obj) OBJECT_CHECK(ARMGICv2mState, (obj), TYPE_ARM_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 V2M_IIDR11              0xFFC
+
+#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
+
+typedef struct ARMGICv2mState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq spi[GICV2M_NUM_SPI_MAX];
+
+    uint32_t base_spi;
+    uint32_t num_spi;
+} ARMGICv2mState;
+
+static void gicv2m_set_irq(void *opaque, int irq)
+{
+    ARMGICv2mState *s = (ARMGICv2mState *)opaque;
+
+    qemu_irq_pulse(s->spi[irq]);
+}
+
+static uint64_t gicv2m_read(void *opaque, hwaddr offset,
+                            unsigned size)
+{
+    ARMGICv2mState *s = (ARMGICv2mState *)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:
+        /* We don't have any valid implementor so we leave that field as zero
+         * and we return 0 in the arch revision as per the spec.
+         */
+        return (PRODUCT_ID_QEMU << 20);
+    case V2M_IIDR0 ... V2M_IIDR11:
+        /* We do not implement any optional identification registers and the
+         * mandatory MSI_PIDR2 register reads as 0x0, so we capture all
+         * implementation defined registers here.
+         */
+        return 0;
+    default:
+        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)
+{
+    ARMGICv2mState *s = (ARMGICv2mState *)opaque;
+
+    if (size != 2 && 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 >= 0 && 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);
+    }
+}
+
+static const MemoryRegionOps gicv2m_ops = {
+    .read = gicv2m_read,
+    .write = gicv2m_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void gicv2m_realize(DeviceState *dev, Error **errp)
+{
+    ARMGICv2mState *s = ARM_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);
+    ARMGICv2mState *s = ARM_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", ARMGICv2mState, base_spi, 0),
+    DEFINE_PROP_UINT32("num-spi", ARMGICv2mState, 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_ARM_GICV2M,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMGICv2mState),
+    .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

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

[Eric Auger]

Reviewed-by: Eric Auger <eric.auger@linaro.org>

On 05/24/2015 12:51 PM, Christoffer Dall wrote:
> The ARM GICv2m widget is a little device that handles 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>
> ---
> Changes since v2:
>  - Renamed QOM type to "arm-gicv2m"
> Changes since v1:
>  - Check that writes to MSI_SETSPI are within the lower boundary as well
>  - Move gicv2m to common-obj in Makefile
>  - Separate switch case and comment for impdef regs
>  - Clearly document what is emulated
>  - Allow 16 bit lower accesses to MSI_SETSPI regs
>  - Fix commit grammar error
>  - Remove stray pixman commit
> 
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 191 insertions(+)
>  create mode 100644 hw/intc/arm_gicv2m.c
> 
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 843864a..092d8a8 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -11,6 +11,7 @@ common-obj-$(CONFIG_SLAVIO) += slavio_intctl.o
>  common-obj-$(CONFIG_IOAPIC) += ioapic_common.o
>  common-obj-$(CONFIG_ARM_GIC) += arm_gic_common.o
>  common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
> +common-obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>  common-obj-$(CONFIG_OPENPIC) += openpic.o
>  
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> new file mode 100644
> index 0000000..9f84f72
> --- /dev/null
> +++ b/hw/intc/arm_gicv2m.c
> @@ -0,0 +1,190 @@
> +/*
> + *  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/>.
> + */
> +
> +/* This file implements an emulated GICv2m widget as described in the ARM
> + * Server Base System Architecture (SBSA) specification Version 2.2
> + * (ARM-DEN-0029 v2.2) pages 35-39 without any optional implementation defined
> + * identification registers and with a single non-secure MSI register frame.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/pci/msi.h"
> +
> +#define TYPE_ARM_GICV2M "arm-gicv2m"
> +#define ARM_GICV2M(obj) OBJECT_CHECK(ARMGICv2mState, (obj), TYPE_ARM_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 V2M_IIDR11              0xFFC
> +
> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> +
> +typedef struct ARMGICv2mState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> +
> +    uint32_t base_spi;
> +    uint32_t num_spi;
> +} ARMGICv2mState;
> +
> +static void gicv2m_set_irq(void *opaque, int irq)
> +{
> +    ARMGICv2mState *s = (ARMGICv2mState *)opaque;
> +
> +    qemu_irq_pulse(s->spi[irq]);
> +}
> +
> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> +                            unsigned size)
> +{
> +    ARMGICv2mState *s = (ARMGICv2mState *)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:
> +        /* We don't have any valid implementor so we leave that field as zero
> +         * and we return 0 in the arch revision as per the spec.
> +         */
> +        return (PRODUCT_ID_QEMU << 20);
> +    case V2M_IIDR0 ... V2M_IIDR11:
> +        /* We do not implement any optional identification registers and the
> +         * mandatory MSI_PIDR2 register reads as 0x0, so we capture all
> +         * implementation defined registers here.
> +         */
> +        return 0;
> +    default:
> +        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)
> +{
> +    ARMGICv2mState *s = (ARMGICv2mState *)opaque;
> +
> +    if (size != 2 && 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 >= 0 && 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);
> +    }
> +}
> +
> +static const MemoryRegionOps gicv2m_ops = {
> +    .read = gicv2m_read,
> +    .write = gicv2m_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void gicv2m_realize(DeviceState *dev, Error **errp)
> +{
> +    ARMGICv2mState *s = ARM_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);
> +    ARMGICv2mState *s = ARM_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", ARMGICv2mState, base_spi, 0),
> +    DEFINE_PROP_UINT32("num-spi", ARMGICv2mState, 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_ARM_GICV2M,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(ARMGICv2mState),
> +    .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)
> 

[Qemu-devel] [PATCH v3 3/4] target-arm: Extend the gic node properties

[Christoffer Dall]

From: Shanker Donthineni <shankerd@codeaurora.org>

In preparation for adding the GICv2m which requires address specifiers
and is a subnode of the gic, we extend the gic DT definition to specify
the #address-cells and #size-cells properties and add an empty ranges
property properties of the DT node, since this is required to add the
v2m node as a child of the gic node.

Note that we must also expand the irq-map to reference the gic with the
right address-cells as a consequnce of this change.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes since v2:
 - New separate patch factoring out changes to existing code for eased
   bisectability in case we broke something
 - The above fixes the issue with non-MSI compatible guests.

 hw/arm/virt.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f9f7482..6797c6f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -340,7 +340,11 @@ 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_gic(VirtBoardInfo *vbi, qemu_irq *pic)
@@ -604,11 +608,12 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
     g_free(nodename);
 }
 
+#define PCIE_IRQMAP_LEN 10
 static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
                                 int first_irq, const char *nodename)
 {
     int devfn, pin;
-    uint32_t full_irq_map[4 * 4 * 8] = { 0 };
+    uint32_t full_irq_map[4 * 4 * PCIE_IRQMAP_LEN] = { 0 };
     uint32_t *irq_map = full_irq_map;
 
     for (devfn = 0; devfn <= 0x18; devfn += 0x8) {
@@ -619,15 +624,15 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
             int i;
 
             uint32_t map[] = {
-                devfn << 8, 0, 0,                           /* devfn */
-                pin + 1,                                    /* PCI pin */
-                gic_phandle, irq_type, irq_nr, irq_level }; /* GIC irq */
+                devfn << 8, 0, 0,                                 /* devfn */
+                pin + 1,                                          /* PCI pin */
+                gic_phandle, 0, 0, irq_type, irq_nr, irq_level }; /* GIC irq */
 
             /* Convert map to big endian */
-            for (i = 0; i < 8; i++) {
+            for (i = 0; i < PCIE_IRQMAP_LEN; i++) {
                 irq_map[i] = cpu_to_be32(map[i]);
             }
-            irq_map += 8;
+            irq_map += PCIE_IRQMAP_LEN;
         }
     }
 
-- 
2.1.2.330.g565301e.dirty

Re: [Qemu-devel] [PATCH v3 3/4] target-arm: Extend the gic node properties

[Eric Auger]

Reviewed-by: Eric Auger <eric.auger@linaro.org>

only minor things below

On 05/24/2015 12:51 PM, Christoffer Dall wrote:
> From: Shanker Donthineni <shankerd@codeaurora.org>
> 
> In preparation for adding the GICv2m which requires address specifiers
> and is a subnode of the gic, we extend the gic DT definition to specify
> the #address-cells and #size-cells properties and add an empty ranges
> property properties of the DT node, since this is required to add the
> v2m node as a child of the gic node.
> 
> Note that we must also expand the irq-map to reference the gic with the
> right address-cells as a consequnce of this change.
consequence
> 
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes since v2:
>  - New separate patch factoring out changes to existing code for eased
>    bisectability in case we broke something
>  - The above fixes the issue with non-MSI compatible guests.
> 
>  hw/arm/virt.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f9f7482..6797c6f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -340,7 +340,11 @@ 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);
> +
spare empty line
>  }
>  
>  static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
> @@ -604,11 +608,12 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>  
> +#define PCIE_IRQMAP_LEN 10
>  static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
>                                  int first_irq, const char *nodename)
>  {
>      int devfn, pin;
> -    uint32_t full_irq_map[4 * 4 * 8] = { 0 };
> +    uint32_t full_irq_map[4 * 4 * PCIE_IRQMAP_LEN] = { 0 };
>      uint32_t *irq_map = full_irq_map;
>  
>      for (devfn = 0; devfn <= 0x18; devfn += 0x8) {
> @@ -619,15 +624,15 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
>              int i;
>  
>              uint32_t map[] = {
> -                devfn << 8, 0, 0,                           /* devfn */
> -                pin + 1,                                    /* PCI pin */
> -                gic_phandle, irq_type, irq_nr, irq_level }; /* GIC irq */
> +                devfn << 8, 0, 0,                                 /* devfn */
> +                pin + 1,                                          /* PCI pin */
> +                gic_phandle, 0, 0, irq_type, irq_nr, irq_level }; /* GIC irq */
>  
>              /* Convert map to big endian */
> -            for (i = 0; i < 8; i++) {
> +            for (i = 0; i < PCIE_IRQMAP_LEN; i++) {
>                  irq_map[i] = cpu_to_be32(map[i]);
>              }
> -            irq_map += 8;
> +            irq_map += PCIE_IRQMAP_LEN;
>          }
>      }
>  
> 

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

[Christoffer Dall]

Add a GICv2m device to the virt board to enable MSIs on the generic PCI
host controller.  We allocate 64 SPIs in the IRQ space for now (this can
be increased/decreased later) and map the GICv2m right after the GIC in
the memory map.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes since v2:
 - Factored out changes to GIC DT node to previous patch.
 - Renamed QOM type name to "arm-gicv2m"
Changes since v1:
 - Remove stray merge conflict line
 - Reworded commmit message.

 hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6797c6f..2972bb3 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);
 
@@ -347,6 +364,25 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
 
 }
 
+static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    int i;
+    int irq = vbi->irqmap[VIRT_GIC_V2M];
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "arm-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 */
@@ -397,6 +433,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)
@@ -707,6 +745,8 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
     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);
     qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
-- 
2.1.2.330.g565301e.dirty

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

[Pavel Fedin]

 Hello!

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

 Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be
used by GICv3 ITS implementation.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

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

[Peter Maydell]

On 25 May 2015 at 14:09, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>>  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;
>
>  Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be
> used by GICv3 ITS implementation.

Why? The v2m device isn't really related to the GICv3...

-- PMM

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

[Eric Auger]

On 05/25/2015 05:01 PM, Peter Maydell wrote:
> On 25 May 2015 at 14:09, Pavel Fedin <p.fedin@samsung.com> wrote:
>>  Hello!
>>
>>>  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;
>>
>>  Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be
>> used by GICv3 ITS implementation.
> 
> Why? The v2m device isn't really related to the GICv3...
In the future this handle could point to GICv2m or GICv3 ITS or GICv3
(if I understand it correctly, when it supports message base interrupts
and no ITS). They all would play the same role of msi-controller.

Best Regards

Eric
> 
> -- PMM
> 

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

[Pavel Fedin]

 Hi!

> > Why? The v2m device isn't really related to the GICv3...
> In the future this handle could point to GICv2m or GICv3 ITS or GICv3
> (if I understand it correctly, when it supports message base interrupts
> and no ITS). They all would play the same role of msi-controller.

 Yes, exactly. In my implementation being developed i actually reused first patch from this set, and indeed 'v2m_handle' appeared to be useful for ITS. My code flow is something like:
 if (gicv3)
  create_its()
else
  create_v2m()

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

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

[Christoffer Dall]

Hi Pavel,

On Mon, May 25, 2015 at 04:09:58PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> >  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;
> 
>  Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be
> used by GICv3 ITS implementation.
> 
That's sort of how to speculate about.  Why can't those patches just
rename the variable then?  Right now, as the code stands, msi_phandle
would be less clear IMHO.

-Christoffer

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

[Eric Auger]

Reviewed-by: Eric Auger <eric.auger@linaro.org>

The only question I have is related to mid-term virt strategy about
GICv3 integration. Are we going to reuse that memory map for the machine
instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere
else to leave space for GICv3 redistributors, assuming we reuse the
shared distributor region. I understood the memory map is difficult to
change once applied once.

Best Regards

Eric

On 05/24/2015 12:51 PM, Christoffer Dall wrote:
> Add a GICv2m device to the virt board to enable MSIs on the generic PCI
> host controller.  We allocate 64 SPIs in the IRQ space for now (this can
> be increased/decreased later) and map the GICv2m right after the GIC in
> the memory map.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes since v2:
>  - Factored out changes to GIC DT node to previous patch.
>  - Renamed QOM type name to "arm-gicv2m"
> Changes since v1:
>  - Remove stray merge conflict line
>  - Reworded commmit message.
> 
>  hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 6797c6f..2972bb3 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);
>  
> @@ -347,6 +364,25 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi)
>  
>  }
>  
> +static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    int i;
> +    int irq = vbi->irqmap[VIRT_GIC_V2M];
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "arm-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 */
> @@ -397,6 +433,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)
> @@ -707,6 +745,8 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
>      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);
>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> 

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

[Peter Maydell]

On 26 May 2015 at 13:54, Eric Auger <eric.auger@linaro.org> wrote:
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
>
> The only question I have is related to mid-term virt strategy about
> GICv3 integration. Are we going to reuse that memory map for the machine
> instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere
> else to leave space for GICv3 redistributors, assuming we reuse the
> shared distributor region. I understood the memory map is difficult to
> change once applied once.

I wouldn't expect that you'd have a GICv2M at all in a
system with a GICv3 in it, would you?

-- PMM

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

[Eric Auger]

On 05/26/2015 02:55 PM, Peter Maydell wrote:
> On 26 May 2015 at 13:54, Eric Auger <eric.auger@linaro.org> wrote:
>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
>>
>> The only question I have is related to mid-term virt strategy about
>> GICv3 integration. Are we going to reuse that memory map for the machine
>> instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere
>> else to leave space for GICv3 redistributors, assuming we reuse the
>> shared distributor region. I understood the memory map is difficult to
>> change once applied once.
> 
> I wouldn't expect that you'd have a GICv2M at all in a
> system with a GICv3 in it, would you?

no indeed. but we currently use a single static a15memmap memory map in
virt. This one is currently planned to be reused for machines
instantiating GICv3 so we start seeing things like
VIRT_GIC_CPU =  VIRT_GIC_REDIST. I fear this is going to become messy.

What is your guidance, should we introduce new memory maps for GICv3
enabled machine or should we move to a single dynamic memory map?

Best Regards

Eric

> 
> -- PMM
> 

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

[Peter Maydell]

On 26 May 2015 at 14:07, Eric Auger <eric.auger@linaro.org> wrote:
> On 05/26/2015 02:55 PM, Peter Maydell wrote:
>> On 26 May 2015 at 13:54, Eric Auger <eric.auger@linaro.org> wrote:
>>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> The only question I have is related to mid-term virt strategy about
>>> GICv3 integration. Are we going to reuse that memory map for the machine
>>> instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere
>>> else to leave space for GICv3 redistributors, assuming we reuse the
>>> shared distributor region. I understood the memory map is difficult to
>>> change once applied once.
>>
>> I wouldn't expect that you'd have a GICv2M at all in a
>> system with a GICv3 in it, would you?
>
> no indeed. but we currently use a single static a15memmap memory map in
> virt. This one is currently planned to be reused for machines
> instantiating GICv3 so we start seeing things like
> VIRT_GIC_CPU =  VIRT_GIC_REDIST. I fear this is going to become messy.

I think the answer is not to reuse constant names like that.
The v3 redistributor is not a v2 CPU interface, just as a
UART is not an RTC.

-- PMM

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

[Pavel Fedin]

 Hi! My word...

> What is your guidance, should we introduce new memory maps for GICv3
> enabled machine or should we move to a single dynamic memory map?

 IMHO there's no reason to introduce another memory map. I have already done test integration some time ago, and here is what i got:
--- cut ---
enum {
    VIRT_FLASH,
    VIRT_MEM,
    VIRT_CPUPERIPHS,
    VIRT_GIC_DIST,
    VIRT_GIC_CPU,
    VIRT_GIC_DIST_SPI = VIRT_GIC_CPU,
    VIRT_ITS_CONTROL,
    VIRT_ITS_TRANSLATION,
    VIRT_LPI,
    VIRT_UART,
    VIRT_MMIO,
    VIRT_RTC,
    VIRT_FW_CFG,
    VIRT_PCIE,
    VIRT_GIC_V2M = VIRT_ITS_CONTROL,
};
--- cut ---
static const MemMapEntry a15memmap[] = {
    /* Space up to 0x8000000 is reserved for a boot ROM */
    [VIRT_FLASH] =           {          0, 0x08000000 },
    [VIRT_CPUPERIPHS] =      { 0x08000000, 0x00020000 },
    /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
    [VIRT_GIC_DIST] =        { 0x08000000, 0x00010000 },
    [VIRT_GIC_CPU] =         { 0x08010000, 0x00010000 }, /* VIRT_GIC_DIST_SPI for v3 */
    [VIRT_ITS_CONTROL] =     { 0x08020000, 0x0010000 }, /* VIRT_GIC_V2M for v2 */
    [VIRT_ITS_TRANSLATION] = { 0x08030000, 0x00010000 },
    [VIRT_LPI] =             { 0x08040000, 0x00800000 },
    [VIRT_UART] =            { 0x09000000, 0x00001000 },
    [VIRT_RTC] =             { 0x09010000, 0x00001000 },
    [VIRT_FW_CFG] =          { 0x09020000, 0x0000000a },
    [VIRT_MMIO] =            { 0x0a000000, 0x00000200 },
    /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
    /*
     * PCIE verbose map:
     *
     * MMIO window      { 0x10000000, 0x2eff0000 },
     * PIO window       { 0x3eff0000, 0x00010000 },
     * ECAM             { 0x3f000000, 0x01000000 },
     */
    [VIRT_PCIE] =       { 0x10000000, 0x30000000 },
    [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
};
--- cut ---
 As you can see, it's perfectly readable and memory maps are identical. I even see no problem with merging ITS_CONTROL and ITS_TRANSPATION, because i don't see any harm in increasing v2m area.
 P.S. And yes, VIRT_GIC_DIST_SPI should be VIRT_GIC_DIST_MBI instead (Reviewed-by: Eric Auger <eric.auger@linaro.org>), just this fragment is from my old integration branch, which i currently don't work on, because my new test environment doesn't use GICv2 at all.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia