[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.1 2/2] i386/acpi-build: support hotplug of
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH for-2.1 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF |
Date: |
Mon, 17 Mar 2014 13:30:16 +0200 |
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 <address@hidden>
> >> ---
> >> 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