[Qemu-devel] [PATCH v3 0/7] pc: Ensure APIC ID limits before aborting or corrupting memory

[Qemu-devel] [PATCH v3 0/7] pc: Ensure APIC ID limits before aborting or corrupting memory

[Eduardo Habkost]

Changes v2 -> v3:
 * Don't use MAX_CPUMASK_BITS on acpi-build.c, use ACPI_CPU_HOTPLUG_ID_LIMIT;
 * Rename MAX_CPUMASK_BITS to MAX_CPUS, and document it;
 * Use MAX_CPUS when checking max_cpus limit on vl.c.

Changes v1 -> v2:
 * None. v1 was tagged locally by mistake and never submitted to qemu-devel.

This series adds checks for APIC ID limits on initialization and CPU hotplug
code.  This fixes multiple issues:

1) Assertion failure when -smp parameter results in a too large APIC ID. e.g.:

    $ ./install/bin/qemu-system-x86_64 -S -smp 254,cores=17,threads=17,sockets=17,maxcpus=254 -nographic
    **
    ERROR:hw/acpi/cpu_hotplug.c:58:AcpiCpuHotplug_init: assertion failed: ((id / 8) < ACPI_GPE_PROC_LEN)
    Aborted (core dumped)

2) Memory corruption on AcpiCpuHotplug_add() when APIC ID is too large (similar
   to the case above, but on CPU hotplug).

Eduardo Habkost (7):
  acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro
  pc: Refuse CPU hotplug if the resulting APIC ID is too large
  acpi: Assert sts array limit on AcpiCpuHotplug_add()
  acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap
  pc: Refuse max_cpus if it results in too large APIC ID
  vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS
  vl.c: Use MAX_CPUS macro instead of hardcoded constant

 hw/acpi/cpu_hotplug.c              |  1 +
 hw/i386/acpi-build.c               |  4 ++--
 hw/i386/pc.c                       | 16 ++++++++++++++++
 include/hw/acpi/cpu_hotplug_defs.h |  8 ++++++++
 include/sysemu/sysemu.h            |  9 ++++++++-
 vl.c                               | 12 ++++++------
 6 files changed, 41 insertions(+), 9 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v3 1/7] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro

[Eduardo Habkost]

The new macro will be helpful to allow us to detect too large SMP limits
before it is too late.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/acpi/cpu_hotplug_defs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/hw/acpi/cpu_hotplug_defs.h b/include/hw/acpi/cpu_hotplug_defs.h
index 2725b50..9f33663 100644
--- a/include/hw/acpi/cpu_hotplug_defs.h
+++ b/include/hw/acpi/cpu_hotplug_defs.h
@@ -17,7 +17,15 @@
  * between C and ASL code.
  */
 #define ACPI_CPU_HOTPLUG_STATUS 4
+
+/* Limit for CPU arch IDs for CPU hotplug. All hotpluggable CPUs should
+ * have CPUClass.get_arch_id() < ACPI_CPU_HOTPLUG_ID_LIMIT.
+ */
+#define ACPI_CPU_HOTPLUG_ID_LIMIT 256
+
+/* 256 CPU IDs, 8 bits per entry: */
 #define ACPI_GPE_PROC_LEN 32
+
 #define ICH9_CPU_HOTPLUG_IO_BASE 0x0CD8
 #define PIIX4_CPU_HOTPLUG_IO_BASE 0xaf00
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 2/7] pc: Refuse CPU hotplug if the resulting APIC ID is too large

[Eduardo Habkost]

The ACPI CPU hotplug code requires APIC IDs to be smaller than
ACPI_CPU_HOTPLUG_ID_LIMIT, so enforce the limit before trying to hotplug
a new vCPU, returning an error instead of crashing.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..74cb4f9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -53,6 +53,7 @@
 #include "qemu/bitmap.h"
 #include "qemu/config-file.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/cpu_hotplug.h"
 #include "hw/cpu/icc_bus.h"
 #include "hw/boards.h"
 #include "hw/pci/pci_host.h"
@@ -974,6 +975,13 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
+    if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", resulting APIC ID (%" PRIi64 ") is too large",
+                   id, apic_id);
+        return;
+    }
+
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
     pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 3/7] acpi: Assert sts array limit on AcpiCpuHotplug_add()

[Eduardo Habkost]

AcpiCpuHotplug_add() can't handle vCPU arch IDs larger than
ACPI_CPU_HOTPLUG_ID_LIMIT. Instead of corrupting memory in case the vCPU
ID is too large, use g_assert() to ensure we are not over the limit.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/acpi/cpu_hotplug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 48928dc..2ad83a0 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -43,6 +43,7 @@ void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu)
 
     *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
     cpu_id = k->get_arch_id(CPU(cpu));
