From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwVxo-0003EP-4Y for qemu-devel@nongnu.org; Tue, 18 Oct 2016 11:06:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwVxj-0002Hm-4I for qemu-devel@nongnu.org; Tue, 18 Oct 2016 11:06:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39694) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwVxi-0002HC-Sc for qemu-devel@nongnu.org; Tue, 18 Oct 2016 11:05:55 -0400 Date: Tue, 18 Oct 2016 13:05:51 -0200 From: Eduardo Habkost Message-ID: <20161018150551.GY3275@thinpad.lan.raisama.net> References: <1476352367-69400-1-git-send-email-imammedo@redhat.com> <1476352367-69400-5-git-send-email-imammedo@redhat.com> <20161018133831.GS3275@thinpad.lan.raisama.net> <20161018163453.59d7d794@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161018163453.59d7d794@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 04/13] acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, kraxel@redhat.com, liuxiaojian6@huawei.com, mst@redhat.com, rkrcmar@redhat.com, peterx@redhat.com, kevin@koconnor.net, pbonzini@redhat.com, lersek@redhat.com, chao.gao@intel.com On Tue, Oct 18, 2016 at 04:34:53PM +0200, Igor Mammedov wrote: > On Tue, 18 Oct 2016 11:38:31 -0200 > Eduardo Habkost wrote: > > > On Thu, Oct 13, 2016 at 11:52:38AM +0200, Igor Mammedov wrote: > > > Switch to modern cpu hotplug at machine startup time if > > > a cpu present at boot has apic-id in range unsupported > > > by legacy cpu hotplug interface (i.e. > 254), to avoid > > > killing QEMU from legacy cpu hotplug code with error: > > > "acpi: invalid cpu id: #apic-id#" > > > > > > Signed-off-by: Igor Mammedov > > > --- > > > hw/acpi/cpu_hotplug.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c > > > index e19d902..c2ab9b8 100644 > > > --- a/hw/acpi/cpu_hotplug.c > > > +++ b/hw/acpi/cpu_hotplug.c > > > @@ -63,7 +63,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu, > > > > > > cpu_id = k->get_arch_id(cpu); > > > if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) { > > > - error_setg(errp, "acpi: invalid cpu id: %" PRIi64, cpu_id); > > > + object_property_set_bool(g->device, false, "cpu-hotplug-legacy", > > > + &error_abort); > > > > What happens we are in legacy mode and this is triggered during > > hotplug instead of machine init? Would it break something, or is > > it safe? > case 1: guest with legacy hotplug AML (migrated for example) would use > legacy interface and it won't be possible to trigger this path > as target should be started with the same CLI as source > (hence < 255 cpus) > case 2: guest started on new QEMU will have new hotplug AML which switches > QEMU to modern cpu hotplug interface at ACPI tables _INI time > so this path is unreachable. I see. Thanks for the explanation! > > Originally it's been static rule: > since 2.7 use new hotplug interface and old one for older machine types > Well it's complex but Michael insisted on keeping legacy hotplug > by default and do dynamic switching, so here we are. > This means PCMachineClass::legacy_cpu_hotplug means "legacy CPU hotplug _only_", because legacy CPU hotplug is always available on startup, right? > this behavior is since 2.7: > commit 679dd1a957df418453efdd3ed2914dba5cd73773 > pc: use new CPU hotplug interface since 2.7 machine type > > > > Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an > > assert(!value). I assume this means we must replace the QOM > > property with something that the user can't fiddle with, right? > it's readonly to user after machine starts, but allows user > to play modern hotplug interface on old machine types if needed. > assert is there to trap mistake of switching to legacy mode > (which is default) from compat_properties. What exactly makes the property read-only to the user? Maybe we should make the setter return an error instead, as all object_property_set_bool("cpu-hotplug-legacy") calls already use &error_abort? > > > > > > return; > > > } > > > > > > @@ -85,13 +86,14 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > > > { > > > CPUState *cpu; > > > > > > - CPU_FOREACH(cpu) { > > > - acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort); > > > - } > > > memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops, > > > gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN); > > > memory_region_add_subregion(parent, base, &gpe_cpu->io); > > > gpe_cpu->device = owner; > > > + > > > + CPU_FOREACH(cpu) { > > > + acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort); > > > + } > > > } > > > > > > void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu, > > > -- > > > 2.7.4 > > > > > > -- Eduardo