| Message ID | 1395072317-15679-3-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 17, 2014 at 05:05:17PM +0100, Laszlo Ersek wrote: > Building on the previous patch, raise the maximal count of processor > objects / NTFY branches / CPON elements from 255 to 256. This allows the > VCPU with APIC ID 0xFF to be hotplugged. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> I note that we still have: if (endvalue >= MAX_CPUMASK_BITS) { endvalue = MAX_CPUMASK_BITS - 1; fprintf(stderr, "qemu: NUMA: A max of %d VCPUs are supported\n", MAX_CPUMASK_BITS); } and MAX_CPUMASK_BITS is 255. Seems inconsistent? > --- > hw/i386/acpi-build.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 2bbefb5..c9fe07f 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -999,11 +999,16 @@ 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); > + unsigned acpi_cpus = guest_info->apic_id_limit; > int ssdt_start = table_data->len; > uint8_t *ssdt_ptr; > int i; > > + /* The current AML generator can cover the APIC ID range [0..255], > + * inclusive, for VCPU hotplug. */ > + QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256); > + g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT); > + > /* Copy header and patch values in the S3_ / S4_ / S5_ packages */ > ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml)); > memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml)); > -- > 1.8.3.1
On Tue, Mar 18, 2014 at 04:03:25PM +0200, Michael S. Tsirkin wrote: > On Mon, Mar 17, 2014 at 05:05:17PM +0100, Laszlo Ersek wrote: > > Building on the previous patch, raise the maximal count of processor > > objects / NTFY branches / CPON elements from 255 to 256. This allows the > > VCPU with APIC ID 0xFF to be hotplugged. > > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > > I note that we still have: > if (endvalue >= MAX_CPUMASK_BITS) { > endvalue = MAX_CPUMASK_BITS - 1; > fprintf(stderr, > "qemu: NUMA: A max of %d VCPUs are supported\n", > MAX_CPUMASK_BITS); > } > and MAX_CPUMASK_BITS is 255. > > Seems inconsistent? > MAX_CPUMASK_BITS (now renamed to MAX_CPUS) limits CPU indexes and total CPU count. This patch is about APIC IDs (which may be larger than max_cpus if threads-per-core or cores-per-socket is not a power of 2). (That doesn't mean we can't decide to increase MAX_CPUS later, too.) > > --- > > hw/i386/acpi-build.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 2bbefb5..c9fe07f 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -999,11 +999,16 @@ 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); > > + unsigned acpi_cpus = guest_info->apic_id_limit; > > int ssdt_start = table_data->len; > > uint8_t *ssdt_ptr; > > int i; > > > > + /* The current AML generator can cover the APIC ID range [0..255], > > + * inclusive, for VCPU hotplug. */ > > + QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256); > > + g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT); > > + > > /* Copy header and patch values in the S3_ / S4_ / S5_ packages */ > > ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml)); > > memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml)); > > -- > > 1.8.3.1
On 03/18/14 15:54, Eduardo Habkost wrote: > On Tue, Mar 18, 2014 at 04:03:25PM +0200, Michael S. Tsirkin wrote: >> On Mon, Mar 17, 2014 at 05:05:17PM +0100, Laszlo Ersek wrote: >>> Building on the previous patch, raise the maximal count of processor >>> objects / NTFY branches / CPON elements from 255 to 256. This allows the >>> VCPU with APIC ID 0xFF to be hotplugged. >>> >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> >> >> I note that we still have: >> if (endvalue >= MAX_CPUMASK_BITS) { >> endvalue = MAX_CPUMASK_BITS - 1; >> fprintf(stderr, >> "qemu: NUMA: A max of %d VCPUs are supported\n", >> MAX_CPUMASK_BITS); >> } >> and MAX_CPUMASK_BITS is 255. >> >> Seems inconsistent? >> > > MAX_CPUMASK_BITS (now renamed to MAX_CPUS) limits CPU indexes and total > CPU count. This patch is about APIC IDs (which may be larger than > max_cpus if threads-per-core or cores-per-socket is not a power of 2). Yea I welcome Eduardo's patchset not only because it fixes the out-of-range accesses caused by "uncontrolled" APIC IDs, but also because it disentangles these limits from one another. Thanks Laszlo
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 2bbefb5..c9fe07f 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -999,11 +999,16 @@ 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); + unsigned acpi_cpus = guest_info->apic_id_limit; int ssdt_start = table_data->len; uint8_t *ssdt_ptr; int i; + /* The current AML generator can cover the APIC ID range [0..255], + * inclusive, for VCPU hotplug. */ + QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256); + g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT); + /* Copy header and patch values in the S3_ / S4_ / S5_ packages */ ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml)); memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
Building on the previous patch, raise the maximal count of processor objects / NTFY branches / CPON elements from 255 to 256. This allows the VCPU with APIC ID 0xFF to be hotplugged. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/i386/acpi-build.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)