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

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