Re: [Qemu-devel] [RFC/PATCH 1/1] s390: implement CPU hotplug

[Igor Mammedov <imammedo@redhat.com>]

On Wed,  3 Apr 2013 08:42:25 +0200
Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:

> From: Thang Pham <thang.pham@us.ibm.com>
> 
[...]
>  
> @@ -31,25 +34,152 @@ static inline S390SCLPDevice *get_event_facility(void)
>  static void read_SCP_info(SCCB *sccb)
>  {
>      ReadInfo *read_info = (ReadInfo *) sccb;
> +    int cpu_count = standby_cpus + smp_cpus;
> +    int i = 0;
> +    int max_vcpus = standby_cpus + smp_cpus;
>      int shift = 0;
> -
> +#ifdef CONFIG_KVM
> +    max_vcpus = kvm_check_extension(kvm_state, KVM_CAP_MAX_VCPUS);
> +#endif
>      while ((ram_size >> (20 + shift)) > 65535) {
>          shift++;
>      }
>      read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
>      read_info->rnsize = 1 << shift;
> +
> +    /* CPU information */
> +    read_info->entries_cpu = cpu_to_be16(standby_cpus + smp_cpus);
see below comment, possibly entries_cpu value might be +off.

> +    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
> +    read_info->highest_cpu = cpu_to_be16(max_vcpus);
> +
> +    /*
> +     * Insert a valid 16-byte entry for each CPU in each the
> +     * list (configured & standby)
> +     */
> +    for (i = 0; i < cpu_count; i++) {
> +        read_info->entries[i].address = i;
> +        read_info->entries[i].type = 0; /* General purpose CPU */
> +    }
> +
> +    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO);
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +}
> +
> +/* Provide information about the CPU */
> +static void sclp_read_cpu_info(SCCB *sccb)
> +{
> +    ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> +    int cpu_count = standby_cpus + smp_cpus;
in qemu_system_cpu_hot_add() standby_cpus increased but  smp_cpus isn't
touched. That might create access out of array bound in below for loop,
and.

> +    int i = 0;
> +
> +    cpu_info->nr_configured = cpu_to_be16(smp_cpus);
> +    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo,
> entries));
> +    cpu_info->nr_standby = cpu_to_be16(standby_cpus);
> +
> +    /* The standby offset is 16-byte for each CPU */
> +    cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
> +        + cpu_info->nr_configured*sizeof(CpuEntry));
> +
> +    /*
> +     * Insert a valid 16-byte entry for each CPU in each the
> +     * list (configured & standby)
> +     */
> +    for (i = 0; i < cpu_count; i++) {
> +        cpu_info->entries[i].address = i;
> +        cpu_info->entries[i].type = 0; /* General purpose CPU */
> +    }
> +
>      sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
>  }
>  
> +static void sclp_configure_cpu(SCCB *sccb, uint16_t cpu_addr)
> +{
> +    /* Create underlying CPU thread */
> +    S390CPU *target_cpu = s390_cpu_addr2state(cpu_addr);
> +
> +    if (!target_cpu) {
> +        /* Create new vCPU thread and associate it with the CPU address */
> +        target_cpu = configure_cpu(cpu_addr);
> +    }
> +
> +    /*
> +     * Bring CPU from standby to configure state. Increment configure CPU
> count
> +     * and decrement standby CPU count.
> +     */
> +    smp_cpus++;
> +    if (standby_cpus) {
> +        standby_cpus--;
> +    }
what if guest calls it several times for the same CPU?
It could increase its VCPU limit given on cmd line.

> +
> +    /* CPU hotplug requires SCCB response code */
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> +static void sclp_deconfigure_cpu(SCCB *sccb, uint16_t cpu_addr)
> +{
> +    /*
> +     * Bring configured CPU to standby state.
> +     * Underlying CPU thread should be halted.
> +     */
> +    S390CPU *target_cpu = s390_cpu_addr2state(cpu_addr);
> +    CPUState *target_cpu_state = CPU(target_cpu);
> +    CPUS390XState *target_env;
> +
> +    if (target_cpu) {
> +        target_env = &target_cpu->env;
> +
> +        /* Halt CPU */
> +        if (target_cpu_state->halted != 1) {
> +            fprintf(stderr, "CPU %d must be off-lined first before it can
> be "
> +                        "de-configured\n", cpu_addr);
> +            sccb->h.response_code =
> +                    cpu_to_be16(SCLP_RC_IMPROPER_RESOURCE_STATE);
> +            return;
> +        }
> +
> +        /* Do not de-configure the last configured CPU */
> +        if (smp_cpus == 1) {
> +            fprintf(stderr, "At least one CPU must be running. CPU %d
> cannot "
> +                        "be de-configured\n", cpu_addr);
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_REQUIRED_RESOURCE);
> +            return;
> +        }
> +
> +        standby_cpus++;
> +        smp_cpus--;
> +
> +        qemu_cpu_kick(ENV_GET_CPU(target_env));
> +
> +        /* CPU hotplug done on guest requires SCCB response code*/
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +    } else {
> +        /* CPU was not created if target_cpu is NULL */
> +        fprintf(stderr, "CPU %d does not exist\n", cpu_addr);
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_RESOURCE_ID);
> +    }
> +}
> +
>  static void sclp_execute(SCCB *sccb, uint64_t code)
>  {
>      S390SCLPDevice *sdev = get_event_facility();
>  
> -    switch (code) {
> +    switch (code & SCLP_NO_CMD_PARM) {
>      case SCLP_CMDW_READ_SCP_INFO:
>      case SCLP_CMDW_READ_SCP_INFO_FORCED:
>          read_SCP_info(sccb);
>          break;
> +    case SCLP_CMDW_READ_CPU_INFO:
> +        sclp_read_cpu_info(sccb);
> +        break;
> +    case SCLP_CMDW_CONFIGURE_CPU:
> +        sclp_configure_cpu(sccb,
> +                (uint16_t) (code & SCLP_CMDW_CPU_CMD_PARM) >> 8);
> +        break;
> +    case SCLP_CMDW_DECONFIGURE_CPU:
> +        /* Obtain CPU address from code: (uint16_t) (code & 0xff00) >> 8 */
> +        sclp_deconfigure_cpu(sccb,
> +                (uint16_t) (code & SCLP_CMDW_CPU_CMD_PARM) >> 8);
> +        break;
>      default:
>          sdev->sclp_command_handler(sdev->ef, sccb, code);
>          break;
[...]

> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 23fe51f..71afe6e 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -28,12 +28,71 @@
>  #include "qemu/timer.h"
>  #include "hw/hw.h"
>  #ifndef CONFIG_USER_ONLY
> +#include "hw/s390x/sclp.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
>  
>  #define CR0_RESET       0xE0UL
>  #define CR14_RESET      0xC2000000UL;
>  
> +#ifndef CONFIG_USER_ONLY
> +
> +void qemu_system_cpu_hot_add(int cpu, int state)
> +{
> +    int max_vcpus = smp_cpus + standby_cpus;
> +    int next_cpu = smp_cpus + standby_cpus;
/me confused
from vl.c

+    if (max_cpus) {
+        standby_cpus = max_cpus - smp_cpus;
+    }

> +#ifdef CONFIG_KVM
> +    max_vcpus = kvm_check_extension(kvm_state, KVM_CAP_MAX_VCPUS);
> +#endif
MIN(max_vcpus, max_cpus) ? maybe machine should be aborted at startup if
kvm_check_extension(kvm_state, KVM_CAP_MAX_VCPUS) < max_cpus

> +
[...]
> +    /*
> +     * Find out next CPU address that could be attached.
> +     * Only standby CPUs can be added by the monitor and it must be in
> +     * sequential order
> +     */
> +    if (next_cpu >= max_vcpus) {
> +        fprintf(stderr, "The maximum number of configurable CPUs has been "
> +                    "reached\n");
> +        return;
> +    }
from above vl.c snippet
next_cpu might be == max_cpu ???

> +
> +    if (cpu != next_cpu) {
> +        fprintf(stderr, "The next standby CPU that can be hotplugged is "
> +                    "CPU %d\n", next_cpu);
> +        return;
> +    }
> +
> +    /* Configure standby CPU */
> +    configure_cpu((uint16_t) cpu);
> +
> +    /* Configure is invoked from monitor. Increment standby CPU count. */
> +    standby_cpus++;
Is smp_cpus-- missing here intentionally?

> +
> +    /* Trigger SCLP interrupt */
> +    raise_irq_cpu_hotplug();
> +}
> +#endif
> +
>  /* generate CPU information for cpu -? */
>  void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index e351005..12bea46 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -316,6 +316,12 @@ S390CPU *cpu_s390x_init(const char *cpu_model);
>  void s390x_translate_init(void);
>  int cpu_s390x_exec(CPUS390XState *s);
>  
> +#ifndef CONFIG_USER_ONLY
> +/* CPU hotplug support on s390 */
> +void qemu_system_cpu_hot_add(int cpu, int state);
> +S390CPU *s390x_cpu_hotplug(int cpu_addr);
> +#endif
> +
>  /* you can call this signal handler from your SIGBUS and SIGSEGV
>     signal handlers to inform the virtual CPU of exceptions. non zero
>     is returned if the signal was handled by the virtual CPU.  */
> @@ -373,6 +379,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU
> *cpu, int type, }
>  #endif
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
> +void s390_cpu_add_state(uint16_t cpu_addr, S390CPU *state);
>  void s390_add_running_cpu(S390CPU *cpu);
>  unsigned s390_del_running_cpu(S390CPU *cpu);
>  
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index b425054..c159c78 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -51,6 +51,8 @@
>  #endif
>  
>  #ifndef CONFIG_USER_ONLY
> +static int last_hotplug_cpu; /* Track which CPU was last hotplugged */
> +
>  void s390x_tod_timer(void *opaque)
>  {
>      S390CPU *cpu = opaque;
> @@ -68,6 +70,47 @@ void s390x_cpu_timer(void *opaque)
>      env->pending_int |= INTERRUPT_CPUTIMER;
>      cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
>  }
> +
> +S390CPU *s390x_cpu_hotplug(int cpu_addr)
> +{
> +    const char *cpu_model = "host";
> +    S390CPU *new_cpu;
> +    CPUS390XState *new_env;
> +    CPUState *new_cs;
> +    S390CPU *conf_cpu;
> +    CPUS390XState *conf_env;
> +
> +    /* Initialize new CPU */
> +    new_cpu = S390_CPU(object_new(TYPE_S390_CPU));
> +    new_cs = CPU(new_cpu);
> +    new_env = &new_cpu->env;
> +    new_env->cpu_model_str = cpu_model;
> +    new_env->cpu_num = cpu_addr;
> +    new_cs->cpu_index = cpu_addr;
usage of cpu_num and cpu_index looks like a complete mess.
just do:
 git grep cpu_num target-s390x hw/s390x

It make sense to return cpu_num from kvm_arch_vcpu_id() and drop usage of
cpu_index altogether.

moreover cpu_index is assigned in cpu_exec_init() and used there by
vmstate_*(), yes CPU marked as unmigrateable but why intentionally make
the current state worse.

> +
> +    /*
> +     * Find the last CPU hotplugged and chain it to this one
> +     */
> +    if (!last_hotplug_cpu) {
> +        /*
> +         * If no CPU was hotplugged before, set it to the last configured
> CPU
> +         * and/or standby CPU brought online
> +         */
> +        last_hotplug_cpu = smp_cpus + standby_cpus - 1;
> +    }
> +
> +    /* Use the last hotplugged CPU */
> +    conf_cpu = s390_cpu_addr2state((uint16_t) last_hotplug_cpu);
since goal is sequential hot-add then all this last_hotplug_cpu and
cpu_num/cpu_index fiddling looks unnecessary.
cpu_num provides this kind of counter and increments with every new CPU
in initfn().

> +    conf_env = &conf_cpu->env;
> +    conf_env->next_cpu = new_env;
> +    new_env->next_cpu = NULL;
cpu_exec_init() does this for you, doesn't it?
why this meddling with next_cpu needed?

> +    qemu_init_vcpu(new_env);
Something wrong here, ^^^ is part of realizefn() of CPU class and probably
shouldn't be accessed directly outside of it, ever.

> +
> +    /* Update last hotplugged CPU */
> +    last_hotplug_cpu = cpu_addr;
> +
> +    return new_cpu;
> +}
>  #endif
>  
>  S390CPU *cpu_s390x_init(const char *cpu_model)
> diff --git a/vl.c b/vl.c
> index 52eacca..fa2d45f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -211,6 +211,7 @@ int win2k_install_hack = 0;
>  int singlestep = 0;
>  int smp_cpus = 1;
>  int max_cpus = 0;
> +int standby_cpus = 0;
target specific, pls do it inside target code if possible

>  int smp_cores = 1;
>  int smp_threads = 1;
>  #ifdef CONFIG_VNC
> @@ -1423,6 +1424,11 @@ static void smp_parse(const char *optarg)
>      smp_threads = threads > 0 ? threads : 1;
>      if (max_cpus == 0)
>          max_cpus = smp_cpus;
> +
> +    /* Compute standby CPUs */
> +    if (max_cpus) {
> +        standby_cpus = max_cpus - smp_cpus;
> +    }
ditto

>  }
>  
>  /***********************************************************/