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<imamm...@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