Re: [Qemu-devel] [PATCH v2 4/4] pc: Refuse max_cpus if it results in too large APIC ID

[Laszlo Ersek <lersek@redhat.com>]

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