diff mbox

[2/3] hw/arm/gic: Kill code duplication

Message ID 0abba616ceb63d88fe02fbc94b2677c98f5a250f.1438170554.git.p.fedin@samsung.com
State New
Headers show

Commit Message

Pavel Fedin July 29, 2015, 11:54 a.m. UTC
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(-)

Comments

Peter Maydell Aug. 4, 2015, 3:46 p.m. UTC | #1
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
Pavel Fedin Aug. 5, 2015, 6:30 a.m. UTC | #2
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
diff mbox

Patch

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