From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45364) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aPBDU-0007EU-75 for qemu-devel@nongnu.org; Fri, 29 Jan 2016 10:44:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aPBDO-0000he-BP for qemu-devel@nongnu.org; Fri, 29 Jan 2016 10:44:08 -0500 Received: from mail-pf0-x22e.google.com ([2607:f8b0:400e:c00::22e]:36375) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aPBDN-0000hS-Uw for qemu-devel@nongnu.org; Fri, 29 Jan 2016 10:44:02 -0500 Received: by mail-pf0-x22e.google.com with SMTP id n128so43645729pfn.3 for ; Fri, 29 Jan 2016 07:44:01 -0800 (PST) Message-ID: <56AB88D8.1020907@linaro.org> Date: Fri, 29 Jan 2016 23:44:24 +0800 From: Shannon Zhao MIME-Version: 1.0 References: <1454077485-242598-1-git-send-email-imammedo@redhat.com> <56AB7E54.4000608@linaro.org> <20160129152603.GG4340@hawk.localdomain> In-Reply-To: <20160129152603.GG4340@hawk.localdomain> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] arm: virt-acpi: each MADT.GICC entry as enabled unconditionally List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Jones Cc: wei@redhat.com, peter.maydell@linaro.org, mst@redhat.com, qemu-devel@nongnu.org, qemu-arm@nongnu.org, zhaoshenglong@huawei.com, Igor Mammedov 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 >>> > >--- >>> > >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