[Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Igor Mammedov]

in current impl. condition

build_madt() {
  ...
  if (test_bit(i, cpuinfo->found_cpus))

is always true since loop handles only present CPUs
in range [0..smp_cpus).
But to fill usless cpuinfo->found_cpus we do unnecessary
scan over QOM tree to find the same CPUs.
So mark GICC as present always and drop not needed
code that fills cpuinfo->found_cpus.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
It's just simple cleanup but I'm trying to generalize
a bit CPU related ACPI tables and as part of it get rid
of found_cpus bitmap and if possible cpu_index usage
in ACPI parts of code.
---
 hw/arm/virt-acpi-build.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 87fbe7c..3ed39fc 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -46,20 +46,6 @@
 #define ARM_SPI_BASE 32
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
 
-typedef struct VirtAcpiCpuInfo {
-    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
-} VirtAcpiCpuInfo;
-
-static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
-{
-    CPUState *cpu;
-
-    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
-    CPU_FOREACH(cpu) {
-        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
-    }
-}
-
 static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
 {
     uint16_t i;
@@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
 
 /* MADT */
 static void
-build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
-           VirtAcpiCpuInfo *cpuinfo)
+build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
 {
     int madt_start = table_data->len;
     const MemMapEntry *memmap = guest_info->memmap;
@@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
         gicc->cpu_interface_number = i;
         gicc->arm_mpidr = armcpu->mp_affinity;
         gicc->uid = i;
-        if (test_bit(i, cpuinfo->found_cpus)) {
-            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
-        }
+        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
     }
 
     if (guest_info->gic_version == 3) {
@@ -599,11 +582,8 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
 {
     GArray *table_offsets;
     unsigned dsdt, rsdt;
-    VirtAcpiCpuInfo cpuinfo;
     GArray *tables_blob = tables->table_data;
 
-    virt_acpi_get_cpu_info(&cpuinfo);
-
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
 
@@ -630,7 +610,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
     build_fadt(tables_blob, tables->linker, dsdt);
 
     acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, guest_info, &cpuinfo);
+    build_madt(tables_blob, tables->linker, guest_info);
 
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Shannon Zhao]

On 2016/1/29 22:24, Igor Mammedov wrote:
> in current impl. condition
>
> build_madt() {
>    ...
>    if (test_bit(i, cpuinfo->found_cpus))
>
> is always true since loop handles only present CPUs
> in range [0..smp_cpus).
> But to fill usless cpuinfo->found_cpus we do unnecessary
> scan over QOM tree to find the same CPUs.
> So mark GICC as present always and drop not needed
> code that fills cpuinfo->found_cpus.
>
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> ---
> It's just simple cleanup but I'm trying to generalize
> a bit CPU related ACPI tables and as part of it get rid
> of found_cpus bitmap and if possible cpu_index usage
> in ACPI parts of code.
> ---
>   hw/arm/virt-acpi-build.c | 26 +++-----------------------
>   1 file changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 87fbe7c..3ed39fc 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -46,20 +46,6 @@
>   #define ARM_SPI_BASE 32
>   #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>
> -typedef struct VirtAcpiCpuInfo {
> -    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
> -} VirtAcpiCpuInfo;
> -
> -static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> -{
> -    CPUState *cpu;
> -
> -    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> -    CPU_FOREACH(cpu) {
> -        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> -    }
> -}
> -
>   static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>   {
>       uint16_t i;
> @@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
>
>   /* MADT */
>   static void
> -build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> -           VirtAcpiCpuInfo *cpuinfo)
> +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>   {
>       int madt_start = table_data->len;
>       const MemMapEntry *memmap = guest_info->memmap;
> @@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>           gicc->cpu_interface_number = i;
>           gicc->arm_mpidr = armcpu->mp_affinity;
>           gicc->uid = i;
> -        if (test_bit(i, cpuinfo->found_cpus)) {
> -            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> -        }
> +        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>       }
Ah, yes, it uses smp_cpus not max_cpus. But we still needs to support 
max_cpus usage even though it doesn't support vcpu hotplug currently. So 
we may need to introduce guest_info->max_cpus and use it here.
And below check in virt.c is not right while it should compare the 
global max_cpus with the max_cpus GIC supports.

     if (smp_cpus > max_cpus) {
         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
                      "supported by machine 'mach-virt' (%d)",
                      smp_cpus, max_cpus);
         exit(1);
     }

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Andrew Jones]

