| Message ID | 1394835772-32194-3-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 14, 2014 at 11:22:52PM +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> > --- > hw/i386/acpi-build.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 2bbefb5..51162fc 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -999,11 +999,15 @@ 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); Maybe just make this line int acpi_cpus = guest_info->apic_id_limit; and then the patch will be smaller. > 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(guest_info->apic_id_limit <= 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)); > @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker, > build_append_nameseg(sb_scope, "_SB_"); > > /* build Processor object for each processor */ > - for (i = 0; i < acpi_cpus; i++) { > + for (i = 0; i < guest_info->apic_id_limit; i++) { > uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF); > memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF); > proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4); > @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker, > * Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...} > */ > /* Arg0 = Processor ID = APIC ID */ > - build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus); > + build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", > + guest_info->apic_id_limit); > > /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */ > build_append_byte(sb_scope, 0x08); /* NameOp */ > @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker, > GArray *package = build_alloc_array(); > uint8_t op = 0x13; /* VarPackageOp */ > > - build_append_int(package, acpi_cpus); /* VarNumElements */ > - for (i = 0; i < acpi_cpus; i++) { > + build_append_int(package, > + guest_info->apic_id_limit); /* VarNumElements */ > + for (i = 0; i < guest_info->apic_id_limit; i++) { > uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00; > build_append_byte(package, b); > } > -- > 1.8.3.1
On 03/16/14 13:31, Michael S. Tsirkin wrote: > On Fri, Mar 14, 2014 at 11:22:52PM +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> >> --- >> hw/i386/acpi-build.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 2bbefb5..51162fc 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -999,11 +999,15 @@ 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); > > Maybe just make this line > int acpi_cpus = guest_info->apic_id_limit; > and then the patch will be smaller. I did think of that, but I didn't like the unchecked unsigned --> int conversion. The limit checks below cover that too. Also, the patch renders the function similar to the other acpi builder functions; there are a handful of loops that directly name "guest_info->apic_id_limit" in their controlling expressions. Anyway, would you be OK with a patch that did the assignment you suggest (rather than modifying the loops), and kept the BUILD_BUG and the g_assert below? Thanks Laszlo > >> 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(guest_info->apic_id_limit <= 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)); >> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker, >> build_append_nameseg(sb_scope, "_SB_"); >> >> /* build Processor object for each processor */ >> - for (i = 0; i < acpi_cpus; i++) { >> + for (i = 0; i < guest_info->apic_id_limit; i++) { >> uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF); >> memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF); >> proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4); >> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker, >> * Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...} >> */ >> /* Arg0 = Processor ID = APIC ID */ >> - build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus); >> + build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", >> + guest_info->apic_id_limit); >> >> /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */ >> build_append_byte(sb_scope, 0x08); /* NameOp */ >> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker, >> GArray *package = build_alloc_array(); >> uint8_t op = 0x13; /* VarPackageOp */ >> >> - build_append_int(package, acpi_cpus); /* VarNumElements */ >> - for (i = 0; i < acpi_cpus; i++) { >> + build_append_int(package, >> + guest_info->apic_id_limit); /* VarNumElements */ >> + for (i = 0; i < guest_info->apic_id_limit; i++) { >> uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00; >> build_append_byte(package, b); >> } >> -- >> 1.8.3.1
On Mon, Mar 17, 2014 at 11:08:46AM +0100, Laszlo Ersek wrote: > On 03/16/14 13:31, Michael S. Tsirkin wrote: > > On Fri, Mar 14, 2014 at 11:22:52PM +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> > >> --- > >> hw/i386/acpi-build.c | 16 +++++++++++----- > >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >> index 2bbefb5..51162fc 100644 > >> --- a/hw/i386/acpi-build.c > >> +++ b/hw/i386/acpi-build.c > >> @@ -999,11 +999,15 @@ 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); > > > > Maybe just make this line > > int acpi_cpus = guest_info->apic_id_limit; > > and then the patch will be smaller. > > I did think of that, but I didn't like the unchecked unsigned --> int > conversion. The limit checks below cover that too. Make it unsigned acpi_cpus then? > Also, the patch renders the function similar to the other acpi builder > functions; there are a handful of loops that directly name > "guest_info->apic_id_limit" in their controlling expressions. I'm fine with refactoring this if you feel it's more readable (I don't bit I wrote this so I know I'm biased), but let's make it a separate patch then. > Anyway, would you be OK with a patch that did the assignment you suggest > (rather than modifying the loops), and kept the BUILD_BUG and the > g_assert below? > > Thanks > Laszlo Yes, fine. > > > >> 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(guest_info->apic_id_limit <= 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)); > >> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker, > >> build_append_nameseg(sb_scope, "_SB_"); > >> > >> /* build Processor object for each processor */ > >> - for (i = 0; i < acpi_cpus; i++) { > >> + for (i = 0; i < guest_info->apic_id_limit; i++) { > >> uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF); > >> memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF); > >> proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4); > >> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker, > >> * Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...} > >> */ > >> /* Arg0 = Processor ID = APIC ID */ > >> - build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus); > >> + build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", > >> + guest_info->apic_id_limit); > >> > >> /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */ > >> build_append_byte(sb_scope, 0x08); /* NameOp */ > >> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker, > >> GArray *package = build_alloc_array(); > >> uint8_t op = 0x13; /* VarPackageOp */ > >> > >> - build_append_int(package, acpi_cpus); /* VarNumElements */ > >> - for (i = 0; i < acpi_cpus; i++) { > >> + build_append_int(package, > >> + guest_info->apic_id_limit); /* VarNumElements */ > >> + for (i = 0; i < guest_info->apic_id_limit; i++) { > >> uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00; > >> build_append_byte(package, b); > >> } > >> -- > >> 1.8.3.1
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 2bbefb5..51162fc 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -999,11 +999,15 @@ 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); 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(guest_info->apic_id_limit <= 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)); @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker, build_append_nameseg(sb_scope, "_SB_"); /* build Processor object for each processor */ - for (i = 0; i < acpi_cpus; i++) { + for (i = 0; i < guest_info->apic_id_limit; i++) { uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF); memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF); proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4); @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker, * Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...} */ /* Arg0 = Processor ID = APIC ID */ - build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus); + build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", + guest_info->apic_id_limit); /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */ build_append_byte(sb_scope, 0x08); /* NameOp */ @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker, GArray *package = build_alloc_array(); uint8_t op = 0x13; /* VarPackageOp */ - build_append_int(package, acpi_cpus); /* VarNumElements */ - for (i = 0; i < acpi_cpus; i++) { + build_append_int(package, + guest_info->apic_id_limit); /* VarNumElements */ + for (i = 0; i < guest_info->apic_id_limit; i++) { uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00; build_append_byte(package, b); }
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 | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)