[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/7] acpi: Don't use MAX_CPUMASK_BITS for API
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v3 4/7] acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap |
Date: |
Fri, 14 Mar 2014 20:56:43 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
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 <address@hidden>
>> ---
>> 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 <address@hidden>
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
- [Qemu-devel] [PATCH v3 0/7] pc: Ensure APIC ID limits before aborting or corrupting memory, Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 1/7] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro, Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 3/7] acpi: Assert sts array limit on AcpiCpuHotplug_add(), Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 2/7] pc: Refuse CPU hotplug if the resulting APIC ID is too large, Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 4/7] acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap, Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 6/7] vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS, Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 5/7] pc: Refuse max_cpus if it results in too large APIC ID, Eduardo Habkost, 2014/03/14
- [Qemu-devel] [PATCH v3 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant, Eduardo Habkost, 2014/03/14