+    g_assert((cpu_id / 8) < ACPI_GPE_PROC_LEN);
     g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
 }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 4/7] acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap

[Eduardo Habkost]

MAX_CPUMASK_BITS is a limit for max_cpus and CPU indexes, not for APIC
IDs.

ACPI_CPU_HOTPLUG_ID_LIMIT is the right macro for the limit on APIC IDs
on the ACPI and CPU hotplug code.

There are no functional changes introduced by this patch, as
MAX_CPUMASK_BITS + 1 == 255 + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b667d31..749af1e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -52,7 +52,7 @@
 #include "qom/qom-qobject.h"
 
 typedef struct AcpiCpuInfo {
-    DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1);
+    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
 } AcpiCpuInfo;
 
 typedef struct AcpiMcfgInfo {
@@ -117,7 +117,7 @@ int acpi_add_cpu_info(Object *o, void *opaque)
 
     if (object_dynamic_cast(o, TYPE_CPU)) {
         apic_id = object_property_get_int(o, "apic-id", NULL);
-        assert(apic_id <= MAX_CPUMASK_BITS);
+        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
 
         set_bit(apic_id, cpu->found_cpus);
     }
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 5/7] pc: Refuse max_cpus if it results in too large APIC ID

[Eduardo Habkost]

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>
---
Changes v2 -> v3:
 * No need to check against MAX_CPUMASK_BITS, as MAX_CPUMASK_BITS
   is used only for CPU-index-based bitmaps on NUMA code, not for APIC
   IDs.
---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 74cb4f9..14f0d91 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,13 @@ 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) {
+        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);
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 6/7] vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS

[Eduardo Habkost]

Also, document what the macro is really useful for.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/sysemu/sysemu.h |  9 ++++++++-
 vl.c                    | 10 +++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b90df9a..a73b226 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -133,7 +133,14 @@ extern uint8_t qemu_extra_params_fw[2];
 extern QEMUClockType rtc_clock;
 
 #define MAX_NODES 64
-#define MAX_CPUMASK_BITS 255
+
+/* The following should be true for all CPUs:
+ *   cpu->cpu_index < max_cpus <= MAX_CPUS
+ *
+ * Note that cpu->get_arch_id() may be larger than MAX_CPUS.
+ */
+#define MAX_CPUS 255
+
 extern int nb_numa_nodes;
 extern uint64_t node_mem[MAX_NODES];
 extern unsigned long *node_cpumask[MAX_NODES];
diff --git a/vl.c b/vl.c
index bca5c95..64b38a5 100644
--- a/vl.c
+++ b/vl.c
@@ -1278,11 +1278,11 @@ static void numa_node_parse_cpus(int nodenr, const char *cpus)
         goto error;
     }
 
-    if (endvalue >= MAX_CPUMASK_BITS) {
-        endvalue = MAX_CPUMASK_BITS - 1;
+    if (endvalue >= MAX_CPUS) {
+        endvalue = MAX_CPUS - 1;
         fprintf(stderr,
             "qemu: NUMA: A max of %d VCPUs are supported\n",
-             MAX_CPUMASK_BITS);
+             MAX_CPUS);
     }
 
     if (endvalue < value) {
@@ -2954,7 +2954,7 @@ int main(int argc, char **argv, char **envp)
 
     for (i = 0; i < MAX_NODES; i++) {
         node_mem[i] = 0;
-        node_cpumask[i] = bitmap_new(MAX_CPUMASK_BITS);
+        node_cpumask[i] = bitmap_new(MAX_CPUS);
     }
 
     nb_numa_nodes = 0;
@@ -4245,7 +4245,7 @@ int main(int argc, char **argv, char **envp)
         }
 
         for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(node_cpumask[i], MAX_CPUMASK_BITS)) {
+            if (!bitmap_empty(node_cpumask[i], MAX_CPUS)) {
                 break;
             }
         }
-- 
1.8.5.3

[Qemu-devel] [PATCH v3 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 64b38a5..62cc734 100644
--- a/vl.c
+++ b/vl.c
@@ -1413,7 +1413,7 @@ static void smp_parse(QemuOpts *opts)
         max_cpus = smp_cpus;
     }
 
-    if (max_cpus > 255) {
+    if (max_cpus > MAX_CPUS) {
         fprintf(stderr, "Unsupported number of maxcpus\n");
         exit(1);
     }
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v3 4/7] acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap

