Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:
> On Wed, 19 Oct 2016 11:15:46 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:
> > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > to get number of CPUs present at boot. However 1 byte is
> > > not enough to handle more than 255 CPUs.  So add a new
> > > fw_cfg file that would allow QEMU to tell it.
> > > For compat reasons add file only for machine types that
> > > support more than 255 CPUs.
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>  
> > [...]
> > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > >                            Error **errp)
> > >  {
> > > @@ -1232,6 +1221,11 @@ 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)
> > > +{
> > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);  
> > 
> > If we have more than 255 CPUs, shouldn't we at least tell the old
> > BIOS that we have 255, instead of silently truncating bits?
> It won't do any good to BIOS as it would hang in AP wakeup due to
>  (expected != woken up) condition.

Even in this case, truncating bits makes it a bit unpredictable:
having 257 CPUs would set RTC memory to 0, BIOS will believe it
is a UP system.

> 
> > 
> > > +}
> > > +
> > >  static
> > >  void pc_machine_done(Notifier *notifier, void *data)
> > >  {
> > > @@ -1240,7 +1234,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, le16_to_cpu(pcms->boot_cpus_le));
> > >  
> > >      if (bus) {
> > >          int extra_hosts = 0;
> > > @@ -1261,8 +1255,15 @@ 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));
> > > +        }
> > >      }
> > >  }
> > >  
> > > @@ -1786,9 +1787,11 @@ 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);  
> > 
> > Is this really safe? What if the guest is in the middle of a
> > etc/boot-cpus read?
> It's safe for boot CPUs but
> it's not safe to hotplug cpus CPU during BIOS boot at all
> as number of CPUs read from boot_cpus might not match number
> of CPUs that received INIT/SIPI wakeup.
> This problem is ignored for now, I've dropped related SeaBIOS patch
> by Kevin's request and would explore it some more to avoid race there.
> 
> Anyways,
> Do you have an idea how to improve reading from pcms->boot_cpus_le and make 
> it atomic?

No idea. SeaBIOS seems to use insb to read fw_cfg files, so we
could be updating the data between two reads. I don't think we
want to design a fw_cfg guest<->host synchronization mechanism.

I believe the solution is to not change it at all after booting
the guest. I suggest initializing/reinitializing fw_cfg data only
on reset.

After that, we could block or delay CPU hotplug until we know the
guest will be able to handle it. Do we have anything that
prevents or delays hotplug until we know the guest is able to
handle the hotplug events?

> 
> > 
> > >      if (dev->hotplugged) {
> > > -        /* increment the number of CPUs */
> > > -        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) 
> > > + 1);
> > > +        /* Update the number of CPUs in CMOS */
> > > +        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > >      }
> > >  
> > >      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> > > @@ -1842,7 +1845,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_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));
> > >   out:
> > >      error_propagate(errp, local_err);
> > >  }
> > > -- 
> > > 2.7.4
> > >   
> > 
> 

-- 
Eduardo