On Fri, Jan 29, 2016 at 10:59:32PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/1/29 22:24, Igor Mammedov wrote:
> >in current impl. condition
> >
> >build_madt() {
> >   ...
> >   if (test_bit(i, cpuinfo->found_cpus))
> >
> >is always true since loop handles only present CPUs
> >in range [0..smp_cpus).
> >But to fill usless cpuinfo->found_cpus we do unnecessary
> >scan over QOM tree to find the same CPUs.
> >So mark GICC as present always and drop not needed
> >code that fills cpuinfo->found_cpus.
> >
> >Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >---
> >It's just simple cleanup but I'm trying to generalize
> >a bit CPU related ACPI tables and as part of it get rid
> >of found_cpus bitmap and if possible cpu_index usage
> >in ACPI parts of code.
> >---
> >  hw/arm/virt-acpi-build.c | 26 +++-----------------------
> >  1 file changed, 3 insertions(+), 23 deletions(-)
> >
> >diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >index 87fbe7c..3ed39fc 100644
> >--- a/hw/arm/virt-acpi-build.c
> >+++ b/hw/arm/virt-acpi-build.c
> >@@ -46,20 +46,6 @@
> >  #define ARM_SPI_BASE 32
> >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >
> >-typedef struct VirtAcpiCpuInfo {
> >-    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
> >-} VirtAcpiCpuInfo;
> >-
> >-static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> >-{
> >-    CPUState *cpu;
> >-
> >-    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> >-    CPU_FOREACH(cpu) {
> >-        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> >-    }
> >-}
> >-
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >  {
> >      uint16_t i;
> >@@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
> >
> >  /* MADT */
> >  static void
> >-build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> >-           VirtAcpiCpuInfo *cpuinfo)
> >+build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >  {
> >      int madt_start = table_data->len;
> >      const MemMapEntry *memmap = guest_info->memmap;
> >@@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> >          gicc->cpu_interface_number = i;
> >          gicc->arm_mpidr = armcpu->mp_affinity;
> >          gicc->uid = i;
> >-        if (test_bit(i, cpuinfo->found_cpus)) {
> >-            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >-        }
> >+        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >      }
> Ah, yes, it uses smp_cpus not max_cpus. But we still needs to support
> max_cpus usage even though it doesn't support vcpu hotplug currently. So we
> may need to introduce guest_info->max_cpus and use it here.

We should leave that for when the hotplug patches come, and we should
probably leave the hotplug patches until we see what Igor plans for
sharing more ACPI code between x86 and ARM.

> And below check in virt.c is not right while it should compare the global
> max_cpus with the max_cpus GIC supports.
> 
>     if (smp_cpus > max_cpus) {
>         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
>                      "supported by machine 'mach-virt' (%d)",
>                      smp_cpus, max_cpus);
>         exit(1);
>     }

max_cpus is getting set to the number the gic supports just above this
check. So max_cpus == gic_supported_cpus already, and this check is just
confirming the number of cpus the user has selected is OK.

drew

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Shannon Zhao]