[Laszlo Ersek]

On 03/14/14 19:52, Eduardo Habkost wrote:
> MAX_CPUMASK_BITS is a limit for max_cpus and CPU indexes, not for APIC
> IDs.
> 
> ACPI_CPU_HOTPLUG_ID_LIMIT is the right macro for the limit on APIC IDs
> on the ACPI and CPU hotplug code.
> 
> There are no functional changes introduced by this patch, as
> MAX_CPUMASK_BITS + 1 == 255 + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/acpi-build.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b667d31..749af1e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -52,7 +52,7 @@
>  #include "qom/qom-qobject.h"
>  
>  typedef struct AcpiCpuInfo {
> -    DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1);
> +    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>  } AcpiCpuInfo;
>  
>  typedef struct AcpiMcfgInfo {
> @@ -117,7 +117,7 @@ int acpi_add_cpu_info(Object *o, void *opaque)
>  
>      if (object_dynamic_cast(o, TYPE_CPU)) {
>          apic_id = object_property_get_int(o, "apic-id", NULL);
> -        assert(apic_id <= MAX_CPUMASK_BITS);
> +        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
>  
>          set_bit(apic_id, cpu->found_cpus);
>      }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Laszlo Ersek]

On 03/14/14 19:52, 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>
> ---
> Changes v2 -> v3:
>  * No need to check against MAX_CPUMASK_BITS, as MAX_CPUMASK_BITS
>    is used only for CPU-index-based bitmaps on NUMA code, not for APIC
>    IDs.
> ---
>  hw/i386/pc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 74cb4f9..14f0d91 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,13 @@ 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) {
> +        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 patch is now good, but you've forgotten to update the commit
message -- the v2->v3 change block (that you've put under the ---
separator) actually contradicts the commit message.

Please repost the patch with the updated commit message -- I think it
suffices to simply drop the second paragraph! --, and feel free to add
my R-b in that v4 posting.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v3 6/7] vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS

[Laszlo Ersek]

one important nit :)

