Re: [Qemu-arm] [RFC v2 7/7] hw/arm/virt: Register two redistributor regions when necessary

[Auger Eric]

Hi,

On 05/22/2018 02:43 PM, Peter Maydell wrote:
> On 13 May 2018 at 15:35, Eric Auger <address@hidden> 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 <address@hidden>
>> ---
>>  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
>