On 2016/1/29 23:26, Andrew Jones wrote:
> On Fri, Jan 29, 2016 at 10:59:32PM +0800, Shannon Zhao wrote:
>> >
>> >
>> >On 2016/1/29 22:24, Igor Mammedov wrote:
>>> > >in current impl. condition
>>> > >
>>> > >build_madt() {
>>> > >   ...
>>> > >   if (test_bit(i, cpuinfo->found_cpus))
>>> > >
>>> > >is always true since loop handles only present CPUs
>>> > >in range [0..smp_cpus).
>>> > >But to fill usless cpuinfo->found_cpus we do unnecessary
>>> > >scan over QOM tree to find the same CPUs.
>>> > >So mark GICC as present always and drop not needed
>>> > >code that fills cpuinfo->found_cpus.
>>> > >
>>> > >Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>> > >---
>>> > >It's just simple cleanup but I'm trying to generalize
>>> > >a bit CPU related ACPI tables and as part of it get rid
>>> > >of found_cpus bitmap and if possible cpu_index usage
>>> > >in ACPI parts of code.
>>> > >---
>>> > >  hw/arm/virt-acpi-build.c | 26 +++-----------------------
>>> > >  1 file changed, 3 insertions(+), 23 deletions(-)
>>> > >
>>> > >diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> > >index 87fbe7c..3ed39fc 100644
>>> > >--- a/hw/arm/virt-acpi-build.c
>>> > >+++ b/hw/arm/virt-acpi-build.c
>>> > >@@ -46,20 +46,6 @@
>>> > >  #define ARM_SPI_BASE 32
>>> > >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>> > >
>>> > >-typedef struct VirtAcpiCpuInfo {
>>> > >-    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
>>> > >-} VirtAcpiCpuInfo;
>>> > >-
>>> > >-static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
>>> > >-{
>>> > >-    CPUState *cpu;
>>> > >-
>>> > >-    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
>>> > >-    CPU_FOREACH(cpu) {
>>> > >-        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
>>> > >-    }
>>> > >-}
>>> > >-
>>> > >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>> > >  {
>>> > >      uint16_t i;
>>> > >@@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
>>> > >
>>> > >  /* MADT */
>>> > >  static void
>>> > >-build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>>> > >-           VirtAcpiCpuInfo *cpuinfo)
>>> > >+build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>>> > >  {
>>> > >      int madt_start = table_data->len;
>>> > >      const MemMapEntry *memmap = guest_info->memmap;
>>> > >@@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>>> > >          gicc->cpu_interface_number = i;
>>> > >          gicc->arm_mpidr = armcpu->mp_affinity;
>>> > >          gicc->uid = i;
>>> > >-        if (test_bit(i, cpuinfo->found_cpus)) {
>>> > >-            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>>> > >-        }
>>> > >+        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>>> > >      }
>> >Ah, yes, it uses smp_cpus not max_cpus. But we still needs to support
>> >max_cpus usage even though it doesn't support vcpu hotplug currently. So we
>> >may need to introduce guest_info->max_cpus and use it here.
> We should leave that for when the hotplug patches come, and we should
> probably leave the hotplug patches until we see what Igor plans for
> sharing more ACPI code between x86 and ARM.
>
Even if ignoring the vcpu hotplug, we still need to support max_cpus and 
smp_cpus usage like -smp 1,maxcpus=4.

>> >And below check in virt.c is not right while it should compare the global
>> >max_cpus with the max_cpus GIC supports.
>> >
>> >     if (smp_cpus > max_cpus) {
>> >         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
>> >                      "supported by machine 'mach-virt' (%d)",
>> >                      smp_cpus, max_cpus);
>> >         exit(1);
>> >     }
> max_cpus is getting set to the number the gic supports just above this
> check. So max_cpus == gic_supported_cpus already, and this check is just
> confirming the number of cpus the user has selected is OK.
No, the global max_cpus (which is defined in vl.c and exported in 
sysemu/sysemu.h) is not the local variable max_cpus.

-- 
Shannon

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Andrew Jones]

