Re: [Qemu-devel] [PATCH 06/16] target-i386: pc: update rtc_cmos on CPU hot-plug

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 22 Apr 2013 16:56:59 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 16.04.2013 00:12, schrieb Igor Mammedov:
> > ... so that on reboot BIOS could read current available CPU count
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c                   | 20 ++++++++++++++++++++
> >  hw/timer/mc146818rtc.c         |  7 +++++++
> >  include/hw/timer/mc146818rtc.h |  1 +
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 8d75b34..dc1a78b 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -337,6 +337,21 @@ static void pc_cmos_init_late(void *opaque)
> >      qemu_unregister_reset(pc_cmos_init_late, opaque);
> >  }
> >  
> > +typedef struct rtc_cpu_hotplug_arg {
> > +    ISADevice *rtc_state;
> > +    Notifier cpu_added_notifier;
> > +} rtc_cpu_hotplug_arg;
> 
> Are the arguments (rtc_state) intentionally placed before the common part?
no, but it's not QOM type so it doesn't matter.
I'll reorder it on next respin.

> 
> > +
> > +static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> > +{
> > +    rtc_cpu_hotplug_arg *arg = container_of(notifier,
> > rtc_cpu_hotplug_arg,
> > +                                           cpu_added_notifier);
> > +    ISADevice *s = arg->rtc_state;
> > +
> > +    /* increment the number of CPUs */
> > +    rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1);
> > +}
> 
> I think this proves Gleb's point of reordering the notifier and CPU
> resumption since the first instruction executed might theoretically be
> reading the RTC memory.
I've already changed notifier clall place according to Gleb's suggestions.

> 
> Otherwise looks good with one minor nit...
> 
> > +
> >  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
> >                    const char *boot_device,
> >                    ISADevice *floppy, BusState *idebus0, BusState
> > *idebus1, @@ -345,6 +360,7 @@ void pc_cmos_init(ram_addr_t ram_size,
> > ram_addr_t above_4g_mem_size, int val, nb, i;
> >      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
> >      static pc_cmos_init_late_arg arg;
> > +    static rtc_cpu_hotplug_arg cpu_hotplug_cb;
> >  
> >      /* various important CMOS locations needed by PC/Bochs bios */
> >  
> > @@ -383,6 +399,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t
> > above_4g_mem_size, 
> >      /* set the number of CPU */
> >      rtc_set_memory(s, 0x5f, smp_cpus - 1);
> > +    /* init CPU hotplug notifier */
> > +    cpu_hotplug_cb.rtc_state = s;
> > +    cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
> > +    qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
> >  
> >      /* set boot devices, and disable floppy signature check if requested
> > */ if (set_boot_dev(s, boot_device, fd_bootchk)) {
> > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > index 69e6844..e639942 100644
> > --- a/hw/timer/mc146818rtc.c
> > +++ b/hw/timer/mc146818rtc.c
> > @@ -677,6 +677,13 @@ void rtc_set_memory(ISADevice *dev, int addr, int
> > val) s->cmos_data[addr] = val;
> >  }
> >  
> > +int rtc_get_memory(ISADevice *dev, int addr)
> > +{
> > +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
> 
> If we apply my mc146818rtc.c QOM'ification patch to qom-cpu first, this
> can become MC146818_RTC(dev). Anthony just ack'ed that, but qemu.git is
> currently broken for testing, so I'm still holding off changes.
if you push it into your qom-cpu tree now, I'll rebase on top of it.

> 
> Andreas
> 
> > +    assert(addr >= 0 && addr <= 127);
> > +    return s->cmos_data[addr];
> > +}
> > +
> >  static void rtc_set_date_from_host(ISADevice *dev)
> >  {
> >      RTCState *s = DO_UPCAST(RTCState, dev, dev);
> > diff --git a/include/hw/timer/mc146818rtc.h
> > b/include/hw/timer/mc146818rtc.h index 854ea3f..09f37b7 100644
> > --- a/include/hw/timer/mc146818rtc.h
> > +++ b/include/hw/timer/mc146818rtc.h
> > @@ -6,6 +6,7 @@
> >  
> >  ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq);
> >  void rtc_set_memory(ISADevice *dev, int addr, int val);
> > +int rtc_get_memory(ISADevice *dev, int addr);
> >  void rtc_set_date(ISADevice *dev, const struct tm *tm);
> >  
> >  #endif /* !MC146818RTC_H */
>