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

[Peter Maydell]

On 29 June 2018 at 14:29, Luc Michel <address@hidden> 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