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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 11 Apr 2013 15:59:40 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 <imammedo@redhat.com>
> > 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.

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

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