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

[Andrew Jones <drjones@redhat.com>]

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