From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35811) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwsdl-0004p5-De for qemu-devel@nongnu.org; Wed, 19 Oct 2016 11:18:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwsdg-0001sF-D6 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 11:18:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34578) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwsdg-0001rv-5r for qemu-devel@nongnu.org; Wed, 19 Oct 2016 11:18:44 -0400 Date: Wed, 19 Oct 2016 17:18:38 +0200 From: Igor Mammedov Message-ID: <20161019171838.4a9343f6@nial.brq.redhat.com> In-Reply-To: <20161019131546.GL5057@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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Eduardo Habkost 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, 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. > > > +} > > + > > 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? > > > 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 > > >