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

[Igor Mammedov]

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.

> 
> > +}
> > +
> >  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?

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