Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug

[Eduardo Habkost]

On Fri, Apr 12, 2013 at 12:53:51PM +0200, Igor Mammedov wrote:
> On Thu, 11 Apr 2013 15:59:40 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Thu, Apr 11, 2013 at 04:51:46PM +0200, Igor Mammedov wrote:
> > > ... so that on reboot BIOS could read current available CPU count
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > v2:
> > >   * s/qemu_register_cpu_add_notifier()/qemu_register_cpu_added_notifier()/
> > > ---
> > >  hw/timer/mc146818rtc.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > 
> > Initialization of the cmos fields (including 0x5F) is done on
> > pc.c:pc_cmos_init(). What about making the field increment inside pc.c
> > as well?
> I looked at possibility but discarded it because to increment it there initial
> value should be -1 (field is zero based) which is not obvious, plug ugly
> casting to singed variable.
> Result looked ugly.

I was thinking about simply adding exactly the same code with exactly
the same logic, but inside pc.c instead of of mc146818rtc.c. Instead of
registering the notifier inside rtc_initfn(), register exactly the same
notifier with exactly the same code, but inside pc_cmos_init() (that's
where 0x5f is initialized).

It would even be safer and easier review and ensure correctness: with
this patch, the notifier is registered very early, even before
pc_cmos_init() initializes 0x5f to smp_cpus. CPU hotplug events are
unlikely to be emitted before pc_cmos_init() is called, but still: why
make the initialization ordering so subtle if we don't have to?

> 
> > 
> > What happens if a CPU is hotplugged after the machine has started but
> > before the guest OS has booted? Are we supposed to make sure the BIOS do
> > the right thing if a CPU is hotplugged before the OS has booted, or this
> > simply won't be supported?
> BIOS uses this value to set in ACPI tables what CPUs are present.
>  1. if hot-plug happens before BIOS reads it then OS will see all CPUs
> and SCI it receives will be nop.
>  2. if hot-plug happens after BIOS reads it, OS will handle SCI as usual
>     and hotplug CPU instead of initializing it smp_boot() time.
> BIOS itself has nothing to do with hot-plug, it's OSPM job.

Makes sense, thanks.

What happens if the CPU is hotplugged after the BIOS builds the ACPI
tables, but long before the OS starts handling ACPI events? Is the OS
guaranteed to run the CPU hotplug ACPI method (that's \_GPE._E02 in the
DSDT, right?), even if that happens?

> 
> > 
> > > 
> > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > > index 69e6844..958ed6b 100644
> > > --- a/hw/timer/mc146818rtc.c
> > > +++ b/hw/timer/mc146818rtc.c
> > > @@ -82,6 +82,7 @@ typedef struct RTCState {
> > >      Notifier clock_reset_notifier;
> > >      LostTickPolicy lost_tick_policy;
> > >      Notifier suspend_notifier;
> > > +    Notifier cpu_added_notifier;
> > >  } RTCState;
> > >  
> > >  static void rtc_set_time(RTCState *s);
> > > @@ -759,6 +760,14 @@ static void rtc_notify_suspend(Notifier *notifier, 
> > > void *data)
> > >      rtc_set_memory(&s->dev, 0xF, 0xFE);
> > >  }
> > >  
> > > +static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> > > +{
> > > +    RTCState *s = container_of(notifier, RTCState, cpu_added_notifier);
> > > +
> > > +    /* increment the number of CPUs */
> > > +    s->cmos_data[0x5f] += 1;
> > > +}
> > > +
> > >  static void rtc_reset(void *opaque)
> > >  {
> > >      RTCState *s = opaque;
> > > @@ -852,6 +861,9 @@ static int rtc_initfn(ISADevice *dev)
> > >      s->suspend_notifier.notify = rtc_notify_suspend;
> > >      qemu_register_suspend_notifier(&s->suspend_notifier);
> > >  
> > > +    s->cpu_added_notifier.notify = rtc_notify_cpu_added;
> > > +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> > > +
> > >      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
> > >      isa_register_ioport(dev, &s->io, base);
> > >  
> > > -- 
> > > 1.8.2
> > > 
> > 
> > -- 
> > Eduardo
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo