From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwRnd-0003oT-I3 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 06:39:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwRnZ-0007Xo-K8 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 06:39:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47662) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwRnZ-0007Xa-C9 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 06:39:09 -0400 Date: Tue, 18 Oct 2016 08:39:05 -0200 From: Eduardo Habkost Message-ID: <20161018103905.GH3275@thinpad.lan.raisama.net> References: <1476352367-69400-1-git-send-email-imammedo@redhat.com> <1476352367-69400-6-git-send-email-imammedo@redhat.com> <20161017214452.GE3275@thinpad.lan.raisama.net> <20161018111204.74bc8827@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161018111204.74bc8827@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, kraxel@redhat.com, liuxiaojian6@huawei.com, mst@redhat.com, rkrcmar@redhat.com, peterx@redhat.com, kevin@koconnor.net, pbonzini@redhat.com, lersek@redhat.com, chao.gao@intel.com On Tue, Oct 18, 2016 at 11:12:04AM +0200, Igor Mammedov wrote: > On Mon, 17 Oct 2016 19:44:52 -0200 > Eduardo Habkost wrote: > > > On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote: > > [...] > > > @@ -236,7 +237,11 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine, > > > /* The current AML generator can cover the APIC ID range [0..255], > > > * inclusive, for VCPU hotplug. */ > > > QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256); > > > - g_assert(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT); > > > + if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) { > > > + error_report("max_cpus is too large. APIC ID of last CPU is %u", > > > + pcms->apic_id_limit - 1); > > > + exit(1); > > > + } > > > > Moving the check here seems to make sense, but: > > > > > > > > /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */ > > > dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE)); > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 93ff49c..f1c1013 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms) > > > > [Added more context below to show the code around the change] > > > > > numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes); > > > numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes); > > > for (i = 0; i < max_cpus; i++) { > > > unsigned int apic_id = x86_cpu_apic_id_from_index(i); > > > - assert(apic_id < pcms->apic_id_limit); > > > > If you really needed to remove this assert, that means you can > > write beyond the end of numa_fw_fg[] below. Are you sure you need > > to remove it? > > > > > j = numa_get_node_for_cpu(i); > > > if (j < nb_numa_nodes) { > > > numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > > > > ^^^^^^^^^^^ here > > > > > } > Another more radical way to deal with legacy FW_CFG_NUMA > could be to remove it altogether if machine has x2APIC cpus. > It'd be possible to do due to BIOS using QEMU provided > BIOS tables and not using/calling code for built in/legacy > ACPI tables. > > Could do it as patch on top if that's sounds ok. I don't mind either way. Initializing the fields even if they are not going to be used seems harmless, but if you believe skipping it would make things simpler, go ahead. In this case, what would happen if x2apic CPUs are available but the BIOS is too old? -- Eduardo