From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwvcO-0004yA-KF for qemu-devel@nongnu.org; Wed, 19 Oct 2016 14:29:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwvcK-0000Dh-LI for qemu-devel@nongnu.org; Wed, 19 Oct 2016 14:29:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58080) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwvcK-0000DI-EL for qemu-devel@nongnu.org; Wed, 19 Oct 2016 14:29:32 -0400 Date: Wed, 19 Oct 2016 16:29:29 -0200 From: Eduardo Habkost Message-ID: <20161019182929.GR5057@thinpad.lan.raisama.net> References: <1476878743-144953-1-git-send-email-imammedo@redhat.com> <1476878743-144953-12-git-send-email-imammedo@redhat.com> <20161019131546.GL5057@thinpad.lan.raisama.net> <20161019171838.4a9343f6@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161019171838.4a9343f6@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs 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 Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote: > On Wed, 19 Oct 2016 11:15:46 -0200 > Eduardo Habkost wrote: > > > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote: > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS > > > to get number of CPUs present at boot. However 1 byte is > > > not enough to handle more than 255 CPUs. So add a new > > > fw_cfg file that would allow QEMU to tell it. > > > For compat reasons add file only for machine types that > > > support more than 255 CPUs. > > > > > > Signed-off-by: Igor Mammedov > > [...] > > > static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id, > > > Error **errp) > > > { > > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms) > > > fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val)); > > > } > > > > > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count) > > > +{ > > > + rtc_set_memory(rtc, 0x5f, cpus_count - 1); > > > > If we have more than 255 CPUs, shouldn't we at least tell the old > > BIOS that we have 255, instead of silently truncating bits? > It won't do any good to BIOS as it would hang in AP wakeup due to > (expected != woken up) condition. Even in this case, truncating bits makes it a bit unpredictable: having 257 CPUs would set RTC memory to 0, BIOS will believe it is a UP system. > > > > > > +} > > > + > > > static > > > void pc_machine_done(Notifier *notifier, void *data) > > > { > > > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data) > > > PCIBus *bus = pcms->bus; > > > > > > /* set the number of CPUs */ > > > - rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1); > > > + rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le)); > > > > > > if (bus) { > > > int extra_hosts = 0; > > > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data) > > > > > > acpi_setup(); > > > if (pcms->fw_cfg) { > > > + MachineClass *mc = MACHINE_GET_CLASS(pcms); > > > + > > > pc_build_smbios(pcms->fw_cfg); > > > pc_build_feature_control_file(pcms); > > > + > > > + if (mc->max_cpus > 255) { > > > + fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le, > > > + sizeof(pcms->boot_cpus_le)); > > > + } > > > } > > > } > > > > > > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > > > } > > > } > > > > > > + /* increment the number of CPUs */ > > > + pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1); > > > > Is this really safe? What if the guest is in the middle of a > > etc/boot-cpus read? > It's safe for boot CPUs but > it's not safe to hotplug cpus CPU during BIOS boot at all > as number of CPUs read from boot_cpus might not match number > of CPUs that received INIT/SIPI wakeup. > This problem is ignored for now, I've dropped related SeaBIOS patch > by Kevin's request and would explore it some more to avoid race there. > > Anyways, > Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic? No idea. SeaBIOS seems to use insb to read fw_cfg files, so we could be updating the data between two reads. I don't think we want to design a fw_cfg guest<->host synchronization mechanism. I believe the solution is to not change it at all after booting the guest. I suggest initializing/reinitializing fw_cfg data only on reset. After that, we could block or delay CPU hotplug until we know the guest will be able to handle it. Do we have anything that prevents or delays hotplug until we know the guest is able to handle the hotplug events? > > > > > > if (dev->hotplugged) { > > > - /* increment the number of CPUs */ > > > - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); > > > + /* Update the number of CPUs in CMOS */ > > > + rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le)); > > > } > > > > > > found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL); > > > @@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev, > > > found_cpu->cpu = NULL; > > > object_unparent(OBJECT(dev)); > > > > > > - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1); > > > + /* decrement the number of CPUs */ > > > + pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1); > > > + /* Update the number of CPUs in CMOS */ > > > + rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le)); > > > out: > > > error_propagate(errp, local_err); > > > } > > > -- > > > 2.7.4 > > > > > > -- Eduardo