Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: don't use a15memmap directly

[Peter Maydell]

On 6 October 2015 at 15:37, Andrew Jones <address@hidden> wrote:
> We should always go through VirtBoardInfo when we need the memmap.
> To avoid using a15memmap directly, in this case, we need to defer
> the max-cpus check from class init time to instance init time. In
> class init we now use MAX_CPUMASK_BITS for max_cpus initialization,
> which is the maximum QEMU supports, and also, incidentally, the
> maximum KVM/gicv3 currently supports. Also, a nice side-effect of
> delaying the max-cpus check is that we now get more appropriate
> error messages for gicv2 machines that try to configure more than
> 123 cpus. Before this patch it would complain that the requested
> number of cpus was greater than 123, but for gicv2 configs, it
> should complain that the number is greater than 8.

Yes, this seems like a good plan.

> Signed-off-by: Andrew Jones <address@hidden>
> ---
>  hw/arm/virt.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d25d6cfce74cd..a9901983731ae 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -918,7 +918,7 @@ static void machvirt_init(MachineState *machine)
>      qemu_irq pic[NUM_IRQS];
>      MemoryRegion *sysmem = get_system_memory();
>      int gic_version = vms->gic_version;
> -    int n;
> +    int n, max_cpus;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      const char *cpu_model = machine->cpu_model;
>      VirtBoardInfo *vbi;
> @@ -952,6 +952,21 @@ static void machvirt_init(MachineState *machine)
>          exit(1);
>      }
>
> +    /* The maximum number of CPUs depends on the GIC version, or on how
> +     * many redistributors we can fit into the memory map.
> +     */
> +    if (gic_version == 3) {
> +        max_cpus = vbi->memmap[VIRT_GIC_REDIST].size / 0x20000;
> +    } else {
> +        max_cpus = GICV2_NCPU;
> +    }
> +
> +    if (smp_cpus > max_cpus) {
> +        error_report("mach-virt: Number of SMP cpus requested (%d), "

The comma here in the error message is unnecessary.

> +                     "exceeds max cpus supported %d", smp_cpus, max_cpus);

...and there should be parens around the %d here for consistency.

Since this is a user-facing error message, "CPUs" is nicer than
"cpus".

> +        exit(1);
> +    }
> +
>      vbi->smp_cpus = smp_cpus;
>
>      if (machine->ram_size > vbi->memmap[VIRT_MEM].size) {
> @@ -1150,10 +1165,7 @@ static void virt_class_init(ObjectClass *oc, void 
> *data)
>
>      mc->desc = "ARM Virtual Machine",
>      mc->init = machvirt_init;
> -    /* Our maximum number of CPUs depends on how many redistributors
> -     * we can fit into memory map
> -     */
> -    mc->max_cpus = a15memmap[VIRT_GIC_REDIST].size / 0x20000;
> +    mc->max_cpus = MAX_CPUMASK_BITS;

A brief comment that we do a more restrictive check later at init
time would be a good idea I think.

>      mc->has_dynamic_sysbus = true;
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
> --

thanks
-- PMM