On Fri, Jan 29, 2016 at 11:44:24PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/1/29 23:26, Andrew Jones wrote:
> >On Fri, Jan 29, 2016 at 10:59:32PM +0800, Shannon Zhao wrote:
> >>>
> >>>
> >>>On 2016/1/29 22:24, Igor Mammedov wrote:
> >>>> >in current impl. condition
> >>>> >
> >>>> >build_madt() {
> >>>> >   ...
> >>>> >   if (test_bit(i, cpuinfo->found_cpus))
> >>>> >
> >>>> >is always true since loop handles only present CPUs
> >>>> >in range [0..smp_cpus).
> >>>> >But to fill usless cpuinfo->found_cpus we do unnecessary
> >>>> >scan over QOM tree to find the same CPUs.
> >>>> >So mark GICC as present always and drop not needed
> >>>> >code that fills cpuinfo->found_cpus.
> >>>> >
> >>>> >Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>> >---
> >>>> >It's just simple cleanup but I'm trying to generalize
> >>>> >a bit CPU related ACPI tables and as part of it get rid
> >>>> >of found_cpus bitmap and if possible cpu_index usage
> >>>> >in ACPI parts of code.
> >>>> >---
> >>>> >  hw/arm/virt-acpi-build.c | 26 +++-----------------------
> >>>> >  1 file changed, 3 insertions(+), 23 deletions(-)
> >>>> >
> >>>> >diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>>> >index 87fbe7c..3ed39fc 100644
> >>>> >--- a/hw/arm/virt-acpi-build.c
> >>>> >+++ b/hw/arm/virt-acpi-build.c
> >>>> >@@ -46,20 +46,6 @@
> >>>> >  #define ARM_SPI_BASE 32
> >>>> >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >>>> >
> >>>> >-typedef struct VirtAcpiCpuInfo {
> >>>> >-    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
> >>>> >-} VirtAcpiCpuInfo;
> >>>> >-
> >>>> >-static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> >>>> >-{
> >>>> >-    CPUState *cpu;
> >>>> >-
> >>>> >-    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> >>>> >-    CPU_FOREACH(cpu) {
> >>>> >-        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> >>>> >-    }
> >>>> >-}
> >>>> >-
> >>>> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >>>> >  {
> >>>> >      uint16_t i;
> >>>> >@@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
> >>>> >
> >>>> >  /* MADT */
> >>>> >  static void
> >>>> >-build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> >>>> >-           VirtAcpiCpuInfo *cpuinfo)
> >>>> >+build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >>>> >  {
> >>>> >      int madt_start = table_data->len;
> >>>> >      const MemMapEntry *memmap = guest_info->memmap;
> >>>> >@@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> >>>> >          gicc->cpu_interface_number = i;
> >>>> >          gicc->arm_mpidr = armcpu->mp_affinity;
> >>>> >          gicc->uid = i;
> >>>> >-        if (test_bit(i, cpuinfo->found_cpus)) {
> >>>> >-            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >>>> >-        }
> >>>> >+        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >>>> >      }
> >>>Ah, yes, it uses smp_cpus not max_cpus. But we still needs to support
> >>>max_cpus usage even though it doesn't support vcpu hotplug currently. So we
> >>>may need to introduce guest_info->max_cpus and use it here.
> >We should leave that for when the hotplug patches come, and we should
> >probably leave the hotplug patches until we see what Igor plans for
> >sharing more ACPI code between x86 and ARM.
> >
> Even if ignoring the vcpu hotplug, we still need to support max_cpus and
> smp_cpus usage like -smp 1,maxcpus=4.

OK, without hotplug, max > smp doesn't gain anything, max < smp results
in an error, and therefore the only useful case is max == smp.

> 
> >>>And below check in virt.c is not right while it should compare the global
> >>>max_cpus with the max_cpus GIC supports.
> >>>
> >>>     if (smp_cpus > max_cpus) {
> >>>         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> >>>                      "supported by machine 'mach-virt' (%d)",
> >>>                      smp_cpus, max_cpus);
> >>>         exit(1);
> >>>     }
> >max_cpus is getting set to the number the gic supports just above this
> >check. So max_cpus == gic_supported_cpus already, and this check is just
> >confirming the number of cpus the user has selected is OK.
> No, the global max_cpus (which is defined in vl.c and exported in
> sysemu/sysemu.h) is not the local variable max_cpus.

I now see what you mean though. If we don't want something like
-smp 1,maxcpus=9 to erroneously succeed on a gicv2 machine, then we
should be checking the global max_cpus here. I agree it should be
fixed, because, even though it changes nothing atm, we don't want to
allow invalid command lines.

Will you send the patch?

Thanks,
drew

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Igor Mammedov]

