| Message ID | 1479212236-183810-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 15, 2016 at 01:17:16PM +0100, Igor Mammedov wrote: [...] > @@ -1265,6 +1267,8 @@ void pc_machine_done(Notifier *notifier, void *data) > if (pcms->fw_cfg) { > pc_build_smbios(pcms->fw_cfg); > pc_build_feature_control_file(pcms); > + /* update FW_CFG_NB_CPUS to account for -device added CPUs */ > + fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > } > > if (pcms->apic_id_limit > 255) { > @@ -1342,7 +1346,7 @@ void xen_load_linux(PCMachineState *pcms) > assert(MACHINE(pcms)->kernel_filename != NULL); > > fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); > - fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); > + fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > rom_set_fw(fw_cfg); > > load_linux(pcms, fw_cfg); > @@ -1824,9 +1828,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > } > } > > + /* increment the number of CPUs */ > + pcms->boot_cpus++; > if (dev->hotplugged) { > - /* increment the number of CPUs */ > - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > } > > found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL); > @@ -1880,7 +1885,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--; > + /* Update the number of CPUs in CMOS */ > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); Don't we need to call fw_cfg_modify_i16() on hotplug/hot-unplug, too?
On Tue, 15 Nov 2016 15:34:45 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Nov 15, 2016 at 01:17:16PM +0100, Igor Mammedov wrote: > [...] > > @@ -1265,6 +1267,8 @@ void pc_machine_done(Notifier *notifier, void *data) > > if (pcms->fw_cfg) { > > pc_build_smbios(pcms->fw_cfg); > > pc_build_feature_control_file(pcms); > > + /* update FW_CFG_NB_CPUS to account for -device added CPUs */ > > + fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > > } > > > > if (pcms->apic_id_limit > 255) { > > @@ -1342,7 +1346,7 @@ void xen_load_linux(PCMachineState *pcms) > > assert(MACHINE(pcms)->kernel_filename != NULL); > > > > fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); > > - fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); > > + fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > > rom_set_fw(fw_cfg); > > > > load_linux(pcms, fw_cfg); > > @@ -1824,9 +1828,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > > } > > } > > > > + /* increment the number of CPUs */ > > + pcms->boot_cpus++; > > if (dev->hotplugged) { > > - /* increment the number of CPUs */ > > - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); > > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > > } > > > > found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL); > > @@ -1880,7 +1885,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--; > > + /* Update the number of CPUs in CMOS */ > > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > > Don't we need to call fw_cfg_modify_i16() on hotplug/hot-unplug, > too? Indeed, it should be updated otherwise it will hang on reboot in BIOS waiting for wrong number of CPUs if CPUs count is above 256. the same bug has been present in the reverted "pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs" Thanks for noticing it! I'll post v3 as reply to this thread.
On Wed, Nov 16, 2016 at 01:24:11PM +0100, Igor Mammedov wrote: > On Tue, 15 Nov 2016 15:34:45 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Nov 15, 2016 at 01:17:16PM +0100, Igor Mammedov wrote: > > [...] > > > @@ -1265,6 +1267,8 @@ void pc_machine_done(Notifier *notifier, void *data) > > > if (pcms->fw_cfg) { > > > pc_build_smbios(pcms->fw_cfg); > > > pc_build_feature_control_file(pcms); > > > + /* update FW_CFG_NB_CPUS to account for -device added CPUs */ > > > + fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > > > } > > > > > > if (pcms->apic_id_limit > 255) { > > > @@ -1342,7 +1346,7 @@ void xen_load_linux(PCMachineState *pcms) > > > assert(MACHINE(pcms)->kernel_filename != NULL); > > > > > > fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); > > > - fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); > > > + fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > > > rom_set_fw(fw_cfg); > > > > > > load_linux(pcms, fw_cfg); > > > @@ -1824,9 +1828,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > > > } > > > } > > > > > > + /* increment the number of CPUs */ > > > + pcms->boot_cpus++; > > > if (dev->hotplugged) { > > > - /* increment the number of CPUs */ > > > - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); > > > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > > > } > > > > > > found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL); > > > @@ -1880,7 +1885,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--; > > > + /* Update the number of CPUs in CMOS */ > > > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > > > > Don't we need to call fw_cfg_modify_i16() on hotplug/hot-unplug, > > too? > Indeed, it should be updated > otherwise it will hang on reboot in BIOS waiting for wrong number of CPUs > if CPUs count is above 256. > > the same bug has been present in the reverted > "pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs" The "etc/boot-cpus" patch changed boot_cpus_le on the plug/unplug callbacks. > Thanks for noticing it! > I'll post v3 as reply to this thread. Thanks!
On Wed, 16 Nov 2016 10:39:33 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Nov 16, 2016 at 01:24:11PM +0100, Igor Mammedov wrote: > > On Tue, 15 Nov 2016 15:34:45 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Nov 15, 2016 at 01:17:16PM +0100, Igor Mammedov wrote: > > > [...] > > > > @@ -1265,6 +1267,8 @@ void pc_machine_done(Notifier *notifier, void *data) > > > > if (pcms->fw_cfg) { > > > > pc_build_smbios(pcms->fw_cfg); > > > > pc_build_feature_control_file(pcms); > > > > + /* update FW_CFG_NB_CPUS to account for -device added CPUs */ > > > > + fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > > > > } > > > > > > > > if (pcms->apic_id_limit > 255) { > > > > @@ -1342,7 +1346,7 @@ void xen_load_linux(PCMachineState *pcms) > > > > assert(MACHINE(pcms)->kernel_filename != NULL); > > > > > > > > fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); > > > > - fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); > > > > + fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); > > > > rom_set_fw(fw_cfg); > > > > > > > > load_linux(pcms, fw_cfg); > > > > @@ -1824,9 +1828,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > > > > } > > > > } > > > > > > > > + /* increment the number of CPUs */ > > > > + pcms->boot_cpus++; > > > > if (dev->hotplugged) { > > > > - /* increment the number of CPUs */ > > > > - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); > > > > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > > > > } > > > > > > > > found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL); > > > > @@ -1880,7 +1885,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--; > > > > + /* Update the number of CPUs in CMOS */ > > > > + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); > > > > > > Don't we need to call fw_cfg_modify_i16() on hotplug/hot-unplug, > > > too? > > Indeed, it should be updated > > otherwise it will hang on reboot in BIOS waiting for wrong number of CPUs > > if CPUs count is above 256. > > > > the same bug has been present in the reverted > > "pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs" > > The "etc/boot-cpus" patch changed boot_cpus_le on the plug/unplug > callbacks. Ah yes, I've forgotten that boot_cpus_le has been directly accessible by fwcfg > > > > Thanks for noticing it! > > I'll post v3 as reply to this thread. > > Thanks! >
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index e32e957..67a1a9e 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -36,6 +36,7 @@ /** * PCMachineState: * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling + * @boot_cpus: number of present VCPUs */ struct PCMachineState { /*< private >*/ @@ -70,6 +71,7 @@ struct PCMachineState { bool apic_xrupt_override; unsigned apic_id_limit; CPUArchIdList *possible_cpus; + uint16_t boot_cpus; /* NUMA information: */ uint64_t numa_nodes; diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 5aeae7d..e87a0e7 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -744,7 +744,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms) int i, j; fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as); - fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); + fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86: * @@ -1087,17 +1087,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) } } -static int pc_present_cpus_count(PCMachineState *pcms) -{ - int i, boot_cpus = 0; - for (i = 0; i < pcms->possible_cpus->len; i++) { - if (pcms->possible_cpus->cpus[i].cpu) { - boot_cpus++; - } - } - return boot_cpus; -} - static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id, Error **errp) { @@ -1234,6 +1223,19 @@ 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) +{ + if (cpus_count > 0xff) { + /* If the number of CPUs can't be represented in 8 bits, the + * BIOS must use "FW_CFG_NB_CPUS". Set RTC field to 0 just + * to make old BIOSes fail more predictably. + */ + rtc_set_memory(rtc, 0x5f, 0); + } else { + rtc_set_memory(rtc, 0x5f, cpus_count - 1); + } +} + static void pc_machine_done(Notifier *notifier, void *data) { @@ -1242,7 +1244,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, pcms->boot_cpus); if (bus) { int extra_hosts = 0; @@ -1265,6 +1267,8 @@ void pc_machine_done(Notifier *notifier, void *data) if (pcms->fw_cfg) { pc_build_smbios(pcms->fw_cfg); pc_build_feature_control_file(pcms); + /* update FW_CFG_NB_CPUS to account for -device added CPUs */ + fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); } if (pcms->apic_id_limit > 255) { @@ -1342,7 +1346,7 @@ void xen_load_linux(PCMachineState *pcms) assert(MACHINE(pcms)->kernel_filename != NULL); fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); - fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); + fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus); rom_set_fw(fw_cfg); load_linux(pcms, fw_cfg); @@ -1824,9 +1828,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, } } + /* increment the number of CPUs */ + pcms->boot_cpus++; if (dev->hotplugged) { - /* increment the number of CPUs */ - rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); } found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL); @@ -1880,7 +1885,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--; + /* Update the number of CPUs in CMOS */ + rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus); out: error_propagate(errp, local_err); }
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- include/hw/i386/pc.h | 2 ++ hw/i386/pc.c | 42 +++++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 17 deletions(-)