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

[Laszlo Ersek <lersek@redhat.com>]

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