[2/2] hw/arm/virt: don't use a15memmap directly

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.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

On 6 October 2015 at 15:37, Andrew Jones <drjones@redhat.com> 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 <drjones@redhat.com>
> ---
>  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

On Fri, Oct 09, 2015 at 05:45:24PM +0100, Peter Maydell wrote:
> On 6 October 2015 at 15:37, Andrew Jones <drjones@redhat.com> 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 <drjones@redhat.com>
> > ---
> >  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".

Sounds good to me, but I actually took that message from vl.c, where
there's already a message for the same purpose, so I was trying to be
consistent with that. Maybe we should clean that one up too.

> 
> > +        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.

OK

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

Thanks for the review. I'll send out a v2 soon.

drew

Hello!

> 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.

 Actually, gicv2 code has own check, and it would complain about >8 CPU (see
arm_gic_common_realize()).

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

On Mon, Oct 12, 2015 at 10:00:19AM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > 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.
> 
>  Actually, gicv2 code has own check, and it would complain about >8 CPU (see
> arm_gic_common_realize()).

It only gets a chance to do that check if the class init check doesn't
fail first, i.e. nr_cpus > 8 <= 123 will fail with a "> 8" message, but
nr_cpus > 123, as I wrote above, will fail with a "> 123" message.

drew

> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 
>