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