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

[Shannon Zhao <shannon.zhao@linaro.org>]

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