[Qemu-devel] [PATCH 0/3] hw/arm/gic: Code duplication removal

[Qemu-devel] [PATCH 0/3] hw/arm/gic: Code duplication removal

[Pavel Fedin]

I decided to make this small patchset in order to try to push some of my
changes before 2.4 is out. Idea of first patch of this set came during
vGICv3 implementation. In order to be able to upstream it earlier i decided
to make it doing something useful and refactored GICv2 code.

This patchset contains no functional enhancements. Only cleanup and refactor.

Pavel Fedin (3):
  Merge memory_region_init_reservation() into memory_region_init_io()
  hw/arm/gic: Kill code duplication
  Introduce gic_class_name() instead of repeating condition

 hw/arm/virt.c                    |  7 ++---
 hw/cpu/a15mpcore.c               |  8 ++----
 hw/intc/arm_gic.c                | 61 +++++++++++-----------------------------
 hw/intc/arm_gic_common.c         | 37 ++++++++++++++++++++++++
 hw/intc/arm_gic_kvm.c            | 28 +-----------------
 include/exec/memory.h            | 14 +++++++--
 include/hw/intc/arm_gic_common.h |  3 ++
 memory.c                         | 10 +------
 target-arm/kvm_arm.h             |  5 ++++
 9 files changed, 79 insertions(+), 94 deletions(-)

-- 
1.9.5.msysgit.0

[Qemu-devel] [PATCH 1/3] Merge memory_region_init_reservation() into memory_region_init_io()

[Pavel Fedin]

Just speficying ops = NULL in some cases can be more convenient than having
two functions.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 14 +++++++++++---
 memory.c              | 10 +---------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 94d20ea..b18b351 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -437,6 +437,9 @@ void memory_region_init_alias(MemoryRegion *mr,
  * memory_region_init_rom_device:  Initialize a ROM memory region.  Writes are
  *                                 handled via callbacks.
  *
+ * If NULL callbacks pointer is given, then I/O space is not supposed to be
+ * handled by QEMU itself. Any access via the memory API will cause an abort().
+ *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
  * @ops: callbacks for write access handling.
@@ -459,16 +462,21 @@ void memory_region_init_rom_device(MemoryRegion *mr,
  * A reservation region primariy serves debugging purposes.  It claims I/O
  * space that is not supposed to be handled by QEMU itself.  Any access via
  * the memory API will cause an abort().
+ * This function is deprecated. Use memory_region_init_io() with NULL
+ * callbacks instead.
  *
  * @mr: the #MemoryRegion to be initialized
  * @owner: the object that tracks the region's reference count
  * @name: used for debugging; not visible to the user or ABI
  * @size: size of the region.
  */
-void memory_region_init_reservation(MemoryRegion *mr,
-                                    struct Object *owner,
+static inline void memory_region_init_reservation(MemoryRegion *mr,
+                                    Object *owner,
                                     const char *name,
-                                    uint64_t size);
+                                    uint64_t size)
+{
+    memory_region_init_io(mr, owner, NULL, mr, name, size);
+}
 
 /**
  * memory_region_init_iommu: Initialize a memory region that translates
diff --git a/memory.c b/memory.c
index 4eb138a..0d8b2d9 100644
--- a/memory.c
+++ b/memory.c
@@ -1182,7 +1182,7 @@ void memory_region_init_io(MemoryRegion *mr,
                            uint64_t size)
 {
     memory_region_init(mr, owner, name, size);
-    mr->ops = ops;
+    mr->ops = ops ? ops : &unassigned_mem_ops;
     mr->opaque = opaque;
     mr->terminates = true;
 }
@@ -1300,14 +1300,6 @@ void memory_region_init_iommu(MemoryRegion *mr,
     notifier_list_init(&mr->iommu_notify);
 }
 
-void memory_region_init_reservation(MemoryRegion *mr,
-                                    Object *owner,
-                                    const char *name,
-                                    uint64_t size)
-{
-    memory_region_init_io(mr, owner, &unassigned_mem_ops, mr, name, size);
-}
-
 static void memory_region_finalize(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH 1/3] Merge memory_region_init_reservation() into memory_region_init_io()

[Peter Maydell]

On 29 July 2015 at 12:54, Pavel Fedin <p.fedin@samsung.com> wrote:
> Just speficying ops = NULL in some cases can be more convenient than having
> two functions.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH 2/3] hw/arm/gic: Kill code duplication

[Pavel Fedin]

Extracted duplicated initialization code from SW-emulated and KVM GIC
implementations and put into gic_init_irqs_and_mmio()

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/intc/arm_gic.c                | 61 +++++++++++-----------------------------
 hw/intc/arm_gic_common.c         | 37 ++++++++++++++++++++++++
 hw/intc/arm_gic_kvm.c            | 28 +-----------------
 include/hw/intc/arm_gic_common.h |  3 ++
 4 files changed, 57 insertions(+), 72 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 454bfd7..9dbbc80 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -922,12 +922,6 @@ static MemTxResult gic_dist_write(void *opaque, hwaddr offset, uint64_t data,
     }
 }
 
-static const MemoryRegionOps gic_dist_ops = {
-    .read_with_attrs = gic_dist_read,
-    .write_with_attrs = gic_dist_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
 static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
                                 uint64_t *data, MemTxAttrs attrs)
 {
@@ -1056,10 +1050,17 @@ static MemTxResult gic_do_cpu_write(void *opaque, hwaddr addr,
     return gic_cpu_write(s, id, addr, value, attrs);
 }
 
-static const MemoryRegionOps gic_thiscpu_ops = {
-    .read_with_attrs = gic_thiscpu_read,
-    .write_with_attrs = gic_thiscpu_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+static const MemoryRegionOps gic_ops[2] = {
+    {
+        .read_with_attrs = gic_dist_read,
+        .write_with_attrs = gic_dist_write,
+        .endianness = DEVICE_NATIVE_ENDIAN,
+    },
+    {
+        .read_with_attrs = gic_thiscpu_read,
+        .write_with_attrs = gic_thiscpu_write,
+        .endianness = DEVICE_NATIVE_ENDIAN,
+    }
 };
 
 static const MemoryRegionOps gic_cpu_ops = {
@@ -1068,31 +1069,10 @@ static const MemoryRegionOps gic_cpu_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+/* This function is used by nvic model */
 void gic_init_irqs_and_distributor(GICState *s)
 {
-    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
-    int i;
-
-    i = s->num_irq - GIC_INTERNAL;
-    /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
-     * GPIO array layout is thus:
-     *  [0..N-1] SPIs
-     *  [N..N+31] PPIs for CPU 0
-     *  [N+32..N+63] PPIs for CPU 1
-     *   ...
-     */
-    if (s->revision != REV_NVIC) {
-        i += (GIC_INTERNAL * s->num_cpu);
-    }
-    qdev_init_gpio_in(DEVICE(s), gic_set_irq, i);
-    for (i = 0; i < NUM_CPU(s); i++) {
-        sysbus_init_irq(sbd, &s->parent_irq[i]);
-    }
-    for (i = 0; i < NUM_CPU(s); i++) {
-        sysbus_init_irq(sbd, &s->parent_fiq[i]);
-    }
-    memory_region_init_io(&s->iomem, OBJECT(s), &gic_dist_ops, s,
-                          "gic_dist", 0x1000);
+    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops);
 }
 
 static void arm_gic_realize(DeviceState *dev, Error **errp)
@@ -1110,28 +1090,19 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    gic_init_irqs_and_distributor(s);
+    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops);
 