On Fri, 29 Jan 2016 22:59:32 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> On 2016/1/29 22:24, Igor Mammedov wrote:
> > in current impl. condition
> >
> > build_madt() {
> >    ...
> >    if (test_bit(i, cpuinfo->found_cpus))
> >
> > is always true since loop handles only present CPUs
> > in range [0..smp_cpus).
> > But to fill usless cpuinfo->found_cpus we do unnecessary
> > scan over QOM tree to find the same CPUs.
> > So mark GICC as present always and drop not needed
> > code that fills cpuinfo->found_cpus.
> >
> > Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> > ---
> > It's just simple cleanup but I'm trying to generalize
> > a bit CPU related ACPI tables and as part of it get rid
> > of found_cpus bitmap and if possible cpu_index usage
> > in ACPI parts of code.
> > ---
> >   hw/arm/virt-acpi-build.c | 26 +++-----------------------
> >   1 file changed, 3 insertions(+), 23 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 87fbe7c..3ed39fc 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -46,20 +46,6 @@
> >   #define ARM_SPI_BASE 32
> >   #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >
> > -typedef struct VirtAcpiCpuInfo {
> > -    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
> > -} VirtAcpiCpuInfo;
> > -
> > -static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> > -{
> > -    CPUState *cpu;
> > -
> > -    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> > -    CPU_FOREACH(cpu) {
> > -        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> > -    }
> > -}
> > -
> >   static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >   {
> >       uint16_t i;
> > @@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
> >
> >   /* MADT */
> >   static void
> > -build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> > -           VirtAcpiCpuInfo *cpuinfo)
> > +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >   {
> >       int madt_start = table_data->len;
> >       const MemMapEntry *memmap = guest_info->memmap;
> > @@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> >           gicc->cpu_interface_number = i;
> >           gicc->arm_mpidr = armcpu->mp_affinity;
> >           gicc->uid = i;
> > -        if (test_bit(i, cpuinfo->found_cpus)) {
> > -            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> > -        }
> > +        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >       }  
> Ah, yes, it uses smp_cpus not max_cpus. But we still needs to support 
> max_cpus usage even though it doesn't support vcpu hotplug currently. So
> we may need to introduce guest_info->max_cpus and use it here.

We should support max_cpus but only when hotplug is supported.
Problem with hotplug is that currently it's assumed that
cpu_index is in range [0..max_cpus) and that works for now but
with a large number of CPUs that won't scale, that's a problem
we are facing now in x86.
I'm trying to re-factor CPU related ACPI parts to use CPU id
used in hardware (APIC ID for x86) and while it make it reusable
for ARM as well where as such ID we could use 'mpidr'.

So this clean up beside of removing not needed code also
reduces ACPI dependency on cpu_index.

> And below check in virt.c is not right while it should compare the 
> global max_cpus with the max_cpus GIC supports.
> 
>      if (smp_cpus > max_cpus) {
>          error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
>                       "supported by machine 'mach-virt' (%d)",
>                       smp_cpus, max_cpus);
>          exit(1);
>      }
> 
> Thanks,

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Shannon Zhao]

On 2016/1/29 22:24, Igor Mammedov wrote:
> in current impl. condition
> 
> build_madt() {
>   ...
>   if (test_bit(i, cpuinfo->found_cpus))
> 
> is always true since loop handles only present CPUs
> in range [0..smp_cpus).
> But to fill usless cpuinfo->found_cpus we do unnecessary
> scan over QOM tree to find the same CPUs.
> So mark GICC as present always and drop not needed
> code that fills cpuinfo->found_cpus.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> It's just simple cleanup but I'm trying to generalize
> a bit CPU related ACPI tables and as part of it get rid
> of found_cpus bitmap and if possible cpu_index usage
> in ACPI parts of code.
> ---
>  hw/arm/virt-acpi-build.c | 26 +++-----------------------
>  1 file changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 87fbe7c..3ed39fc 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -46,20 +46,6 @@
>  #define ARM_SPI_BASE 32
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>  
> -typedef struct VirtAcpiCpuInfo {
> -    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
The definition of VIRT_ACPI_CPU_ID_LIMIT should be removed as well.

Otherwise:

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> -} VirtAcpiCpuInfo;
> -
> -static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> -{
> -    CPUState *cpu;
> -
> -    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> -    CPU_FOREACH(cpu) {
> -        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> -    }
> -}
> -
>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>  {
>      uint16_t i;
> @@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
>  
>  /* MADT */
>  static void
> -build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> -           VirtAcpiCpuInfo *cpuinfo)
> +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      int madt_start = table_data->len;
>      const MemMapEntry *memmap = guest_info->memmap;
> @@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>          gicc->cpu_interface_number = i;
>          gicc->arm_mpidr = armcpu->mp_affinity;
>          gicc->uid = i;
> -        if (test_bit(i, cpuinfo->found_cpus)) {
> -            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> -        }
> +        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>      }
>  
>      if (guest_info->gic_version == 3) {
> @@ -599,11 +582,8 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>  {
>      GArray *table_offsets;
>      unsigned dsdt, rsdt;
> -    VirtAcpiCpuInfo cpuinfo;
>      GArray *tables_blob = tables->table_data;
>  
> -    virt_acpi_get_cpu_info(&cpuinfo);
> -
>      table_offsets = g_array_new(false, true /* clear */,
>                                          sizeof(uint32_t));
>  
> @@ -630,7 +610,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>      build_fadt(tables_blob, tables->linker, dsdt);
>  
>      acpi_add_table(table_offsets, tables_blob);
> -    build_madt(tables_blob, tables->linker, guest_info, &cpuinfo);
> +    build_madt(tables_blob, tables->linker, guest_info);
>  
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker);

-- 
Shannon

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Igor Mammedov]

