From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNrJV-0003Ce-AH for qemu-devel@nongnu.org; Wed, 12 Mar 2014 18:07:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNrJP-0001fb-6D for qemu-devel@nongnu.org; Wed, 12 Mar 2014 18:07:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNrJO-0001fU-UL for qemu-devel@nongnu.org; Wed, 12 Mar 2014 18:07:43 -0400 Message-ID: <5320DAAA.7060307@redhat.com> Date: Wed, 12 Mar 2014 23:07:38 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1394648890-933-1-git-send-email-ehabkost@redhat.com> <1394648890-933-5-git-send-email-ehabkost@redhat.com> In-Reply-To: <1394648890-933-5-git-send-email-ehabkost@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/4] pc: Refuse max_cpus if it results in too large APIC ID List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , qemu-devel@nongnu.org Cc: Igor Mammedov , =?ISO-8859-1?Q?Andreas_F=E4rber?= , "Michael S. Tsirkin" comments below On 03/12/14 19:28, Eduardo Habkost wrote: > This changes the PC initialization code to reject max_cpus if it results > in an APIC ID that's too large, instead of aborting or erroring out when > it is already too late. > > Currently there are two limits we need to check: the CPU hotplug APIC ID > limit (due to the AcpiCpuHotplug.sts array length), and the > MAX_CPUMASK_BITS limit (that's used for CPU bitmaps on NUMA code and > hw/i386/acpi-build.c). > > Signed-off-by: Eduardo Habkost > --- > hw/i386/pc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 74cb4f9..50376a3 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -992,6 +992,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) > int i; > X86CPU *cpu = NULL; > Error *error = NULL; > + unsigned long apic_id_limit; Seems unnecessary, pc_apic_id_limit() returns "unsigned int". > > /* init CPUs */ > if (cpu_model == NULL) { > @@ -1003,6 +1004,14 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) > } > current_cpu_model = cpu_model; > > + apic_id_limit = pc_apic_id_limit(max_cpus); OK, so keep in mind this is an exclusive limit... > + if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT ... hence this comparison is correct, because ACPI_CPU_HOTPLUG_ID_LIMIT is also an exclusive limit. > + || apic_id_limit > MAX_CPUMASK_BITS) { However, this check is off-by-one, because MAX_CPUMASK_BITS is an inclusive limit. It should say (apic_id_limit > MAX_CPUMASK_BITS + 1). (1) See the type definition AcpiCpuInfo in "hw/i386/acpi-build.c": typedef struct AcpiCpuInfo { DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1); } AcpiCpuInfo; (2) The acpi_add_cpu_info() function in the same file: assert(apic_id <= MAX_CPUMASK_BITS); On the other hand: (3) numa_node_parse_cpus(): if (endvalue >= MAX_CPUMASK_BITS) { endvalue = MAX_CPUMASK_BITS - 1; fprintf(stderr, "qemu: NUMA: A max of %d VCPUs are supported\n", MAX_CPUMASK_BITS); } implies an exclusive limit, and: (4) the two uses in main() also imply an exclusive limit. (Although I honestly can't find the definition of the bitmap_new() function!) Since you authored (3) -- in commit c881e20e --, I *think* MAX_CPUMASK_BITS could indeed be exclusive: then your new patch is correct, but (1) and (2) -- which seem to be ports from SeaBIOS -- are "wrong" (as in, "too permissive"). So which is it? ... Aaaah, I understand now! (1) and (2) should actually use ACPI_CPU_HOTPLUG_ID_LIMIT (which you are just introducing). Basically, your patchset is completely unrelated to NUMA, the impression arises *only* because "acpi-build.c" creatively repurposed MAX_CPUMASK_BITS. So here goes: - AcpiCpuInfo is already correct *numerically*: MAX_CPUMASK_BITS == 255 MAX_CPUMASK_BITS + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT but it has nothing to do with NUMA -- it should actually use ACPI_CPU_HOTPLUG_ID_LIMIT. Please include another patch to convert this use. - acpi_add_cpu_info() is already correct *numerically*: assert(apic_id <= MAX_CPUMASK_BITS); assert(apic_id < MAX_CPUMASK_BITS + 1); assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT); but it has nothing to do with NUMA. Please convert this comparison in the same additional patch as the AcpiCpuInfo typedef. - numa_node_parse_cpus() is correct (MAX_CPUMASK_BITS is exclusive). No need to care about it in this patchset though -- it's NUMA. - The two "node_cpumask" uses in main() are correct (MAX_CPUMASK_BITS is exclusive). No need to care about them either in this patchset. As a result, *this* patch of yours won't mention MAX_CPUMASK_BITS at all. A single (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) check will suffice, because ACPI code will use ACPI_CPU_HOTPLUG_ID_LIMIT exclusively and uniformly, and NUMA code will use the completely independent MAX_CPUMASK_BITS. (numa_node_parse_cpus(), which is the function that sets bit segments in the node masks, doesn't care about APIC IDs at all.) > + error_report("max_cpus is too large. APIC ID of last CPU is %lu", > + apic_id_limit - 1); > + exit(1); > + } > + If you update the type of "apic_id_limit" to "unsigned int" (I'm not saying that you should), don't forget to update the conversion specifier here. > for (i = 0; i < smp_cpus; i++) { > cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), > icc_bridge, &error); > Thanks! Laszlo