-    /* Memory regions for the CPU interfaces (NVIC doesn't have these):
-     * a region for "CPU interface for this core", then a region for
-     * "CPU interface for core 0", "for core 1", ...
+    /* Extra core-specific regions for the CPU interfaces
      * NB that the memory region size of 0x100 applies for the 11MPCore
      * and also cores following the GIC v1 spec (ie A9).
      * GIC v2 defines a larger memory region (0x1000) so this will need
      * to be extended when we implement A15.
      */
-    memory_region_init_io(&s->cpuiomem[0], OBJECT(s), &gic_thiscpu_ops, s,
-                          "gic_cpu", 0x100);
     for (i = 0; i < NUM_CPU(s); i++) {
         s->backref[i] = s;
         memory_region_init_io(&s->cpuiomem[i+1], OBJECT(s), &gic_cpu_ops,
                               &s->backref[i], "gic_cpu", 0x100);
-    }
-    /* Distributor */
-    sysbus_init_mmio(sbd, &s->iomem);
-    /* cpu interfaces (one for "current cpu" plus one per cpu) */
-    for (i = 0; i <= NUM_CPU(s); i++) {
-        sysbus_init_mmio(sbd, &s->cpuiomem[i]);
+        sysbus_init_mmio(sbd, &s->cpuiomem[i+1]);
     }
 }
 
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index a64d071..661dad0 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -84,6 +84,43 @@ static const VMStateDescription vmstate_gic = {
     }
 };
 
+void gic_init_irqs_and_mmio(GICState *s, qemu_irq_handler handler,
+                            const MemoryRegionOps *ops)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
+    int i = s->num_irq - GIC_INTERNAL;
+
+    /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
+     * GPIO array layout is thus:
+     *  [0..N-1] SPIs
+     *  [N..N+31] PPIs for CPU 0
+     *  [N+32..N+63] PPIs for CPU 1
+     *   ...
+     */
+    if (s->revision != REV_NVIC) {
+        i += (GIC_INTERNAL * s->num_cpu);
+    }
+    qdev_init_gpio_in(DEVICE(s), handler, i);
+
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->parent_irq[i]);
+    }
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->parent_fiq[i]);
+    }
+
+    /* Distributor */
+    memory_region_init_io(&s->iomem, OBJECT(s), ops, s, "gic_dist", 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
+
+    if (s->revision != REV_NVIC) {
+        /* CPU interface (NVIC doesn't have this) */
+        memory_region_init_io(&s->cpuiomem[0], OBJECT(s), ops ? &ops[1] : NULL,
+                              s, "gic_cpu", 0x1000);
+        sysbus_init_mmio(sbd, &s->cpuiomem[0]);
+    }
+}
+
 static void arm_gic_common_realize(DeviceState *dev, Error **errp)
 {
     GICState *s = ARM_GIC_COMMON(dev);
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index f56bff1..e5d0f67 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -543,7 +543,6 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
 {
     int i;
     GICState *s = KVM_ARM_GIC(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
     Error *local_err = NULL;
     int ret;
@@ -560,32 +559,13 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    i = s->num_irq - GIC_INTERNAL;
-    /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
-     * GPIO array layout is thus:
-     *  [0..N-1] SPIs
-     *  [N..N+31] PPIs for CPU 0
-     *  [N+32..N+63] PPIs for CPU 1
-     *   ...
-     */
-    i += (GIC_INTERNAL * s->num_cpu);
-    qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);
+    gic_init_irqs_and_mmio(s, kvm_arm_gic_set_irq, NULL);
 
     for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
         qemu_irq irq = qdev_get_gpio_in(dev, i);
         kvm_irqchip_set_qemuirq_gsi(kvm_state, irq, i);
     }
 
-    /* We never use our outbound IRQ/FIQ lines but provide them so that
-     * we maintain the same interface as the non-KVM GIC.
-     */
-    for (i = 0; i < s->num_cpu; i++) {
-        sysbus_init_irq(sbd, &s->parent_irq[i]);
-    }
-    for (i = 0; i < s->num_cpu; i++) {
-        sysbus_init_irq(sbd, &s->parent_fiq[i]);
-    }
-
     /* Try to create the device via the device control API */
     s->dev_fd = -1;
     ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V2, false);
@@ -609,9 +589,6 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
     }
 
     /* Distributor */
-    memory_region_init_reservation(&s->iomem, OBJECT(s),
-                                   "kvm-gic_dist", 0x1000);
-    sysbus_init_mmio(sbd, &s->iomem);
     kvm_arm_register_device(&s->iomem,
                             (KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT)
                             | KVM_VGIC_V2_ADDR_TYPE_DIST,
@@ -622,9 +599,6 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
      * provide the "interface for core #N" memory regions, because
      * cores with a VGIC don't have those.
      */
-    memory_region_init_reservation(&s->cpuiomem[0], OBJECT(s),
-                                   "kvm-gic_cpu", 0x1000);
-    sysbus_init_mmio(sbd, &s->cpuiomem[0]);
     kvm_arm_register_device(&s->cpuiomem[0],
                             (KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT)
                             | KVM_VGIC_V2_ADDR_TYPE_CPU,
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 899db3d..edca3e0 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -138,4 +138,7 @@ typedef struct ARMGICCommonClass {
     void (*post_load)(GICState *s);
 } ARMGICCommonClass;
 
+void gic_init_irqs_and_mmio(GICState *s, qemu_irq_handler handler,
+                            const MemoryRegionOps *ops);
+
 #endif
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH 2/3] hw/arm/gic: Kill code duplication

[Peter Maydell]

On 29 July 2015 at 12:54, Pavel Fedin <p.fedin@samsung.com> wrote:
> Extracted duplicated initialization code from SW-emulated and KVM GIC
> implementations and put into gic_init_irqs_and_mmio()
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

> @@ -1110,28 +1090,19 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    gic_init_irqs_and_distributor(s);
> +    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops);
>
> -    /* Memory regions for the CPU interfaces (NVIC doesn't have these):
> -     * a region for "CPU interface for this core", then a region for
> -     * "CPU interface for core 0", "for core 1", ...

Losing this bit of comment means we no longer have any documentation
of what the memory region interface between the GIC and its users is
(ie which sysbus memory regions are what).

> +    /* Extra core-specific regions for the CPU interfaces
>       * NB that the memory region size of 0x100 applies for the 11MPCore
>       * and also cores following the GIC v1 spec (ie A9).
>       * GIC v2 defines a larger memory region (0x1000) so this will need
>       * to be extended when we implement A15.
>       */
> -    memory_region_init_io(&s->cpuiomem[0], OBJECT(s), &gic_thiscpu_ops, s,
> -                          "gic_cpu", 0x100);

This memory region is size 0x100, as the comment says it must be...