On Sat, 30 Jan 2016 09:50:30 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> On 2016/1/29 22:24, Igor Mammedov wrote:
> > in current impl. condition
> > 
> > build_madt() {
> >   ...
> >   if (test_bit(i, cpuinfo->found_cpus))
> > 
> > is always true since loop handles only present CPUs
> > in range [0..smp_cpus).
> > But to fill usless cpuinfo->found_cpus we do unnecessary
> > scan over QOM tree to find the same CPUs.
> > So mark GICC as present always and drop not needed
> > code that fills cpuinfo->found_cpus.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > It's just simple cleanup but I'm trying to generalize
> > a bit CPU related ACPI tables and as part of it get rid
> > of found_cpus bitmap and if possible cpu_index usage
> > in ACPI parts of code.
> > ---
> >  hw/arm/virt-acpi-build.c | 26 +++-----------------------
> >  1 file changed, 3 insertions(+), 23 deletions(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 87fbe7c..3ed39fc 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -46,20 +46,6 @@
> >  #define ARM_SPI_BASE 32
> >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >  
> > -typedef struct VirtAcpiCpuInfo {
> > -    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);  
> The definition of VIRT_ACPI_CPU_ID_LIMIT should be removed as well.
Ok, I'll send v2 with fixup.

> 
> Otherwise:
> 
> Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>
> 
> > -} VirtAcpiCpuInfo;
> > -
> > -static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
> > -{
> > -    CPUState *cpu;
> > -
> > -    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
> > -    CPU_FOREACH(cpu) {
> > -        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
> > -    }
> > -}
> > -
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >  {
> >      uint16_t i;
> > @@ -458,8 +444,7 @@ build_gtdt(GArray *table_data, GArray *linker)
> >  
> >  /* MADT */
> >  static void
> > -build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> > -           VirtAcpiCpuInfo *cpuinfo)
> > +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >  {
> >      int madt_start = table_data->len;
> >      const MemMapEntry *memmap = guest_info->memmap;
> > @@ -489,9 +474,7 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
> >          gicc->cpu_interface_number = i;
> >          gicc->arm_mpidr = armcpu->mp_affinity;
> >          gicc->uid = i;
> > -        if (test_bit(i, cpuinfo->found_cpus)) {
> > -            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> > -        }
> > +        gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> >      }
> >  
> >      if (guest_info->gic_version == 3) {
> > @@ -599,11 +582,8 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> >  {
> >      GArray *table_offsets;
> >      unsigned dsdt, rsdt;
> > -    VirtAcpiCpuInfo cpuinfo;
> >      GArray *tables_blob = tables->table_data;
> >  
> > -    virt_acpi_get_cpu_info(&cpuinfo);
> > -
> >      table_offsets = g_array_new(false, true /* clear */,
> >                                          sizeof(uint32_t));
> >  
> > @@ -630,7 +610,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> >      build_fadt(tables_blob, tables->linker, dsdt);
> >  
> >      acpi_add_table(table_offsets, tables_blob);
> > -    build_madt(tables_blob, tables->linker, guest_info, &cpuinfo);
> > +    build_madt(tables_blob, tables->linker, guest_info);
> >  
> >      acpi_add_table(table_offsets, tables_blob);
> >      build_gtdt(tables_blob, tables->linker);  
> 

Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally

[Andrew Jones]

On Fri, Jan 29, 2016 at 05:07:31PM +0100, Andrew Jones wrote:
> On Fri, Jan 29, 2016 at 11:44:24PM +0800, Shannon Zhao wrote:
> > >>>And below check in virt.c is not right while it should compare the global
> > >>>max_cpus with the max_cpus GIC supports.
> > >>>
> > >>>     if (smp_cpus > max_cpus) {
> > >>>         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> > >>>                      "supported by machine 'mach-virt' (%d)",
> > >>>                      smp_cpus, max_cpus);
> > >>>         exit(1);
> > >>>     }
> > >max_cpus is getting set to the number the gic supports just above this
> > >check. So max_cpus == gic_supported_cpus already, and this check is just
> > >confirming the number of cpus the user has selected is OK.
> > No, the global max_cpus (which is defined in vl.c and exported in
> > sysemu/sysemu.h) is not the local variable max_cpus.
> 
> I now see what you mean though. If we don't want something like
> -smp 1,maxcpus=9 to erroneously succeed on a gicv2 machine, then we
> should be checking the global max_cpus here. I agree it should be
> fixed, because, even though it changes nothing atm, we don't want to
> allow invalid command lines.
> 
> Will you send the patch?

I'll send one in a second.

drew