[Qemu-devel] [PATCH 0/2] hw/arm/virt: max-cpus init/check fixup

[Qemu-devel] [PATCH 0/2] hw/arm/virt: max-cpus init/check fixup

[Andrew Jones]

Andrew Jones (2):
  [RFC] arm_gic_common.h: add gicv2 aliases for defines
  hw/arm/virt: don't use a15memmap directly

 hw/arm/virt.c                    | 22 +++++++++++++++++-----
 include/hw/intc/arm_gic_common.h |  2 ++
 2 files changed, 19 insertions(+), 5 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] [RFC] arm_gic_common.h: add gicv2 aliases for defines

[Andrew Jones]

I'm not sure if arm_gic_common.h is supposed to be common, not
only between tcg and kvm, but also v2 and v3, but it currently
is (arm_gicv3_common.h includes it, and it's the only gic header
included by hw/arm/virt.c). If it should be the super-common
header, then it's unfortunate that the define names are too
generic. This patch doesn't help much, as it doesn't rename
anything, but it does start heading down the right path. With
it, code including the super-common header can start using more
appropriate names for a couple very gic-version-specific defines.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 include/hw/intc/arm_gic_common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 564a72b2cf77f..299226064a30f 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -25,11 +25,13 @@
 
 /* Maximum number of possible interrupts, determined by the GIC architecture */
 #define GIC_MAXIRQ 1020
+#define GICV2_MAXIRQ GIC_MAXIRQ
 /* First 32 are private to each CPU (SGIs and PPIs). */
 #define GIC_INTERNAL 32
 #define GIC_NR_SGIS 16
 /* Maximum number of possible CPU interfaces, determined by GIC architecture */
 #define GIC_NCPU 8
+#define GICV2_NCPU GIC_NCPU
 
 #define MAX_NR_GROUP_PRIO 128
 #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
-- 
2.4.3

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

[Andrew Jones]

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(-)

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), "
+                     "exceeds max cpus supported %d", smp_cpus, max_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;
     mc->has_dynamic_sysbus = true;
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/2] [RFC] arm_gic_common.h: add gicv2 aliases for defines

[Peter Maydell]

On 6 October 2015 at 15:37, Andrew Jones <drjones@redhat.com> wrote:
> I'm not sure if arm_gic_common.h is supposed to be common, not
> only between tcg and kvm, but also v2 and v3, but it currently
> is (arm_gicv3_common.h includes it, and it's the only gic header
> included by hw/arm/virt.c).

I don't think it should be included. We're doing completely
separate implementations of the GICv3 from the old GICv1/2
code.

It looks like the only thing in the gicv3 code that is using
a define from the arm_gic_common.h file is "GIC_INTERNAL",
so we can just put a suitable define of that into the v3 header
(maybe giving it a better name in the process).

> If it should be the super-common
> header, then it's unfortunate that the define names are too
> generic. This patch doesn't help much, as it doesn't rename
> anything, but it does start heading down the right path. With
> it, code including the super-common header can start using more
> appropriate names for a couple very gic-version-specific defines.

All the things in this header should be GICv2 only, really.
It's unfortunate that all our naming (including the source files
and the device names) uses just "gic", but a mass renaming doesn't
seem like a great idea to me.

Given that, perhaps having a few symbols with a "GICV2"
prefix is going to be more confusing than helpful.

thanks
-- PMM

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

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

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH 1/2] [RFC] arm_gic_common.h: add gicv2 aliases for defines

[Pavel Fedin]

 Hi!

> It looks like the only thing in the gicv3 code that is using
> a define from the arm_gic_common.h file is "GIC_INTERNAL",
> so we can just put a suitable define of that into the v3 header
> (maybe giving it a better name in the process).

 Yes, indeed. Actually, first versions of my GICv3 patches did use own #define, but i was criticized for using GICV3_INTERNAL in my code and having GIC_INTERNAL in kvm_arm_gic_set_irq(), which is shared by both KVM implementations. So, i decided to use GIC_INTERNAL everywhere and inherited it from v2.

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

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

[Pavel Fedin]

 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

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

[Andrew Jones]

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