[Qemu-devel] [RFC 00/12] target-i386: CPU hot-add with cpu_set QMP command

[Qemu-devel] [RFC 00/12] target-i386: CPU hot-add with cpu_set QMP command

[Igor Mammedov]

Implements alternative way for hot-adding CPU using cpu_set QMP command,
wich could be useful until it would be possible to add CPUs via device_add.

Patches 
  1-10 are common to both approaches
  11-12 are specific to adding CPU using cpu_set

To hot-add CPU use following command from qmp-shell:
 cpu_set in=[apic id] status=online

git tree for testing: https://github.com/imammedo/qemu/tree/cpu_set

Igor Mammedov (12):
  target-i386: consolidate error propagation in x86_cpu_realizefn()
  target-i386: split APIC creation from initialization in
    x86_cpu_realizefn()
  target-i386: split out CPU creation and features parsing into
    cpu_x86_create()
  target-i386: introduce apic-id property
  target-i386: push hot-plugged VCPU state to KVM and unstop it
  target-i386: replace FROM_SYSBUS() with QOM type cast
  target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it
  introduce CPU hot-plug notifier
  rtc: update rtc_cmos on CPU hot-plug
  acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest
  qmp: add cpu_set qmp command
  target-i386: implement CPU hot-add

 cpus.c                                  |   11 ++-
 hw/acpi_piix4.c                         |  107 ++++++++++++++++++++-
 hw/apic_common.c                        |   14 ++-
 hw/apic_internal.h                      |    6 +-
 hw/i386/Makefile.objs                   |    2 +-
 hw/i386/kvmvapic.c                      |   22 +++--
 hw/i386/pc.c                            |   47 +++++++++-
 hw/icc_bus.c                            |   75 ++++++++++++++
 hw/icc_bus.h                            |   47 +++++++++
 hw/mc146818rtc.c                        |   12 +++
 include/sysemu/cpus.h                   |    3 +
 include/sysemu/sysemu.h                 |    6 +
 qapi-schema.json                        |    9 ++
 qmp-commands.hx                         |   26 +++++
 qmp.c                                   |   12 +++
 stubs/Makefile.objs                     |    4 +
 stubs/do_cpu_hot_add.c                  |    7 ++
 stubs/get_icc_bus.c                     |    6 +
 stubs/qemu_system_cpu_hotplug_request.c |    5 +
 stubs/resume_vcpu.c                     |    6 +
 target-i386/cpu.c                       |  164 ++++++++++++++++++++++++++-----
 target-i386/cpu.h                       |    3 +
 vl.c                                    |   14 +++
 23 files changed, 561 insertions(+), 47 deletions(-)
 create mode 100644 hw/icc_bus.c
 create mode 100644 hw/icc_bus.h
 create mode 100644 stubs/do_cpu_hot_add.c
 create mode 100644 stubs/get_icc_bus.c
 create mode 100644 stubs/qemu_system_cpu_hotplug_request.c
 create mode 100644 stubs/resume_vcpu.c

[Qemu-devel] [PATCH 01/12] target-i386: consolidate error propagation in x86_cpu_realizefn()

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a0640db..e905bcf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2100,9 +2100,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;
-#ifndef CONFIG_USER_ONLY
     Error *local_err = NULL;
-#endif
 
     if (env->cpuid_7_0_ebx_features && env->cpuid_level < 7) {
         env->cpuid_level = 7;
@@ -2135,8 +2133,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 #endif
         if (check_cpuid && kvm_check_features_against_host(cpu)
             && enforce_cpuid) {
-            error_setg(errp, "Host's CPU doesn't support requested features");
-            return;
+            error_setg(&local_err,
+                       "Host's CPU doesn't support requested features");
+            goto out;
         }
     }
 
@@ -2146,8 +2145,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
         x86_cpu_apic_init(cpu, &local_err);
         if (local_err != NULL) {
-            error_propagate(errp, local_err);
-            return;
+            goto out;
         }
     }
 #endif
@@ -2156,7 +2154,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_init_vcpu(&cpu->env);
     cpu_reset(CPU(cpu));
 
-    xcc->parent_realize(dev, errp);
+    xcc->parent_realize(dev, &local_err);
+out:
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 /* Enables contiguous-apic-ID mode, for compatibility */
-- 
1.7.1

Re: [Qemu-devel] [PATCH 01/12] target-i386: consolidate error propagation in x86_cpu_realizefn()

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a0640db..e905bcf 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2100,9 +2100,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      X86CPU *cpu = X86_CPU(dev);
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>      CPUX86State *env = &cpu->env;
> -#ifndef CONFIG_USER_ONLY
>      Error *local_err = NULL;
> -#endif
>  
>      if (env->cpuid_7_0_ebx_features && env->cpuid_level < 7) {
>          env->cpuid_level = 7;
> @@ -2135,8 +2133,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  #endif
>          if (check_cpuid && kvm_check_features_against_host(cpu)
>              && enforce_cpuid) {
> -            error_setg(errp, "Host's CPU doesn't support requested features");
> -            return;
> +            error_setg(&local_err,
> +                       "Host's CPU doesn't support requested features");
> +            goto out;
>          }
>      }
>  
> @@ -2146,8 +2145,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
>          x86_cpu_apic_init(cpu, &local_err);
>          if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> -            return;
> +            goto out;
>          }
>      }
>  #endif
> @@ -2156,7 +2154,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      qemu_init_vcpu(&cpu->env);
>      cpu_reset(CPU(cpu));
>  
> -    xcc->parent_realize(dev, errp);
> +    xcc->parent_realize(dev, &local_err);
> +out:
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>  
>  /* Enables contiguous-apic-ID mode, for compatibility */
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 01/12] target-i386: consolidate error propagation in x86_cpu_realizefn()

[Eduardo Habkost]

On Thu, Mar 21, 2013 at 03:28:34PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  target-i386/cpu.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a0640db..e905bcf 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2100,9 +2100,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      X86CPU *cpu = X86_CPU(dev);
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>      CPUX86State *env = &cpu->env;
> -#ifndef CONFIG_USER_ONLY
>      Error *local_err = NULL;
> -#endif
>  
>      if (env->cpuid_7_0_ebx_features && env->cpuid_level < 7) {
>          env->cpuid_level = 7;
> @@ -2135,8 +2133,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  #endif
>          if (check_cpuid && kvm_check_features_against_host(cpu)
>              && enforce_cpuid) {
> -            error_setg(errp, "Host's CPU doesn't support requested features");
> -            return;
> +            error_setg(&local_err,
> +                       "Host's CPU doesn't support requested features");
> +            goto out;
>          }
>      }
>  
> @@ -2146,8 +2145,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
>          x86_cpu_apic_init(cpu, &local_err);
>          if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> -            return;
> +            goto out;
>          }
>      }
>  #endif
> @@ -2156,7 +2154,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      qemu_init_vcpu(&cpu->env);
>      cpu_reset(CPU(cpu));
>  
> -    xcc->parent_realize(dev, errp);
> +    xcc->parent_realize(dev, &local_err);
> +out:
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>  
>  /* Enables contiguous-apic-ID mode, for compatibility */
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH 02/12] target-i386: split APIC creation from initialization in x86_cpu_realizefn()

[Igor Mammedov]

When APIC is hotplugged during CPU hotplug, device_set_realized()
calls device_reset() on it. And if QEMU runs in KVM mode, following
call chain will fail:
    apic_reset_common()
        -> kvm_apic_vapic_base_update()
            -> kvm_vcpu_ioctl(cpu->kvm_fd,...)
due to cpu->kvm_fd not being initialized yet.

cpu->kvm_fd is initialized during qemu_init_vcpu() call but x86_cpu_apic_init()
can't be moved after it because kvm_init_vcpu() -> kvm_arch_reset_vcpu()
relies on APIC to determine if CPU is BSP for setting initial env->mp_state.

So split APIC device creation from its initialization and realize APIC
after CPU is created, when it's safe to call APIC's reset method.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e905bcf..affbb76 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2051,9 +2051,8 @@ static void mce_init(X86CPU *cpu)
 #define MSI_ADDR_BASE 0xfee00000
 
 #ifndef CONFIG_USER_ONLY
-static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    static int apic_mapped;
     CPUX86State *env = &cpu->env;
     APICCommonState *apic;
     const char *apic_type = "apic";
@@ -2076,6 +2075,16 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
     /* TODO: convert to link<> */
     apic = APIC_COMMON(env->apic_state);
     apic->cpu = cpu;
+}
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+    CPUX86State *env = &cpu->env;
+    static int apic_mapped;
+
+    if (env->apic_state == NULL) {
+        return;
+    }
 
     if (qdev_init(env->apic_state)) {
         error_setg(errp, "APIC device '%s' could not be initialized",
@@ -2093,6 +2102,10 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
         apic_mapped = 1;
     }
 }
+#else
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+}
 #endif
 
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -2143,7 +2156,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
     if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
-        x86_cpu_apic_init(cpu, &local_err);
+        x86_cpu_apic_create(cpu, &local_err);
         if (local_err != NULL) {
             goto out;
         }
@@ -2152,6 +2165,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     mce_init(cpu);
     qemu_init_vcpu(&cpu->env);
+
+    x86_cpu_apic_init(cpu, &local_err);
+    if (local_err != NULL) {
+        goto out;
+    }
     cpu_reset(CPU(cpu));
 
     xcc->parent_realize(dev, &local_err);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 02/12] target-i386: split APIC creation from initialization in x86_cpu_realizefn()

[Andreas Färber]

Am 21.03.2013 15:28, schrieb Igor Mammedov:
> When APIC is hotplugged during CPU hotplug, device_set_realized()
> calls device_reset() on it. And if QEMU runs in KVM mode, following
> call chain will fail:
>     apic_reset_common()
>         -> kvm_apic_vapic_base_update()
>             -> kvm_vcpu_ioctl(cpu->kvm_fd,...)
> due to cpu->kvm_fd not being initialized yet.
> 
> cpu->kvm_fd is initialized during qemu_init_vcpu() call but x86_cpu_apic_init()
> can't be moved after it because kvm_init_vcpu() -> kvm_arch_reset_vcpu()
> relies on APIC to determine if CPU is BSP for setting initial env->mp_state.
> 
> So split APIC device creation from its initialization and realize APIC
> after CPU is created, when it's safe to call APIC's reset method.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |   24 +++++++++++++++++++++---
>  1 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e905bcf..affbb76 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2051,9 +2051,8 @@ static void mce_init(X86CPU *cpu)
>  #define MSI_ADDR_BASE 0xfee00000
>  
>  #ifndef CONFIG_USER_ONLY
> -static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  {
> -    static int apic_mapped;
>      CPUX86State *env = &cpu->env;
>      APICCommonState *apic;
>      const char *apic_type = "apic";
> @@ -2076,6 +2075,16 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(env->apic_state);
>      apic->cpu = cpu;
> +}
> +
> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> +{
> +    CPUX86State *env = &cpu->env;
> +    static int apic_mapped;
> +
> +    if (env->apic_state == NULL) {
> +        return;
> +    }
>  
>      if (qdev_init(env->apic_state)) {
>          error_setg(errp, "APIC device '%s' could not be initialized",
> @@ -2093,6 +2102,10 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
>          apic_mapped = 1;
>      }
>  }
> +#else
> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> +{
> +}
>  #endif
>  
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> @@ -2143,7 +2156,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>  
>      if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
> -        x86_cpu_apic_init(cpu, &local_err);
> +        x86_cpu_apic_create(cpu, &local_err);
>          if (local_err != NULL) {
>              goto out;
>          }
> @@ -2152,6 +2165,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      mce_init(cpu);
>      qemu_init_vcpu(&cpu->env);
> +
> +    x86_cpu_apic_init(cpu, &local_err);

If we use the function like this, my request on IRC was to rename it to
_realize please instead of _init.

What's the plan for APIC link<>s above? I'm not opposed to the function
split, but I wondered if - since we know there are only three possible
APIC types - a union would allow us to make this an _initialize
function, more closely following the QOM workflow? Could be done as
follow-up.

Andreas

> +    if (local_err != NULL) {
> +        goto out;
> +    }
>      cpu_reset(CPU(cpu));
>  
>      xcc->parent_realize(dev, &local_err);
> 


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

Re: [Qemu-devel] [PATCH 02/12] target-i386: split APIC creation from initialization in x86_cpu_realizefn()

[Igor Mammedov]

On Thu, 04 Apr 2013 10:59:55 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 21.03.2013 15:28, schrieb Igor Mammedov:
> > When APIC is hotplugged during CPU hotplug, device_set_realized()
> > calls device_reset() on it. And if QEMU runs in KVM mode, following
> > call chain will fail:
> >     apic_reset_common()
> >         -> kvm_apic_vapic_base_update()
> >             -> kvm_vcpu_ioctl(cpu->kvm_fd,...)
> > due to cpu->kvm_fd not being initialized yet.
> > 
> > cpu->kvm_fd is initialized during qemu_init_vcpu() call but x86_cpu_apic_init()
> > can't be moved after it because kvm_init_vcpu() -> kvm_arch_reset_vcpu()
> > relies on APIC to determine if CPU is BSP for setting initial env->mp_state.
> > 
> > So split APIC device creation from its initialization and realize APIC
> > after CPU is created, when it's safe to call APIC's reset method.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |   24 +++++++++++++++++++++---
> >  1 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e905bcf..affbb76 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2051,9 +2051,8 @@ static void mce_init(X86CPU *cpu)
> >  #define MSI_ADDR_BASE 0xfee00000
> >  
> >  #ifndef CONFIG_USER_ONLY
> > -static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >  {
> > -    static int apic_mapped;
> >      CPUX86State *env = &cpu->env;
> >      APICCommonState *apic;
> >      const char *apic_type = "apic";
> > @@ -2076,6 +2075,16 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> >      /* TODO: convert to link<> */
> >      apic = APIC_COMMON(env->apic_state);
> >      apic->cpu = cpu;
> > +}
> > +
> > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +{
> > +    CPUX86State *env = &cpu->env;
> > +    static int apic_mapped;
> > +
> > +    if (env->apic_state == NULL) {
> > +        return;
> > +    }
> >  
> >      if (qdev_init(env->apic_state)) {
> >          error_setg(errp, "APIC device '%s' could not be initialized",
> > @@ -2093,6 +2102,10 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> >          apic_mapped = 1;
> >      }
> >  }
> > +#else
> > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +{
> > +}
> >  #endif
> >  
> >  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > @@ -2143,7 +2156,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> >  
> >      if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
> > -        x86_cpu_apic_init(cpu, &local_err);
> > +        x86_cpu_apic_create(cpu, &local_err);
> >          if (local_err != NULL) {
> >              goto out;
> >          }
> > @@ -2152,6 +2165,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >  
> >      mce_init(cpu);
> >      qemu_init_vcpu(&cpu->env);
> > +
> > +    x86_cpu_apic_init(cpu, &local_err);
> 
> If we use the function like this, my request on IRC was to rename it to
> _realize please instead of _init.
Sure, I'll do it.
> 
> What's the plan for APIC link<>s above? I'm not opposed to the function
> split, but I wondered if - since we know there are only three possible
> APIC types - a union would allow us to make this an _initialize
> function, more closely following the QOM workflow? Could be done as
> follow-up.
I plan to post follow on for APIC links<> after this is in, based on your
original patch.
But for links to work we need CPU to have parent before it's realize is
called. I'll add qdev patch to this series that in device_set_realized() sets
parent before child's realize() is called. That would make
device_set_realized() consistent with device_add() where parent set before
realize() and will allow to use links<> in realize() of children of DEVICE.

About union, I've tried that it some time ago but gave up due to another
header deps hell it unleashes when embedding APIC in CPU.
It was doable though, perhaps after CPU unplug there will be time for it.

> 
> Andreas
> 
> > +    if (local_err != NULL) {
> > +        goto out;
> > +    }
> >      cpu_reset(CPU(cpu));
> >  
> >      xcc->parent_realize(dev, &local_err);
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 03/12] target-i386: split out CPU creation and features parsing into cpu_x86_create()

[Igor Mammedov]

Move CPU creation and features parsing into a separate cpu_x86_create()
function, so that board would be able to set board specific CPU
properties before CPU is realized.

Keep cpu_x86_init() for compatibility with the code that uses cpu_init()
and doesn't need to modify CPU properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   28 +++++++++++++++++++---------
 target-i386/cpu.h |    1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index affbb76..c20b81f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1562,17 +1562,16 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu = NULL;
     CPUX86State *env;
     gchar **model_pieces;
     char *name, *features;
-    Error *error = NULL;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        error_setg(&error, "Invalid/empty CPU model name");
+        error_setg(errp, "Invalid/empty CPU model name");
         goto out;
     }
     name = model_pieces[0];
@@ -1582,23 +1581,34 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    cpu_x86_register(cpu, name, &error);
-    if (error) {
+    cpu_x86_register(cpu, name, errp);
+    if (error_is_set(errp)) {
         goto out;
     }
 
-    cpu_x86_parse_featurestr(cpu, features, &error);
-    if (error) {
+    cpu_x86_parse_featurestr(cpu, features, errp);
+    if (error_is_set(errp)) {
         goto out;
     }
 
-    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
+out:
+    g_strfreev(model_pieces);
+    return cpu;
+}
+
+X86CPU *cpu_x86_init(const char *cpu_model)
+{
+    Error *error = NULL;
+    X86CPU *cpu;
+
+    cpu = cpu_x86_create(cpu_model, &error);
     if (error) {
         goto out;
     }
 
+    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
+
 out:
-    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 48f41ca..7de18a2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -896,6 +896,7 @@ typedef struct CPUX86State {
 #include "cpu-qom.h"
 
 X86CPU *cpu_x86_init(const char *cpu_model);
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 void x86_cpudef_setup(void);
-- 
1.7.1

[Qemu-devel] [PATCH 04/12] target-i386: introduce apic-id property

[Igor Mammedov]

... and use it from board level to set APIC ID for CPUs
it creates.

pc_new_cpu() function will be reused later from CPU hot-plug
QMP handler.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      |   25 +++++++++++++++++-
 target-i386/cpu.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu.h |    2 +
 3 files changed, 99 insertions(+), 1 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ed7d9ba..7481f73 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -863,9 +863,29 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
+{
+    X86CPU *cpu;
+
+    cpu = cpu_x86_create(cpu_model, errp);
+    if (!cpu) {
+        return;
+    }
+
+    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
+    object_property_set_bool(OBJECT(cpu), true, "realized", errp);
+
+    if (error_is_set(errp)) {
+        if (cpu != NULL) {
+            object_unref(OBJECT(cpu));
+        }
+    }
+}
+
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
+    Error *error = NULL;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -877,7 +897,10 @@ void pc_cpus_init(const char *cpu_model)
     }
 
     for (i = 0; i < smp_cpus; i++) {
-        if (!cpu_x86_init(cpu_model)) {
+        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
+        if (error) {
+            fprintf(stderr, "%s\n", error_get_pretty(error));
+            error_free(error);
             exit(1);
         }
     }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c20b81f..d03ff73 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -41,6 +41,7 @@
 #endif
 
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen.h"
 #include "hw/sysbus.h"
@@ -1271,6 +1272,77 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+/**
+ * x86_cpu_is_cpu_exist:
+ * @obj - root of QOM tree to start search from
+ * @opaque - pointer to int64_t variable with searched apic_id
+ *
+ * Search for CPU with APIC ID specified by @opaque.
+ *
+ * Returns: 1 - CPU is found, 0 - CPU isn't found
+ */
+int x86_cpu_is_cpu_exist(Object *obj, void *opaque)
+{
+    int64_t apic_id = *(int64_t *)opaque;
+    Object *cpu_obj = object_dynamic_cast(obj, TYPE_X86_CPU);
+
+    if (cpu_obj) {
+        X86CPU *cpu = X86_CPU(cpu_obj);
+        if (cpu->env.cpuid_apic_id == apic_id) {
+            return 1;
+        }
+    }
+    return object_child_foreach(obj, x86_cpu_is_cpu_exist, opaque);
+}
+
+static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    int64_t value = cpu->env.cpuid_apic_id;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    const int64_t min = 0;
+    const int64_t max = UINT32_MAX;
+    int64_t value;
+
+    visit_type_int(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (value < min || value > max) {
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
+                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
+                   object_get_typename(obj), name, value, min, max);
+        return;
+    }
+
+    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &value)) {
+        error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
+        return;
+    }
+    cpu->env.cpuid_apic_id = value;
+}
+
+static PropertyInfo qdev_prop_apic_id = {
+    .name  = "uint32",
+    .get   = x86_cpuid_get_apic_id,
+    .set   = x86_cpuid_set_apic_id,
+};
+#define DEFINE_PROP_APIC_ID(_n)                                                \
+    DEFINE_PROP(_n, X86CPU, env.cpuid_apic_id, qdev_prop_apic_id, uint32_t)
+
+static Property cpu_x86_properties[] = {
+    DEFINE_PROP_APIC_ID("apic-id"),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
     x86_def_t *def;
@@ -2277,6 +2349,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     xcc->parent_realize = dc->realize;
+    dc->props = cpu_x86_properties;
     dc->realize = x86_cpu_realizefn;
 
     xcc->parent_reset = cc->reset;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 7de18a2..b7ba241 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1268,4 +1268,6 @@ const char *get_register_name_32(unsigned int reg);
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
+int x86_cpu_is_cpu_exist(Object *obj, void *opaque);
+
 #endif /* CPU_I386_H */
-- 
1.7.1

[Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Igor Mammedov]

 * synchronize CPU state to KVM
 * resume CPU, it makes CPU be able to handle interrupts in TCG mode
   and/or with user-space APIC.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpus.c                |   11 ++++++++---
 include/sysemu/cpus.h |    3 +++
 stubs/Makefile.objs   |    1 +
 stubs/resume_vcpu.c   |    6 ++++++
 target-i386/cpu.c     |    5 +++++
 5 files changed, 23 insertions(+), 3 deletions(-)
 create mode 100644 stubs/resume_vcpu.c

diff --git a/cpus.c b/cpus.c
index e919dd7..ae479bf 100644
--- a/cpus.c
+++ b/cpus.c
@@ -973,6 +973,13 @@ void pause_all_vcpus(void)
     }
 }
 
+void resume_vcpu(CPUState *cpu)
+{
+    cpu->stop = false;
+    cpu->stopped = false;
+    qemu_cpu_kick(cpu);
+}
+
 void resume_all_vcpus(void)
 {
     CPUArchState *penv = first_cpu;
@@ -980,9 +987,7 @@ void resume_all_vcpus(void)
     qemu_clock_enable(vm_clock, true);
     while (penv) {
         CPUState *pcpu = ENV_GET_CPU(penv);
-        pcpu->stop = false;
-        pcpu->stopped = false;
-        qemu_cpu_kick(pcpu);
+        resume_vcpu(pcpu);
         penv = penv->next_cpu;
     }
 }
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 6502488..9437df5 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -1,11 +1,14 @@
 #ifndef QEMU_CPUS_H
 #define QEMU_CPUS_H
 
+#include "qom/cpu.h"
+
 /* cpus.c */
 void qemu_init_cpu_loop(void);
 void resume_all_vcpus(void);
 void pause_all_vcpus(void);
 void cpu_stop_current(void);
+void resume_vcpu(CPUState *cpu);
 
 void cpu_synchronize_all_states(void);
 void cpu_synchronize_all_post_reset(void);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9c55b34..28fb4f8 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -23,3 +23,4 @@ stub-obj-y += sysbus.o
 stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
+stub-obj-y += resume_vcpu.o
diff --git a/stubs/resume_vcpu.c b/stubs/resume_vcpu.c
new file mode 100644
index 0000000..383b71a
--- /dev/null
+++ b/stubs/resume_vcpu.c
@@ -0,0 +1,6 @@
+#include "qemu-common.h"
+#include "sysemu/cpus.h"
+
+void resume_vcpu(CPUState *cpu)
+{
+}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d03ff73..631bcd8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2260,6 +2260,11 @@ out:
         error_propagate(errp, local_err);
         return;
     }
+
+    if (dev->hotplugged) {
+        cpu_synchronize_post_init(env);
+        resume_vcpu(CPU(cpu));
+    }
 }
 
 /* Enables contiguous-apic-ID mode, for compatibility */
-- 
1.7.1

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
>  * synchronize CPU state to KVM
>  * resume CPU, it makes CPU be able to handle interrupts in TCG mode
>    and/or with user-space APIC.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  cpus.c                |   11 ++++++++---
>  include/sysemu/cpus.h |    3 +++
>  stubs/Makefile.objs   |    1 +
>  stubs/resume_vcpu.c   |    6 ++++++
>  target-i386/cpu.c     |    5 +++++
>  5 files changed, 23 insertions(+), 3 deletions(-)
>  create mode 100644 stubs/resume_vcpu.c
> 
> diff --git a/cpus.c b/cpus.c
> index e919dd7..ae479bf 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
>      }
>  }
>  
> +void resume_vcpu(CPUState *cpu)
> +{
> +    cpu->stop = false;
> +    cpu->stopped = false;
> +    qemu_cpu_kick(cpu);
> +}
> +
>  void resume_all_vcpus(void)
>  {
>      CPUArchState *penv = first_cpu;
> @@ -980,9 +987,7 @@ void resume_all_vcpus(void)
>      qemu_clock_enable(vm_clock, true);
>      while (penv) {
>          CPUState *pcpu = ENV_GET_CPU(penv);
> -        pcpu->stop = false;
> -        pcpu->stopped = false;
> -        qemu_cpu_kick(pcpu);
> +        resume_vcpu(pcpu);
>          penv = penv->next_cpu;
>      }
>  }
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index 6502488..9437df5 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -1,11 +1,14 @@
>  #ifndef QEMU_CPUS_H
>  #define QEMU_CPUS_H
>  
> +#include "qom/cpu.h"
> +
>  /* cpus.c */
>  void qemu_init_cpu_loop(void);
>  void resume_all_vcpus(void);
>  void pause_all_vcpus(void);
>  void cpu_stop_current(void);
> +void resume_vcpu(CPUState *cpu);
>  
>  void cpu_synchronize_all_states(void);
>  void cpu_synchronize_all_post_reset(void);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9c55b34..28fb4f8 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -23,3 +23,4 @@ stub-obj-y += sysbus.o
>  stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
> +stub-obj-y += resume_vcpu.o
> diff --git a/stubs/resume_vcpu.c b/stubs/resume_vcpu.c
> new file mode 100644
> index 0000000..383b71a
> --- /dev/null
> +++ b/stubs/resume_vcpu.c
> @@ -0,0 +1,6 @@
> +#include "qemu-common.h"
> +#include "sysemu/cpus.h"
> +
> +void resume_vcpu(CPUState *cpu)
> +{
> +}
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d03ff73..631bcd8 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2260,6 +2260,11 @@ out:
>          error_propagate(errp, local_err);
>          return;
>      }
> +
> +    if (dev->hotplugged) {
> +        cpu_synchronize_post_init(env);
> +        resume_vcpu(CPU(cpu));
> +    }

I think the resume_vcpu should not be here, but in the superclass.

Paolo

>  }
>  
>  /* Enables contiguous-apic-ID mode, for compatibility */
> 

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Igor Mammedov]

On Wed, 27 Mar 2013 12:01:20 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> >  * synchronize CPU state to KVM
> >  * resume CPU, it makes CPU be able to handle interrupts in TCG mode
> >    and/or with user-space APIC.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  cpus.c                |   11 ++++++++---
> >  include/sysemu/cpus.h |    3 +++
> >  stubs/Makefile.objs   |    1 +
> >  stubs/resume_vcpu.c   |    6 ++++++
> >  target-i386/cpu.c     |    5 +++++
> >  5 files changed, 23 insertions(+), 3 deletions(-)
> >  create mode 100644 stubs/resume_vcpu.c
> > 
> > diff --git a/cpus.c b/cpus.c
> > index e919dd7..ae479bf 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
> >      }
> >  }
> >  
> > +void resume_vcpu(CPUState *cpu)
> > +{
> > +    cpu->stop = false;
> > +    cpu->stopped = false;
> > +    qemu_cpu_kick(cpu);
> > +}
> > +
> >  void resume_all_vcpus(void)
> >  {
> >      CPUArchState *penv = first_cpu;
> > @@ -980,9 +987,7 @@ void resume_all_vcpus(void)
> >      qemu_clock_enable(vm_clock, true);
> >      while (penv) {
> >          CPUState *pcpu = ENV_GET_CPU(penv);
> > -        pcpu->stop = false;
> > -        pcpu->stopped = false;
> > -        qemu_cpu_kick(pcpu);
> > +        resume_vcpu(pcpu);
> >          penv = penv->next_cpu;
> >      }
> >  }
> > diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> > index 6502488..9437df5 100644
> > --- a/include/sysemu/cpus.h
> > +++ b/include/sysemu/cpus.h
> > @@ -1,11 +1,14 @@
> >  #ifndef QEMU_CPUS_H
> >  #define QEMU_CPUS_H
> >  
> > +#include "qom/cpu.h"
> > +
> >  /* cpus.c */
> >  void qemu_init_cpu_loop(void);
> >  void resume_all_vcpus(void);
> >  void pause_all_vcpus(void);
> >  void cpu_stop_current(void);
> > +void resume_vcpu(CPUState *cpu);
> >  
> >  void cpu_synchronize_all_states(void);
> >  void cpu_synchronize_all_post_reset(void);
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 9c55b34..28fb4f8 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -23,3 +23,4 @@ stub-obj-y += sysbus.o
> >  stub-obj-y += vm-stop.o
> >  stub-obj-y += vmstate.o
> >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> > +stub-obj-y += resume_vcpu.o
> > diff --git a/stubs/resume_vcpu.c b/stubs/resume_vcpu.c
> > new file mode 100644
> > index 0000000..383b71a
> > --- /dev/null
> > +++ b/stubs/resume_vcpu.c
> > @@ -0,0 +1,6 @@
> > +#include "qemu-common.h"
> > +#include "sysemu/cpus.h"
> > +
> > +void resume_vcpu(CPUState *cpu)
> > +{
> > +}
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index d03ff73..631bcd8 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2260,6 +2260,11 @@ out:
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> > +
> > +    if (dev->hotplugged) {
> > +        cpu_synchronize_post_init(env);
> > +        resume_vcpu(CPU(cpu));
> > +    }
> 
> I think the resume_vcpu should not be here, but in the superclass.

Then I should move following parts to superclass:

    if (dev->hotplugged) {
        cpu_synchronize_post_init(env);
        resume_vcpu(CPU(cpu));
    }

because in case of KVM we should make sure that CPU in sane state before
allowing CPU to become run-able.

> Paolo
> 
> >  }
> >  
> >  /* Enables contiguous-apic-ID mode, for compatibility */
> > 
> 
> 

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Andreas Färber]

Am 27.03.2013 13:12, schrieb Igor Mammedov:
> On Wed, 27 Mar 2013 12:01:20 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index d03ff73..631bcd8 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -2260,6 +2260,11 @@ out:
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>> +
>>> +    if (dev->hotplugged) {
>>> +        cpu_synchronize_post_init(env);
>>> +        resume_vcpu(CPU(cpu));
>>> +    }
>>
>> I think the resume_vcpu should not be here, but in the superclass.
> 
> Then I should move following parts to superclass:
> 
>     if (dev->hotplugged) {
>         cpu_synchronize_post_init(env);
>         resume_vcpu(CPU(cpu));
>     }
> 
> because in case of KVM we should make sure that CPU in sane state before
> allowing CPU to become run-able.

That's not possible until we change cpu_synchronize_post_init() argument
to CPUState, which is somewhere down my TODO list. Currently I have
mostly flushed my refactorings out, so if you wanted to dive into that,
that would be appreciated. :)

Andreas

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

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Igor Mammedov]

On Wed, 27 Mar 2013 13:17:45 +0100
Andreas Färber <afaerber@suse.de> wrote:
> > Then I should move following parts to superclass:
> > 
> >     if (dev->hotplugged) {
> >         cpu_synchronize_post_init(env);
> >         resume_vcpu(CPU(cpu));
> >     }
> > 
> > because in case of KVM we should make sure that CPU in sane state before
> > allowing CPU to become run-able.
> 
> That's not possible until we change cpu_synchronize_post_init() argument
> to CPUState, which is somewhere down my TODO list. Currently I have
> mostly flushed my refactorings out, so if you wanted to dive into that,
> that would be appreciated. :)
> 
> Andreas

Would something like this be acceptable?
---
From 15502168009b7b5ae2b46d854c461e5ad031cdc6 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <imammedo@redhat.com>
Date: Wed, 27 Mar 2013 14:21:20 +0100
Subject: [PATCH] cpu: Pass CPUState to *cpu_synchronize_post*()

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpus.c               |  4 ++--
 include/sysemu/kvm.h | 12 ++++++------
 kvm-all.c            |  8 ++------
 kvm-stub.c           |  4 ++--
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/cpus.c b/cpus.c
index e919dd7..9b9a32f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -419,7 +419,7 @@ void cpu_synchronize_all_post_reset(void)
     CPUArchState *cpu;
 
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
-        cpu_synchronize_post_reset(cpu);
+        cpu_synchronize_post_reset(ENV_GET_CPU(cpu));
     }
 }
 
@@ -428,7 +428,7 @@ void cpu_synchronize_all_post_init(void)
     CPUArchState *cpu;
 
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
-        cpu_synchronize_post_init(cpu);
+        cpu_synchronize_post_init(ENV_GET_CPU(cpu));
     }
 }
 
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index f2d97b5..495e6f8 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -250,8 +250,8 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 void kvm_cpu_synchronize_state(CPUArchState *env);
-void kvm_cpu_synchronize_post_reset(CPUArchState *env);
-void kvm_cpu_synchronize_post_init(CPUArchState *env);
+void kvm_cpu_synchronize_post_reset(CPUState *cpu);
+void kvm_cpu_synchronize_post_init(CPUState *cpu);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -262,17 +262,17 @@ static inline void cpu_synchronize_state(CPUArchState *env)
     }
 }
 
-static inline void cpu_synchronize_post_reset(CPUArchState *env)
+static inline void cpu_synchronize_post_reset(CPUState *cpu)
 {
     if (kvm_enabled()) {
-        kvm_cpu_synchronize_post_reset(env);
+        kvm_cpu_synchronize_post_reset(cpu);
     }
 }
 
-static inline void cpu_synchronize_post_init(CPUArchState *env)
+static inline void cpu_synchronize_post_init(CPUState *cpu)
 {
     if (kvm_enabled()) {
-        kvm_cpu_synchronize_post_init(env);
+        kvm_cpu_synchronize_post_init(cpu);
     }
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 9b433d3..fc4e17c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1510,18 +1510,14 @@ void kvm_cpu_synchronize_state(CPUArchState *env)
     }
 }
 
-void kvm_cpu_synchronize_post_reset(CPUArchState *env)
+void kvm_cpu_synchronize_post_reset(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
-
     kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
 
-void kvm_cpu_synchronize_post_init(CPUArchState *env)
+void kvm_cpu_synchronize_post_init(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
-
     kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
diff --git a/kvm-stub.c b/kvm-stub.c
index 760aadc..82875dd 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -42,11 +42,11 @@ void kvm_cpu_synchronize_state(CPUArchState *env)
 {
 }
 
-void kvm_cpu_synchronize_post_reset(CPUArchState *env)
+void kvm_cpu_synchronize_post_reset(CPUState *env)
 {
 }
 
-void kvm_cpu_synchronize_post_init(CPUArchState *env)
+void kvm_cpu_synchronize_post_init(CPUState *cpu)
 {
 }
 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Andreas Färber]

Am 27.03.2013 14:27, schrieb Igor Mammedov:
> On Wed, 27 Mar 2013 13:17:45 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>>> Then I should move following parts to superclass:
>>>
>>>     if (dev->hotplugged) {
>>>         cpu_synchronize_post_init(env);
>>>         resume_vcpu(CPU(cpu));
>>>     }
>>>
>>> because in case of KVM we should make sure that CPU in sane state before
>>> allowing CPU to become run-able.
>>
>> That's not possible until we change cpu_synchronize_post_init() argument
>> to CPUState, which is somewhere down my TODO list. Currently I have
>> mostly flushed my refactorings out, so if you wanted to dive into that,
>> that would be appreciated. :)
> 
> Would something like this be acceptable?
> ---
> From 15502168009b7b5ae2b46d854c461e5ad031cdc6 Mon Sep 17 00:00:00 2001
> From: Igor Mammedov <imammedo@redhat.com>
> Date: Wed, 27 Mar 2013 14:21:20 +0100
> Subject: [PATCH] cpu: Pass CPUState to *cpu_synchronize_post*()
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  cpus.c               |  4 ++--
>  include/sysemu/kvm.h | 12 ++++++------
>  kvm-all.c            |  8 ++------
>  kvm-stub.c           |  4 ++--
>  4 files changed, 12 insertions(+), 16 deletions(-)

Looks good so far, did you grep for further occurrences?

Andreas

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

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Igor Mammedov]

On Wed, 27 Mar 2013 15:30:30 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 27.03.2013 14:27, schrieb Igor Mammedov:
> > On Wed, 27 Mar 2013 13:17:45 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> >>> Then I should move following parts to superclass:
> >>>
> >>>     if (dev->hotplugged) {
> >>>         cpu_synchronize_post_init(env);
> >>>         resume_vcpu(CPU(cpu));
> >>>     }
> >>>
> >>> because in case of KVM we should make sure that CPU in sane state before
> >>> allowing CPU to become run-able.
> >>
> >> That's not possible until we change cpu_synchronize_post_init() argument
> >> to CPUState, which is somewhere down my TODO list. Currently I have
> >> mostly flushed my refactorings out, so if you wanted to dive into that,
> >> that would be appreciated. :)
> > 
> > Would something like this be acceptable?
> > ---
> > From 15502168009b7b5ae2b46d854c461e5ad031cdc6 Mon Sep 17 00:00:00 2001
> > From: Igor Mammedov <imammedo@redhat.com>
> > Date: Wed, 27 Mar 2013 14:21:20 +0100
> > Subject: [PATCH] cpu: Pass CPUState to *cpu_synchronize_post*()
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  cpus.c               |  4 ++--
> >  include/sysemu/kvm.h | 12 ++++++------
> >  kvm-all.c            |  8 ++------
> >  kvm-stub.c           |  4 ++--
> >  4 files changed, 12 insertions(+), 16 deletions(-)
> 
> Looks good so far, did you grep for further occurrences?
yep, I re-factored every *cpu_synchronize_post*() call,

but considering an intention to call cpu_synchronize_post_init() from
qom/cpu.c this patch won't work nice since it will pull with itself
kvm-stub.o to *-user target.

Due to qom/cpu.c is built only once for both softmmu and *-user targets, I
consider to move cpu_synchronize_post_init() & cpu_synchronize_post_reset()
from include/sysemu/kvm.h into include/sysemu/cpus.h with definition moved
into cpus.c + stubs for cpu_synchronize_post_init() &resume_vcpu() in
libqemustub for *-user target.
Adding stubs to libqemustub could be avoided if resume_vcpu() and
cpu_synchronize_post_init() are called from x86_cpu_realizefn()
at the cost of some ifdeffenery in include/sysemu/cpus.h though.

But moving resume_vcpu() & cpu_synchronize_post_init() into qom/cpu.c looks
like good candidate for being reused by other targets. 

Paolo,
  would it be acceptable to add resume_vcpu() & cpu_synchronize_post_init()
  stubs into libqemustub? 


> Andreas
> 

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Paolo Bonzini]

Il 27/03/2013 16:16, Igor Mammedov ha scritto:
> yep, I re-factored every *cpu_synchronize_post*() call,
> 
> but considering an intention to call cpu_synchronize_post_init() from
> qom/cpu.c this patch won't work nice since it will pull with itself
> kvm-stub.o to *-user target.
> 
> Due to qom/cpu.c is built only once for both softmmu and *-user targets, I
> consider to move cpu_synchronize_post_init() & cpu_synchronize_post_reset()
> from include/sysemu/kvm.h into include/sysemu/cpus.h with definition moved
> into cpus.c + stubs for cpu_synchronize_post_init() &resume_vcpu() in
> libqemustub for *-user target.
> Adding stubs to libqemustub could be avoided if resume_vcpu() and
> cpu_synchronize_post_init() are called from x86_cpu_realizefn()
> at the cost of some ifdeffenery in include/sysemu/cpus.h though.
> 
> But moving resume_vcpu() & cpu_synchronize_post_init() into qom/cpu.c looks
> like good candidate for being reused by other targets. 
> 
> Paolo,
>   would it be acceptable to add resume_vcpu() & cpu_synchronize_post_init()
>   stubs into libqemustub? 

Can you instead add all of kvm-stub.c?

Paolo

Re: [Qemu-devel] [PATCH 05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

[Igor Mammedov]

On Wed, 27 Mar 2013 16:20:15 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 27/03/2013 16:16, Igor Mammedov ha scritto:
> > yep, I re-factored every *cpu_synchronize_post*() call,
> > 
> > but considering an intention to call cpu_synchronize_post_init() from
> > qom/cpu.c this patch won't work nice since it will pull with itself
> > kvm-stub.o to *-user target.
> > 
> > Due to qom/cpu.c is built only once for both softmmu and *-user targets, I
> > consider to move cpu_synchronize_post_init() & cpu_synchronize_post_reset()
> > from include/sysemu/kvm.h into include/sysemu/cpus.h with definition moved
> > into cpus.c + stubs for cpu_synchronize_post_init() &resume_vcpu() in
> > libqemustub for *-user target.
> > Adding stubs to libqemustub could be avoided if resume_vcpu() and
> > cpu_synchronize_post_init() are called from x86_cpu_realizefn()
> > at the cost of some ifdeffenery in include/sysemu/cpus.h though.
> > 
> > But moving resume_vcpu() & cpu_synchronize_post_init() into qom/cpu.c looks
> > like good candidate for being reused by other targets. 
> > 
> > Paolo,
> >   would it be acceptable to add resume_vcpu() & cpu_synchronize_post_init()
> >   stubs into libqemustub? 
> 
> Can you instead add all of kvm-stub.c?
> 
> Paolo
> 
It's possible but not all of it, I'll post 3 patches that replace 5/12, linked
to this thread.

-- 
Regards,
  Igor

[Qemu-devel] [PATCH 05/14] cpu: Pass CPUState to *cpu_synchronize_post*()

[Igor Mammedov]

... so it could be called from without requiring cpu.h

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpus.c               |  4 ++--
 include/sysemu/kvm.h | 12 ++++++------
 kvm-all.c            |  8 ++------
 kvm-stub.c           |  4 ++--
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/cpus.c b/cpus.c
index e919dd7..9b9a32f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -419,7 +419,7 @@ void cpu_synchronize_all_post_reset(void)
     CPUArchState *cpu;
 
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
-        cpu_synchronize_post_reset(cpu);
+        cpu_synchronize_post_reset(ENV_GET_CPU(cpu));
     }
 }
 
@@ -428,7 +428,7 @@ void cpu_synchronize_all_post_init(void)
     CPUArchState *cpu;
 
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
-        cpu_synchronize_post_init(cpu);
+        cpu_synchronize_post_init(ENV_GET_CPU(cpu));
     }
 }
 
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index f2d97b5..495e6f8 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -250,8 +250,8 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 void kvm_cpu_synchronize_state(CPUArchState *env);
-void kvm_cpu_synchronize_post_reset(CPUArchState *env);
-void kvm_cpu_synchronize_post_init(CPUArchState *env);
+void kvm_cpu_synchronize_post_reset(CPUState *cpu);
+void kvm_cpu_synchronize_post_init(CPUState *cpu);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -262,17 +262,17 @@ static inline void cpu_synchronize_state(CPUArchState *env)
     }
 }
 
-static inline void cpu_synchronize_post_reset(CPUArchState *env)
+static inline void cpu_synchronize_post_reset(CPUState *cpu)
 {
     if (kvm_enabled()) {
-        kvm_cpu_synchronize_post_reset(env);
+        kvm_cpu_synchronize_post_reset(cpu);
     }
 }
 
-static inline void cpu_synchronize_post_init(CPUArchState *env)
+static inline void cpu_synchronize_post_init(CPUState *cpu)
 {
     if (kvm_enabled()) {
-        kvm_cpu_synchronize_post_init(env);
+        kvm_cpu_synchronize_post_init(cpu);
     }
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 9b433d3..fc4e17c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1510,18 +1510,14 @@ void kvm_cpu_synchronize_state(CPUArchState *env)
     }
 }
 
-void kvm_cpu_synchronize_post_reset(CPUArchState *env)
+void kvm_cpu_synchronize_post_reset(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
-
     kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
 
-void kvm_cpu_synchronize_post_init(CPUArchState *env)
+void kvm_cpu_synchronize_post_init(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
-
     kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
diff --git a/kvm-stub.c b/kvm-stub.c
index 760aadc..82875dd 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -42,11 +42,11 @@ void kvm_cpu_synchronize_state(CPUArchState *env)
 {
 }
 
-void kvm_cpu_synchronize_post_reset(CPUArchState *env)
+void kvm_cpu_synchronize_post_reset(CPUState *env)
 {
 }
 
-void kvm_cpu_synchronize_post_init(CPUArchState *env)
+void kvm_cpu_synchronize_post_init(CPUState *cpu)
 {
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/14] cpu: call cpu_synchronize_post_init() from CPUClass.realize() if hotplugged

[Igor Mammedov]

... to synchronize CPU state to KVM

* in addition link kvm-stub.o to *-user target and fix related compiling issues.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 Makefile.target      |  6 ++++++
 include/sysemu/kvm.h | 22 ++++++++++++----------
 kvm-all.c            |  1 +
 kvm-stub.c           |  5 +++++
 qom/cpu.c            |  4 ++++
 vl.c                 |  1 -
 6 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 2bd6d14..0c2c24a 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -76,6 +76,12 @@ obj-y += disas.o
 obj-$(CONFIG_GDBSTUB_XML) += gdbstub-xml.o
 
 #########################################################
+# user emulator target
+ifdef CONFIG_USER_ONLY
+obj-y += kvm-stub.o
+endif #CONFIG_USER_ONLY
+
+#########################################################
 # Linux user emulator target
 
 ifdef CONFIG_LINUX_USER
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 495e6f8..8534e44 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -144,10 +144,10 @@ int kvm_cpu_exec(CPUArchState *env);
 #if !defined(CONFIG_USER_ONLY)
 void *kvm_vmalloc(ram_addr_t size);
 void *kvm_arch_vmalloc(ram_addr_t size);
-void kvm_setup_guest_memory(void *start, size_t size);
+#endif
 
+void kvm_setup_guest_memory(void *start, size_t size);
 void kvm_flush_coalesced_mmio_buffer(void);
-#endif
 
 int kvm_insert_breakpoint(CPUArchState *current_env, target_ulong addr,
                           target_ulong len, int type);
@@ -250,8 +250,6 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 void kvm_cpu_synchronize_state(CPUArchState *env);
-void kvm_cpu_synchronize_post_reset(CPUState *cpu);
-void kvm_cpu_synchronize_post_init(CPUState *cpu);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -262,6 +260,16 @@ static inline void cpu_synchronize_state(CPUArchState *env)
     }
 }
 
+#if !defined(CONFIG_USER_ONLY)
+int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
+                                       hwaddr *phys_addr);
+#endif
+
+#endif
+
+void kvm_cpu_synchronize_post_reset(CPUState *cpu);
+void kvm_cpu_synchronize_post_init(CPUState *cpu);
+
 static inline void cpu_synchronize_post_reset(CPUState *cpu)
 {
     if (kvm_enabled()) {
@@ -277,12 +285,6 @@ static inline void cpu_synchronize_post_init(CPUState *cpu)
 }
 
 
-#if !defined(CONFIG_USER_ONLY)
-int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
-                                       hwaddr *phys_addr);
-#endif
-
-#endif
 int kvm_set_ioeventfd_mmio(int fd, uint32_t adr, uint32_t val, bool assign,
                            uint32_t size);
 
diff --git a/kvm-all.c b/kvm-all.c
index fc4e17c..1d17128 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -109,6 +109,7 @@ bool kvm_async_interrupts_allowed;
 bool kvm_irqfds_allowed;
 bool kvm_msi_via_irqfd_allowed;
 bool kvm_gsi_routing_allowed;
+bool kvm_allowed;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
diff --git a/kvm-stub.c b/kvm-stub.c
index 82875dd..f3e9438 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -12,7 +12,9 @@
 
 #include "qemu-common.h"
 #include "hw/hw.h"
+#ifndef CONFIG_USER_ONLY
 #include "hw/pci/msi.h"
+#endif
 #include "cpu.h"
 #include "exec/gdbstub.h"
 #include "sysemu/kvm.h"
@@ -23,6 +25,7 @@ bool kvm_async_interrupts_allowed;
 bool kvm_irqfds_allowed;
 bool kvm_msi_via_irqfd_allowed;
 bool kvm_gsi_routing_allowed;
+bool kvm_allowed;
 
 int kvm_init_vcpu(CPUState *cpu)
 {
@@ -122,6 +125,7 @@ int kvm_on_sigbus(int code, void *addr)
     return 1;
 }
 
+#ifndef CONFIG_USER_ONLY
 int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
 {
     return -ENOSYS;
@@ -145,3 +149,4 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
 {
     return -ENOSYS;
 }
+#endif
diff --git a/qom/cpu.c b/qom/cpu.c
index e242dcb..0c76712 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -20,6 +20,7 @@
 
 #include "qom/cpu.h"
 #include "qemu-common.h"
+#include "sysemu/kvm.h"
 
 void cpu_reset_interrupt(CPUState *cpu, int mask)
 {
@@ -57,6 +58,9 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
 
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
+    if (dev->hotplugged) {
+        cpu_synchronize_post_init(CPU(dev));
+    }
 }
 
 static void cpu_class_init(ObjectClass *klass, void *data)
diff --git a/vl.c b/vl.c
index 7643f16..d983c1a 100644
--- a/vl.c
+++ b/vl.c
@@ -267,7 +267,6 @@ static NotifierList machine_init_done_notifiers =
     NOTIFIER_LIST_INITIALIZER(machine_init_done_notifiers);
 
 static bool tcg_allowed = true;
-bool kvm_allowed;
 bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/14] cpu: introduce CPUClass.resume() method

[Igor Mammedov]

... and call it if defined from CPUClass.realize() if CPU was hotplugged

by default leave .resume() unset (i.e. NULL) and override it for softmmu
in qemu_init_vcpu() if it's still unset.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpus.c            | 15 ++++++++++++---
 include/qom/cpu.h |  2 ++
 qom/cpu.c         |  5 +++++
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/cpus.c b/cpus.c
index 9b9a32f..6b793c5 100644
--- a/cpus.c
+++ b/cpus.c
@@ -973,6 +973,13 @@ void pause_all_vcpus(void)
     }
 }
 
+static void resume_vcpu(CPUState *cpu)
+{
+    cpu->stop = false;
+    cpu->stopped = false;
+    qemu_cpu_kick(cpu);
+}
+
 void resume_all_vcpus(void)
 {
     CPUArchState *penv = first_cpu;
@@ -980,9 +987,7 @@ void resume_all_vcpus(void)
     qemu_clock_enable(vm_clock, true);
     while (penv) {
         CPUState *pcpu = ENV_GET_CPU(penv);
-        pcpu->stop = false;
-        pcpu->stopped = false;
-        qemu_cpu_kick(pcpu);
+        resume_vcpu(pcpu);
         penv = penv->next_cpu;
     }
 }
@@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env)
 {
     CPUArchState *env = _env;
     CPUState *cpu = ENV_GET_CPU(env);
+    CPUClass *klass = CPU_GET_CLASS(cpu);
 
+    if (klass->resume == NULL) {
+        klass->resume = resume_vcpu;
+    }
     cpu->nr_cores = smp_cores;
     cpu->nr_threads = smp_threads;
     cpu->stopped = true;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3664a1b..6d6eb7a 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -45,6 +45,7 @@ typedef struct CPUState CPUState;
  * instantiatable CPU type.
  * @reset: Callback to reset the #CPUState to its initial state.
  * @do_interrupt: Callback for interrupt handling.
+ * @resume: Callback for putting CPU in runable state
  * @vmsd: State description for migration.
  *
  * Represents a CPU family or model.
@@ -58,6 +59,7 @@ typedef struct CPUClass {
 
     void (*reset)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
+    void (*resume)(CPUState *cpu);
 
     const struct VMStateDescription *vmsd;
 } CPUClass;
diff --git a/qom/cpu.c b/qom/cpu.c
index 0c76712..c062e00 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -58,8 +58,13 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
 
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
+    CPUClass *klass = CPU_GET_CLASS(dev);
+
     if (dev->hotplugged) {
         cpu_synchronize_post_init(CPU(dev));
+        if (klass->resume != NULL) {
+            klass->resume(CPU(dev));
+        }
     }
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/12] target-i386: replace FROM_SYSBUS() with QOM type cast

[Igor Mammedov]

... and define type name and type cast macro for kvmvapic according
to accepted convention.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/kvmvapic.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index c151c95..21551a5 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -62,6 +62,9 @@ typedef struct VAPICROMState {
     bool rom_mapped_writable;
 } VAPICROMState;
 
+#define TYPE_VAPIC_DEVICE "kvmvapic"
+#define VAPIC_DEVICE(obj) OBJECT_CHECK(VAPICROMState, (obj), TYPE_VAPIC_DEVICE)
+
 #define TPR_INSTR_ABS_MODRM             0x1
 #define TPR_INSTR_MATCH_MODRM_REG       0x2
 
@@ -692,7 +695,7 @@ static const MemoryRegionOps vapic_ops = {
 
 static int vapic_init(SysBusDevice *dev)
 {
-    VAPICROMState *s = FROM_SYSBUS(VAPICROMState, dev);
+    VAPICROMState *s = VAPIC_DEVICE(dev);
 
     memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
     sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
@@ -808,7 +811,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo vapic_type = {
-    .name          = "kvmvapic",
+    .name          = TYPE_VAPIC_DEVICE,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(VAPICROMState),
     .class_init    = vapic_class_init,
-- 
1.7.1

Re: [Qemu-devel] [PATCH 06/12] target-i386: replace FROM_SYSBUS() with QOM type cast

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> ... and define type name and type cast macro for kvmvapic according
> to accepted convention.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/kvmvapic.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index c151c95..21551a5 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -62,6 +62,9 @@ typedef struct VAPICROMState {
>      bool rom_mapped_writable;
>  } VAPICROMState;
>  
> +#define TYPE_VAPIC_DEVICE "kvmvapic"
> +#define VAPIC_DEVICE(obj) OBJECT_CHECK(VAPICROMState, (obj), TYPE_VAPIC_DEVICE)
> +
>  #define TPR_INSTR_ABS_MODRM             0x1
>  #define TPR_INSTR_MATCH_MODRM_REG       0x2
>  
> @@ -692,7 +695,7 @@ static const MemoryRegionOps vapic_ops = {
>  
>  static int vapic_init(SysBusDevice *dev)
>  {
> -    VAPICROMState *s = FROM_SYSBUS(VAPICROMState, dev);
> +    VAPICROMState *s = VAPIC_DEVICE(dev);
>  
>      memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
>      sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
> @@ -808,7 +811,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo vapic_type = {
> -    .name          = "kvmvapic",
> +    .name          = TYPE_VAPIC_DEVICE,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(VAPICROMState),
>      .class_init    = vapic_class_init,
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 06/12] target-i386: replace FROM_SYSBUS() with QOM type cast

[Andreas Färber]

Am 21.03.2013 15:28, schrieb Igor Mammedov:
> ... and define type name and type cast macro for kvmvapic according
> to accepted convention.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

This looks great and a cherry-pick candidate if you agree?

Just wondering, was there a name conflict for shorter VAPIC()?
It's file-local, so doesn't really matter.

Andreas

> ---
>  hw/i386/kvmvapic.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index c151c95..21551a5 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -62,6 +62,9 @@ typedef struct VAPICROMState {
>      bool rom_mapped_writable;
>  } VAPICROMState;
>  
> +#define TYPE_VAPIC_DEVICE "kvmvapic"
> +#define VAPIC_DEVICE(obj) OBJECT_CHECK(VAPICROMState, (obj), TYPE_VAPIC_DEVICE)
> +
>  #define TPR_INSTR_ABS_MODRM             0x1
>  #define TPR_INSTR_MATCH_MODRM_REG       0x2
>  
> @@ -692,7 +695,7 @@ static const MemoryRegionOps vapic_ops = {
>  
>  static int vapic_init(SysBusDevice *dev)
>  {
> -    VAPICROMState *s = FROM_SYSBUS(VAPICROMState, dev);
> +    VAPICROMState *s = VAPIC_DEVICE(dev);
>  
>      memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
>      sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
> @@ -808,7 +811,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo vapic_type = {
> -    .name          = "kvmvapic",
> +    .name          = TYPE_VAPIC_DEVICE,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(VAPICROMState),
>      .class_init    = vapic_class_init,
> 


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

Re: [Qemu-devel] [PATCH 06/12] target-i386: replace FROM_SYSBUS() with QOM type cast

[Igor Mammedov]

On Thu, 04 Apr 2013 11:03:53 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 21.03.2013 15:28, schrieb Igor Mammedov:
> > ... and define type name and type cast macro for kvmvapic according
> > to accepted convention.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> This looks great and a cherry-pick candidate if you agree?
Np, BTW I have a second similar clean-up in v2 for IOAPIC

> 
> Just wondering, was there a name conflict for shorter VAPIC()?
> It's file-local, so doesn't really matter.
Nope, I just followed *_DEVICE model like for SYS_BUS_DEVICE()

> 
> Andreas
> 
> > ---
> >  hw/i386/kvmvapic.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > index c151c95..21551a5 100644
> > --- a/hw/i386/kvmvapic.c
> > +++ b/hw/i386/kvmvapic.c
> > @@ -62,6 +62,9 @@ typedef struct VAPICROMState {
> >      bool rom_mapped_writable;
> >  } VAPICROMState;
> >  
> > +#define TYPE_VAPIC_DEVICE "kvmvapic"
> > +#define VAPIC_DEVICE(obj) OBJECT_CHECK(VAPICROMState, (obj), TYPE_VAPIC_DEVICE)
> > +
> >  #define TPR_INSTR_ABS_MODRM             0x1
> >  #define TPR_INSTR_MATCH_MODRM_REG       0x2
> >  
> > @@ -692,7 +695,7 @@ static const MemoryRegionOps vapic_ops = {
> >  
> >  static int vapic_init(SysBusDevice *dev)
> >  {
> > -    VAPICROMState *s = FROM_SYSBUS(VAPICROMState, dev);
> > +    VAPICROMState *s = VAPIC_DEVICE(dev);
> >  
> >      memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
> >      sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
> > @@ -808,7 +811,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
> >  }
> >  
> >  static const TypeInfo vapic_type = {
> > -    .name          = "kvmvapic",
> > +    .name          = TYPE_VAPIC_DEVICE,
> >      .parent        = TYPE_SYS_BUS_DEVICE,
> >      .instance_size = sizeof(VAPICROMState),
> >      .class_init    = vapic_class_init,
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 06/12] target-i386: replace FROM_SYSBUS() with QOM type cast

[Andreas Färber]

Am 04.04.2013 11:59, schrieb Igor Mammedov:
> On Thu, 04 Apr 2013 11:03:53 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 21.03.2013 15:28, schrieb Igor Mammedov:
>>> ... and define type name and type cast macro for kvmvapic according
>>> to accepted convention.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>
>> This looks great and a cherry-pick candidate if you agree?
> Np, BTW I have a second similar clean-up in v2 for IOAPIC
> 
>>
>> Just wondering, was there a name conflict for shorter VAPIC()?
>> It's file-local, so doesn't really matter.
> Nope, I just followed *_DEVICE model like for SYS_BUS_DEVICE()

Okay, then let's avoid unnecessary _DEVICE in your upcoming resend. The
only other place for actual devices where I remember we needed it was to
distinguish between PCI host bridge and its PCI device representation on
the bus. There is also no "Device" in the struct name.

Andreas

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

Re: [Qemu-devel] [PATCH 06/12] target-i386: replace FROM_SYSBUS() with QOM type cast

[Igor Mammedov]

On Thu, 04 Apr 2013 12:05:22 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 04.04.2013 11:59, schrieb Igor Mammedov:
> > On Thu, 04 Apr 2013 11:03:53 +0200
> > Andreas Färber <afaerber@suse.de> wrote:
> > 
> >> Am 21.03.2013 15:28, schrieb Igor Mammedov:
> >>> ... and define type name and type cast macro for kvmvapic according
> >>> to accepted convention.
> >>>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>
> >> This looks great and a cherry-pick candidate if you agree?
> > Np, BTW I have a second similar clean-up in v2 for IOAPIC
> > 
> >>
> >> Just wondering, was there a name conflict for shorter VAPIC()?
> >> It's file-local, so doesn't really matter.
> > Nope, I just followed *_DEVICE model like for SYS_BUS_DEVICE()
> 
> Okay, then let's avoid unnecessary _DEVICE in your upcoming resend. The
> only other place for actual devices where I remember we needed it was to
> distinguish between PCI host bridge and its PCI device representation on
> the bus. There is also no "Device" in the struct name.
done

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


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Igor Mammedov]

Introduce hot-pluggable ICC_BUS and convert APIC devices from SysBusDevice
to ICCDevice wich has ICC_BUS as default one.

 * attach APIC and kvmvapic to ICC_BUS during creation
 * use memory API directly instead of using SysBus proxy functions
 * introduce get_icc_bus() for getting access to system wide ICC_BUS
 * make ICC_BUS default one for X86CPU class, so that device_add would
   set it for CPU instead of SysBus
 * attach cold-pluged CPUs to icc_bus in softmmu mode, so they could be
   uplugged in future
 * if kvmvapic init() fails return -1 instead of aborting.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic_common.c      |   14 ++++++---
 hw/apic_internal.h    |    6 ++--
 hw/i386/Makefile.objs |    2 +-
 hw/i386/kvmvapic.c    |   15 +++++----
 hw/icc_bus.c          |   75 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/icc_bus.h          |   47 ++++++++++++++++++++++++++++++
 stubs/Makefile.objs   |    1 +
 stubs/get_icc_bus.c   |    6 ++++
 target-i386/cpu.c     |   16 ++++++++--
 9 files changed, 162 insertions(+), 20 deletions(-)
 create mode 100644 hw/icc_bus.c
 create mode 100644 hw/icc_bus.h
 create mode 100644 stubs/get_icc_bus.c

diff --git a/hw/apic_common.c b/hw/apic_common.c
index d0c2616..61599d4 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -21,6 +21,7 @@
 #include "hw/apic_internal.h"
 #include "trace.h"
 #include "sysemu/kvm.h"
+#include "qdev.h"
 
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
@@ -282,9 +283,10 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static int apic_init_common(SysBusDevice *dev)
+static int apic_init_common(ICCDevice *dev)
 {
     APICCommonState *s = APIC_COMMON(dev);
+    DeviceState *d = DEVICE(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
@@ -297,12 +299,14 @@ static int apic_init_common(SysBusDevice *dev)
     info = APIC_COMMON_GET_CLASS(s);
     info->init(s);
 
-    sysbus_init_mmio(dev, &s->io_memory);
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
         ram_size >= 1024 * 1024) {
-        vapic = sysbus_create_simple("kvmvapic", -1, NULL);
+        vapic = qdev_try_create(d->parent_bus, "kvmvapic");
+        if ((vapic == NULL) || (qdev_init(vapic) != 0)) {
+            return -1;
+        }
     }
     s->vapic = vapic;
     if (apic_report_tpr_access && info->enable_tpr_reporting) {
@@ -375,7 +379,7 @@ static Property apic_properties_common[] = {
 
 static void apic_common_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
+    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_apic_common;
@@ -387,7 +391,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo apic_common_type = {
     .name = TYPE_APIC_COMMON,
-    .parent = TYPE_SYS_BUS_DEVICE,
+    .parent = TYPE_ICC_DEVICE,
     .instance_size = sizeof(APICCommonState),
     .class_size = sizeof(APICCommonClass),
     .class_init = apic_common_class_init,
diff --git a/hw/apic_internal.h b/hw/apic_internal.h
index 578241f..e7b378e 100644
--- a/hw/apic_internal.h
+++ b/hw/apic_internal.h
@@ -21,7 +21,7 @@
 #define QEMU_APIC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "hw/sysbus.h"
+#include "hw/icc_bus.h"
 #include "qemu/timer.h"
 
 /* APIC Local Vector Table */
@@ -80,7 +80,7 @@ typedef struct APICCommonState APICCommonState;
 
 typedef struct APICCommonClass
 {
-    SysBusDeviceClass parent_class;
+    ICCDeviceClass parent_class;
 
     void (*init)(APICCommonState *s);
     void (*set_base)(APICCommonState *s, uint64_t val);
@@ -94,7 +94,7 @@ typedef struct APICCommonClass
 } APICCommonClass;
 
 struct APICCommonState {
-    SysBusDevice busdev;
+    ICCDevice busdev;
 
     MemoryRegion io_memory;
     X86CPU *cpu;
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index a78c0b2..316f999 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -1,5 +1,5 @@
 obj-y += mc146818rtc.o
-obj-y += apic_common.o apic.o
+obj-y += apic_common.o apic.o icc_bus.o
 obj-y += sga.o ioapic_common.o ioapic.o piix_pci.o
 obj-y += vmport.o
 obj-y += pci/pci-hotplug.o wdt_ib700.o
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 21551a5..7de75db 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -12,6 +12,8 @@
 #include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "hw/apic_internal.h"
+#include "migration/vmstate.h"
+#include "exec/address-spaces.h"
 
 #define APIC_DEFAULT_ADDRESS    0xfee00000
 
@@ -49,7 +51,7 @@ typedef struct GuestROMState {
 } QEMU_PACKED GuestROMState;
 
 typedef struct VAPICROMState {
-    SysBusDevice busdev;
+    ICCDevice busdev;
     MemoryRegion io;
     MemoryRegion rom;
     uint32_t state;
@@ -581,7 +583,7 @@ static void vapic_map_rom_writable(VAPICROMState *s)
     size_t rom_size;
     uint8_t *ram;
 
-    as = sysbus_address_space(&s->busdev);
+    as = get_system_memory();
 
     if (s->rom_mapped_writable) {
         memory_region_del_subregion(as, &s->rom);
@@ -693,13 +695,12 @@ static const MemoryRegionOps vapic_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int vapic_init(SysBusDevice *dev)
+static int vapic_init(ICCDevice *dev)
 {
     VAPICROMState *s = VAPIC_DEVICE(dev);
 
     memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
-    sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
-    sysbus_init_ioports(dev, VAPIC_IO_PORT, 2);
+    memory_region_add_subregion(get_system_io(), VAPIC_IO_PORT, &s->io);
 
     option_rom[nb_option_roms].name = "kvmvapic.bin";
     option_rom[nb_option_roms].bootindex = -1;
@@ -801,7 +802,7 @@ static const VMStateDescription vmstate_vapic = {
 
 static void vapic_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
+    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->no_user = 1;
@@ -812,7 +813,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo vapic_type = {
     .name          = TYPE_VAPIC_DEVICE,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_ICC_DEVICE,
     .instance_size = sizeof(VAPICROMState),
     .class_init    = vapic_class_init,
 };
diff --git a/hw/icc_bus.c b/hw/icc_bus.c
new file mode 100644
index 0000000..b170648
--- /dev/null
+++ b/hw/icc_bus.c
@@ -0,0 +1,75 @@
+/* icc_bus.c
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * Copyright (c) 2013 Red Hat, Inc
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#include "icc_bus.h"
+#include "qemu/module.h"
+
+static const TypeInfo icc_bus_info = {
+    .name = TYPE_ICC_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(ICCBus),
+};
+
+static int icc_device_init(DeviceState *dev)
+{
+    ICCDevice *id = ICC_DEVICE(dev);
+    ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
+
+    return k->init(id);
+}
+
+static void icc_device_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *k = DEVICE_CLASS(klass);
+
+    k->init = icc_device_init;
+    k->bus_type = TYPE_ICC_BUS;
+}
+
+static const TypeInfo icc_device_info = {
+    .name = TYPE_ICC_DEVICE,
+    .parent = TYPE_DEVICE,
+    .abstract = true,
+    .instance_size = sizeof(ICCDevice),
+    .class_size = sizeof(ICCDeviceClass),
+    .class_init = icc_device_class_init,
+};
+
+static BusState *icc_bus;
+
+BusState *get_icc_bus(void)
+{
+    if (icc_bus == NULL) {
+        icc_bus = g_malloc0(icc_bus_info.instance_size);
+        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
+        icc_bus->allow_hotplug = 1;
+        OBJECT(icc_bus)->free = g_free;
+        object_property_add_child(container_get(qdev_get_machine(),
+                                                "/unattached"),
+                                  "icc-bus", OBJECT(icc_bus), NULL);
+    }
+    return BUS(icc_bus);
+}
+
+static void icc_bus_register_types(void)
+{
+    type_register_static(&icc_bus_info);
+    type_register_static(&icc_device_info);
+}
+
+type_init(icc_bus_register_types)
diff --git a/hw/icc_bus.h b/hw/icc_bus.h
new file mode 100644
index 0000000..a00fef5
--- /dev/null
+++ b/hw/icc_bus.h
@@ -0,0 +1,47 @@
+/* icc_bus.h
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * Copyright (c) 2013 Red Hat, Inc
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#ifndef ICC_BUS_H
+#define ICC_BUS_H
+
+#include "hw/qdev-core.h"
+
+typedef struct ICCBus {
+    BusState qbus;
+} ICCBus;
+#define TYPE_ICC_BUS "ICC"
+#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
+
+typedef struct ICCDevice {
+    DeviceState qdev;
+} ICCDevice;
+
+typedef struct ICCDeviceClass {
+    DeviceClass parent_class;
+    int (*init)(ICCDevice *dev);
+} ICCDeviceClass;
+#define TYPE_ICC_DEVICE "icc-device"
+#define ICC_DEVICE(obj) OBJECT_CHECK(ICCDevice, (obj), TYPE_ICC_DEVICE)
+#define ICC_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ICCDeviceClass, (klass), TYPE_ICC_DEVICE)
+#define ICC_DEVICE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ICCDeviceClass, (obj), TYPE_ICC_DEVICE)
+
+BusState *get_icc_bus(void);
+
+#endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 28fb4f8..9741e16 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -24,3 +24,4 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += resume_vcpu.o
+stub-obj-y += get_icc_bus.o
diff --git a/stubs/get_icc_bus.c b/stubs/get_icc_bus.c
new file mode 100644
index 0000000..43db181
--- /dev/null
+++ b/stubs/get_icc_bus.c
@@ -0,0 +1,6 @@
+#include "hw/icc_bus.h"
+
+BusState *get_icc_bus(void)
+{
+    return NULL;
+}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 631bcd8..ae46f81 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -42,10 +42,11 @@
 
 #include "sysemu/sysemu.h"
 #include "hw/qdev-properties.h"
+#include "hw/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen.h"
-#include "hw/sysbus.h"
 #include "hw/apic_internal.h"
+#include "exec/address-spaces.h"
 #endif
 
 static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
@@ -1640,6 +1641,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
     CPUX86State *env;
     gchar **model_pieces;
     char *name, *features;
+    BusState *icc_bus = get_icc_bus();
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
@@ -1650,6 +1652,10 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
     features = model_pieces[1];
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    if (icc_bus) {
+        qdev_set_parent_bus(DEVICE(cpu), icc_bus);
+        object_unref(OBJECT(cpu));
+    }
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
@@ -2145,7 +2151,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         apic_type = "xen-apic";
     }
 
-    env->apic_state = qdev_try_create(NULL, apic_type);
+    env->apic_state = qdev_try_create(get_icc_bus(), apic_type);
     if (env->apic_state == NULL) {
         error_setg(errp, "APIC device '%s' could not be created", apic_type);
         return;
@@ -2176,11 +2182,12 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
 
     /* XXX: mapping more APICs at the same memory location */
     if (apic_mapped == 0) {
+        APICCommonState *apic = APIC_COMMON(env->apic_state);
         /* NOTE: the APIC is directly connected to the CPU - it is not
            on the global memory bus. */
         /* XXX: what if the base changes? */
-        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
-                                MSI_ADDR_BASE, 0x1000);
+        memory_region_add_subregion_overlap(get_system_memory(), MSI_ADDR_BASE,
+                                            &apic->io_memory, 0x1000);
         apic_mapped = 1;
     }
 }
@@ -2356,6 +2363,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     xcc->parent_realize = dc->realize;
     dc->props = cpu_x86_properties;
     dc->realize = x86_cpu_realizefn;
+    dc->bus_type = TYPE_ICC_BUS;
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> Introduce hot-pluggable ICC_BUS and convert APIC devices from SysBusDevice
> to ICCDevice wich has ICC_BUS as default one.
> 
>  * attach APIC and kvmvapic to ICC_BUS during creation
>  * use memory API directly instead of using SysBus proxy functions
>  * introduce get_icc_bus() for getting access to system wide ICC_BUS
>  * make ICC_BUS default one for X86CPU class, so that device_add would
>    set it for CPU instead of SysBus
>  * attach cold-pluged CPUs to icc_bus in softmmu mode, so they could be
>    uplugged in future
>  * if kvmvapic init() fails return -1 instead of aborting.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/apic_common.c      |   14 ++++++---
>  hw/apic_internal.h    |    6 ++--
>  hw/i386/Makefile.objs |    2 +-
>  hw/i386/kvmvapic.c    |   15 +++++----
>  hw/icc_bus.c          |   75 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/icc_bus.h          |   47 ++++++++++++++++++++++++++++++
>  stubs/Makefile.objs   |    1 +
>  stubs/get_icc_bus.c   |    6 ++++
>  target-i386/cpu.c     |   16 ++++++++--
>  9 files changed, 162 insertions(+), 20 deletions(-)
>  create mode 100644 hw/icc_bus.c
>  create mode 100644 hw/icc_bus.h
>  create mode 100644 stubs/get_icc_bus.c
> 
> diff --git a/hw/apic_common.c b/hw/apic_common.c
> index d0c2616..61599d4 100644
> --- a/hw/apic_common.c
> +++ b/hw/apic_common.c
> @@ -21,6 +21,7 @@
>  #include "hw/apic_internal.h"
>  #include "trace.h"
>  #include "sysemu/kvm.h"
> +#include "qdev.h"
>  
>  static int apic_irq_delivered;
>  bool apic_report_tpr_access;
> @@ -282,9 +283,10 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> -static int apic_init_common(SysBusDevice *dev)
> +static int apic_init_common(ICCDevice *dev)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> +    DeviceState *d = DEVICE(dev);
>      APICCommonClass *info;
>      static DeviceState *vapic;
>      static int apic_no;
> @@ -297,12 +299,14 @@ static int apic_init_common(SysBusDevice *dev)
>      info = APIC_COMMON_GET_CLASS(s);
>      info->init(s);
>  
> -    sysbus_init_mmio(dev, &s->io_memory);
>  
>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
>          ram_size >= 1024 * 1024) {
> -        vapic = sysbus_create_simple("kvmvapic", -1, NULL);
> +        vapic = qdev_try_create(d->parent_bus, "kvmvapic");
> +        if ((vapic == NULL) || (qdev_init(vapic) != 0)) {
> +            return -1;
> +        }
>      }
>      s->vapic = vapic;
>      if (apic_report_tpr_access && info->enable_tpr_reporting) {
> @@ -375,7 +379,7 @@ static Property apic_properties_common[] = {
>  
>  static void apic_common_class_init(ObjectClass *klass, void *data)
>  {
> -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->vmsd = &vmstate_apic_common;
> @@ -387,7 +391,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo apic_common_type = {
>      .name = TYPE_APIC_COMMON,
> -    .parent = TYPE_SYS_BUS_DEVICE,
> +    .parent = TYPE_ICC_DEVICE,
>      .instance_size = sizeof(APICCommonState),
>      .class_size = sizeof(APICCommonClass),
>      .class_init = apic_common_class_init,
> diff --git a/hw/apic_internal.h b/hw/apic_internal.h
> index 578241f..e7b378e 100644
> --- a/hw/apic_internal.h
> +++ b/hw/apic_internal.h
> @@ -21,7 +21,7 @@
>  #define QEMU_APIC_INTERNAL_H
>  
>  #include "exec/memory.h"
> -#include "hw/sysbus.h"
> +#include "hw/icc_bus.h"
>  #include "qemu/timer.h"
>  
>  /* APIC Local Vector Table */
> @@ -80,7 +80,7 @@ typedef struct APICCommonState APICCommonState;
>  
>  typedef struct APICCommonClass
>  {
> -    SysBusDeviceClass parent_class;
> +    ICCDeviceClass parent_class;
>  
>      void (*init)(APICCommonState *s);
>      void (*set_base)(APICCommonState *s, uint64_t val);
> @@ -94,7 +94,7 @@ typedef struct APICCommonClass
>  } APICCommonClass;
>  
>  struct APICCommonState {
> -    SysBusDevice busdev;
> +    ICCDevice busdev;
>  
>      MemoryRegion io_memory;
>      X86CPU *cpu;
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index a78c0b2..316f999 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -1,5 +1,5 @@
>  obj-y += mc146818rtc.o
> -obj-y += apic_common.o apic.o
> +obj-y += apic_common.o apic.o icc_bus.o
>  obj-y += sga.o ioapic_common.o ioapic.o piix_pci.o
>  obj-y += vmport.o
>  obj-y += pci/pci-hotplug.o wdt_ib700.o
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 21551a5..7de75db 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -12,6 +12,8 @@
>  #include "sysemu/cpus.h"
>  #include "sysemu/kvm.h"
>  #include "hw/apic_internal.h"
> +#include "migration/vmstate.h"
> +#include "exec/address-spaces.h"
>  
>  #define APIC_DEFAULT_ADDRESS    0xfee00000
>  
> @@ -49,7 +51,7 @@ typedef struct GuestROMState {
>  } QEMU_PACKED GuestROMState;
>  
>  typedef struct VAPICROMState {
> -    SysBusDevice busdev;
> +    ICCDevice busdev;
>      MemoryRegion io;
>      MemoryRegion rom;
>      uint32_t state;
> @@ -581,7 +583,7 @@ static void vapic_map_rom_writable(VAPICROMState *s)
>      size_t rom_size;
>      uint8_t *ram;
>  
> -    as = sysbus_address_space(&s->busdev);
> +    as = get_system_memory();
>  
>      if (s->rom_mapped_writable) {
>          memory_region_del_subregion(as, &s->rom);
> @@ -693,13 +695,12 @@ static const MemoryRegionOps vapic_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static int vapic_init(SysBusDevice *dev)
> +static int vapic_init(ICCDevice *dev)
>  {
>      VAPICROMState *s = VAPIC_DEVICE(dev);
>  
>      memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
> -    sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
> -    sysbus_init_ioports(dev, VAPIC_IO_PORT, 2);
> +    memory_region_add_subregion(get_system_io(), VAPIC_IO_PORT, &s->io);
>  
>      option_rom[nb_option_roms].name = "kvmvapic.bin";
>      option_rom[nb_option_roms].bootindex = -1;
> @@ -801,7 +802,7 @@ static const VMStateDescription vmstate_vapic = {
>  
>  static void vapic_class_init(ObjectClass *klass, void *data)
>  {
> -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->no_user = 1;
> @@ -812,7 +813,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo vapic_type = {
>      .name          = TYPE_VAPIC_DEVICE,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_ICC_DEVICE,
>      .instance_size = sizeof(VAPICROMState),
>      .class_init    = vapic_class_init,
>  };
> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> new file mode 100644
> index 0000000..b170648
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,75 @@
> +/* icc_bus.c
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * Copyright (c) 2013 Red Hat, Inc
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +#include "icc_bus.h"
> +#include "qemu/module.h"
> +
> +static const TypeInfo icc_bus_info = {
> +    .name = TYPE_ICC_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(ICCBus),
> +};
> +
> +static int icc_device_init(DeviceState *dev)
> +{
> +    ICCDevice *id = ICC_DEVICE(dev);
> +    ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
> +
> +    return k->init(id);
> +}
> +
> +static void icc_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *k = DEVICE_CLASS(klass);
> +
> +    k->init = icc_device_init;
> +    k->bus_type = TYPE_ICC_BUS;
> +}
> +
> +static const TypeInfo icc_device_info = {
> +    .name = TYPE_ICC_DEVICE,
> +    .parent = TYPE_DEVICE,
> +    .abstract = true,
> +    .instance_size = sizeof(ICCDevice),
> +    .class_size = sizeof(ICCDeviceClass),
> +    .class_init = icc_device_class_init,
> +};
> +
> +static BusState *icc_bus;
> +
> +BusState *get_icc_bus(void)
> +{
> +    if (icc_bus == NULL) {
> +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> +        icc_bus->allow_hotplug = 1;
> +        OBJECT(icc_bus)->free = g_free;

You can just use qbus_create.

> +        object_property_add_child(container_get(qdev_get_machine(),
> +                                                "/unattached"),

Please put it straight under /machine, not /unattached.

But actually, you lack a device that instantiates the bus.  That device
would be a simple container device similar hw/a15mpcore.c, and would
also take care of registering the MSI memory region.  Create that device
in the boards, and you won't need a special object_property_add_child.

get_icc_bus() would then become simply
object_resolve_path_type("icc-bus", TYPE_ICC_BUS, NULL) or something
like that.

Paolo

> +                                  "icc-bus", OBJECT(icc_bus), NULL);
> +    }
> +    return BUS(icc_bus);
> +}
> +
> +static void icc_bus_register_types(void)
> +{
> +    type_register_static(&icc_bus_info);
> +    type_register_static(&icc_device_info);
> +}
> +
> +type_init(icc_bus_register_types)
> diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> new file mode 100644
> index 0000000..a00fef5
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,47 @@
> +/* icc_bus.h
> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> + *
> + * Copyright (c) 2013 Red Hat, Inc
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +#ifndef ICC_BUS_H
> +#define ICC_BUS_H
> +
> +#include "hw/qdev-core.h"
> +
> +typedef struct ICCBus {
> +    BusState qbus;
> +} ICCBus;
> +#define TYPE_ICC_BUS "ICC"
> +#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
> +
> +typedef struct ICCDevice {
> +    DeviceState qdev;
> +} ICCDevice;
> +
> +typedef struct ICCDeviceClass {
> +    DeviceClass parent_class;
> +    int (*init)(ICCDevice *dev);
> +} ICCDeviceClass;
> +#define TYPE_ICC_DEVICE "icc-device"
> +#define ICC_DEVICE(obj) OBJECT_CHECK(ICCDevice, (obj), TYPE_ICC_DEVICE)
> +#define ICC_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(ICCDeviceClass, (klass), TYPE_ICC_DEVICE)
> +#define ICC_DEVICE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(ICCDeviceClass, (obj), TYPE_ICC_DEVICE)
> +
> +BusState *get_icc_bus(void);
> +
> +#endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 28fb4f8..9741e16 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -24,3 +24,4 @@ stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += resume_vcpu.o
> +stub-obj-y += get_icc_bus.o
> diff --git a/stubs/get_icc_bus.c b/stubs/get_icc_bus.c
> new file mode 100644
> index 0000000..43db181
> --- /dev/null
> +++ b/stubs/get_icc_bus.c
> @@ -0,0 +1,6 @@
> +#include "hw/icc_bus.h"
> +
> +BusState *get_icc_bus(void)
> +{
> +    return NULL;
> +}
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 631bcd8..ae46f81 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -42,10 +42,11 @@
>  
>  #include "sysemu/sysemu.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/icc_bus.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/xen.h"
> -#include "hw/sysbus.h"
>  #include "hw/apic_internal.h"
> +#include "exec/address-spaces.h"
>  #endif
>  
>  static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> @@ -1640,6 +1641,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>      CPUX86State *env;
>      gchar **model_pieces;
>      char *name, *features;
> +    BusState *icc_bus = get_icc_bus();
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> @@ -1650,6 +1652,10 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>      features = model_pieces[1];
>  
>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> +    if (icc_bus) {
> +        qdev_set_parent_bus(DEVICE(cpu), icc_bus);
> +        object_unref(OBJECT(cpu));
> +    }
>      env = &cpu->env;
>      env->cpu_model_str = cpu_model;
>  
> @@ -2145,7 +2151,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>          apic_type = "xen-apic";
>      }
>  
> -    env->apic_state = qdev_try_create(NULL, apic_type);
> +    env->apic_state = qdev_try_create(get_icc_bus(), apic_type);
>      if (env->apic_state == NULL) {
>          error_setg(errp, "APIC device '%s' could not be created", apic_type);
>          return;
> @@ -2176,11 +2182,12 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
>  
>      /* XXX: mapping more APICs at the same memory location */
>      if (apic_mapped == 0) {
> +        APICCommonState *apic = APIC_COMMON(env->apic_state);
>          /* NOTE: the APIC is directly connected to the CPU - it is not
>             on the global memory bus. */
>          /* XXX: what if the base changes? */
> -        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
> -                                MSI_ADDR_BASE, 0x1000);
> +        memory_region_add_subregion_overlap(get_system_memory(), MSI_ADDR_BASE,
> +                                            &apic->io_memory, 0x1000);
>          apic_mapped = 1;
>      }
>  }
> @@ -2356,6 +2363,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      xcc->parent_realize = dc->realize;
>      dc->props = cpu_x86_properties;
>      dc->realize = x86_cpu_realizefn;
> +    dc->bus_type = TYPE_ICC_BUS;
>  
>      xcc->parent_reset = cc->reset;
>      cc->reset = x86_cpu_reset;
> 

Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Igor Mammedov]

On Wed, 27 Mar 2013 11:57:53 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
> > Introduce hot-pluggable ICC_BUS and convert APIC devices from SysBusDevice
> > to ICCDevice wich has ICC_BUS as default one.
> > 
> >  * attach APIC and kvmvapic to ICC_BUS during creation
> >  * use memory API directly instead of using SysBus proxy functions
> >  * introduce get_icc_bus() for getting access to system wide ICC_BUS
> >  * make ICC_BUS default one for X86CPU class, so that device_add would
> >    set it for CPU instead of SysBus
> >  * attach cold-pluged CPUs to icc_bus in softmmu mode, so they could be
> >    uplugged in future
> >  * if kvmvapic init() fails return -1 instead of aborting.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/apic_common.c      |   14 ++++++---
> >  hw/apic_internal.h    |    6 ++--
> >  hw/i386/Makefile.objs |    2 +-
> >  hw/i386/kvmvapic.c    |   15 +++++----
> >  hw/icc_bus.c          |   75 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/icc_bus.h          |   47 ++++++++++++++++++++++++++++++
> >  stubs/Makefile.objs   |    1 +
> >  stubs/get_icc_bus.c   |    6 ++++
> >  target-i386/cpu.c     |   16 ++++++++--
> >  9 files changed, 162 insertions(+), 20 deletions(-)
> >  create mode 100644 hw/icc_bus.c
> >  create mode 100644 hw/icc_bus.h
> >  create mode 100644 stubs/get_icc_bus.c
> > 
> > diff --git a/hw/apic_common.c b/hw/apic_common.c
> > index d0c2616..61599d4 100644
> > --- a/hw/apic_common.c
> > +++ b/hw/apic_common.c
> > @@ -21,6 +21,7 @@
> >  #include "hw/apic_internal.h"
> >  #include "trace.h"
> >  #include "sysemu/kvm.h"
> > +#include "qdev.h"
> >  
> >  static int apic_irq_delivered;
> >  bool apic_report_tpr_access;
> > @@ -282,9 +283,10 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
> >      return 0;
> >  }
> >  
> > -static int apic_init_common(SysBusDevice *dev)
> > +static int apic_init_common(ICCDevice *dev)
> >  {
> >      APICCommonState *s = APIC_COMMON(dev);
> > +    DeviceState *d = DEVICE(dev);
> >      APICCommonClass *info;
> >      static DeviceState *vapic;
> >      static int apic_no;
> > @@ -297,12 +299,14 @@ static int apic_init_common(SysBusDevice *dev)
> >      info = APIC_COMMON_GET_CLASS(s);
> >      info->init(s);
> >  
> > -    sysbus_init_mmio(dev, &s->io_memory);
> >  
> >      /* Note: We need at least 1M to map the VAPIC option ROM */
> >      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> >          ram_size >= 1024 * 1024) {
> > -        vapic = sysbus_create_simple("kvmvapic", -1, NULL);
> > +        vapic = qdev_try_create(d->parent_bus, "kvmvapic");
> > +        if ((vapic == NULL) || (qdev_init(vapic) != 0)) {
> > +            return -1;
> > +        }
> >      }
> >      s->vapic = vapic;
> >      if (apic_report_tpr_access && info->enable_tpr_reporting) {
> > @@ -375,7 +379,7 @@ static Property apic_properties_common[] = {
> >  
> >  static void apic_common_class_init(ObjectClass *klass, void *data)
> >  {
> > -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> > +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> >      dc->vmsd = &vmstate_apic_common;
> > @@ -387,7 +391,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
> >  
> >  static const TypeInfo apic_common_type = {
> >      .name = TYPE_APIC_COMMON,
> > -    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .parent = TYPE_ICC_DEVICE,
> >      .instance_size = sizeof(APICCommonState),
> >      .class_size = sizeof(APICCommonClass),
> >      .class_init = apic_common_class_init,
> > diff --git a/hw/apic_internal.h b/hw/apic_internal.h
> > index 578241f..e7b378e 100644
> > --- a/hw/apic_internal.h
> > +++ b/hw/apic_internal.h
> > @@ -21,7 +21,7 @@
> >  #define QEMU_APIC_INTERNAL_H
> >  
> >  #include "exec/memory.h"
> > -#include "hw/sysbus.h"
> > +#include "hw/icc_bus.h"
> >  #include "qemu/timer.h"
> >  
> >  /* APIC Local Vector Table */
> > @@ -80,7 +80,7 @@ typedef struct APICCommonState APICCommonState;
> >  
> >  typedef struct APICCommonClass
> >  {
> > -    SysBusDeviceClass parent_class;
> > +    ICCDeviceClass parent_class;
> >  
> >      void (*init)(APICCommonState *s);
> >      void (*set_base)(APICCommonState *s, uint64_t val);
> > @@ -94,7 +94,7 @@ typedef struct APICCommonClass
> >  } APICCommonClass;
> >  
> >  struct APICCommonState {
> > -    SysBusDevice busdev;
> > +    ICCDevice busdev;
> >  
> >      MemoryRegion io_memory;
> >      X86CPU *cpu;
> > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > index a78c0b2..316f999 100644
> > --- a/hw/i386/Makefile.objs
> > +++ b/hw/i386/Makefile.objs
> > @@ -1,5 +1,5 @@
> >  obj-y += mc146818rtc.o
> > -obj-y += apic_common.o apic.o
> > +obj-y += apic_common.o apic.o icc_bus.o
> >  obj-y += sga.o ioapic_common.o ioapic.o piix_pci.o
> >  obj-y += vmport.o
> >  obj-y += pci/pci-hotplug.o wdt_ib700.o
> > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > index 21551a5..7de75db 100644
> > --- a/hw/i386/kvmvapic.c
> > +++ b/hw/i386/kvmvapic.c
> > @@ -12,6 +12,8 @@
> >  #include "sysemu/cpus.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/apic_internal.h"
> > +#include "migration/vmstate.h"
> > +#include "exec/address-spaces.h"
> >  
> >  #define APIC_DEFAULT_ADDRESS    0xfee00000
> >  
> > @@ -49,7 +51,7 @@ typedef struct GuestROMState {
> >  } QEMU_PACKED GuestROMState;
> >  
> >  typedef struct VAPICROMState {
> > -    SysBusDevice busdev;
> > +    ICCDevice busdev;
> >      MemoryRegion io;
> >      MemoryRegion rom;
> >      uint32_t state;
> > @@ -581,7 +583,7 @@ static void vapic_map_rom_writable(VAPICROMState *s)
> >      size_t rom_size;
> >      uint8_t *ram;
> >  
> > -    as = sysbus_address_space(&s->busdev);
> > +    as = get_system_memory();
> >  
> >      if (s->rom_mapped_writable) {
> >          memory_region_del_subregion(as, &s->rom);
> > @@ -693,13 +695,12 @@ static const MemoryRegionOps vapic_ops = {
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> > -static int vapic_init(SysBusDevice *dev)
> > +static int vapic_init(ICCDevice *dev)
> >  {
> >      VAPICROMState *s = VAPIC_DEVICE(dev);
> >  
> >      memory_region_init_io(&s->io, &vapic_ops, s, "kvmvapic", 2);
> > -    sysbus_add_io(dev, VAPIC_IO_PORT, &s->io);
> > -    sysbus_init_ioports(dev, VAPIC_IO_PORT, 2);
> > +    memory_region_add_subregion(get_system_io(), VAPIC_IO_PORT, &s->io);
> >  
> >      option_rom[nb_option_roms].name = "kvmvapic.bin";
> >      option_rom[nb_option_roms].bootindex = -1;
> > @@ -801,7 +802,7 @@ static const VMStateDescription vmstate_vapic = {
> >  
> >  static void vapic_class_init(ObjectClass *klass, void *data)
> >  {
> > -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
> > +    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> >      dc->no_user = 1;
> > @@ -812,7 +813,7 @@ static void vapic_class_init(ObjectClass *klass, void *data)
> >  
> >  static const TypeInfo vapic_type = {
> >      .name          = TYPE_VAPIC_DEVICE,
> > -    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .parent        = TYPE_ICC_DEVICE,
> >      .instance_size = sizeof(VAPICROMState),
> >      .class_init    = vapic_class_init,
> >  };
> > diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> > new file mode 100644
> > index 0000000..b170648
> > --- /dev/null
> > +++ b/hw/icc_bus.c
> > @@ -0,0 +1,75 @@
> > +/* icc_bus.c
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> > + */
> > +#include "icc_bus.h"
> > +#include "qemu/module.h"
> > +
> > +static const TypeInfo icc_bus_info = {
> > +    .name = TYPE_ICC_BUS,
> > +    .parent = TYPE_BUS,
> > +    .instance_size = sizeof(ICCBus),
> > +};
> > +
> > +static int icc_device_init(DeviceState *dev)
> > +{
> > +    ICCDevice *id = ICC_DEVICE(dev);
> > +    ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
> > +
> > +    return k->init(id);
> > +}
> > +
> > +static void icc_device_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *k = DEVICE_CLASS(klass);
> > +
> > +    k->init = icc_device_init;
> > +    k->bus_type = TYPE_ICC_BUS;
> > +}
> > +
> > +static const TypeInfo icc_device_info = {
> > +    .name = TYPE_ICC_DEVICE,
> > +    .parent = TYPE_DEVICE,
> > +    .abstract = true,
> > +    .instance_size = sizeof(ICCDevice),
> > +    .class_size = sizeof(ICCDeviceClass),
> > +    .class_init = icc_device_class_init,
> > +};
> > +
> > +static BusState *icc_bus;
> > +
> > +BusState *get_icc_bus(void)
> > +{
> > +    if (icc_bus == NULL) {
> > +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> > +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> > +        icc_bus->allow_hotplug = 1;
> > +        OBJECT(icc_bus)->free = g_free;  
> 
> You can just use qbus_create.
>   
> > +        object_property_add_child(container_get(qdev_get_machine(),
> > +                                                "/unattached"),  
> 
> Please put it straight under /machine, not /unattached.  

sure

> 
> But actually, you lack a device that instantiates the bus.  That device
> would be a simple container device similar hw/a15mpcore.c, and would  

About a year ago something like that device was proposed "cpu_sockets", but
there was suggestion to drop it:
http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
So I've opted in favor of parent-less bus, but I could create it
explicitly at board level as you suggest.

However having parent sysbus device for icc-bus will allow to see it via
monitor info qtree and reset on SysBus could be propagated through it as well,
it may be good to add it later.

> also take care of registering the MSI memory region.  Create that device  

it probably impossible to do, because [kvm]apic sets its private callbacks
to it when initializing memory region, and CPU just maps it at specified
address, or this region and APIC might not even exists at all if qemu started
with -apic feature flag.

> in the boards, and you won't need a special object_property_add_child.
> 
> get_icc_bus() would then become simply
> object_resolve_path_type("icc-bus", TYPE_ICC_BUS, NULL) or something
> like that.
> 
> Paolo
>   
> > +                                  "icc-bus", OBJECT(icc_bus), NULL);
> > +    }
> > +    return BUS(icc_bus);
> > +}
> > +
> > +static void icc_bus_register_types(void)
> > +{
> > +    type_register_static(&icc_bus_info);
> > +    type_register_static(&icc_device_info);
> > +}
> > +
> > +type_init(icc_bus_register_types)
> > diff --git a/hw/icc_bus.h b/hw/icc_bus.h
> > new file mode 100644
> > index 0000000..a00fef5
> > --- /dev/null
> > +++ b/hw/icc_bus.h
> > @@ -0,0 +1,47 @@
> > +/* icc_bus.h
> > + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> > + */
> > +#ifndef ICC_BUS_H
> > +#define ICC_BUS_H
> > +
> > +#include "hw/qdev-core.h"
> > +
> > +typedef struct ICCBus {
> > +    BusState qbus;
> > +} ICCBus;
> > +#define TYPE_ICC_BUS "ICC"
> > +#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
> > +
> > +typedef struct ICCDevice {
> > +    DeviceState qdev;
> > +} ICCDevice;
> > +
> > +typedef struct ICCDeviceClass {
> > +    DeviceClass parent_class;
> > +    int (*init)(ICCDevice *dev);
> > +} ICCDeviceClass;
> > +#define TYPE_ICC_DEVICE "icc-device"
> > +#define ICC_DEVICE(obj) OBJECT_CHECK(ICCDevice, (obj), TYPE_ICC_DEVICE)
> > +#define ICC_DEVICE_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(ICCDeviceClass, (klass), TYPE_ICC_DEVICE)
> > +#define ICC_DEVICE_GET_CLASS(obj) \
> > +     OBJECT_GET_CLASS(ICCDeviceClass, (obj), TYPE_ICC_DEVICE)
> > +
> > +BusState *get_icc_bus(void);
> > +
> > +#endif
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 28fb4f8..9741e16 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -24,3 +24,4 @@ stub-obj-y += vm-stop.o
> >  stub-obj-y += vmstate.o
> >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> >  stub-obj-y += resume_vcpu.o
> > +stub-obj-y += get_icc_bus.o
> > diff --git a/stubs/get_icc_bus.c b/stubs/get_icc_bus.c
> > new file mode 100644
> > index 0000000..43db181
> > --- /dev/null
> > +++ b/stubs/get_icc_bus.c
> > @@ -0,0 +1,6 @@
> > +#include "hw/icc_bus.h"
> > +
> > +BusState *get_icc_bus(void)
> > +{
> > +    return NULL;
> > +}
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 631bcd8..ae46f81 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -42,10 +42,11 @@
> >  
> >  #include "sysemu/sysemu.h"
> >  #include "hw/qdev-properties.h"
> > +#include "hw/icc_bus.h"
> >  #ifndef CONFIG_USER_ONLY
> >  #include "hw/xen.h"
> > -#include "hw/sysbus.h"
> >  #include "hw/apic_internal.h"
> > +#include "exec/address-spaces.h"
> >  #endif
> >  
> >  static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > @@ -1640,6 +1641,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
> >      CPUX86State *env;
> >      gchar **model_pieces;
> >      char *name, *features;
> > +    BusState *icc_bus = get_icc_bus();
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> >      if (!model_pieces[0]) {
> > @@ -1650,6 +1652,10 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
> >      features = model_pieces[1];
> >  
> >      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > +    if (icc_bus) {
> > +        qdev_set_parent_bus(DEVICE(cpu), icc_bus);
> > +        object_unref(OBJECT(cpu));
> > +    }
> >      env = &cpu->env;
> >      env->cpu_model_str = cpu_model;
> >  
> > @@ -2145,7 +2151,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >          apic_type = "xen-apic";
> >      }
> >  
> > -    env->apic_state = qdev_try_create(NULL, apic_type);
> > +    env->apic_state = qdev_try_create(get_icc_bus(), apic_type);
> >      if (env->apic_state == NULL) {
> >          error_setg(errp, "APIC device '%s' could not be created", apic_type);
> >          return;
> > @@ -2176,11 +2182,12 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> >  
> >      /* XXX: mapping more APICs at the same memory location */
> >      if (apic_mapped == 0) {
> > +        APICCommonState *apic = APIC_COMMON(env->apic_state);
> >          /* NOTE: the APIC is directly connected to the CPU - it is not
> >             on the global memory bus. */
> >          /* XXX: what if the base changes? */
> > -        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
> > -                                MSI_ADDR_BASE, 0x1000);
> > +        memory_region_add_subregion_overlap(get_system_memory(), MSI_ADDR_BASE,
> > +                                            &apic->io_memory, 0x1000);
> >          apic_mapped = 1;
> >      }
> >  }
> > @@ -2356,6 +2363,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> >      xcc->parent_realize = dc->realize;
> >      dc->props = cpu_x86_properties;
> >      dc->realize = x86_cpu_realizefn;
> > +    dc->bus_type = TYPE_ICC_BUS;
> >  
> >      xcc->parent_reset = cc->reset;
> >      cc->reset = x86_cpu_reset;
> >   
>   


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[li guang]

在 2013-03-28四的 11:55 +0100，Igor Mammedov写道：
> On Wed, 27 Mar 2013 11:57:53 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
> > > Introduce hot-pluggable ICC_BUS and convert APIC devices from SysBusDevice
> > > to ICCDevice wich has ICC_BUS as default one.
> > > 
> > >  * attach APIC and kvmvapic to ICC_BUS during creation
> > >  * use memory API directly instead of using SysBus proxy functions
> > >  * introduce get_icc_bus() for getting access to system wide ICC_BUS
> > >  * make ICC_BUS default one for X86CPU class, so that device_add would
> > >    set it for CPU instead of SysBus
> > >  * attach cold-pluged CPUs to icc_bus in softmmu mode, so they could be
> > >    uplugged in future
> > >  * if kvmvapic init() fails return -1 instead of aborting.
[...]
> > > +};
> > > +
> > > +static BusState *icc_bus;
> > > +
> > > +BusState *get_icc_bus(void)
> > > +{
> > > +    if (icc_bus == NULL) {
> > > +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> > > +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> > > +        icc_bus->allow_hotplug = 1;
> > > +        OBJECT(icc_bus)->free = g_free;  
> > 
> > You can just use qbus_create.
> >   
> > > +        object_property_add_child(container_get(qdev_get_machine(),
> > > +                                                "/unattached"),  
> > 
> > Please put it straight under /machine, not /unattached.  
> 
> sure
> 
> > 
> > But actually, you lack a device that instantiates the bus.  That device
> > would be a simple container device similar hw/a15mpcore.c, and would  
> 
> About a year ago something like that device was proposed "cpu_sockets", but

what about link<Socket> implementation which was mentioned by Andreas
few days ago at a thread discussion for acpi memory hotplug?
(http://list-archives.org/2012/12/18/qemu-devel-nongnu-org/rfc-patch-v4-00-30-acpi-memory-hotplug/f/4115902514)
do consider to implement a link<> property between CPU and cpu_socket?

> there was suggestion to drop it:
> http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
> So I've opted in favor of parent-less bus, but I could create it
> explicitly at board level as you suggest.
> 
> However having parent sysbus device for icc-bus will allow to see it via
> monitor info qtree and reset on SysBus could be propagated through it as well,
> it may be good to add it later.
> 
> > also take care of registering the MSI memory region.  Create that device  
> 
> it probably impossible to do, because [kvm]apic sets its private callbacks
> to it when initializing memory region, and CPU just maps it at specified
> address, or this region and APIC might not even exists at all if qemu started
> with -apic feature flag.
> 
> > in the boards, and you won't need a special object_property_add_child.
> > 
> > get_icc_bus() would then become simply
> > object_resolve_path_type("icc-bus", TYPE_ICC_BUS, NULL) or something
> > like that.
> > 
> > Paolo
> >   
> > > +                                  "icc-bus", OBJECT(icc_bus), NULL);
> > > +    }
> > > +    return BUS(icc_bus);
> > > +}
> > > +

Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Igor Mammedov]

On Fri, 29 Mar 2013 15:22:47 +0800
li guang <lig.fnst@cn.fujitsu.com> wrote:

> 在 2013-03-28四的 11:55 +0100，Igor Mammedov写道：
> > On Wed, 27 Mar 2013 11:57:53 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
> > > > Introduce hot-pluggable ICC_BUS and convert APIC devices from SysBusDevice
> > > > to ICCDevice wich has ICC_BUS as default one.
> > > > 
> > > >  * attach APIC and kvmvapic to ICC_BUS during creation
> > > >  * use memory API directly instead of using SysBus proxy functions
> > > >  * introduce get_icc_bus() for getting access to system wide ICC_BUS
> > > >  * make ICC_BUS default one for X86CPU class, so that device_add would
> > > >    set it for CPU instead of SysBus
> > > >  * attach cold-pluged CPUs to icc_bus in softmmu mode, so they could be
> > > >    uplugged in future
> > > >  * if kvmvapic init() fails return -1 instead of aborting.
> [...]
> > > > +};
> > > > +
> > > > +static BusState *icc_bus;
> > > > +
> > > > +BusState *get_icc_bus(void)
> > > > +{
> > > > +    if (icc_bus == NULL) {
> > > > +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> > > > +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> > > > +        icc_bus->allow_hotplug = 1;
> > > > +        OBJECT(icc_bus)->free = g_free;  
> > > 
> > > You can just use qbus_create.
> > >   
> > > > +        object_property_add_child(container_get(qdev_get_machine(),
> > > > +                                                "/unattached"),  
> > > 
> > > Please put it straight under /machine, not /unattached.  
> > 
> > sure
> > 
> > > 
> > > But actually, you lack a device that instantiates the bus.  That device
> > > would be a simple container device similar hw/a15mpcore.c, and would  
> > 
> > About a year ago something like that device was proposed "cpu_sockets", but
> 
> what about link<Socket> implementation which was mentioned by Andreas
> few days ago at a thread discussion for acpi memory hotplug?
> (http://list-archives.org/2012/12/18/qemu-devel-nongnu-org/rfc-patch-v4-00-30-acpi-memory-hotplug/f/4115902514)
> do consider to implement a link<> property between CPU and cpu_socket?
since CPU slowly moving towards being able to be added via device_add,
there is a limited usefulness in link<> property. It still could be used
to expose possible APIC IDs to user so that user won't have calculate
them by itself. But it requires fixing device_realize first so I haven't
included it in this series.

> 
> > there was suggestion to drop it:
> > http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
> > So I've opted in favor of parent-less bus, but I could create it
> > explicitly at board level as you suggest.
> > 
> > However having parent sysbus device for icc-bus will allow to see it via
> > monitor info qtree and reset on SysBus could be propagated through it as well,
> > it may be good to add it later.
> > 
> > > also take care of registering the MSI memory region.  Create that device  
> > 
> > it probably impossible to do, because [kvm]apic sets its private callbacks
> > to it when initializing memory region, and CPU just maps it at specified
> > address, or this region and APIC might not even exists at all if qemu started
> > with -apic feature flag.
> > 
> > > in the boards, and you won't need a special object_property_add_child.
> > > 
> > > get_icc_bus() would then become simply
> > > object_resolve_path_type("icc-bus", TYPE_ICC_BUS, NULL) or something
> > > like that.
> > > 
> > > Paolo
> > >   
> > > > +                                  "icc-bus", OBJECT(icc_bus), NULL);
> > > > +    }
> > > > +    return BUS(icc_bus);
> > > > +}
> > > > +
> 
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Andreas Färber]

Am 28.03.2013 11:55, schrieb Igor Mammedov:
> On Wed, 27 Mar 2013 11:57:53 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
>>> +static BusState *icc_bus;
>>> +
>>> +BusState *get_icc_bus(void)
>>> +{
>>> +    if (icc_bus == NULL) {
>>> +        icc_bus = g_malloc0(icc_bus_info.instance_size);
>>> +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
>>> +        icc_bus->allow_hotplug = 1;
>>> +        OBJECT(icc_bus)->free = g_free;  
>>
>> You can just use qbus_create.
>>   
>>> +        object_property_add_child(container_get(qdev_get_machine(),
>>> +                                                "/unattached"),  
>>
>> Please put it straight under /machine, not /unattached.  
> 
> sure
> 
>>
>> But actually, you lack a device that instantiates the bus.  That device
>> would be a simple container device similar hw/a15mpcore.c, and would  
> 
> About a year ago something like that device was proposed "cpu_sockets", but
> there was suggestion to drop it:
> http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
> So I've opted in favor of parent-less bus, but I could create it
> explicitly at board level as you suggest.
> 
> However having parent sysbus device for icc-bus will allow to see it via
> monitor info qtree

As I said on IRC, info qtree should not influence our design decisions.
All that's missing is a convenient script that pretty-prints the output
from the QOM graph using qom-list and qom-get in a non-interactive way.
Whether CPUs or busses are shown in info qtree is totally irrelevant IMO
by now.

> and reset on SysBus could be propagated through it as well,
> it may be good to add it later.

What exactly is this ICC bus needed for? Is it just a workaround for
qdev_init() or something making assumptions about busses and falling
back to SysBus otherwise? Or about avoiding SysBus?

And where would it sit if be hosted you think of something like Qseven
modules - on the board or on the module? I don't see pc.c being touched
here at all...

Andreas

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

Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Igor Mammedov]

On Thu, 04 Apr 2013 13:10:54 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 28.03.2013 11:55, schrieb Igor Mammedov:
> > On Wed, 27 Mar 2013 11:57:53 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
> >>> +static BusState *icc_bus;
> >>> +
> >>> +BusState *get_icc_bus(void)
> >>> +{
> >>> +    if (icc_bus == NULL) {
> >>> +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> >>> +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> >>> +        icc_bus->allow_hotplug = 1;
> >>> +        OBJECT(icc_bus)->free = g_free;  
> >>
> >> You can just use qbus_create.
> >>   
> >>> +        object_property_add_child(container_get(qdev_get_machine(),
> >>> +                                                "/unattached"),  
> >>
> >> Please put it straight under /machine, not /unattached.  
> > 
> > sure
> > 
> >>
> >> But actually, you lack a device that instantiates the bus.  That device
> >> would be a simple container device similar hw/a15mpcore.c, and would  
> > 
> > About a year ago something like that device was proposed "cpu_sockets", but
> > there was suggestion to drop it:
> > http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
> > So I've opted in favor of parent-less bus, but I could create it
> > explicitly at board level as you suggest.
> > 
> > However having parent sysbus device for icc-bus will allow to see it via
> > monitor info qtree
> 
> As I said on IRC, info qtree should not influence our design decisions.
> All that's missing is a convenient script that pretty-prints the output
> from the QOM graph using qom-list and qom-get in a non-interactive way.
> Whether CPUs or busses are shown in info qtree is totally irrelevant IMO
> by now.
> 
> > and reset on SysBus could be propagated through it as well,
> > it may be good to add it later.
> 
> What exactly is this ICC bus needed for? Is it just a workaround for
> qdev_init() or something making assumptions about busses and falling
> back to SysBus otherwise? Or about avoiding SysBus?
SysBus doesn't allow hotplug on it, I've asked Anthony on IRC and short answer
was do not touch what doesn't exists. Hence hotplug-able bus that exists
on real hw.
x86 hw had ICC bus some time ago through which APICs, IOAPICs and CPUs
communicated. So here it is.

More close to the code:
 - device_add assigns SysBus to any bus-less device, therefore CPU hits
   assertion because hotplug is not allowed on bus.
 - when x86 CPU is realizing, it creates APIC and possible kvmvapic, which are
   SysBus devices presently, it triggers the same assert as above.

> 
> And where would it sit if be hosted you think of something like Qseven
> modules - on the board or on the module? I don't see pc.c being touched
> here at all...
I've reworked it quite a bit to Paolo's suggestions. now board creates
following QOM tree:
  /machine/icc-bridge<SYS_BUS_DEVICE>/--->child<icc-bus>/->link<apic[..]>
                                       |                |   
                                       |                 ->link<kvmvapic[..]>
                                       |                |
                                       |                 ->link<cpu[..]
                                       |                |
                                       |                 ->link<ioapic>
                                        ->child<ioapic>

where icc-bridge is created by board and serves as a parent, which creates
icc-bus, ioapic and takes care about mapping memory regions registered by
*apic* devices.

>

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


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 08/12] introduce CPU hot-plug notifier

[Igor Mammedov]

hot-added CPU id (APIC ID) will be distributed to acpi_piix4 and rtc_cmos

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/sysemu/sysemu.h                 |    4 ++++
 stubs/Makefile.objs                     |    1 +
 stubs/qemu_system_cpu_hotplug_request.c |    5 +++++
 vl.c                                    |   14 ++++++++++++++
 4 files changed, 24 insertions(+), 0 deletions(-)
 create mode 100644 stubs/qemu_system_cpu_hotplug_request.c

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6578782..4b8f721 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -152,6 +152,10 @@ 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_add_notifier(Notifier *notifier);
+void qemu_system_cpu_hotplug_request(uint32_t id);
+
 /* 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/stubs/Makefile.objs b/stubs/Makefile.objs
index 9741e16..6a492f5 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -25,3 +25,4 @@ stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += resume_vcpu.o
 stub-obj-y += get_icc_bus.o
+stub-obj-y += qemu_system_cpu_hotplug_request.o
diff --git a/stubs/qemu_system_cpu_hotplug_request.c b/stubs/qemu_system_cpu_hotplug_request.c
new file mode 100644
index 0000000..ad4f394
--- /dev/null
+++ b/stubs/qemu_system_cpu_hotplug_request.c
@@ -0,0 +1,5 @@
+#include <sysemu/sysemu.h>
+
+void qemu_system_cpu_hotplug_request(uint32_t id)
+{
+}
diff --git a/vl.c b/vl.c
index aeed7f4..fd95e43 100644
--- a/vl.c
+++ b/vl.c
@@ -1723,6 +1723,20 @@ void vm_start(void)
     }
 }
 
+/* CPU hot-plug notifiers */
+static NotifierList cpu_add_notifiers =
+    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
+
+void qemu_register_cpu_add_notifier(Notifier *notifier)
+{
+    notifier_list_add(&cpu_add_notifiers, notifier);
+}
+
+void qemu_system_cpu_hotplug_request(uint32_t id)
+{
+    notifier_list_notify(&cpu_add_notifiers, &id);
+}
+
 /* reset/shutdown handler */
 
 typedef struct QEMUResetEntry {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 08/12] introduce CPU hot-plug notifier

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> hot-added CPU id (APIC ID) will be distributed to acpi_piix4 and rtc_cmos
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/sysemu/sysemu.h                 |    4 ++++
>  stubs/Makefile.objs                     |    1 +
>  stubs/qemu_system_cpu_hotplug_request.c |    5 +++++
>  vl.c                                    |   14 ++++++++++++++
>  4 files changed, 24 insertions(+), 0 deletions(-)
>  create mode 100644 stubs/qemu_system_cpu_hotplug_request.c
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6578782..4b8f721 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -152,6 +152,10 @@ 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_add_notifier(Notifier *notifier);
> +void qemu_system_cpu_hotplug_request(uint32_t id);
> +
>  /* 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/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9741e16..6a492f5 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -25,3 +25,4 @@ stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += resume_vcpu.o
>  stub-obj-y += get_icc_bus.o
> +stub-obj-y += qemu_system_cpu_hotplug_request.o

You're adding one stub per patch.  I think this is a sign that something
can be abstracted at a higher level (e.g. put something in cpus.c if it
is softmmu-specific), or that it is added at the wrong place.

For example, this notifier can go in qom/cpu.c.

(Besides, I noticed now the get_icc_bus stub.  I didn't understand why
it's used, but anyway adding CPU-specific stuff to libqemustub is
absolutely a no-no).

Paolo

> diff --git a/stubs/qemu_system_cpu_hotplug_request.c b/stubs/qemu_system_cpu_hotplug_request.c
> new file mode 100644
> index 0000000..ad4f394
> --- /dev/null
> +++ b/stubs/qemu_system_cpu_hotplug_request.c
> @@ -0,0 +1,5 @@
> +#include <sysemu/sysemu.h>
> +
> +void qemu_system_cpu_hotplug_request(uint32_t id)
> +{
> +}
> diff --git a/vl.c b/vl.c
> index aeed7f4..fd95e43 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1723,6 +1723,20 @@ void vm_start(void)
>      }
>  }
>  
> +/* CPU hot-plug notifiers */
> +static NotifierList cpu_add_notifiers =
> +    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
> +
> +void qemu_register_cpu_add_notifier(Notifier *notifier)
> +{
> +    notifier_list_add(&cpu_add_notifiers, notifier);
> +}
> +
> +void qemu_system_cpu_hotplug_request(uint32_t id)
> +{
> +    notifier_list_notify(&cpu_add_notifiers, &id);
> +}
> +
>  /* reset/shutdown handler */
>  
>  typedef struct QEMUResetEntry {
> 

Re: [Qemu-devel] [PATCH 08/12] introduce CPU hot-plug notifier

[Igor Mammedov]

On Wed, 27 Mar 2013 12:06:10 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> > hot-added CPU id (APIC ID) will be distributed to acpi_piix4 and rtc_cmos
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/sysemu/sysemu.h                 |    4 ++++
> >  stubs/Makefile.objs                     |    1 +
> >  stubs/qemu_system_cpu_hotplug_request.c |    5 +++++
> >  vl.c                                    |   14 ++++++++++++++
> >  4 files changed, 24 insertions(+), 0 deletions(-)
> >  create mode 100644 stubs/qemu_system_cpu_hotplug_request.c
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 6578782..4b8f721 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -152,6 +152,10 @@ 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_add_notifier(Notifier *notifier);
> > +void qemu_system_cpu_hotplug_request(uint32_t id);
> > +
> >  /* 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/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 9741e16..6a492f5 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -25,3 +25,4 @@ stub-obj-y += vmstate.o
> >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> >  stub-obj-y += resume_vcpu.o
> >  stub-obj-y += get_icc_bus.o
> > +stub-obj-y += qemu_system_cpu_hotplug_request.o
> 
> You're adding one stub per patch.  I think this is a sign that something
> can be abstracted at a higher level (e.g. put something in cpus.c if it
> is softmmu-specific), or that it is added at the wrong place.
I've put notifier in vl.c since most of them are there and it doesn't make
much difference if it is in cpus.c, but stub is to reduce ifdeffenery in
headers, although target-i386/cpu.c is build per target so I could use
ifdef in include/sysemu/cpus.h
===
#ifndef CONFIG_USER_ONLY
void qemu_system_cpu_hotplug_request(uint32_t id);
#esle
static inline void qemu_system_cpu_hotplug_request(uint32_t id)
{
}
#endif
===

> 
> For example, this notifier can go in qom/cpu.c.
Yep there wouldn't be need for stub if notifier is in qom/cpu.c,
but I've figured out that people would object to put it there
since it's build only once for softmmu and *-user targets and
*-user target doesn't need it at all.

Andreas,
 would it be acceptable if notifier goes in qom/cpu.c, (it would
 add nop code to *-user target)?

 
> (Besides, I noticed now the get_icc_bus stub.  I didn't understand why
> it's used, but anyway adding CPU-specific stuff to libqemustub is
> absolutely a no-no).

True, If icc_bus was created at board level then there wouldn't be any need
for get_icc_bus(), it could be just looked up in qom tree. I'll try to do it.

BTW: is there any guidelines what might be added to libqemustub?
> 
> Paolo

Re: [Qemu-devel] [PATCH 08/12] introduce CPU hot-plug notifier

[Paolo Bonzini]

Il 27/03/2013 16:24, Igor Mammedov ha scritto:
> I've put notifier in vl.c since most of them are there

They are there, because the code that invokes them is also there.  In
fact, most calls of notifier_list_notify in vl.c are from static functions.

> Yep there wouldn't be need for stub if notifier is in qom/cpu.c,
> but I've figured out that people would object to put it there
> since it's build only once for softmmu and *-user targets and
> *-user target doesn't need it at all.
> 
> Andreas,
>  would it be acceptable if notifier goes in qom/cpu.c, (it would
>  add nop code to *-user target)?

I think adding dead code to *-user is fine.

>> > (Besides, I noticed now the get_icc_bus stub.  I didn't understand why
>> > it's used, but anyway adding CPU-specific stuff to libqemustub is
>> > absolutely a no-no).
> True, If icc_bus was created at board level then there wouldn't be any need
> for get_icc_bus(), it could be just looked up in qom tree. I'll try to do it.
> 
> BTW: is there any guidelines what might be added to libqemustub?

Nothing. :)

Seriously, you really should use it only when an entire subsystem does
not exist in tools or user-level emulation (monitor, vm_clock,
migration, slirp).  Everything else will probably be served better by
methods, in all likelihood.

Paolo

[Qemu-devel] [PATCH 09/12] rtc: update rtc_cmos on CPU hot-plug

[Igor Mammedov]

... so that on reboot BIOS could read current available CPU count

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/mc146818rtc.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index a2119ad..c32ab92 100644
--- a/hw/mc146818rtc.c
+++ b/hw/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_add_notifier(&s->cpu_added_notifier);
+
     memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
     isa_register_ioport(dev, &s->io, base);
 
-- 
1.7.1

[Qemu-devel] [PATCH 10/12] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

[Igor Mammedov]

* introduce processor status bitmask visible to guest at 0xaf00 addr,
  where Seabios expects it
* set bit corresponding to APIC ID in processor status bitmask on
  receiving CPU hot-plug notification
* trigger CPU hot-plug SCI, expected by Seabios on receiving CPU
  hot-plug notification

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi_piix4.c |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 7a4b712..ddb3981 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -48,19 +48,28 @@
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
 
+#define PROC_BASE 0xaf00
+#define PROC_LEN 32
+
 #define PIIX4_PCI_HOTPLUG_STATUS 2
+#define PIIX4_CPU_HOTPLUG_STATUS 4
 
 struct pci_status {
     uint32_t up; /* deprecated, maintained for migration compatibility */
     uint32_t down;
 };
 
+struct cpu_status {
+    uint8_t sts[PROC_LEN];
+};
+
 typedef struct PIIX4PMState {
     PCIDevice dev;
 
     MemoryRegion io;
     MemoryRegion io_gpe;
     MemoryRegion io_pci;
+    MemoryRegion io_cpu;
     ACPIREGS ar;
 
     APMState apm;
@@ -82,6 +91,9 @@ typedef struct PIIX4PMState {
     uint8_t disable_s3;
     uint8_t disable_s4;
     uint8_t s4_val;
+
+    struct cpu_status gpe_cpu;
+    Notifier cpu_add_notifier;
 } PIIX4PMState;
 
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
@@ -100,8 +112,8 @@ static void pm_update_sci(PIIX4PMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
-        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
-          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
+        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
+          (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
 
     qemu_set_irq(s->irq, sci_level);
     /* schedule a timer interruption if needed */
@@ -257,6 +269,17 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
     return ret;
 }
 
+#define VMSTATE_CPU_STATUS_ARRAY(_field, _state)                             \
+ {                                                                           \
+     .name       = (stringify(_field)),                                      \
+     .version_id = 0,                                                        \
+     .num        = PROC_LEN,                                                 \
+     .info       = &vmstate_info_uint8,                                      \
+     .size       = sizeof(uint8_t),                                          \
+     .flags      = VMS_ARRAY,                                                \
+     .offset     = vmstate_offset_array(_state, _field, uint8_t, PROC_LEN),  \
+ }
+
 /* qemu-kvm 1.2 uses version 3 but advertised as 2
  * To support incoming qemu-kvm 1.2 migration, change version_id
  * and minimum_version_id to 2 below (which breaks migration from
@@ -281,6 +304,7 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
         VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
                        struct pci_status),
+        VMSTATE_CPU_STATUS_ARRAY(gpe_cpu.sts, PIIX4PMState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -585,6 +609,78 @@ static const MemoryRegionOps piix4_pci_ops = {
     },
 };
 
+static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned width)
+{
+    PIIX4PMState *s = opaque;
+    struct cpu_status *cpus = &s->gpe_cpu;
+    uint64_t val = cpus->sts[addr];
+    return val;
+}
+
+static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
+                             unsigned int size)
+{
+    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
+}
+
+static const MemoryRegionOps cpu_hotplug_ops = {
+    .read = cpu_status_read,
+    .write = cpu_status_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+typedef enum {
+    PLUG,
+    UNPLUG,
+} HotplugEventType;
+
+static void piix4_cpu_hotplug_req(PIIX4PMState *s, uint32_t cpu,
+                               HotplugEventType action)
+{
+    struct cpu_status *g = &s->gpe_cpu;
+    ACPIGPE *gpe = &s->ar.gpe;
+
+    assert(s != NULL);
+    *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
+    if (action == PLUG) {
+        g->sts[cpu / 8] |= (1 << (cpu % 8));
+    } else {
+        g->sts[cpu / 8] &= ~(1 << (cpu % 8));
+    }
+    pm_update_sci(s);
+}
+
+static void piix4_cpu_add_req(Notifier *n, void *opaque)
+{
+    PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_add_notifier);
+    piix4_cpu_hotplug_req(s, *(uint32_t *)opaque, PLUG);
+}
+
+static int piix4_init_cpu_status(Object *obj, void *opaque)
+{
+    struct cpu_status *g = (struct cpu_status *)opaque;
+    Object *cpu_obj = object_dynamic_cast(obj, TYPE_CPU);
+
+    if (cpu_obj) {
+        struct Error *error = NULL;
+        int64_t apic_id = object_property_get_int(cpu_obj, "apic-id", &error);
+
+        if (error) {
+            fprintf(stderr, "failed to initilize CPU status for ACPI: %s\n",
+                    error_get_pretty(error));
+            error_free(error);
+            abort();
+        }
+        g_assert((apic_id / 8) < PROC_LEN);
+        g->sts[apic_id / 8] |= (1 << (apic_id % 8));
+    }
+    return object_child_foreach(obj, piix4_init_cpu_status, opaque);
+}
+
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                 PCIHotplugState state);
 
@@ -600,6 +696,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
                                 &s->io_pci);
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
+
+    piix4_init_cpu_status(qdev_get_machine(), &s->gpe_cpu);
+    memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug",
+                          PROC_LEN);
+    memory_region_add_subregion(parent, PROC_BASE, &s->io_cpu);
+    s->cpu_add_notifier.notify = piix4_cpu_add_req;
+    qemu_register_cpu_add_notifier(&s->cpu_add_notifier);
 }
 
 static void enable_device(PIIX4PMState *s, int slot)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 10/12] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> * introduce processor status bitmask visible to guest at 0xaf00 addr,
>   where Seabios expects it
> * set bit corresponding to APIC ID in processor status bitmask on
>   receiving CPU hot-plug notification
> * trigger CPU hot-plug SCI, expected by Seabios on receiving CPU
>   hot-plug notification
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi_piix4.c |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 7a4b712..ddb3981 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -48,19 +48,28 @@
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
>  
> +#define PROC_BASE 0xaf00
> +#define PROC_LEN 32
> +
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
> +#define PIIX4_CPU_HOTPLUG_STATUS 4
>  
>  struct pci_status {
>      uint32_t up; /* deprecated, maintained for migration compatibility */
>      uint32_t down;
>  };
>  
> +struct cpu_status {
> +    uint8_t sts[PROC_LEN];
> +};
> +
>  typedef struct PIIX4PMState {
>      PCIDevice dev;
>  
>      MemoryRegion io;
>      MemoryRegion io_gpe;
>      MemoryRegion io_pci;
> +    MemoryRegion io_cpu;
>      ACPIREGS ar;
>  
>      APMState apm;
> @@ -82,6 +91,9 @@ typedef struct PIIX4PMState {
>      uint8_t disable_s3;
>      uint8_t disable_s4;
>      uint8_t s4_val;
> +
> +    struct cpu_status gpe_cpu;
> +    Notifier cpu_add_notifier;
>  } PIIX4PMState;
>  
>  static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> @@ -100,8 +112,8 @@ static void pm_update_sci(PIIX4PMState *s)
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> -        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
> -          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> +        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
> +          (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
>  
>      qemu_set_irq(s->irq, sci_level);
>      /* schedule a timer interruption if needed */
> @@ -257,6 +269,17 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
>      return ret;
>  }
>  
> +#define VMSTATE_CPU_STATUS_ARRAY(_field, _state)                             \
> + {                                                                           \
> +     .name       = (stringify(_field)),                                      \
> +     .version_id = 0,                                                        \
> +     .num        = PROC_LEN,                                                 \
> +     .info       = &vmstate_info_uint8,                                      \
> +     .size       = sizeof(uint8_t),                                          \
> +     .flags      = VMS_ARRAY,                                                \
> +     .offset     = vmstate_offset_array(_state, _field, uint8_t, PROC_LEN),  \
> + }
> +
>  /* qemu-kvm 1.2 uses version 3 but advertised as 2
>   * To support incoming qemu-kvm 1.2 migration, change version_id
>   * and minimum_version_id to 2 below (which breaks migration from
> @@ -281,6 +304,7 @@ static const VMStateDescription vmstate_acpi = {
>          VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
>          VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
>                         struct pci_status),
> +        VMSTATE_CPU_STATUS_ARRAY(gpe_cpu.sts, PIIX4PMState),
>          VMSTATE_END_OF_LIST()
>      }

You need to either bump the version, or add a subsection.   The
subsection is not needed until the first hot-(un)plug action.

>  };
> @@ -585,6 +609,78 @@ static const MemoryRegionOps piix4_pci_ops = {
>      },
>  };
>  
> +static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned width)
> +{
> +    PIIX4PMState *s = opaque;
> +    struct cpu_status *cpus = &s->gpe_cpu;
> +    uint64_t val = cpus->sts[addr];
> +    return val;
> +}
> +
> +static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> +                             unsigned int size)
> +{
> +    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> +}
> +
> +static const MemoryRegionOps cpu_hotplug_ops = {
> +    .read = cpu_status_read,
> +    .write = cpu_status_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +typedef enum {
> +    PLUG,
> +    UNPLUG,
> +} HotplugEventType;
> +
> +static void piix4_cpu_hotplug_req(PIIX4PMState *s, uint32_t cpu,
> +                               HotplugEventType action)
> +{
> +    struct cpu_status *g = &s->gpe_cpu;
> +    ACPIGPE *gpe = &s->ar.gpe;
> +
> +    assert(s != NULL);
> +    *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
> +    if (action == PLUG) {
> +        g->sts[cpu / 8] |= (1 << (cpu % 8));
> +    } else {
> +        g->sts[cpu / 8] &= ~(1 << (cpu % 8));
> +    }
> +    pm_update_sci(s);
> +}
> +
> +static void piix4_cpu_add_req(Notifier *n, void *opaque)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_add_notifier);
> +    piix4_cpu_hotplug_req(s, *(uint32_t *)opaque, PLUG);
> +}
> +
> +static int piix4_init_cpu_status(Object *obj, void *opaque)
> +{
> +    struct cpu_status *g = (struct cpu_status *)opaque;
> +    Object *cpu_obj = object_dynamic_cast(obj, TYPE_CPU);
> +
> +    if (cpu_obj) {
> +        struct Error *error = NULL;
> +        int64_t apic_id = object_property_get_int(cpu_obj, "apic-id", &error);
> +
> +        if (error) {
> +            fprintf(stderr, "failed to initilize CPU status for ACPI: %s\n",
> +                    error_get_pretty(error));
> +            error_free(error);
> +            abort();
> +        }
> +        g_assert((apic_id / 8) < PROC_LEN);
> +        g->sts[apic_id / 8] |= (1 << (apic_id % 8));
> +    }
> +    return object_child_foreach(obj, piix4_init_cpu_status, opaque);
> +}

acpi_piix4 is also used for MIPS.  Be careful.

There is already a cpu_index field in CPU.  I think you should add a
get_firmware_id method to CPU, or something like that, and override it
in X86CPU to return the APIC id.  Similarly, some of the functions you
added in target-i386/ could become class methods on CPU.

Paolo


>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>                                  PCIHotplugState state);
>  
> @@ -600,6 +696,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>      memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
>                                  &s->io_pci);
>      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
> +
> +    piix4_init_cpu_status(qdev_get_machine(), &s->gpe_cpu);
> +    memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug",
> +                          PROC_LEN);
> +    memory_region_add_subregion(parent, PROC_BASE, &s->io_cpu);
> +    s->cpu_add_notifier.notify = piix4_cpu_add_req;
> +    qemu_register_cpu_add_notifier(&s->cpu_add_notifier);
>  }
>  
>  static void enable_device(PIIX4PMState *s, int slot)
> 

[Qemu-devel] [PATCH 11/12] qmp: add cpu_set qmp command

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/sysemu/sysemu.h |    2 ++
 qapi-schema.json        |    9 +++++++++
 qmp-commands.hx         |   26 ++++++++++++++++++++++++++
 qmp.c                   |   12 ++++++++++++
 stubs/Makefile.objs     |    1 +
 stubs/do_cpu_hot_add.c  |    7 +++++++
 6 files changed, 57 insertions(+), 0 deletions(-)
 create mode 100644 stubs/do_cpu_hot_add.c

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 4b8f721..8bcaf26 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict);
 void qemu_register_cpu_add_notifier(Notifier *notifier);
 void qemu_system_cpu_hotplug_request(uint32_t id);
 
+void do_cpu_hot_add(const int64_t id, Error **errp);
+
 /* 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/qapi-schema.json b/qapi-schema.json
index fdaa9da..7acec6e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1385,6 +1385,15 @@
 { 'command': 'cpu', 'data': {'index': 'int'} }
 
 ##
+# @cpu_set
+#
+# Sets specified cpu to online/ofline mode
+#
+# Notes: semantics is : cpu_set id=x status=online|offline
+##
+{ 'command': 'cpu_set', 'data': {'id': 'int', 'status': 'str'} }
+
+##
 # @memsave:
 #
 # Save a portion of guest memory to a file.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b370060..283df76 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -385,6 +385,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
 EQMP
 
     {
+        .name       = "cpu_set",
+        .args_type  = "id:i,status:s",
+        .mhandler.cmd_new = qmp_marshal_input_cpu_set,
+    },
+
+SQMP
+cpu_set
+-------
+
+Sets virtual cpu to online/ofline state
+
+Arguments:
+
+- "id": virtual cpu id (json-int)
+- "status": desired state of cpu, online/offline (json-string)
+
+Example:
+
+-> { "execute": "cpu_set",
+             "arguments": { "id": 2,
+                            "status": "online" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "memsave",
         .args_type  = "val:l,size:i,filename:s,cpu:i?",
         .mhandler.cmd_new = qmp_marshal_input_memsave,
diff --git a/qmp.c b/qmp.c
index 55b056b..5b84779 100644
--- a/qmp.c
+++ b/qmp.c
@@ -108,6 +108,18 @@ void qmp_cpu(int64_t index, Error **errp)
     /* Just do nothing */
 }
 
+void qmp_cpu_set(int64_t id, const char *status, Error **errp)
+{
+    if (!strcmp(status, "online")) {
+        do_cpu_hot_add(id, errp);
+    } else if (!strcmp(status, "offline")) {
+        error_setg(errp, "Unplug is not implemented");
+    } else {
+        error_setg(errp, "Invalid parameter '%s'", status);
+        return;
+    }
+}
+
 #ifndef CONFIG_VNC
 /* If VNC support is enabled, the "true" query-vnc command is
    defined in the VNC subsystem */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 6a492f5..4154a2b 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += resume_vcpu.o
 stub-obj-y += get_icc_bus.o
 stub-obj-y += qemu_system_cpu_hotplug_request.o
+stub-obj-y += do_cpu_hot_add.o
diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c
new file mode 100644
index 0000000..1f6d7a6
--- /dev/null
+++ b/stubs/do_cpu_hot_add.c
@@ -0,0 +1,7 @@
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+
+void do_cpu_hot_add(const int64_t id, Error **errp)
+{
+    error_setg(errp, "Not implemented");
+}
-- 
1.7.1

Re: [Qemu-devel] [PATCH 11/12] qmp: add cpu_set qmp command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

On 03/21/2013 08:28 AM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> +++ b/qapi-schema.json
> @@ -1385,6 +1385,15 @@
>  { 'command': 'cpu', 'data': {'index': 'int'} }
>  
>  ##
> +# @cpu_set
> +#

I already mentioned naming this cpu-set on your cover letter.  Additionally:

> +# Sets specified cpu to online/ofline mode

s/ofline/offline/

> +#
> +# Notes: semantics is : cpu_set id=x status=online|offline
> +##
> +{ 'command': 'cpu_set', 'data': {'id': 'int', 'status': 'str'} }

Don't open-code a 'str'.  My preference would be a bool, with slightly
different naming.

Also, this is not the typical documentation style used in this file.
Rather, it would be more like:

# Sets specified cpu to online/offline mode
#
# @id: cpu id to be updated
#
# @online: true to put the cpu online, false to take it offline
#
##
{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} }


> +void qmp_cpu_set(int64_t id, const char *status, Error **errp)
> +{
> +    if (!strcmp(status, "online")) {
> +        do_cpu_hot_add(id, errp);
> +    } else if (!strcmp(status, "offline")) {
> +        error_setg(errp, "Unplug is not implemented");
> +    } else {
> +        error_setg(errp, "Invalid parameter '%s'", status);
> +        return;

This return could be omitted, with no change in behavior.  But again, I
think a bool rather than an open-coded string parser would make this
simpler:

void qmp_cpu_set(int64_t id, bool online, Error **errp)
{
    if (online) {
        do_cpu_hot_add(id, errp);
    } else {
        error_setg(errp, "Unplug is not implemented");
    }
}

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * s/cpu_set/cpu-set/
  * qmp doc style fix
  * use bool type instead of opencodding online/offline string
     suggested-by: Eric Blake <eblake@redhat.com>
  changes are on WIP branch: https://github.com/imammedo/qemu/tree/cpu_set.WIP

---
 include/sysemu/sysemu.h |    2 ++
 qapi-schema.json        |   12 ++++++++++++
 qmp-commands.hx         |   24 ++++++++++++++++++++++++
 qmp.c                   |    9 +++++++++
 stubs/Makefile.objs     |    1 +
 stubs/do_cpu_hot_add.c  |    7 +++++++
 6 files changed, 55 insertions(+), 0 deletions(-)
 create mode 100644 stubs/do_cpu_hot_add.c

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 4b8f721..8bcaf26 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict);
 void qemu_register_cpu_add_notifier(Notifier *notifier);
 void qemu_system_cpu_hotplug_request(uint32_t id);
 
+void do_cpu_hot_add(const int64_t id, Error **errp);
+
 /* 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/qapi-schema.json b/qapi-schema.json
index 088f4e1..aa5f3dc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1385,6 +1385,18 @@
 { 'command': 'cpu', 'data': {'index': 'int'} }
 
 ##
+# @cpu-set
+#
+# Sets specified cpu to online/offline mode
+#
+# @id: cpu id to be updated
+#
+# @online: true to put the cpu online, false to take it offline
+#
+##
+{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} }
+
+##
 # @memsave:
 #
 # Save a portion of guest memory to a file.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b370060..2f9c256 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -385,6 +385,30 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
 EQMP
 
     {
+        .name       = "cpu-set",
+        .args_type  = "id:i,online:b",
+        .mhandler.cmd_new = qmp_marshal_input_cpu_set,
+    },
+
+SQMP
+cpu-set
+-------
+
+Sets virtual cpu to online/ofline mode
+
+Arguments:
+
+- "id": cpu id (json-int)
+- "online": true to put the cpu online, false to take it offline (json-bool)
+
+Example:
+
+-> { "execute": "cpu-set", "arguments": { "id": 2, "online": true } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "memsave",
         .args_type  = "val:l,size:i,filename:s,cpu:i?",
         .mhandler.cmd_new = qmp_marshal_input_memsave,
diff --git a/qmp.c b/qmp.c
index 55b056b..c211da5 100644
--- a/qmp.c
+++ b/qmp.c
@@ -108,6 +108,15 @@ void qmp_cpu(int64_t index, Error **errp)
     /* Just do nothing */
 }
 
+void qmp_cpu_set(int64_t id, const bool online, Error **errp)
+{
+    if (online) {
+        do_cpu_hot_add(id, errp);
+    } else {
+        error_setg(errp, "Unplug is not implemented");
+    }
+}
+
 #ifndef CONFIG_VNC
 /* If VNC support is enabled, the "true" query-vnc command is
    defined in the VNC subsystem */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 6a492f5..4154a2b 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += resume_vcpu.o
 stub-obj-y += get_icc_bus.o
 stub-obj-y += qemu_system_cpu_hotplug_request.o
+stub-obj-y += do_cpu_hot_add.o
diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c
new file mode 100644
index 0000000..1f6d7a6
--- /dev/null
+++ b/stubs/do_cpu_hot_add.c
@@ -0,0 +1,7 @@
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+
+void do_cpu_hot_add(const int64_t id, Error **errp)
+{
+    error_setg(errp, "Not implemented");
+}
-- 
1.7.1

Re: [Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Luiz Capitulino]

On Mon, 25 Mar 2013 16:35:11 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * s/cpu_set/cpu-set/
>   * qmp doc style fix
>   * use bool type instead of opencodding online/offline string
>      suggested-by: Eric Blake <eblake@redhat.com>
>   changes are on WIP branch: https://github.com/imammedo/qemu/tree/cpu_set.WIP
> 
> ---
>  include/sysemu/sysemu.h |    2 ++
>  qapi-schema.json        |   12 ++++++++++++
>  qmp-commands.hx         |   24 ++++++++++++++++++++++++
>  qmp.c                   |    9 +++++++++
>  stubs/Makefile.objs     |    1 +
>  stubs/do_cpu_hot_add.c  |    7 +++++++
>  6 files changed, 55 insertions(+), 0 deletions(-)
>  create mode 100644 stubs/do_cpu_hot_add.c
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 4b8f721..8bcaf26 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict);
>  void qemu_register_cpu_add_notifier(Notifier *notifier);
>  void qemu_system_cpu_hotplug_request(uint32_t id);
>  
> +void do_cpu_hot_add(const int64_t id, Error **errp);
> +
>  /* 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/qapi-schema.json b/qapi-schema.json
> index 088f4e1..aa5f3dc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1385,6 +1385,18 @@
>  { 'command': 'cpu', 'data': {'index': 'int'} }
>  
>  ##
> +# @cpu-set
> +#
> +# Sets specified cpu to online/offline mode
> +#
> +# @id: cpu id to be updated
> +#
> +# @online: true to put the cpu online, false to take it offline
> +#
> +##
> +{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} }
> +
> +##
>  # @memsave:
>  #
>  # Save a portion of guest memory to a file.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b370060..2f9c256 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -385,6 +385,30 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
>  EQMP
>  
>      {
> +        .name       = "cpu-set",
> +        .args_type  = "id:i,online:b",
> +        .mhandler.cmd_new = qmp_marshal_input_cpu_set,
> +    },
> +
> +SQMP
> +cpu-set
> +-------
> +
> +Sets virtual cpu to online/ofline mode
> +
> +Arguments:
> +
> +- "id": cpu id (json-int)
> +- "online": true to put the cpu online, false to take it offline (json-bool)
> +
> +Example:
> +
> +-> { "execute": "cpu-set", "arguments": { "id": 2, "online": true } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "memsave",
>          .args_type  = "val:l,size:i,filename:s,cpu:i?",
>          .mhandler.cmd_new = qmp_marshal_input_memsave,
> diff --git a/qmp.c b/qmp.c
> index 55b056b..c211da5 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -108,6 +108,15 @@ void qmp_cpu(int64_t index, Error **errp)
>      /* Just do nothing */
>  }
>  
> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> +{
> +    if (online) {
> +        do_cpu_hot_add(id, errp);
> +    } else {
> +        error_setg(errp, "Unplug is not implemented");
> +    }
> +}

As a general rule, we don't allow command extensions to be done this
way because this is not queriable. A client would have to try online=off
to see if QEMU version X supports it, worse: the client would have to
parse the error message to be sure the failure actually corresponds
to unplug not implemented.

The alternative is to have cpu-set-online and later cpu-set-offline. Quite
verbose, but doesn't have issues.

Eric, what do you think?

> +
>  #ifndef CONFIG_VNC
>  /* If VNC support is enabled, the "true" query-vnc command is
>     defined in the VNC subsystem */
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 6a492f5..4154a2b 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += resume_vcpu.o
>  stub-obj-y += get_icc_bus.o
>  stub-obj-y += qemu_system_cpu_hotplug_request.o
> +stub-obj-y += do_cpu_hot_add.o
> diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c
> new file mode 100644
> index 0000000..1f6d7a6
> --- /dev/null
> +++ b/stubs/do_cpu_hot_add.c
> @@ -0,0 +1,7 @@
> +#include "qapi/error.h"
> +#include "sysemu/sysemu.h"
> +
> +void do_cpu_hot_add(const int64_t id, Error **errp)
> +{
> +    error_setg(errp, "Not implemented");
> +}

Re: [Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]

On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> On Mon, 25 Mar 2013 16:35:11 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 

>> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
>> +{
>> +    if (online) {
>> +        do_cpu_hot_add(id, errp);
>> +    } else {
>> +        error_setg(errp, "Unplug is not implemented");
>> +    }
>> +}
> 
> As a general rule, we don't allow command extensions to be done this
> way because this is not queriable. A client would have to try online=off
> to see if QEMU version X supports it, worse: the client would have to
> parse the error message to be sure the failure actually corresponds
> to unplug not implemented.
> 
> The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> verbose, but doesn't have issues.
> 
> Eric, what do you think?

Good point.  What is the likelihood of getting offline working before
1.5 is released?  If we are certain that offline cpu support won't make
this release, then having separate commands would indeed be easier to query.

On the other hand, why aren't we mirroring the QMP behavior to be
similar to what Lazlo already did for qga:
https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01031.html

That is, by having a way to query details on the set of all possible
cpus (via a new command that returns an array with max cpus elements,
rather than just the single int of query-cpu-max in
https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg04441.html),
with information in that query including a second boolean stating
whether that cpu can be taken offline, would be sufficiently queryable.
 The initial implementation would then state that an offline cpu can be
taken online, but that an online cpu must remain in that state.

And while typing that, I also realize that I like Lazlo's approach for
another reason - guest-set-vcpus takes an array of actions, and performs
as many as possible in one transaction; whereas your current cpu-set
command has to be called multiple times to take multiple cpus online.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Igor Mammedov]

On Mon, 25 Mar 2013 14:22:36 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> > On Mon, 25 Mar 2013 16:35:11 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> 
> >> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> >> +{
> >> +    if (online) {
> >> +        do_cpu_hot_add(id, errp);
> >> +    } else {
> >> +        error_setg(errp, "Unplug is not implemented");
> >> +    }
> >> +}
> > 
> > As a general rule, we don't allow command extensions to be done this
> > way because this is not queriable. A client would have to try online=off
> > to see if QEMU version X supports it, worse: the client would have to
> > parse the error message to be sure the failure actually corresponds
> > to unplug not implemented.
> > 
> > The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> > verbose, but doesn't have issues.
It looks like better as way to go. Later on we could keep them for
compatibility sake and map it to device_add/device_del commands when they are
ready.

> > 
> > Eric, what do you think?
> 
> Good point.  What is the likelihood of getting offline working before
> 1.5 is released?  If we are certain that offline cpu support won't make
> this release, then having separate commands would indeed be easier to query.
It's not likely to be ready for 1.5, since "offline" depends on code
introduced in this series and lacks VCPU hot-remove on KVM side. Some time
ago there were patches on list to introduce VCPU hot-remove in KVM, but it
didn't go well I think due to lack of VCPU hot-add and common infrastructure
it introduces.

> 
> On the other hand, why aren't we mirroring the QMP behavior to be
> similar to what Lazlo already did for qga:
> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01031.html
Looks like terms online/offline are confusing, it would be better if
"cpu-set id=xx online=xxx" is replaced with cpu_add/cpu_del commands
following pattern of drive_add/del, pci_add/del ...

> That is, by having a way to query details on the set of all possible
> cpus (via a new command that returns an array with max cpus elements,
> rather than just the single int of query-cpu-max in
> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg04441.html),
> with information in that query including a second boolean stating
> whether that cpu can be taken offline, would be sufficiently queryable.
>  The initial implementation would then state that an offline cpu can be
> taken online, but that an online cpu must remain in that state.
All of it wouldn't be necessary if we have only cpu_add without cpu_del
implemented.

As for querying present VCPUs, one could use qom to enumerate
/machine/unattached/icc-bus childs, looking for childs with type == *-cpu
It's possible to pre-allocate empty shortcut links like /machine/cpu[id] for
all VCPUs and populate corresponding links in x86_cpu_realizefn() when it
is going complete successfully. But query-ability of all possible CPUs could
be a bit out of scope of this series and requires changes to qdev, so I've
left it out for another time.

> And while typing that, I also realize that I like Lazlo's approach for
> another reason - guest-set-vcpus takes an array of actions, and performs
> as many as possible in one transaction; whereas your current cpu-set
> command has to be called multiple times to take multiple cpus online.
Yep, it has to be called for each CPU since it provides low level API at
QEMU level, allowing management to implement higher level ops like
transactions and all the logic associated with it (it is like device_add/del
commands towards which CPUs are moving slowly).

Considering all said above, would it be acceptable to replace cpu-set with
"cpu_add id=xx" command?

Re: [Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Luiz Capitulino]

On Tue, 26 Mar 2013 14:43:01 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 25 Mar 2013 14:22:36 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> > > On Mon, 25 Mar 2013 16:35:11 +0100
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > 
> > >> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> > >> +{
> > >> +    if (online) {
> > >> +        do_cpu_hot_add(id, errp);
> > >> +    } else {
> > >> +        error_setg(errp, "Unplug is not implemented");
> > >> +    }
> > >> +}
> > > 
> > > As a general rule, we don't allow command extensions to be done this
> > > way because this is not queriable. A client would have to try online=off
> > > to see if QEMU version X supports it, worse: the client would have to
> > > parse the error message to be sure the failure actually corresponds
> > > to unplug not implemented.
> > > 
> > > The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> > > verbose, but doesn't have issues.
> It looks like better as way to go. Later on we could keep them for
> compatibility sake and map it to device_add/device_del commands when they are
> ready.

I find the batch API to be a bit overkill (I'd add it in the future if really
needed), but I won't oppose to it.

Re: [Qemu-devel] [PATCH 11/12 v2] qmp: add cpu-set qmp command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On 03/26/2013 07:43 AM, Igor Mammedov wrote:

>>> The alternative is to have cpu-set-online and later cpu-set-offline. Quite
>>> verbose, but doesn't have issues.
> It looks like better as way to go. Later on we could keep them for
> compatibility sake and map it to device_add/device_del commands when they are
> ready.
> 

> Considering all said above, would it be acceptable to replace cpu-set with
> "cpu_add id=xx" command?

cpu_add id=xx from HMP, cpu-add from QMP is fine for getting things
started; it still leaves room for a future 'cpu-set' batching command if
batching turns out to be useful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 11/12] qmp: add cpu_set qmp command

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/sysemu/sysemu.h |    2 ++
>  qapi-schema.json        |    9 +++++++++
>  qmp-commands.hx         |   26 ++++++++++++++++++++++++++
>  qmp.c                   |   12 ++++++++++++
>  stubs/Makefile.objs     |    1 +
>  stubs/do_cpu_hot_add.c  |    7 +++++++
>  6 files changed, 57 insertions(+), 0 deletions(-)
>  create mode 100644 stubs/do_cpu_hot_add.c
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 4b8f721..8bcaf26 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict);
>  void qemu_register_cpu_add_notifier(Notifier *notifier);
>  void qemu_system_cpu_hotplug_request(uint32_t id);
>  
> +void do_cpu_hot_add(const int64_t id, Error **errp);
> +
>  /* 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/qapi-schema.json b/qapi-schema.json
> index fdaa9da..7acec6e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1385,6 +1385,15 @@
>  { 'command': 'cpu', 'data': {'index': 'int'} }
>  
>  ##
> +# @cpu_set
> +#
> +# Sets specified cpu to online/ofline mode
> +#
> +# Notes: semantics is : cpu_set id=x status=online|offline
> +##
> +{ 'command': 'cpu_set', 'data': {'id': 'int', 'status': 'str'} }
> +
> +##
>  # @memsave:
>  #
>  # Save a portion of guest memory to a file.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b370060..283df76 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -385,6 +385,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
>  EQMP
>  
>      {
> +        .name       = "cpu_set",
> +        .args_type  = "id:i,status:s",
> +        .mhandler.cmd_new = qmp_marshal_input_cpu_set,
> +    },
> +
> +SQMP
> +cpu_set
> +-------
> +
> +Sets virtual cpu to online/ofline state
> +
> +Arguments:
> +
> +- "id": virtual cpu id (json-int)
> +- "status": desired state of cpu, online/offline (json-string)
> +
> +Example:
> +
> +-> { "execute": "cpu_set",
> +             "arguments": { "id": 2,
> +                            "status": "online" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "memsave",
>          .args_type  = "val:l,size:i,filename:s,cpu:i?",
>          .mhandler.cmd_new = qmp_marshal_input_memsave,
> diff --git a/qmp.c b/qmp.c
> index 55b056b..5b84779 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -108,6 +108,18 @@ void qmp_cpu(int64_t index, Error **errp)
>      /* Just do nothing */
>  }
>  
> +void qmp_cpu_set(int64_t id, const char *status, Error **errp)
> +{
> +    if (!strcmp(status, "online")) {
> +        do_cpu_hot_add(id, errp);
> +    } else if (!strcmp(status, "offline")) {
> +        error_setg(errp, "Unplug is not implemented");
> +    } else {
> +        error_setg(errp, "Invalid parameter '%s'", status);
> +        return;
> +    }
> +}

Rather than adding stubs, what about the following:

1) put the QEMUMachineInitArgs in a global;

2) put a pointer to do_cpu_hot_add into the QEMUMachine;

3) call the callback from qmp_cpu_set if non-null, using the CPU model
from the QEMUMachineInitArgs

Paolo

>  #ifndef CONFIG_VNC
>  /* If VNC support is enabled, the "true" query-vnc command is
>     defined in the VNC subsystem */
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 6a492f5..4154a2b 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += resume_vcpu.o
>  stub-obj-y += get_icc_bus.o
>  stub-obj-y += qemu_system_cpu_hotplug_request.o
> +stub-obj-y += do_cpu_hot_add.o
> diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c
> new file mode 100644
> index 0000000..1f6d7a6
> --- /dev/null
> +++ b/stubs/do_cpu_hot_add.c
> @@ -0,0 +1,7 @@
> +#include "qapi/error.h"
> +#include "sysemu/sysemu.h"
> +
> +void do_cpu_hot_add(const int64_t id, Error **errp)
> +{
> +    error_setg(errp, "Not implemented");
> +}
> 

[Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov]

... via do_cpu_hot_add() hook called by cpu_set QMP command,
for x86 target.

* add extra check that APIC ID is in allowed range
* return error if CPU with requested APIC ID exists before creating
  a new instance. (CPU removal is broken now, will be fixed with CPU unplug)
* call CPU add notifier as the last step of x86_cpu_realizefn() to
  update rtc_cmos and trigger CPU hot-plug ACPI GPE to notify guest.
  Doing it from x86_cpu_realizefn() will allow to do the same when
  it would be possible to add CPU using device_add.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      |   22 ++++++++++++++++++++++
 target-i386/cpu.c |    1 +
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7481f73..e3ba9ee 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -867,6 +867,19 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
 {
     X86CPU *cpu;
 
+    if (apic_id >= pc_apic_id_limit(max_cpus)) {
+        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
+                   ", max allowed ID: 0x%x", apic_id,
+                   pc_apic_id_limit(max_cpus) - 1);
+        return;
+    }
+
+    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
+        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
+                   ", it's already exists", apic_id);
+        return;
+    }
+
     cpu = cpu_x86_create(cpu_model, errp);
     if (!cpu) {
         return;
@@ -882,6 +895,8 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
     }
 }
 
+static const char *saved_cpu_model;
+
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
@@ -895,6 +910,8 @@ void pc_cpus_init(const char *cpu_model)
         cpu_model = "qemu32";
 #endif
     }
+    saved_cpu_model = cpu_model;
+
 
     for (i = 0; i < smp_cpus; i++) {
         pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
@@ -906,6 +923,11 @@ void pc_cpus_init(const char *cpu_model)
     }
 }
 
+void do_cpu_hot_add(const int64_t id, Error **errp)
+{
+    pc_new_cpu(saved_cpu_model, id, errp);
+}
+
 void pc_acpi_init(const char *default_dsdt)
 {
     char *filename = NULL, *arg = NULL;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ae46f81..d127141 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2271,6 +2271,7 @@ out:
     if (dev->hotplugged) {
         cpu_synchronize_post_init(env);
         resume_vcpu(CPU(cpu));
+        qemu_system_cpu_hotplug_request(env->cpuid_apic_id);
     }
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

On 03/21/2013 08:28 AM, Igor Mammedov wrote:
> ... via do_cpu_hot_add() hook called by cpu_set QMP command,
> for x86 target.
> 
> * add extra check that APIC ID is in allowed range

> +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
> +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> +                   ", it's already exists", apic_id);

s/it's/it/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov]

On Thu, 21 Mar 2013 20:46:57 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/21/2013 08:28 AM, Igor Mammedov wrote:
> > ... via do_cpu_hot_add() hook called by cpu_set QMP command,
> > for x86 target.
> > 
> > * add extra check that APIC ID is in allowed range
> 
> > +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
> > +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > +                   ", it's already exists", apic_id);
> 
> s/it's/it/
> 

fixed on WIP branch:
https://github.com/imammedo/qemu/tree/cpu_set.WIP

Thanks!

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Paolo Bonzini]

Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> ... via do_cpu_hot_add() hook called by cpu_set QMP command,
> for x86 target.
> 
> * add extra check that APIC ID is in allowed range
> * return error if CPU with requested APIC ID exists before creating
>   a new instance. (CPU removal is broken now, will be fixed with CPU unplug)
> * call CPU add notifier as the last step of x86_cpu_realizefn() to
>   update rtc_cmos and trigger CPU hot-plug ACPI GPE to notify guest.
>   Doing it from x86_cpu_realizefn() will allow to do the same when
>   it would be possible to add CPU using device_add.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c      |   22 ++++++++++++++++++++++
>  target-i386/cpu.c |    1 +
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7481f73..e3ba9ee 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -867,6 +867,19 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
>  {
>      X86CPU *cpu;
>  
> +    if (apic_id >= pc_apic_id_limit(max_cpus)) {
> +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> +                   ", max allowed ID: 0x%x", apic_id,
> +                   pc_apic_id_limit(max_cpus) - 1);
> +        return;
> +    }

This seems the wrong place to do this check.  It should be done in
do_cpu_hot_add, simply comparing against max_cpus.  Here, instead, you
should _assert_ that the APIC ID is in range.

> +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {

Similarly, can this be done in qmp_cpu_set?  And should it really be an
error?  Onlining an already-online CPU is fine.

Again, here you could assert that the CPU is not a duplicate, instead.

> +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> +                   ", it's already exists", apic_id);
> +        return;
> +    }
>      cpu = cpu_x86_create(cpu_model, errp);
>      if (!cpu) {
>          return;
> @@ -882,6 +895,8 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
>      }
>  }
>  
> +static const char *saved_cpu_model;
> +
>  void pc_cpus_init(const char *cpu_model)

Instead of using this global, see the approach in the previous patch.

>  {
>      int i;
> @@ -895,6 +910,8 @@ void pc_cpus_init(const char *cpu_model)
>          cpu_model = "qemu32";
>  #endif
>      }
> +    saved_cpu_model = cpu_model;
> +

>      for (i = 0; i < smp_cpus; i++) {
>          pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> @@ -906,6 +923,11 @@ void pc_cpus_init(const char *cpu_model)
>      }
>  }
>  
> +void do_cpu_hot_add(const int64_t id, Error **errp)
> +{
> +    pc_new_cpu(saved_cpu_model, id, errp);
> +}
> +

Missing x86_cpu_apic_id_from_index(id)?

>  void pc_acpi_init(const char *default_dsdt)
>  {
>      char *filename = NULL, *arg = NULL;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ae46f81..d127141 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2271,6 +2271,7 @@ out:
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(env);
>          resume_vcpu(CPU(cpu));
> +        qemu_system_cpu_hotplug_request(env->cpuid_apic_id);

As mentioned earlier, this notifier should be invoked at the CPU level,
not X86CPU.

Paolo

>      }
>  }
>  
> 

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov]

On Wed, 27 Mar 2013 12:19:01 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> > ... via do_cpu_hot_add() hook called by cpu_set QMP command,
> > for x86 target.
> > 
> > * add extra check that APIC ID is in allowed range
> > * return error if CPU with requested APIC ID exists before creating
> >   a new instance. (CPU removal is broken now, will be fixed with CPU unplug)
> > * call CPU add notifier as the last step of x86_cpu_realizefn() to
> >   update rtc_cmos and trigger CPU hot-plug ACPI GPE to notify guest.
> >   Doing it from x86_cpu_realizefn() will allow to do the same when
> >   it would be possible to add CPU using device_add.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c      |   22 ++++++++++++++++++++++
> >  target-i386/cpu.c |    1 +
> >  2 files changed, 23 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7481f73..e3ba9ee 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -867,6 +867,19 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> >  {
> >      X86CPU *cpu;
> >  
> > +    if (apic_id >= pc_apic_id_limit(max_cpus)) {
> > +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > +                   ", max allowed ID: 0x%x", apic_id,
> > +                   pc_apic_id_limit(max_cpus) - 1);
> > +        return;
> > +    }
> 
> This seems the wrong place to do this check.  It should be done in
> do_cpu_hot_add, simply comparing against max_cpus.  Here, instead, you
> should _assert_ that the APIC ID is in range.
I'll move it there,  but I'd leave pc_apic_id_limit(max_cpus) since id it
compares with is APIC ID.

> 
> > +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
> 
> Similarly, can this be done in qmp_cpu_set?  And should it really be an
I've tried to abstract qmp part from target/implementation details.
We could introduce global func like, and then use it if from QMP subsystem:
  bool is_cpu_exist(int64_t id) {
     ... iterate over CPUs ...
     if (cpu->get_firmware_id() == id)
         return true;
     ...
  }

Andreas, Is it acceptable if I add it to qom/cpu.h, qom/cpu.c ?

> error?  Onlining an already-online CPU is fine.
CPU is not just onlined though, it's created and it couldn't be right to
create the same CPU. Hence a error here, so user would know that it tries to
add the same device.

> 
> Again, here you could assert that the CPU is not a duplicate, instead.
sure

> 
> > +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > +                   ", it's already exists", apic_id);
> > +        return;
> > +    }
> >      cpu = cpu_x86_create(cpu_model, errp);
> >      if (!cpu) {
> >          return;
> > @@ -882,6 +895,8 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> >      }
> >  }
> >  
> > +static const char *saved_cpu_model;
> > +
> >  void pc_cpus_init(const char *cpu_model)
> 
> Instead of using this global, see the approach in the previous patch.
> 
> >  {
> >      int i;
> > @@ -895,6 +910,8 @@ void pc_cpus_init(const char *cpu_model)
> >          cpu_model = "qemu32";
> >  #endif
> >      }
> > +    saved_cpu_model = cpu_model;
> > +
> 
> >      for (i = 0; i < smp_cpus; i++) {
> >          pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> > @@ -906,6 +923,11 @@ void pc_cpus_init(const char *cpu_model)
> >      }
> >  }
> >  
> > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > +{
> > +    pc_new_cpu(saved_cpu_model, id, errp);
> > +}
> > +
> 
> Missing x86_cpu_apic_id_from_index(id)?
There was(is?) opposition to using cpu_index to identify x86 CPU.
So, it is expected from management to provide APIC ID instead of cpu_index.
It could be useful to make hotplug to a specific NUMA node/cpu to work in
future.
Though interface of possible APIC IDs discovery is not part of this series.

> 
> >  void pc_acpi_init(const char *default_dsdt)
> >  {
> >      char *filename = NULL, *arg = NULL;
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ae46f81..d127141 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2271,6 +2271,7 @@ out:
> >      if (dev->hotplugged) {
> >          cpu_synchronize_post_init(env);
> >          resume_vcpu(CPU(cpu));
> > +        qemu_system_cpu_hotplug_request(env->cpuid_apic_id);
> 
> As mentioned earlier, this notifier should be invoked at the CPU level,
> not X86CPU.
done

> Paolo
> 
> >      }
> >  }
> >  
> > 
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Eduardo Habkost]

On Wed, Apr 03, 2013 at 07:58:00PM +0200, Igor Mammedov wrote:
<snip>
> > > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > > +{
> > > +    pc_new_cpu(saved_cpu_model, id, errp);
> > > +}
> > > +
> > 
> > Missing x86_cpu_apic_id_from_index(id)?
> There was(is?) opposition to using cpu_index to identify x86 CPU.

Really? Do you have a pointer to the discussion?


> So, it is expected from management to provide APIC ID instead of cpu_index.
> It could be useful to make hotplug to a specific NUMA node/cpu to work in
> future.
> Though interface of possible APIC IDs discovery is not part of this series.

That's exactly the opposite of what I expect. The APIC ID is an internal
implementation detail, and external tools must _not_ be required to deal
with it and to calculate it.

Communication with the BIOS, on the other hand, is entirely based on the
APIC ID, and not CPU indexes. So QEMU needs to translate the CPU indexes
(used to communicate with the outside world) to APIC IDs when talking to
the BIOS.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov]

On Wed, 3 Apr 2013 15:10:05 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 03, 2013 at 07:58:00PM +0200, Igor Mammedov wrote:
> <snip>
> > > > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > > > +{
> > > > +    pc_new_cpu(saved_cpu_model, id, errp);
> > > > +}
> > > > +
> > > 
> > > Missing x86_cpu_apic_id_from_index(id)?
> > There was(is?) opposition to using cpu_index to identify x86 CPU.
> 
> Really? Do you have a pointer to the discussion?
Here is what I could find in my mail box:
http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02770.html
Jan could correct me if I'm wrong.

> 
> 
> > So, it is expected from management to provide APIC ID instead of cpu_index.
> > It could be useful to make hotplug to a specific NUMA node/cpu to work in
> > future.
> > Though interface of possible APIC IDs discovery is not part of this series.
> 
> That's exactly the opposite of what I expect. The APIC ID is an internal
> implementation detail, and external tools must _not_ be required to deal
> with it and to calculate it.
> 
> Communication with the BIOS, on the other hand, is entirely based on the
> APIC ID, and not CPU indexes. So QEMU needs to translate the CPU indexes
> (used to communicate with the outside world) to APIC IDs when talking to
> the BIOS.
cpu_index won't work nicely with hot-adding CPU to specific numa node though.
with APIC ID (mgmt might treat it as opaque) we could expose something like

/machine/icc-bridge/link<CPU[apic_id_n]
...

for all possible CPUs, with empty links for non existing ones.

and later add on something like this:

/machine/numa_node[x]/link<CPU[apic_id_n]>
...

Libvirt than could just pickup ready apic id from desired place and add CPU
either using cpu-add id=xxx or device_add x86-cpu-...,apic_id=xxx

+1 more cpu_index is QEMU implementation detail and we could not add to x86 CPU
cpu-index property since hardware doesn't have such feature, so it won't be
available with device_add.
 
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Eduardo Habkost]

On Wed, Apr 03, 2013 at 08:59:07PM +0200, Igor Mammedov wrote:
> On Wed, 3 Apr 2013 15:10:05 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Apr 03, 2013 at 07:58:00PM +0200, Igor Mammedov wrote:
> > <snip>
> > > > > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > > > > +{
> > > > > +    pc_new_cpu(saved_cpu_model, id, errp);
> > > > > +}
> > > > > +
> > > > 
> > > > Missing x86_cpu_apic_id_from_index(id)?
> > > There was(is?) opposition to using cpu_index to identify x86 CPU.
> > 
> > Really? Do you have a pointer to the discussion?
> Here is what I could find in my mail box:
> http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02770.html
> Jan could correct me if I'm wrong.



So, quoting Jan:
> From my POV, cpu_index could become equal to the physical APIC ID.
> As long as we can set it freely (provided it remains unique) and
> non-continuously, we don't need separate indexes."

We can't choose APIC IDs freely, because the APIC ID is calculated based
on the CPU topology (socket + core + thread IDs).

So, the cpu_index could be the same as the APIC ID if the cpu_index
value declared opaque, being just a "CPU identifier" that is chosen by
QEMU arbitrarily (and that happens to match the APIC ID). But we will
probably have some problems with this:

- The CPU index are currently allocated contiguously, and probably
  existing interfaces already assume that (e.g. the "-numa' option,
  "info numa", "info cpus" and maybe other monitor commands)
- QEMU must be responsible for calculating the APIC ID of each CPU,
  because it is based on the CPU topology.
- If QEMU is the one who calculates the APIC ID, what kind of identifier
  we can use for a CPU object in the command-line (e.g. in the "-numa"
  option)?
- We may need to redefine the meaning of the "maxcpus" -smp option, if
  all our interfaces are now based in non-contiguous and freely-set CPU
  identifiers.

In short, getting rid of the contiguous CPU indexes sounds very
difficult. We could introduce other kind of identifiers, but probably we
may need to keep the CPU indexes contiguous to keep existing interfaces
working.

> 
> > 
> > 
> > > So, it is expected from management to provide APIC ID instead of cpu_index.
> > > It could be useful to make hotplug to a specific NUMA node/cpu to work in
> > > future.
> > > Though interface of possible APIC IDs discovery is not part of this series.
> > 
> > That's exactly the opposite of what I expect. The APIC ID is an internal
> > implementation detail, and external tools must _not_ be required to deal
> > with it and to calculate it.
> > 
> > Communication with the BIOS, on the other hand, is entirely based on the
> > APIC ID, and not CPU indexes. So QEMU needs to translate the CPU indexes
> > (used to communicate with the outside world) to APIC IDs when talking to
> > the BIOS.
> cpu_index won't work nicely with hot-adding CPU to specific numa node though.

Well, the "-numa node" options are already based on CPU indexes, so it
would match it the existing NUMA configuration interface.

> with APIC ID (mgmt might treat it as opaque) we could expose something like
> 
> /machine/icc-bridge/link<CPU[apic_id_n]
> ...
> 
> for all possible CPUs, with empty links for non existing ones.
> 
> and later add on something like this:
> 
> /machine/numa_node[x]/link<CPU[apic_id_n]>
> ...
> 
> Libvirt than could just pickup ready apic id from desired place and add CPU
> either using cpu-add id=xxx or device_add x86-cpu-...,apic_id=xxx
> 
> +1 more cpu_index is QEMU implementation detail and we could not add to x86 CPU
> cpu-index property since hardware doesn't have such feature, so it won't be
> available with device_add.

I don't mind hiding cpu_index too. I don't mind if we use a cpu_index,
QOM links, arbitrary IDs set by the user. I just have a problem with
requiring libvirt to set the APIC ID.

If you give libvirt an easy way to convert a CPU "location" (index, numa
node, whatever) to an APIC ID that is pre-calculated by QEMU, then it
could work. But do we really need to require libvirt to deal with APIC
ID directly? If you just set the links properly to reflect the CPU
"location", the CPU could calculate its APIC ID based on its "location"
using the links.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov]

On Wed, 3 Apr 2013 16:27:11 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 03, 2013 at 08:59:07PM +0200, Igor Mammedov wrote:
> > On Wed, 3 Apr 2013 15:10:05 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, Apr 03, 2013 at 07:58:00PM +0200, Igor Mammedov wrote:
> > > <snip>
> > > > > > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > > > > > +{
> > > > > > +    pc_new_cpu(saved_cpu_model, id, errp);
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > Missing x86_cpu_apic_id_from_index(id)?
> > > > There was(is?) opposition to using cpu_index to identify x86 CPU.
> > > 
> > > Really? Do you have a pointer to the discussion?
> > Here is what I could find in my mail box:
> > http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02770.html
> > Jan could correct me if I'm wrong.
> 
> 
> 
> So, quoting Jan:
> > From my POV, cpu_index could become equal to the physical APIC ID.
> > As long as we can set it freely (provided it remains unique) and
> > non-continuously, we don't need separate indexes."
> 
> We can't choose APIC IDs freely, because the APIC ID is calculated based
> on the CPU topology (socket + core + thread IDs).
> 
> So, the cpu_index could be the same as the APIC ID if the cpu_index
> value declared opaque, being just a "CPU identifier" that is chosen by
> QEMU arbitrarily (and that happens to match the APIC ID). But we will
> probably have some problems with this:
> 
> - The CPU index are currently allocated contiguously, and probably
>   existing interfaces already assume that (e.g. the "-numa' option,
>   "info numa", "info cpus" and maybe other monitor commands)
> - QEMU must be responsible for calculating the APIC ID of each CPU,
>   because it is based on the CPU topology.
> - If QEMU is the one who calculates the APIC ID, what kind of identifier
>   we can use for a CPU object in the command-line (e.g. in the "-numa"
>   option)?
using any kind of thread id is problematic since 2 treads from the same core
could end-up on different nodes.
Maybe placement interface could be better described as node[n]=sockets_list?
 
> - We may need to redefine the meaning of the "maxcpus" -smp option, if
>   all our interfaces are now based in non-contiguous and freely-set CPU
>   identifiers.
it's amount of CPUs available to guest, pretty clear from user's POV.
 
> 
> In short, getting rid of the contiguous CPU indexes sounds very
> difficult. We could introduce other kind of identifiers, but probably we
> may need to keep the CPU indexes contiguous to keep existing interfaces
> working.
Once we have CPU unplug, we will have non-contiguous cpu_index. So it will be
part of CPU unplug series to fix cpu_index allocation/usage where necessary.

> 
> > 
> > > 
> > > 
> > > > So, it is expected from management to provide APIC ID instead of cpu_index.
> > > > It could be useful to make hotplug to a specific NUMA node/cpu to work in
> > > > future.
> > > > Though interface of possible APIC IDs discovery is not part of this series.
> > > 
> > > That's exactly the opposite of what I expect. The APIC ID is an internal
> > > implementation detail, and external tools must _not_ be required to deal
> > > with it and to calculate it.
> > > 
> > > Communication with the BIOS, on the other hand, is entirely based on the
> > > APIC ID, and not CPU indexes. So QEMU needs to translate the CPU indexes
> > > (used to communicate with the outside world) to APIC IDs when talking to
> > > the BIOS.
> > cpu_index won't work nicely with hot-adding CPU to specific numa node though.
> 
> Well, the "-numa node" options are already based on CPU indexes, so it
> would match it the existing NUMA configuration interface.
> 
> > with APIC ID (mgmt might treat it as opaque) we could expose something like
> > 
> > /machine/icc-bridge/link<CPU[apic_id_n]
> > ...
> > 
> > for all possible CPUs, with empty links for non existing ones.
> > 
> > and later add on something like this:
> > 
> > /machine/numa_node[x]/link<CPU[apic_id_n]>
> > ...
> > 
> > Libvirt than could just pickup ready apic id from desired place and add CPU
> > either using cpu-add id=xxx or device_add x86-cpu-...,apic_id=xxx
> > 
> > +1 more cpu_index is QEMU implementation detail and we could not add to x86 CPU
> > cpu-index property since hardware doesn't have such feature, so it won't be
> > available with device_add.
> 
> I don't mind hiding cpu_index too. I don't mind if we use a cpu_index,
> QOM links, arbitrary IDs set by the user. I just have a problem with
> requiring libvirt to set the APIC ID.
> 
> If you give libvirt an easy way to convert a CPU "location" (index, numa
> node, whatever) to an APIC ID that is pre-calculated by QEMU, then it
> could work. But do we really need to require libvirt to deal with APIC
> ID directly? If you just set the links properly to reflect the CPU
> "location", the CPU could calculate its APIC ID based on its "location"
> using the links.
What about adding CPU to a specific node then, it would require interface for
communicating to CPU to which node it should be plugged (part of APIC ID, I
guess).

> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Eduardo Habkost]

On Wed, Apr 03, 2013 at 10:09:25PM +0200, Igor Mammedov wrote:
> On Wed, 3 Apr 2013 16:27:11 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Apr 03, 2013 at 08:59:07PM +0200, Igor Mammedov wrote:
> > > On Wed, 3 Apr 2013 15:10:05 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Wed, Apr 03, 2013 at 07:58:00PM +0200, Igor Mammedov wrote:
> > > > <snip>
> > > > > > > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > > > > > > +{
> > > > > > > +    pc_new_cpu(saved_cpu_model, id, errp);
> > > > > > > +}
> > > > > > > +
> > > > > > 
> > > > > > Missing x86_cpu_apic_id_from_index(id)?
> > > > > There was(is?) opposition to using cpu_index to identify x86 CPU.
> > > > 
> > > > Really? Do you have a pointer to the discussion?
> > > Here is what I could find in my mail box:
> > > http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02770.html
> > > Jan could correct me if I'm wrong.
> > 
> > 
> > 
> > So, quoting Jan:
> > > From my POV, cpu_index could become equal to the physical APIC ID.
> > > As long as we can set it freely (provided it remains unique) and
> > > non-continuously, we don't need separate indexes."
> > 
> > We can't choose APIC IDs freely, because the APIC ID is calculated based
> > on the CPU topology (socket + core + thread IDs).
> > 
> > So, the cpu_index could be the same as the APIC ID if the cpu_index
> > value declared opaque, being just a "CPU identifier" that is chosen by
> > QEMU arbitrarily (and that happens to match the APIC ID). But we will
> > probably have some problems with this:
> > 
> > - The CPU index are currently allocated contiguously, and probably
> >   existing interfaces already assume that (e.g. the "-numa' option,
> >   "info numa", "info cpus" and maybe other monitor commands)
> > - QEMU must be responsible for calculating the APIC ID of each CPU,
> >   because it is based on the CPU topology.
> > - If QEMU is the one who calculates the APIC ID, what kind of identifier
> >   we can use for a CPU object in the command-line (e.g. in the "-numa"
> >   option)?
> using any kind of thread id is problematic since 2 treads from the same core
> could end-up on different nodes.
> Maybe placement interface could be better described as node[n]=sockets_list?

The problem here is compatibility: we need to keep existing
command-lines working. And the existing interface (among other things)
doesn't prevent two threads from being in different NUMA nodes.

If today "-smp 9,cores=3,threads=3 -numa node,cpus=0-4 -numa
nodes,cpus=5-8" has a specific meaning, we need to keep the same
meaning.

>  
> > - We may need to redefine the meaning of the "maxcpus" -smp option, if
> >   all our interfaces are now based in non-contiguous and freely-set CPU
> >   identifiers.
> it's amount of CPUs available to guest, pretty clear from user's POV.

I was just worrying if there could be assumptions that "maxcpus is
always > cpu_index". But you are probably right.

>  
> > 
> > In short, getting rid of the contiguous CPU indexes sounds very
> > difficult. We could introduce other kind of identifiers, but probably we
> > may need to keep the CPU indexes contiguous to keep existing interfaces
> > working.
> Once we have CPU unplug, we will have non-contiguous cpu_index. So it will be
> part of CPU unplug series to fix cpu_index allocation/usage where necessary.

Keeping compatibility after CPU unplug is not a problem as CPU unplug
doesn't exist yet. The problem here is to have a realiable identifier
for CPUs that can be used in the command-line. The only identifier we
have for that today is a contiguous CPU index, and if we make them not
contiguous we are going to make existing command-lines that use CPU
indexes (e.g. using "-numa") break.

> 
> > 
> > > 
> > > > 
> > > > 
> > > > > So, it is expected from management to provide APIC ID instead of cpu_index.
> > > > > It could be useful to make hotplug to a specific NUMA node/cpu to work in
> > > > > future.
> > > > > Though interface of possible APIC IDs discovery is not part of this series.
> > > > 
> > > > That's exactly the opposite of what I expect. The APIC ID is an internal
> > > > implementation detail, and external tools must _not_ be required to deal
> > > > with it and to calculate it.
> > > > 
> > > > Communication with the BIOS, on the other hand, is entirely based on the
> > > > APIC ID, and not CPU indexes. So QEMU needs to translate the CPU indexes
> > > > (used to communicate with the outside world) to APIC IDs when talking to
> > > > the BIOS.
> > > cpu_index won't work nicely with hot-adding CPU to specific numa node though.
> > 
> > Well, the "-numa node" options are already based on CPU indexes, so it
> > would match it the existing NUMA configuration interface.
> > 
> > > with APIC ID (mgmt might treat it as opaque) we could expose something like
> > > 
> > > /machine/icc-bridge/link<CPU[apic_id_n]
> > > ...
> > > 
> > > for all possible CPUs, with empty links for non existing ones.
> > > 
> > > and later add on something like this:
> > > 
> > > /machine/numa_node[x]/link<CPU[apic_id_n]>
> > > ...
> > > 
> > > Libvirt than could just pickup ready apic id from desired place and add CPU
> > > either using cpu-add id=xxx or device_add x86-cpu-...,apic_id=xxx
> > > 
> > > +1 more cpu_index is QEMU implementation detail and we could not add to x86 CPU
> > > cpu-index property since hardware doesn't have such feature, so it won't be
> > > available with device_add.
> > 
> > I don't mind hiding cpu_index too. I don't mind if we use a cpu_index,
> > QOM links, arbitrary IDs set by the user. I just have a problem with
> > requiring libvirt to set the APIC ID.
> > 
> > If you give libvirt an easy way to convert a CPU "location" (index, numa
> > node, whatever) to an APIC ID that is pre-calculated by QEMU, then it
> > could work. But do we really need to require libvirt to deal with APIC
> > ID directly? If you just set the links properly to reflect the CPU
> > "location", the CPU could calculate its APIC ID based on its "location"
> > using the links.
> What about adding CPU to a specific node then, it would require interface for
> communicating to CPU to which node it should be plugged (part of APIC ID, I
> guess).

Using QOM we could just use links. The question to me is how to identify
the CPU "location" reliably if we're going to in a "cpu-set" interface.
My point is that cpu_index works perfectly for that (as long as the
rules about how each CPU index is allocated to each NUMA-node, socket,
core, and thread). Later we can have something not based on CPU indexes,
if we move to a 100% link-based QOM interface.

Having to ask QEMU for the APIC ID somehow and requiring the APIC ID to
be provided on the cpu-set command could work, yes. But it must not
require libvirt to calculate and choose the APIC IDs itself.

(Note that I didn't review all the code yet. Maybe you are already doing
all that)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Andreas Färber]

Am 03.04.2013 19:58, schrieb Igor Mammedov:
> On Wed, 27 Mar 2013 12:19:01 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
>>> +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
>>
>> Similarly, can this be done in qmp_cpu_set?  And should it really be an
> I've tried to abstract qmp part from target/implementation details.
> We could introduce global func like, and then use it if from QMP subsystem:
>   bool is_cpu_exist(int64_t id) {
>      ... iterate over CPUs ...
>      if (cpu->get_firmware_id() == id)
>          return true;
>      ...
>   }
> 
> Andreas, Is it acceptable if I add it to qom/cpu.h,

If you make it cpu_exists() then sure,

> qom/cpu.c ?

but this is not going to work yet, you may need to place it in cpus.c
due to first_cpu and next_cpu operating on CPUArchState.

Andreas

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

Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov]

On Wed, 03 Apr 2013 20:22:17 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 03.04.2013 19:58, schrieb Igor Mammedov:
> > On Wed, 27 Mar 2013 12:19:01 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> >>> +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
> >>
> >> Similarly, can this be done in qmp_cpu_set?  And should it really be an
> > I've tried to abstract qmp part from target/implementation details.
> > We could introduce global func like, and then use it if from QMP subsystem:
> >   bool is_cpu_exist(int64_t id) {
> >      ... iterate over CPUs ...
> >      if (cpu->get_firmware_id() == id)
> >          return true;
> >      ...
> >   }
> > 
> > Andreas, Is it acceptable if I add it to qom/cpu.h,
> 
> If you make it cpu_exists() then sure,
> 
> > qom/cpu.c ?
> 
> but this is not going to work yet, you may need to place it in cpus.c
> due to first_cpu and next_cpu operating on CPUArchState.

I actually do recursive search on QOM tree /machine for TYPE_CPU avoiding
usage of first_cpu and next_cpu. So it's abstracted from CPUArchState.

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


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 00/12] target-i386: CPU hot-add with cpu_set QMP command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On 03/21/2013 08:28 AM, Igor Mammedov wrote:
> Implements alternative way for hot-adding CPU using cpu_set QMP command,

The QMP command should probably be named cpu-set.

> wich could be useful until it would be possible to add CPUs via device_add.
> 
> Patches 
>   1-10 are common to both approaches
>   11-12 are specific to adding CPU using cpu_set
> 
> To hot-add CPU use following command from qmp-shell:
>  cpu_set in=[apic id] status=online
> 
> git tree for testing: https://github.com/imammedo/qemu/tree/cpu_set
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC 00/12] target-i386: CPU hot-add with cpu_set QMP command

[Igor Mammedov]

On Thu, 21 Mar 2013 08:44:21 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/21/2013 08:28 AM, Igor Mammedov wrote:
> > Implements alternative way for hot-adding CPU using cpu_set QMP command,
> 
> The QMP command should probably be named cpu-set.
> 
sure, I'll rename it.