With a VGICv3 KVM device, if the number of vcpus exceeds the
capacity of the legacy redistributor region (123 redistributors),
we now attempt to register the second redistributor region. This
extends the number of vcpus to 512 assuming the host kernel supports:
- up to 512 vcpus
- VGICv3 KVM device group/attribute:
  KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

In case the host kernel does not support the registration of several
redistributor regions, the GICv3 device initialization fails
with a proper error message and qemu exits.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f35962a..8d43c51 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -531,6 +531,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     SysBusDevice *gicbusdev;
     const char *gictype;
     int type = vms->gic_version, i;
+    uint32_t nb_redist_regions = 0;
 
     gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
 
@@ -546,15 +547,26 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     }
 
     if (type == 3) {
-        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
+        nb_redist_regions = virt_gicv3_redist_region_count(vms);
+
+        qdev_prop_set_uint32(gicdev, "len-redist-region-count",
+                             nb_redist_regions);
         qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
                              vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
+
+        if (nb_redist_regions == 2) {
+            qdev_prop_set_uint32(gicdev , "redist-region-count[1]",
+                                 vms->memmap[VIRT_GIC_REDIST2].size / 0x20000);
+        }
     }
     qdev_init_nofail(gicdev);
     gicbusdev = SYS_BUS_DEVICE(gicdev);
     sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
     if (type == 3) {
         sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_REDIST].base);
+        if (nb_redist_regions == 2) {
+            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_REDIST2].base);
+        }
     } else {
         sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
     }
@@ -1346,6 +1358,7 @@ static void machvirt_init(MachineState *machine)
      */
     if (vms->gic_version == 3) {
         virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
+        virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
     } else {
         virt_max_cpus = GIC_NCPU;
     }
-- 
2.5.5

On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
> With a VGICv3 KVM device, if the number of vcpus exceeds the
> capacity of the legacy redistributor region (123 redistributors),
> we now attempt to register the second redistributor region. This
> extends the number of vcpus to 512 assuming the host kernel supports:
> - up to 512 vcpus
> - VGICv3 KVM device group/attribute:
>   KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>
> In case the host kernel does not support the registration of several
> redistributor regions, the GICv3 device initialization fails
> with a proper error message and qemu exits.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f35962a..8d43c51 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -531,6 +531,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>      SysBusDevice *gicbusdev;
>      const char *gictype;
>      int type = vms->gic_version, i;
> +    uint32_t nb_redist_regions = 0;
>
>      gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
>
> @@ -546,15 +547,26 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>      }
>
>      if (type == 3) {
> -        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
> +        nb_redist_regions = virt_gicv3_redist_region_count(vms);
> +
> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count",
> +                             nb_redist_regions);
>          qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>                               vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
> +
> +        if (nb_redist_regions == 2) {
> +            qdev_prop_set_uint32(gicdev , "redist-region-count[1]",

Stray extra space before comma.

> +                                 vms->memmap[VIRT_GIC_REDIST2].size / 0x20000);

This is going to ask for more redistributors than we want (cf
earlier review comment).

> +        }
>      }
>      qdev_init_nofail(gicdev);
>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
>      if (type == 3) {
>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_REDIST].base);
> +        if (nb_redist_regions == 2) {
> +            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_REDIST2].base);
> +        }
>      } else {
>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>      }
> @@ -1346,6 +1358,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      if (vms->gic_version == 3) {
>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
> +        virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;

Slight pity that we can't determine whether we can support multiple
redistributor regions here, but I guess it's not a big deal.

>      } else {
>          virt_max_cpus = GIC_NCPU;
>      }
> --
> 2.5.5

thanks
-- PMM

Hi,

On 05/22/2018 02:43 PM, Peter Maydell wrote:
> On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
>> With a VGICv3 KVM device, if the number of vcpus exceeds the
>> capacity of the legacy redistributor region (123 redistributors),
>> we now attempt to register the second redistributor region. This
>> extends the number of vcpus to 512 assuming the host kernel supports:
>> - up to 512 vcpus

I just noticed virt_machine_class_init still clamps the max_cpus to 255.
This is checked in vl.c

Do we want the 3.0 machine expose more than 255 vcpus, 512 for instance?
I understand this mc->max_cpus is something rather static, that is
refined afterwards.

Note the comment in virt_machine_class_init suggest 255 is the max QEMU
supports but I can see that pc_q35.c sets the max_cpus to 288 already.

Thanks

Eric

>> - VGICv3 KVM device group/attribute:
>>   KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>>
>> In case the host kernel does not support the registration of several
>> redistributor regions, the GICv3 device initialization fails
>> with a proper error message and qemu exits.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f35962a..8d43c51 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -531,6 +531,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>      SysBusDevice *gicbusdev;
>>      const char *gictype;
>>      int type = vms->gic_version, i;
>> +    uint32_t nb_redist_regions = 0;
>>
>>      gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
>>
>> @@ -546,15 +547,26 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>      }
>>
>>      if (type == 3) {
>> -        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
>> +        nb_redist_regions = virt_gicv3_redist_region_count(vms);
>> +
>> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count",
>> +                             nb_redist_regions);
>>          qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>>                               vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
>> +
>> +        if (nb_redist_regions == 2) {
>> +            qdev_prop_set_uint32(gicdev , "redist-region-count[1]",
> 
> Stray extra space before comma.
> 
>> +                                 vms->memmap[VIRT_GIC_REDIST2].size / 0x20000);
> 
> This is going to ask for more redistributors than we want (cf
> earlier review comment).
> 
>> +        }
>>      }
>>      qdev_init_nofail(gicdev);
>>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
>>      if (type == 3) {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_REDIST].base);
>> +        if (nb_redist_regions == 2) {
>> +            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_REDIST2].base);
>> +        }
>>      } else {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>>      }
>> @@ -1346,6 +1358,7 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      if (vms->gic_version == 3) {
>>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
>> +        virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
> 
> Slight pity that we can't determine whether we can support multiple
> redistributor regions here, but I guess it's not a big deal.
> 
>>      } else {
>>          virt_max_cpus = GIC_NCPU;
>>      }
>> --
>> 2.5.5
> 
> thanks
> -- PMM
> 

On 29 May 2018 at 14:43, Auger Eric <eric.auger@redhat.com> wrote:
> I just noticed virt_machine_class_init still clamps the max_cpus to 255.
> This is checked in vl.c
>
> Do we want the 3.0 machine expose more than 255 vcpus, 512 for instance?
> I understand this mc->max_cpus is something rather static, that is
> refined afterwards.

Mmm. It's mostly used by the option checking code in vl.c, which
runs before the board init function. So really what we're doing by
setting it to some high number is disabling those checks; we then
have to mess around with checking manually later. Unfortunately
I don't think we have enough info to set the real maximum early
enough to avoid this.

We should probably set max_cpus here to whatever is the maximum the
board can handle in any configuration.

> Note the comment in virt_machine_class_init suggest 255 is the max QEMU
> supports but I can see that pc_q35.c sets the max_cpus to 288 already.

I think that's out of date, yes -- probably when the code was written
255 was the maximum.

thanks
-- PMM