On 03/14/14 19:52, Eduardo Habkost wrote:
> Also, document what the macro is really useful for.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  include/sysemu/sysemu.h |  9 ++++++++-
>  vl.c                    | 10 +++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b90df9a..a73b226 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -133,7 +133,14 @@ extern uint8_t qemu_extra_params_fw[2];
>  extern QEMUClockType rtc_clock;
>  
>  #define MAX_NODES 64
> -#define MAX_CPUMASK_BITS 255
> +
> +/* The following should be true for all CPUs:

Not should, "shall". It's an invariant we enforce, not something we
merely suggest.

You can add my R-b to the v4 posting.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v3 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant

[Laszlo Ersek]

On 03/14/14 19:52, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 64b38a5..62cc734 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1413,7 +1413,7 @@ static void smp_parse(QemuOpts *opts)
>          max_cpus = smp_cpus;
>      }
>  
> -    if (max_cpus > 255) {
> +    if (max_cpus > MAX_CPUS) {
>          fprintf(stderr, "Unsupported number of maxcpus\n");
>          exit(1);
>      }
> 

$ git grep -e '255' --and -e cpus

hw/s390x/s390-virtio-ccw.c:    .max_cpus = 255,
hw/s390x/s390-virtio.c:    .max_cpus = 255,
include/hw/i386/pc.h:    .max_cpus = 255
vl.c:    if (max_cpus > 255) {

I propose to fix those other tree occurrences as well. (I could be
easily wrong of course!, I didn't investigate.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant

[Eduardo Habkost]

On Fri, Mar 14, 2014 at 08:11:50PM +0100, Laszlo Ersek wrote:
> On 03/14/14 19:52, Eduardo Habkost wrote:
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  vl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 64b38a5..62cc734 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1413,7 +1413,7 @@ static void smp_parse(QemuOpts *opts)
> >          max_cpus = smp_cpus;
> >      }
> >  
> > -    if (max_cpus > 255) {
> > +    if (max_cpus > MAX_CPUS) {
> >          fprintf(stderr, "Unsupported number of maxcpus\n");
> >          exit(1);
> >      }
> > 
> 
> $ git grep -e '255' --and -e cpus
> 
> hw/s390x/s390-virtio-ccw.c:    .max_cpus = 255,
> hw/s390x/s390-virtio.c:    .max_cpus = 255,
> include/hw/i386/pc.h:    .max_cpus = 255
> vl.c:    if (max_cpus > 255) {
> 
> I propose to fix those other tree occurrences as well. (I could be
> easily wrong of course!, I didn't investigate.)

We first need to find out the reason behind those machine-specific
limits. Maybe they can be removed (if they are all because of NUMA
and/or ACPI), or maybe they need to use another macro because there are
other limits in machine-specific code.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant

[Laszlo Ersek]

On 03/14/14 20:30, Eduardo Habkost wrote:
> On Fri, Mar 14, 2014 at 08:11:50PM +0100, Laszlo Ersek wrote:
>> On 03/14/14 19:52, Eduardo Habkost wrote:
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  vl.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 64b38a5..62cc734 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -1413,7 +1413,7 @@ static void smp_parse(QemuOpts *opts)
>>>          max_cpus = smp_cpus;
>>>      }
>>>  
>>> -    if (max_cpus > 255) {
>>> +    if (max_cpus > MAX_CPUS) {
>>>          fprintf(stderr, "Unsupported number of maxcpus\n");
>>>          exit(1);
>>>      }
>>>
>>
>> $ git grep -e '255' --and -e cpus
>>
>> hw/s390x/s390-virtio-ccw.c:    .max_cpus = 255,
>> hw/s390x/s390-virtio.c:    .max_cpus = 255,
>> include/hw/i386/pc.h:    .max_cpus = 255
>> vl.c:    if (max_cpus > 255) {
>>
>> I propose to fix those other tree occurrences as well. (I could be
>> easily wrong of course!, I didn't investigate.)
> 
> We first need to find out the reason behind those machine-specific
> limits. Maybe they can be removed (if they are all because of NUMA
> and/or ACPI), or maybe they need to use another macro because there are
> other limits in machine-specific code.

OK. Until then:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH v3 4/7] acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap

[Laszlo Ersek]

On 03/14/14 20:03, Laszlo Ersek wrote:
> On 03/14/14 19:52, Eduardo Habkost wrote:
>> MAX_CPUMASK_BITS is a limit for max_cpus and CPU indexes, not for APIC
>> IDs.
>>
>> ACPI_CPU_HOTPLUG_ID_LIMIT is the right macro for the limit on APIC IDs
>> on the ACPI and CPU hotplug code.
>>
>> There are no functional changes introduced by this patch, as
>> MAX_CPUMASK_BITS + 1 == 255 + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT.
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  hw/i386/acpi-build.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b667d31..749af1e 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -52,7 +52,7 @@
>>  #include "qom/qom-qobject.h"
>>  
>>  typedef struct AcpiCpuInfo {
>> -    DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1);
>> +    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>>  } AcpiCpuInfo;
>>  
>>  typedef struct AcpiMcfgInfo {
>> @@ -117,7 +117,7 @@ int acpi_add_cpu_info(Object *o, void *opaque)
>>  
>>      if (object_dynamic_cast(o, TYPE_CPU)) {
>>          apic_id = object_property_get_int(o, "apic-id", NULL);
>> -        assert(apic_id <= MAX_CPUMASK_BITS);
>> +        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
>>  
>>          set_bit(apic_id, cpu->found_cpus);
>>      }
>>
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I apologize, I repeated the git-grep command with a hex constant as well:

$ git grep -i -e '0xff' --and -e cpus

and that gave me, in this file,

> static void
> build_ssdt(GArray *table_data, GArray *linker,
>            AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
>            PcPciInfo *pci, PcGuestInfo *guest_info)
> {
>     int acpi_cpus = MIN(0xff, guest_info->apic_id_limit);

I wonder if we should update this site as well.

The question is of course what kind of limit this 0xff is. We build CPU
notification methods here. acpi_cpus is used as an exclusive limit in
the loop -- we build [0..254] tops, inclusive.

This is somehow related to the big comment in bochs_bios_init(), added
in commit 1d934e89.

Apparently, we build objects for a contiguous sequence of APIC IDs. I
think we build one object for each bit in the sts array.

... *Except*, that array contains 256 bits, but we build 255 objects
here at maximum. The only reason for that is probably that some
ACPI-building functions require that the *count* fit in one byte as well.

Ideally, I think this logic should be changed like:

    int acpi_cpus;

    g_assert(guest_info->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
    acpi_cpus = guest_info->apic_id_limit;

    /* now the loops can build CP00..CPFF, not just CPFE */

I think there's one spot only that this change would break:

            build_append_byte(package, acpi_cpus); /* NumElements */

The current code basically prevents notification for the hot-plugged CPU
with APIC_ID 255.

But that's a separate patch, for this one my R-b stands.

Thanks,
Laszlo