| Message ID | 1394648890-933-5-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <ehabkost@redhat.com> > --- > 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
On Wed, Mar 12, 2014 at 11:07:38PM +0100, Laszlo Ersek wrote: > 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 <ehabkost@redhat.com> > > --- > > 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. I'll do. Thanks for the thorough analysis! > > - 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. I was not trying to address ACPI-specific or NUMA-specific stuff, but all the potential crashes/corruption bugs I found because the code had the implicit (and incorrect) assumption that apic_id <= max_cpus for all CPUs. > > - The two "node_cpumask" uses in main() are correct (MAX_CPUMASK_BITS is > exclusive). No need to care about them either in this patchset. > The MAX_CPUMASK_BITS bits check was added not because of any NUMA code in vl.c, but because of pc_guest_info_init(), which reads node_cpumask. ...but, wait! The index for node_cpumask is the CPU index, not the APIC ID! You are right and this patch shouldn't do anything about MAX_CPUMASK_BITS. I was being overzealous. > 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.) You are correct. I will submit a new version including your suggestions. Thanks!
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; /* 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); + if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT + || apic_id_limit > MAX_CPUMASK_BITS) { + error_report("max_cpus is too large. APIC ID of last CPU is %lu", + apic_id_limit - 1); + exit(1); + } + for (i = 0; i < smp_cpus; i++) { cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), icc_bridge, &error);
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 <ehabkost@redhat.com> --- hw/i386/pc.c | 9 +++++++++ 1 file changed, 9 insertions(+)