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 16:29:29 -0200
Eduardo Habkost <address@hidden> wrote:

> 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.
and it will do AP wakeup regardless, where old BIOS will hang
due to unexpectedly woken-up CPUs regardless of value in cmos.

Anyways I don't care so if you'd really prefer to have cmos
set to some static value if cpus count more than 256, just
tell me what value and what justification I shall put in
comment so we won't wonder later why it's there.

> > >   
> > > > +}
> > > > +
> > > >  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.
I've checked it once more and value read by BIOS atomically
as both QEMU and SeaBIOS use dma interface to transfer data.
BIOS transfers control to QEMU once via

 outl(PORT_QEMU_CFG_DMA_ADDR_LOW)

and after that access to pcms->boot_cpus_le is protected by BQL.
So there is no need to invent some sort of synchronization
or limit update to fixed points (machine_done/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?
Not that I know of,
and I'd like to fix BIOS side to handle AP wakeup gracefully
even if cpus are hotplugged while BIOS is running instead of
putting band-aids to workaround the issue.


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