Re: [PATCH] cpu hotplug issue

[Gleb Natapov <gleb@redhat.com>]

On Thu, Jul 21, 2011 at 01:06:41PM +0200, Vasilis Liaskovitis wrote:
> Hi,
> 
> On Wed, Jul 20, 2011 at 10:35 AM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jul 19, 2011 at 07:40:55PM +0200, Vasilis Liaskovitis wrote:
> >> Hello,
> >>
> >> I have encountered a problem trying to hotplug a CPU in my x86_64 guest setup.
> >>
> > You do everything right. It's qemu who is buggy. Since qemu need a patch
> > for cpu hotplug to not crash it nobody tests it, so code bit rots.
> 
> thanks for your reply.
> 
> As I mentioned in the original email, onlining a hotplugged-cpu with
> qemu-kvm/master results in:
> 
> >> echo 1 > /sys/devices/system/cpu/cpu1/online
> >> bash: echo: write error: Input/output error
> >>
> >> in the guest, dmesg reports:
> >>
> >> [ 2325.376355] Booting Node 0 Processor 1 APIC 0x1
> >> [ 2325.376357] smpboot cpu 1: start_ip = 9a000
> >> [ 2330.821306] CPU1: Not responding.
> 
> I tried to git-bisect between qemu-kvm-0.13.0 (last known version
> where cpu hotplug works correctly
> for me) and qemu-kvm/master.
> 
> More precisely: To enable cpu-hotplug at each bisect stage, I apply
> this patch derived from:
> http://lists.gnu.org/archive/html/qemu-devel/2010-08/msg00850.html
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 1aa1ea0..aed48ce 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -327,6 +327,7 @@ BusState *sysbus_get_default(void)
>     if (!main_system_bus) {
>         main_system_bus = qbus_create(&system_bus_info, NULL,
>                                       "main-system-bus");
> +       main_system_bus->allow_hotplug = 1;
>     }
>     return main_system_bus;
> }
> 
> and test cpu hotplug functionality.
> The commit that appears to break CPU hotplug is:
> 
Thank you for going through the pain of bisecting it! Bisects that
require patch to be applied between each step are especially annoying.

> commit f4de8c1451f2265148ff4d895a27e21c0a8788aa
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Mon Feb 21 12:28:07 2011 +0100
>    qemu-kvm: Mark VCPU state dirty on creation
> 
Jan can you look at this please?

> Is it possible that kvm_vcpu_dirty should not be set to 1 for a CPU
> that's being hot-plugged?
> I.e. when kvm_cpu_exec() is called for the first time during
> initialization of a hotplugged-CPU,
> we shouldn't try to restore state with kvm_arch_put_registers().
> 
> The following patch enables hotplug and solves the non-responsive
> hotplugged CPU problem,
> by not setting the vcpu_dirty state when hotplugging. Enabling hotplug
> is still done through
> the main_system_bus (see above).
> Tested on amd64host/amd64guest combination. Comments?
> 
> Note that there is probably another bug in qemu-kvm/master regarding
> acpi-udev event delivery for
> a cpu-hotplug event (cpu_set in the qemu_monitor no longer triggers
> the event in the guest, see
> first issue in my original mail). This patch does not address that issue.
> 
Did this work in qemu-0.13?

> Signed-off-by: Vasilis Liaskovitis <vliaskov@gmail.com>
> ---
> 
>  hw/acpi_piix4.c   |    2 +-
>  hw/pc.c           |   13 +++++++++++--
>  hw/qdev.c         |    1 +
>  kvm-all.c         |   22 +++++++++++++++++++++-
>  kvm.h             |    3 +++
>  target-i386/cpu.h |    2 +-
>  6 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index c30a050..b0cac60 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -584,7 +584,7 @@ void qemu_system_cpu_hot_add(int cpu, int state)
>      PIIX4PMState *s = global_piix4_pm_state;
> 
>      if (state && !qemu_get_cpu(cpu)) {
> -        env = pc_new_cpu(global_cpu_model);
> +        env = pc_new_cpu(global_cpu_model, 1);
>          if (!env) {
>              fprintf(stderr, "cpu %d creation failed\n", cpu);
>              return;
> diff --git a/hw/pc.c b/hw/pc.c
> index c0a88e1..8cfbf27 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -924,7 +924,7 @@ static void pc_cpu_reset(void *opaque)
>      env->halted = !cpu_is_bsp(env);
>  }
> 
> -CPUState *pc_new_cpu(const char *cpu_model)
> +CPUState *pc_new_cpu(const char *cpu_model, int hotplugged)
>  {
>      CPUState *env;
> 
> @@ -936,7 +936,16 @@ CPUState *pc_new_cpu(const char *cpu_model)
>  #endif
>      }
> 
> +    if (hotplugged) {
> +        kvm_set_cpuhotplug_req();
> +    }
> +
>      env = cpu_init(cpu_model);
> +
> +    if (hotplugged) {
> +        kvm_reset_cpuhotplug_req();
> +    }
> +
>      if (!env) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          exit(1);
> @@ -956,7 +965,7 @@ void pc_cpus_init(const char *cpu_model)
> 
>      /* init CPUs */
>      for(i = 0; i < smp_cpus; i++) {
> -        pc_new_cpu(cpu_model);
> +        pc_new_cpu(cpu_model, 0);
>      }
>  }
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 292b52f..f85ac0f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -327,6 +327,7 @@ BusState *sysbus_get_default(void)
>      if (!main_system_bus) {
>          main_system_bus = qbus_create(&system_bus_info, NULL,
>                                        "main-system-bus");
> +	main_system_bus->allow_hotplug = 1;
>      }
>      return main_system_bus;
>  }
> diff --git a/kvm-all.c b/kvm-all.c
> index 3ad2459..8aae1d7 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -85,6 +85,7 @@ struct KVMState
>  #endif
>      void *used_gsi_bitmap;
>      int max_gsi;
> +    int cpu_hotplug_req;
>  };
> 
>  KVMState *kvm_state;
> @@ -220,7 +221,9 @@ int kvm_init_vcpu(CPUState *env)
> 
>      env->kvm_fd = ret;
>      env->kvm_state = s;
> -    env->kvm_vcpu_dirty = 1;
> +    if (!kvm_has_cpuhotplug_req()) {
> +        env->kvm_vcpu_dirty = 1;
> +    }
> 
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>      if (mmap_size < 0) {
> @@ -797,6 +800,8 @@ int kvm_init(void)
> 
>      s->pit_in_kernel = kvm_pit;
> 
> +    s->cpu_hotplug_req = 0;
> +
>      ret = kvm_arch_init(s);
>      if (ret < 0) {
>          goto err;
> @@ -1104,6 +1109,21 @@ int kvm_has_vcpu_events(void)
>      return kvm_state->vcpu_events;
>  }
> 
> +int kvm_has_cpuhotplug_req(void)
> +{
> +    return kvm_state->cpu_hotplug_req;
> +}
> +
> +void kvm_set_cpuhotplug_req(void)
> +{
> +    kvm_state->cpu_hotplug_req = 1;
> +}
> +
> +void kvm_reset_cpuhotplug_req(void)
> +{
> +    kvm_state->cpu_hotplug_req = 0;
> +}
> +
>  int kvm_has_robust_singlestep(void)
>  {
>      return kvm_state->robust_singlestep;
> diff --git a/kvm.h b/kvm.h
> index b15e1dd..7fa72fd 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -52,6 +52,9 @@ int kvm_has_xsave(void);
>  int kvm_has_xcrs(void);
>  int kvm_has_many_ioeventfds(void);
>  int kvm_has_pit_state2(void);
> +int kvm_has_cpuhotplug_req(void);
> +void kvm_set_cpuhotplug_req(void);
> +void kvm_reset_cpuhotplug_req(void);
> 
>  #ifdef NEED_CPU_H
>  int kvm_init_vcpu(CPUState *env);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 935d08a..7e7839b 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -923,7 +923,7 @@ void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4);
>  /* hw/pc.c */
>  void cpu_smm_update(CPUX86State *env);
>  uint64_t cpu_get_tsc(CPUX86State *env);
> -CPUState *pc_new_cpu(const char *cpu_model);
> +CPUState *pc_new_cpu(const char *cpu_model, int hotplugged);
> 
>  /* used to debug */
>  #define X86_DUMP_FPU  0x0001 /* dump FPU state too */

--
			Gleb.