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 <ehabk...@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 <ler...@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
