Re: [Qemu-arm] [RFC v2 3/7] hw/intc/arm_gicv3: Introduce redist-region-count array property

[Peter Maydell]

On 13 May 2018 at 15:35, Eric Auger <address@hidden> wrote:
> To prepare for multiple redistributor regions, we introduce
> an array of uint32_t properties that stores the redistributor
> count of each redistributor region.
>
> Non accelerated VGICv3 only supports a single redistributor region.
> The capacity of all redist regions is checked against the number of
> vcpus.
>
> Machvirt is updated to set the count to 123 vcpus for the unique
> redistributor region we currently have.
>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/arm/virt.c                      |  6 ++++++
>  hw/intc/arm_gicv3.c                | 11 ++++++++++-
>  hw/intc/arm_gicv3_common.c         | 35 ++++++++++++++++++++++++++++++-----
>  hw/intc/arm_gicv3_kvm.c            |  9 +++++++--
>  include/hw/intc/arm_gicv3_common.h |  6 ++++--
>  5 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 11b9f59..c9d842d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -525,6 +525,12 @@ static void create_gic(VirtMachineState *vms, qemu_irq 
> *pic)
>      if (!kvm_irqchip_in_kernel()) {
>          qdev_prop_set_bit(gicdev, "has-security-extensions", vms->secure);
>      }
> +
> +    if (type == 3) {
> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
> +        qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
> +                             vms->memmap[VIRT_GIC_REDIST].size / 0x20000);

We used to create a region which had num_cpu redistributors in it;
won't this cause us to create one which has as many redistributors
as will fit in the space ?

> +    }
>      qdev_init_nofail(gicdev);
>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 479c667..38d57ac 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -373,7 +373,16 @@ static void arm_gic_realize(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>
> -    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops);
> +    if (s->nb_redist_regions != 1) {
> +        error_setg(errp, "VGICv3 redist region number(%d) not equal to 1",
> +                   s->nb_redist_regions);

Missing 'return' ?

> +    }
> +
> +    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>
>      gicv3_init_cpuif(s);
>  }
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 7b54d52..f405ae1 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -169,9 +169,10 @@ static const VMStateDescription vmstate_gicv3 = {
>  };
>
>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
> -                              const MemoryRegionOps *ops)
> +                              const MemoryRegionOps *ops, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(s);
> +    int rdist_capacity = 0;
>      int i;
>
>      /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
> @@ -199,11 +200,25 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, 
> qemu_irq_handler handler,
>
>      memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
>                            "gicv3_dist", 0x10000);
> -    memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, 
> s,
> -                          "gicv3_redist", 0x20000 * s->num_cpu);
> -
>      sysbus_init_mmio(sbd, &s->iomem_dist);
> -    sysbus_init_mmio(sbd, &s->iomem_redist);
> +
> +    s->iomem_redist = g_new0(MemoryRegion, s->nb_redist_regions);
> +    for (i = 0; i < s->nb_redist_regions; i++) {
> +        char *name = g_strdup_printf("gicv3_redist_region[%d]", i);
> +
> +        memory_region_init_io(&s->iomem_redist[i], OBJECT(s),
> +                              ops ? &ops[1] : NULL, s,
> +                              name, 0x20000 * s->redist_region_count[i]);
> +        sysbus_init_mmio(sbd, &s->iomem_redist[i]);
> +        rdist_capacity += s->redist_region_count[i];
> +        g_free(name);
> +    }
> +    if (rdist_capacity < s->num_cpu) {
> +        error_setg(errp, "Capacity of the redist regions(%d) "
> +                   "is less than number of vcpus(%d)",
> +                   rdist_capacity, s->num_cpu);

It would be better to make this check before we create a lot of
memory regions, I think. (Though I'm very unsure of the rules about
how to unwind what you've done in a realize method that's about to fail;
we probably get it wrong a lot and don't care because realize failure
is fatal for most uses of most devices.)

> +    }
> +
>  }
>
>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)

thanks
-- PMM