Message ID | 1394823137-4369-5-git-send-email-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
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>
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
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); }
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(-)