[Qemu-devel] [PATCH for-2.8 v2 0/3] pc: remove redundant fw_cfg file "etc/boot-cpus"

[Qemu-devel] [PATCH for-2.8 v2 0/3] pc: remove redundant fw_cfg file "etc/boot-cpus"

[Igor Mammedov]

Changes since v1:
  revert commit 080ac219cc7d9 and redo FW_CFG_NB_CPUS fixes on top of it


Commit 080ac219cc7d9c55adf925c3545b7450055ad625
   pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

added "etc/boot-cpus" fw_cfg file durung 2.8 merge window, however
QEMU alredy had similar legacy FW_CFG_NB_CPUS fw_cfg entry that
should do practically the same. Considering FW_CFG_NB_CPUS's been
around for a long time and is used by external projects (firmwares)
we can't replace it with 'etc/boot-cpus' fw_cfg file.

Drop redundant 'etc/boot-cpus' fw_cfg file  and reuse FW_CFG_NB_CPUS
instead.

So here goes QEMU part of fixup

CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Stefan Hajnoczi <stefanha@gmail.com>
CC: "Kevin O'Connor" <kevin@koconnor.net>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Laszlo Ersek <lersek@redhat.com>
CC: Alexander Graf <agraf@suse.de>
CC: Paolo Bonzini <pbonzini@redhat.com> 
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Artyom Tarasenko <atar4qemu@gmail.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Richard Henderson <rth@twiddle.net>

Igor Mammedov (3):
  Revert "pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than
    255 CPUs"
  fw_cfg: move FW_CFG_NB_CPUS out of fw_cfg_init1()
  pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

 include/hw/i386/pc.h  |  4 ++--
 hw/arm/virt.c         |  4 +++-
 hw/i386/pc.c          | 24 ++++++++++--------------
 hw/nvram/fw_cfg.c     |  1 -
 hw/ppc/mac_newworld.c |  1 +
 hw/ppc/mac_oldworld.c |  1 +
 hw/sparc/sun4m.c      |  1 +
 hw/sparc64/sun4u.c    |  1 +
 8 files changed, 19 insertions(+), 18 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.8 v2 1/3] Revert "pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs"

[Igor Mammedov]

This reverts commit 080ac219cc7d9c55adf925c3545b7450055ad625.

Legacy FW_CFG_NB_CPUS will be reused instead of 'etc/boot-cpus'
fw_cfg file since it does the same and there is no point
to maintaing duplicate guest ABI, if it can be helped.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/pc.h |  2 --
 hw/i386/pc.c         | 44 +++++++++++++++-----------------------------
 2 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8eb517f..e32e957 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -36,7 +36,6 @@
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
- * @boot_cpus_le: number of present VCPUs, referenced by 'etc/boot-cpus' fw_cfg
  */
 struct PCMachineState {
     /*< private >*/
@@ -71,7 +70,6 @@ struct PCMachineState {
     bool apic_xrupt_override;
     unsigned apic_id_limit;
     CPUArchIdList *possible_cpus;
-    uint16_t boot_cpus_le;
 
     /* NUMA information: */
     uint64_t numa_nodes;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2c37a78..effb89b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1086,6 +1086,17 @@ 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)
 {
@@ -1222,19 +1233,6 @@ 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 "etc/boot-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)
 {
@@ -1243,7 +1241,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     PCIBus *bus = pcms->bus;
 
     /* set the number of CPUs */
-    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
+    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
 
     if (bus) {
         int extra_hosts = 0;
@@ -1264,15 +1262,8 @@ 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));
-        }
     }
 
     if (pcms->apic_id_limit > 255) {
@@ -1831,11 +1822,9 @@ 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);
     if (dev->hotplugged) {
-        /* Update the number of CPUs in CMOS */
-        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
+        /* increment the number of CPUs */
+        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
     }
 
     found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
@@ -1889,10 +1878,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
     found_cpu->cpu = NULL;
     object_unparent(OBJECT(dev));
 
