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