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