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 <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.
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!
--
Eduardo
