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 <address@hidden> 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.