Skip to contentSkip to conversation

hw/arm/gic: Code duplication removal

PATCH Applied Pavel Fedin 2 participants 5 messages Aug 5, 2015 – Aug 7, 2015

lore
Share & tools
lore
#0 lore raw

v2 cover hw/arm/gic: Code duplication removal

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.

v1 -> v2:
- Use correct size of CPU interface region for pre-v2 GICs
- Improved comments

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                | 64 ++++++++++++----------------------------
 hw/intc/arm_gic_common.c         | 41 +++++++++++++++++++++++++
 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, 86 insertions(+), 94 deletions(-)
Signature · 1 line
1.9.5.msysgit.0
#1 lore raw

PATCH v2 1/3 Merge memory_region_init_reservation() into memory_region_init_io()

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

---
 include/exec/memory.h | 14 +++++++++++---
 memory.c              | 10 +---------
 2 files changed, 12 insertions(+), 12 deletions(-)
include/exec/memory.h +11 -3
index 94d20ea..b18b351 100644
@@ -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
memory.c +1 -9
index 4eb138a..0d8b2d9 100644
@@ -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);
Signature · 1 line
1.9.5.msysgit.0
#2 lore raw

PATCH v2 2/3 hw/arm/gic: Kill code duplication

In reply to Pavel Fedin

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

---
 hw/intc/arm_gic.c                | 64 ++++++++++++----------------------------
 hw/intc/arm_gic_common.c         | 41 +++++++++++++++++++++++++
 hw/intc/arm_gic_kvm.c            | 28 +-----------------
 include/hw/intc/arm_gic_common.h |  3 ++
 4 files changed, 64 insertions(+), 72 deletions(-)
hw/intc/arm_gic.c +19 -45
index 454bfd7..a8c5d19 100644
@@ -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,22 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    gic_init_irqs_and_distributor(s);
+    /* This creates distributor and main CPU interface (s->cpuiomem[0]) */
+    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. This is
+     * necessary for "franken-GIC" implementations, for example on
+     * Exynos 4.
      * 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]);
     }
 }
 
hw/intc/arm_gic_common.c +41 -0

47 lines not on this page · raw message

hw/intc/arm_gic_kvm.c +1 -27
index f56bff1..e5d0f67 100644
@@ -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,
include/hw/intc/arm_gic_common.h +3 -0
index 899db3d..edca3e0 100644
@@ -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
Signature · 1 line
1.9.5.msysgit.0
#3 lore raw

PATCH v2 3/3 Introduce gic_class_name() instead of repeating condition

In reply to Pavel Fedin

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

---
 hw/arm/virt.c        | 7 +++----
 hw/cpu/a15mpcore.c   | 8 ++------
 target-arm/kvm_arm.h | 5 +++++
 3 files changed, 10 insertions(+), 10 deletions(-)
hw/arm/virt.c +3 -4
index 4846892..943e523 100644
@@ -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);
hw/cpu/a15mpcore.c +2 -6
index acc419e..e31a1f9 100644
@@ -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);
target-arm/kvm_arm.h +5 -0
index 7912d74..b3e0ab7 100644
@@ -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
Signature · 1 line
1.9.5.msysgit.0
#4 lore raw
In reply to Pavel Fedin
Quoted 7 lines · 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.

Quoted 8 lines

v1 -> v2:
- Use correct size of CPU interface region for pre-v2 GICs
- Improved comments

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

Thanks, applied to target-arm-post-2.4.

-- PMM