Re: [Qemu-devel] [RFC][PATCH 6/6] i386: implement cpu interface 'cpu_common_unrealizefn'

["Andreas Färber" <afaerber@suse.de>]

Am 29.08.2013 04:09, schrieb Chen Fan:
> Implement cpu interface 'cpu_common_unrealizefn' for emiting vcpu-remove
> notifier to ACPI, then ACPI could send sci interrupt to OS for hot-remove
> vcpu.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 19 ++++++++++++++++++-
>  qom/cpu.c    | 13 +++++++++++++
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 75fc9bb..9a87ac0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -960,7 +960,24 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>  
>  void pc_hot_del_cpu(const int64_t id, Error **errp)
>  {
> -    /* TODO: hot remove VCPU. */
> +    CPUState *s = NULL;

"cs" since this is in PC code, to free the "s" identifier.

> +    X86CPU *cpu = NULL;
> +    DeviceState *ds = NULL;

"dev" please for consistency.

> +    DeviceClass *dc = NULL;
> +
> +    s = qemu_get_cpu(id);

Same question as on 2/6, whether id == cpu_index here.

> +    if (s == NULL) {
> +        error_setg(errp, "Unable to find cpu-index: %" PRIi64
> +                   ",it non-exists or has been deleted.", id);

Space after comma please in English language and "doesn't exist".

> +        return;
> +    }
> +
> +    cpu = X86_CPU(s);
> +    ds = DEVICE(cpu);
> +    dc = DEVICE_GET_CLASS(ds);
> +    if (dc->unrealize) {
> +        dc->unrealize(ds, errp);
> +    }

Never call this directly, this is missing important device cleanups such
as VMState. What you want is:
object_property_set_bool(OBJECT(cs), false, "realized", errp);

>  }
>  
>  void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3439c5d..d2b0c9e 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -239,6 +239,18 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>      }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    CPUNotifier notifier;
> +
> +    notifier.dev = dev;
> +    notifier.type = UNPLUG;
> +
> +    if (dev->hotplugged) {
> +        notifier_list_notify(&cpu_hotplug_notifiers, &notifier);
> +     }

Apart from the misindentation this looks wrong. If we initialize a
machine with -smp 2 we should be allowed to hot-unplug CPU 1 IMO. Are
there any reasons not to allow that?

I would simply drop the dev->hotplugged check and always emit the
notification. When QEMU is shut down, the unrealizefn and finalizers are
not called, I believe.

Regards,
Andreas

> +}
> +
>  static void cpu_common_initfn(Object *obj)
>  {
>      CPUState *cpu = CPU(obj);
> @@ -269,6 +281,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->gdb_read_register = cpu_common_gdb_read_register;
>      k->gdb_write_register = cpu_common_gdb_write_register;
>      dc->realize = cpu_common_realizefn;
> +    dc->unrealize = cpu_common_unrealizefn;
>      dc->no_user = 1;
>  }
>  
> 


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