Re: [Qemu-devel] [PATCH 05/16] introduce CPU hot-plug notifier

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 22 Apr 2013 14:24:00 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Mon, Apr 22, 2013 at 01:09:19PM +0200, Igor Mammedov wrote:
> > On Mon, 22 Apr 2013 14:00:01 +0300
> > Gleb Natapov <gleb@redhat.com> wrote:
> > 
> > > On Tue, Apr 16, 2013 at 12:12:45AM +0200, Igor Mammedov wrote:
> > > > hot-added CPU will be distributed to acpi_piix4 and rtc_cmos
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > v2:
> > > >   * move notifier to qom/cpu.c and call it from CPUClass.realize() on
> > > > hotplug
> > > >   * remove get_firmware_id() since it belong to other patch
> > > > ---
> > > >  include/sysemu/sysemu.h |  3 +++
> > > >  qom/cpu.c               | 12 ++++++++++++
> > > >  2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > > > index 6578782..a8c3de1 100644
> > > > --- a/include/sysemu/sysemu.h
> > > > +++ b/include/sysemu/sysemu.h
> > > > @@ -152,6 +152,9 @@ void do_pci_device_hot_remove(Monitor *mon, const
> > > > QDict *qdict); /* generic hotplug */
> > > >  void drive_hot_add(Monitor *mon, const QDict *qdict);
> > > >  
> > > > +/* CPU hotplug */
> > > > +void qemu_register_cpu_added_notifier(Notifier *notifier);
> > > > +
> > > >  /* pcie aer error injection */
> > > >  void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
> > > >  int do_pcie_aer_inject_error(Monitor *mon,
> > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > index 40a4259..819986e 100644
> > > > --- a/qom/cpu.c
> > > > +++ b/qom/cpu.c
> > > > @@ -21,6 +21,17 @@
> > > >  #include "qom/cpu.h"
> > > >  #include "qemu-common.h"
> > > >  #include "sysemu/kvm.h"
> > > > +#include "qemu/notify.h"
> > > > +#include "sysemu/sysemu.h"
> > > > +
> > > > +/* CPU hot-plug notifiers */
> > > > +static NotifierList cpu_added_notifiers =
> > > > +    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
> > > > +
> > > > +void qemu_register_cpu_added_notifier(Notifier *notifier)
> > > > +{
> > > > +    notifier_list_add(&cpu_added_notifiers, notifier);
> > > > +}
> > > >  
> > > >  void cpu_reset_interrupt(CPUState *cpu, int mask)
> > > >  {
> > > > @@ -61,6 +72,7 @@ static void cpu_common_realizefn(DeviceState *dev,
> > > > Error **errp) if (dev->hotplugged) {
> > > >          cpu_synchronize_post_init(CPU(dev));
> > > >          resume_vcpu(CPU(dev));
> > > > +        notifier_list_notify(&cpu_added_notifiers, dev);
> > > Why do it after vcpu is running? May be notifier want to do something
> > > that should be done before vcpu is running.
> > currently there is not need for adding_cpu notifier.
> > This one signals that there is a new fully functional CPU available.
> > 
> You are using it to update cmos information for BIOS. It would be strange
> for newly added vcpu to see incorrect number of cpus in cmos if, for
> some reason, it will check it. The "new cpu is added and ready to tun,
> do your last things before it is launched" callback seams more reasonable
> then "new cpu is running wild" one.
IRC,
I'm moving it between cpu_synchronize_post_init() and resume_vcpu(), since
cpu_synchronize_post_init() pushes apic_state into kvm.

> --
> 			Gleb.