> +    if (s->revision != REV_NVIC) {
> +        /* CPU interface (NVIC doesn't have this) */
> +        memory_region_init_io(&s->cpuiomem[0], OBJECT(s), ops ? &ops[1] : NULL,
> +                              s, "gic_cpu", 0x1000);

...but here it is 0x1000.

The a9mpcore container component creates a layout where there are
other things immediately after the 0x100 region, so we can't
make it bigger for GICv1. (We will need to have it be bigger
for GICv2 when we eventually get around to implementing the
split-priority-drop feature.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/arm/gic: Kill code duplication

[Pavel Fedin]

 Hello!

> > -    memory_region_init_io(&s->cpuiomem[0], OBJECT(s), &gic_thiscpu_ops, s,
> > -                          "gic_cpu", 0x100);
> 
> This memory region is size 0x100, as the comment says it must be...
> 
> > +    if (s->revision != REV_NVIC) {
> > +        /* CPU interface (NVIC doesn't have this) */
> > +        memory_region_init_io(&s->cpuiomem[0], OBJECT(s), ops ? &ops[1] : NULL,
> > +                              s, "gic_cpu", 0x1000);
> 
> ...but here it is 0x1000.
> 
> The a9mpcore container component creates a layout where there are
> other things immediately after the 0x100 region, so we can't
> make it bigger for GICv1.

 I have checked the code, We have "revision" property, and for a9mpcore it appears to be set to 1 (default). So will it be OK if i rely on revision here ? I mean: "s->revision == 2 ? 0x1000 : 0x100". Revision == 2 is also used by ZynqMP model, which seems to have a9 CPU (according to some 'a9' names in the code), but its memory layout actually can accommodate this, they say that single region is 4K.
 All models that use KVM set revision to 2 and therefore expect full-sized region.

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

[Qemu-devel] [PATCH 3/3] Introduce gic_class_name() instead of repeating condition

[Pavel Fedin]

This small inline returns correct GIC class name depending on whether we
use KVM acceleration or not. Avoids duplicating the condition everywhere.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/arm/virt.c        | 7 +++----
 hw/cpu/a15mpcore.c   | 8 ++------
 target-arm/kvm_arm.h | 5 +++++
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4846892..943e523 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -48,6 +48,7 @@
 #include "hw/arm/sysbus-fdt.h"
 #include "hw/platform-bus.h"
 #include "hw/arm/fdt.h"
+#include "kvm_arm.h"
 
 /* Number of external interrupt lines to configure the GIC with */
 #define NUM_IRQS 256
@@ -365,12 +366,10 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
     /* We create a standalone GIC v2 */
     DeviceState *gicdev;
     SysBusDevice *gicbusdev;
-    const char *gictype = "arm_gic";
+    const char *gictype;
     int i;
 
-    if (kvm_irqchip_in_kernel()) {
-        gictype = "kvm-arm-gic";
-    }
+    gictype = gic_class_name();
 
     gicdev = qdev_create(NULL, gictype);
     qdev_prop_set_uint32(gicdev, "revision", 2);
diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index acc419e..e31a1f9 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -20,6 +20,7 @@
 
 #include "hw/cpu/a15mpcore.h"
 #include "sysemu/kvm.h"
+#include "kvm_arm.h"
 
 static void a15mp_priv_set_irq(void *opaque, int irq, int level)
 {
@@ -33,16 +34,11 @@ static void a15mp_priv_initfn(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     A15MPPrivState *s = A15MPCORE_PRIV(obj);
     DeviceState *gicdev;
-    const char *gictype = "arm_gic";
-
-    if (kvm_irqchip_in_kernel()) {
-        gictype = "kvm-arm-gic";
-    }
 
     memory_region_init(&s->container, obj, "a15mp-priv-container", 0x8000);
     sysbus_init_mmio(sbd, &s->container);
 
-    object_initialize(&s->gic, sizeof(s->gic), gictype);
+    object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
     gicdev = DEVICE(&s->gic);
     qdev_set_parent_bus(gicdev, sysbus_get_default());
     qdev_prop_set_uint32(gicdev, "revision", 2);
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index 7912d74..b3e0ab7 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -191,4 +191,9 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);
 
 #endif
 
+static inline const char *gic_class_name(void)
+{
+    return kvm_irqchip_in_kernel() ? "kvm-arm-gic" : "arm_gic";
+}
+
 #endif
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH 3/3] Introduce gic_class_name() instead of repeating condition

[Peter Maydell]

On 29 July 2015 at 12:54, Pavel Fedin <p.fedin@samsung.com> wrote:
> This small inline returns correct GIC class name depending on whether we
> use KVM acceleration or not. Avoids duplicating the condition everywhere.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/arm/virt.c        | 7 +++----
>  hw/cpu/a15mpcore.c   | 8 ++------
>  target-arm/kvm_arm.h | 5 +++++
>  3 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM