[Qemu-devel] [PATCH v3 14/20] intc/arm_gic: Wire the vCPU interface

Luc Michel posted 20 patches 7 years, 7 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 14/20] intc/arm_gic: Wire the vCPU interface
Posted by Luc Michel 7 years, 7 months ago
Add the read/write functions to handle accesses to the vCPU interface.
Those accesses are forwarded to the real CPU interface, with the CPU id
being converted to the corresponding vCPU id (vCPU id = CPU id +
GIC_NCPU).

As for the CPU interface, we create a base region for the vCPU interface
that fetches the current vCPU id using the current_cpu global variable, and
one mirror region per vCPU which maps to that specific vCPU id. This is
required by the GIC architecture specification.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
 hw/intc/arm_gic.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 2b1fa280eb..9bbd544a5c 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1488,6 +1488,46 @@ static MemTxResult gic_do_cpu_write(void *opaque, hwaddr addr,
     GICState *s = *backref;
     int id = (backref - s->backref);
     return gic_cpu_write(s, id, addr, value, attrs);
+
+}
+
+static MemTxResult gic_thisvcpu_read(void *opaque, hwaddr addr, uint64_t *data,
+                                    unsigned size, MemTxAttrs attrs)
+{
+    GICState *s = (GICState *)opaque;
+
+    return gic_cpu_read(s, gic_get_current_vcpu(s), addr, data, attrs);
+}
+
+static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
+                                     uint64_t value, unsigned size,
+                                     MemTxAttrs attrs)
+{
+    GICState *s = (GICState *)opaque;
+
+    return gic_cpu_write(s, gic_get_current_vcpu(s), addr, value, attrs);
+}
+
+static MemTxResult gic_do_vcpu_read(void *opaque, hwaddr addr, uint64_t *data,
+                                    unsigned size, MemTxAttrs attrs)
+{
+    GICState **backref = (GICState **)opaque;
+    GICState *s = *backref;
+    int id = (backref - s->backref);
+
+    return gic_cpu_read(s, id + GIC_NCPU, addr, data, attrs);
+}
+
+static MemTxResult gic_do_vcpu_write(void *opaque, hwaddr addr,
+                                     uint64_t value, unsigned size,
+                                     MemTxAttrs attrs)
+{
+    GICState **backref = (GICState **)opaque;
+    GICState *s = *backref;
+    int id = (backref - s->backref);
+
+    return gic_cpu_write(s, id + GIC_NCPU, addr, value, attrs);
+
 }
 
 static const MemoryRegionOps gic_ops[2] = {
@@ -1509,6 +1549,25 @@ static const MemoryRegionOps gic_cpu_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static const MemoryRegionOps gic_virt_ops[2] = {
+    {
+        .read_with_attrs = NULL,
+        .write_with_attrs = NULL,
+        .endianness = DEVICE_NATIVE_ENDIAN,
+    },
+    {
+        .read_with_attrs = gic_thisvcpu_read,
+        .write_with_attrs = gic_thisvcpu_write,
+        .endianness = DEVICE_NATIVE_ENDIAN,
+    }
+};
+
+static const MemoryRegionOps gic_vcpu_ops = {
+    .read_with_attrs = gic_do_vcpu_read,
+    .write_with_attrs = gic_do_vcpu_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 static void arm_gic_realize(DeviceState *dev, Error **errp)
 {
     /* Device instance realize function for the GIC sysbus device */
@@ -1531,7 +1590,7 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
     }
 
     /* This creates distributor and main CPU interface (s->cpuiomem[0]) */
-    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, NULL);
+    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, gic_virt_ops);
 
     /* Extra core-specific regions for the CPU interfaces. This is
      * necessary for "franken-GIC" implementations, for example on
@@ -1547,6 +1606,16 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
                               &s->backref[i], "gic_cpu", 0x100);
         sysbus_init_mmio(sbd, &s->cpuiomem[i+1]);
     }
+
+    if (s->virt_extn) {
+        for (i = 0; i < s->num_cpu; i++) {
+            memory_region_init_io(&s->vcpuiomem[i + 1], OBJECT(s),
+                                  &gic_vcpu_ops, &s->backref[i],
+                                  "gic_vcpu", 0x2000);
+            sysbus_init_mmio(sbd, &s->vcpuiomem[i + 1]);
+        }
+    }
+
 }
 
 static void arm_gic_class_init(ObjectClass *klass, void *data)
-- 
2.17.1


Re: [Qemu-devel] [PATCH v3 14/20] intc/arm_gic: Wire the vCPU interface
Posted by Peter Maydell 7 years, 7 months ago
On 29 June 2018 at 14:29, Luc Michel <luc.michel@greensocs.com> wrote:
> Add the read/write functions to handle accesses to the vCPU interface.
> Those accesses are forwarded to the real CPU interface, with the CPU id
> being converted to the corresponding vCPU id (vCPU id = CPU id +
> GIC_NCPU).
>
> As for the CPU interface, we create a base region for the vCPU interface
> that fetches the current vCPU id using the current_cpu global variable, and
> one mirror region per vCPU which maps to that specific vCPU id. This is
> required by the GIC architecture specification.


>  static void arm_gic_realize(DeviceState *dev, Error **errp)
>  {
>      /* Device instance realize function for the GIC sysbus device */
> @@ -1531,7 +1590,7 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>      }
>
>      /* This creates distributor and main CPU interface (s->cpuiomem[0]) */

Can we also update this comment to indicate what virt-related
memory regions are being created?

> -    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, NULL);
> +    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, gic_virt_ops);
>
>      /* Extra core-specific regions for the CPU interfaces. This is
>       * necessary for "franken-GIC" implementations, for example on
> @@ -1547,6 +1606,16 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>                                &s->backref[i], "gic_cpu", 0x100);
>          sysbus_init_mmio(sbd, &s->cpuiomem[i+1]);
>      }
> +
> +    if (s->virt_extn) {

...and a comment about what these regions are for.

What requires these per-core regions anyway? There's no way to
specify them in the device tree bindings for Linux, which AFAIK
only cares about using the "vcpu i/f for this core" registers.
I don't think the GIC-400 has these. (It does have per-cpu
aliases of the GICH_* registers, but this patchset doesn't seem
to implement those.)

> +        for (i = 0; i < s->num_cpu; i++) {
> +            memory_region_init_io(&s->vcpuiomem[i + 1], OBJECT(s),
> +                                  &gic_vcpu_ops, &s->backref[i],
> +                                  "gic_vcpu", 0x2000);
> +            sysbus_init_mmio(sbd, &s->vcpuiomem[i + 1]);
> +        }
> +    }
> +
>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 14/20] intc/arm_gic: Wire the vCPU interface
Posted by Luc Michel 7 years, 7 months ago

On 07/12/2018 03:37 PM, Peter Maydell wrote:
> On 29 June 2018 at 14:29, Luc Michel <luc.michel@greensocs.com> wrote:
>> Add the read/write functions to handle accesses to the vCPU interface.
>> Those accesses are forwarded to the real CPU interface, with the CPU id
>> being converted to the corresponding vCPU id (vCPU id = CPU id +
>> GIC_NCPU).
>>
>> As for the CPU interface, we create a base region for the vCPU interface
>> that fetches the current vCPU id using the current_cpu global variable, and
>> one mirror region per vCPU which maps to that specific vCPU id. This is
>> required by the GIC architecture specification.
> 
> 
>>  static void arm_gic_realize(DeviceState *dev, Error **errp)
>>  {
>>      /* Device instance realize function for the GIC sysbus device */
>> @@ -1531,7 +1590,7 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>>      }
>>
>>      /* This creates distributor and main CPU interface (s->cpuiomem[0]) */
> 
> Can we also update this comment to indicate what virt-related
> memory regions are being created?
> 
>> -    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, NULL);
>> +    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, gic_virt_ops);
>>
>>      /* Extra core-specific regions for the CPU interfaces. This is
>>       * necessary for "franken-GIC" implementations, for example on
>> @@ -1547,6 +1606,16 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>>                                &s->backref[i], "gic_cpu", 0x100);
>>          sysbus_init_mmio(sbd, &s->cpuiomem[i+1]);
>>      }
>> +
>> +    if (s->virt_extn) {
> 
> ...and a comment about what these regions are for.
> 
> What requires these per-core regions anyway? There's no way to
> specify them in the device tree bindings for Linux, which AFAIK
> only cares about using the "vcpu i/f for this core" registers.
> I don't think the GIC-400 has these. (It does have per-cpu
> aliases of the GICH_* registers, but this patchset doesn't seem
> to implement those.)
My mistake. I misread the specifications. Those aliases should target
the virtual interfaces, and not the virtual CPU interfaces. I'll change
that.

> 
>> +        for (i = 0; i < s->num_cpu; i++) {
>> +            memory_region_init_io(&s->vcpuiomem[i + 1], OBJECT(s),
>> +                                  &gic_vcpu_ops, &s->backref[i],
>> +                                  "gic_vcpu", 0x2000);
>> +            sysbus_init_mmio(sbd, &s->vcpuiomem[i + 1]);
>> +        }
>> +    }
>> +
>>  }
> 
> thanks
> -- PMM
>