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

[Andreas Färber]

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 <address@hidden>
> ---
>  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?

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

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.

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg