qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH v2 4/4] pc: Refuse max_cpus if it results in too large APIC ID
Date: Wed, 12 Mar 2014 21:34:36 -0300
User-agent: Mutt/1.5.21 (2010-09-15)

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 <address@hidden>
> > ---
> >  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]