-    /* 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));
+    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
  out:
     error_propagate(errp, local_err);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8 v2 1/3] Revert "pc: Add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs"

[Eduardo Habkost]

On Tue, Nov 15, 2016 at 01:17:14PM +0100, Igor Mammedov wrote:
> This reverts commit 080ac219cc7d9c55adf925c3545b7450055ad625.
> 
> Legacy FW_CFG_NB_CPUS will be reused instead of 'etc/boot-cpus'
> fw_cfg file since it does the same and there is no point
> to maintaing duplicate guest ABI, if it can be helped.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  include/hw/i386/pc.h |  2 --
>  hw/i386/pc.c         | 44 +++++++++++++++-----------------------------
>  2 files changed, 15 insertions(+), 31 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 8eb517f..e32e957 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -36,7 +36,6 @@
>  /**
>   * PCMachineState:
>   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> - * @boot_cpus_le: number of present VCPUs, referenced by 'etc/boot-cpus' fw_cfg
>   */
>  struct PCMachineState {
>      /*< private >*/
> @@ -71,7 +70,6 @@ struct PCMachineState {
>      bool apic_xrupt_override;
>      unsigned apic_id_limit;
>      CPUArchIdList *possible_cpus;
> -    uint16_t boot_cpus_le;
>  
>      /* NUMA information: */
>      uint64_t numa_nodes;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2c37a78..effb89b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1086,6 +1086,17 @@ 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)
>  {
> @@ -1222,19 +1233,6 @@ 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 "etc/boot-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)
>  {
> @@ -1243,7 +1241,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>      PCIBus *bus = pcms->bus;
>  
>      /* set the number of CPUs */
> -    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> +    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
>  
>      if (bus) {
>          int extra_hosts = 0;
> @@ -1264,15 +1262,8 @@ 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));
> -        }
>      }
>  
>      if (pcms->apic_id_limit > 255) {
> @@ -1831,11 +1822,9 @@ 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);
>      if (dev->hotplugged) {
> -        /* Update the number of CPUs in CMOS */
> -        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> +        /* increment the number of CPUs */
> +        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
>      }
>  
>      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> @@ -1889,10 +1878,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
>      found_cpu->cpu = NULL;
>      object_unparent(OBJECT(dev));
>  
> -    /* 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));
> +    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
>   out:
>      error_propagate(errp, local_err);
>  }
> -- 
> 2.7.4
> 

-- 
Eduardo

[Qemu-devel] [PATCH for-2.8 v2 2/3] fw_cfg: move FW_CFG_NB_CPUS out of fw_cfg_init1()

[Igor Mammedov]

PC will use this field in other way, so move it outside the common
code so PC could set a different value, i.e. all CPUs
regardless of where they are coming from (-smp X | -device cpu...).

It's quick and dirty hack as it could be implemented in more generic
way in MashineClass. But do it in simple way since only PC is affected
so far.

Later we can generalize it when another affected target gets support
for -device cpu.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/arm/virt.c         | 4 +++-
 hw/i386/pc.c          | 2 ++
 hw/nvram/fw_cfg.c     | 1 -
 hw/ppc/mac_newworld.c | 1 +
 hw/ppc/mac_oldworld.c | 1 +
 hw/sparc/sun4m.c      | 1 +
 hw/sparc64/sun4u.c    | 1 +
 7 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 54a8b28..d04e4ac 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -929,9 +929,11 @@ static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
 {
     hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
+    FWCfgState *fw_cfg;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
+    fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index effb89b..5aeae7d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -744,6 +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_MAX_CPUS is a bit confusing/problematic on x86:
      *
@@ -1341,6 +1342,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);
     rom_set_fw(fw_cfg);
 
     load_linux(pcms, fw_cfg);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 1f0c3e9..3ebecb2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -884,7 +884,6 @@ static void fw_cfg_init1(DeviceState *dev)
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
-    fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 7d25106..2bfdb64 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -466,6 +466,7 @@ static void ppc_core99_init(MachineState *machine)
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 4479487..56282c5 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -319,6 +319,7 @@ static void ppc_heathrow_init(MachineState *machine)
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 6224288..f5b6efd 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -1033,6 +1033,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                  hwdef->ecc_version);
 
     fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 271d8bc..4663315 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -855,6 +855,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                            (uint8_t *)&nd_table[0].macaddr);
 
     fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8 v2 2/3] fw_cfg: move FW_CFG_NB_CPUS out of fw_cfg_init1()

[Eduardo Habkost]

On Tue, Nov 15, 2016 at 01:17:15PM +0100, Igor Mammedov wrote:
> PC will use this field in other way, so move it outside the common
> code so PC could set a different value, i.e. all CPUs
> regardless of where they are coming from (-smp X | -device cpu...).
> 
> It's quick and dirty hack as it could be implemented in more generic
> way in MashineClass. But do it in simple way since only PC is affected
> so far.
> 
> Later we can generalize it when another affected target gets support
> for -device cpu.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  hw/arm/virt.c         | 4 +++-
>  hw/i386/pc.c          | 2 ++
>  hw/nvram/fw_cfg.c     | 1 -
>  hw/ppc/mac_newworld.c | 1 +
>  hw/ppc/mac_oldworld.c | 1 +
>  hw/sparc/sun4m.c      | 1 +
>  hw/sparc64/sun4u.c    | 1 +
>  7 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 54a8b28..d04e4ac 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -929,9 +929,11 @@ static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
>  {
>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
> +    FWCfgState *fw_cfg;
>      char *nodename;
>  
> -    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
> +    fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>  
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index effb89b..5aeae7d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -744,6 +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_MAX_CPUS is a bit confusing/problematic on x86:
>       *
> @@ -1341,6 +1342,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);
>      rom_set_fw(fw_cfg);
>  
>      load_linux(pcms, fw_cfg);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 1f0c3e9..3ebecb2 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -884,7 +884,6 @@ static void fw_cfg_init1(DeviceState *dev)
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
> -    fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>      fw_cfg_bootsplash(s);
>      fw_cfg_reboot(s);
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 7d25106..2bfdb64 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -466,6 +466,7 @@ static void ppc_core99_init(MachineState *machine)
>      /* No PCI init: the BIOS will do it */
>  
>      fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 4479487..56282c5 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -319,6 +319,7 @@ static void ppc_heathrow_init(MachineState *machine)
>      /* No PCI init: the BIOS will do it */
>  
>      fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 6224288..f5b6efd 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -1033,6 +1033,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                   hwdef->ecc_version);
>  
>      fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 271d8bc..4663315 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -855,6 +855,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>                             (uint8_t *)&nd_table[0].macaddr);
>  
>      fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
> -- 
> 2.7.4
> 

-- 
Eduardo

[Qemu-devel] [PATCH for-2.8 v2 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Igor Mammedov]

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(-)

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);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8 v2 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Eduardo Habkost]

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?

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.8 v2 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Igor Mammedov]

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.

Re: [Qemu-devel] [PATCH for-2.8 v2 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Eduardo Habkost]

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!

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.8 v2 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Igor Mammedov]

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!
> 

[Qemu-devel] [PATCH for-2.8 v3 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  - Update FW_CFG_NB_CPUS on CPU hot(un)plug to avoid
    hang in BIOS on reboot if number of CPUs is over 256
    (Eduardo)
---
 include/hw/i386/pc.h |  2 ++
 hw/i386/pc.c         | 44 +++++++++++++++++++++++++++-----------------
 2 files changed, 29 insertions(+), 17 deletions(-)

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..677a594 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,11 @@ 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);
+        fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }
 
     found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
@@ -1880,7 +1886,11 @@ 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);
+    fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
  out:
     error_propagate(errp, local_err);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8 v3 3/3] pc: fix FW_CFG_NB_CPUS to account for -device added CPUs

[Eduardo Habkost]

On Wed, Nov 16, 2016 at 02:04:41PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo