Re: [Qemu-ppc] [RFC PATCH v4 08/11] spapr: CPU hotplug support

[David Gibson]

On Thu, Aug 06, 2015 at 10:57:14AM +0530, Bharata B Rao wrote:
> Support CPU hotplug via device-add command. Set up device tree
> entries for the hotplugged CPU core and use the exising EPOW event
> infrastructure to send CPU hotplug notification to the guest.
> 
> Create only cores explicitly from boot path as well as hotplug path
> and let the ->plug() handler of the core create the threads of the core.
> 
> Also support cold plugged CPUs that are specified by -device option
> on cmdline.
> 
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
>  hw/ppc/spapr.c              | 166 
> +++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_events.c       |   3 +
>  hw/ppc/spapr_rtas.c         |  11 +++
>  target-ppc/translate_init.c |   8 +++
>  4 files changed, 186 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a106980..74637b3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -599,6 +599,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>      unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0;
>      uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1;
>      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    int drc_index;
> +
> +    if (smc->dr_cpu_enabled) {
> +        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> +        g_assert(drc);
> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +        drc_index = drck->get_index(drc);
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> +    }
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
>      _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
> @@ -1686,6 +1698,7 @@ static void ppc_spapr_init(MachineState *machine)
>      char *filename;
>      int smt = kvmppc_smt_threads();
>      int smp_max_cores = DIV_ROUND_UP(max_cpus, smp_threads);
> +    int smp_cores = DIV_ROUND_UP(smp_cpus, smp_threads);

This shadows the global variable 'smp_cores' which has a different
meaning, so this is a very bad idea.

>  
>      msi_supported = true;
>  
> @@ -1764,7 +1777,7 @@ static void ppc_spapr_init(MachineState *machine)
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
>      }
> -    for (i = 0; i < smp_cpus; i++) {
> +    for (i = 0; i < smp_cores; i++) {
>          cpu = cpu_ppc_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> @@ -2152,10 +2165,135 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +static void *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> +                                           int *fdt_offset,
> +                                           sPAPRMachineState *spapr)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> +    int id = ppc_get_vcpu_dt_id(cpu);
> +    void *fdt;
> +    int offset, fdt_size;
> +    char *nodename;
> +
> +    fdt = create_device_tree(&fdt_size);
> +    nodename = g_strdup_printf("address@hidden", dc->fw_name, id);
> +    offset = fdt_add_subnode(fdt, 0, nodename);
> +
> +    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +    g_free(nodename);
> +
> +    *fdt_offset = offset;
> +    return fdt;
> +}
> +
> +static void spapr_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                            Error **errp)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> +    CPUState *cs = CPU(dev);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    int id = ppc_get_vcpu_dt_id(cpu);
> +    sPAPRDRConnector *drc =
> +        spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> +    sPAPRDRConnectorClass *drck;
> +    int smt = kvmppc_smt_threads();
> +    Error *local_err = NULL;
> +    void *fdt = NULL;
> +    int i, fdt_offset = 0;
> +
> +    /* Set NUMA node for the added CPUs  */
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) {
> +            cs->numa_node = i;
> +            break;
> +        }
> +    }
> +
> +    /*
> +     * Currently CPU core and threads of a core aren't really different
> +     * from QEMU point of view since all of them are just CPU devices. Hence
> +     * there is no separate realize routines for cores and threads.
> +     * We use the id check below to do things differently for cores and 
> threads.
> +     *
> +     * SMT threads return from here, only main thread (core) will
> +     * continue, create threads and signal hotplug event to the guest.
> +     */
> +    if ((id % smt) != 0) {
> +        return;
> +    }
> +
> +    /* Create SMT threads of the core. */
> +    for (i = 1; i < smp_threads; i++) {
> +        cpu = cpu_ppc_init(current_machine->cpu_model);
> +        if (!cpu) {
> +            error_report("Unable to find PowerPC CPU definition: %s",
> +                          current_machine->cpu_model);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    if (!smc->dr_cpu_enabled) {
> +        /*
> +         * This is a cold plugged CPU but the machine doesn't support
> +         * DR. So skip the hotplug path ensuring that the CPU is brought
> +         * up online with out an associated DR connector.
> +         */
> +        return;
> +    }
> +
> +    g_assert(drc);
> +
> +    /*
> +     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> +     * coldplugged CPUs DT entries are setup in spapr_finalize_fdt().
> +     */
> +    if (dev->hotplugged) {
> +        fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms);
> +    }
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> +    if (local_err) {
> +        g_free(fdt);
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    /*
> +     * We send hotplug notification interrupt to the guest only in case
> +     * of hotplugged CPUs.
> +     */
> +    if (dev->hotplugged) {
> +        spapr_hotplug_req_add_event(drc);
> +    } else {
> +        /*
> +         * HACK to support removal of hotplugged CPU after VM migration:
> +         *
> +         * Since we want to be able to hot-remove those coldplugged CPUs
> +         * started at boot time using -device option at the target VM, we set
> +         * the right allocation_state and isolation_state for them, which for
> +         * the hotplugged CPUs would be set via RTAS calls done from the
> +         * guest during hotplug.
> +         *
> +         * This allows the coldplugged CPUs started using -device option to
> +         * have the right isolation and allocation states as expected by the
> +         * CPU hot removal code.
> +         *
> +         * This hack will be removed once we have DRC states migrated as part
> +         * of VM migration.
> +         */
> +        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +    }
> +}
> +
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          int node;
> @@ -2192,6 +2330,29 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>          }
>  
>          spapr_memory_plug(hotplug_dev, dev, node, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        CPUState *cs = CPU(dev);
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> +            error_setg(errp, "CPU hotplug not supported for this machine");
> +            cpu_remove_sync(cs);
> +            return;
> +        }
> +
> +        if (((smp_cpus % smp_threads) || (max_cpus % smp_threads)) &&
> +            dev->hotplugged) {
> +            error_setg(errp, "CPU hotplug not supported for the topology "
> +                       "with %d threads %d cpus and %d maxcpus since "
> +                       "CPUs can't be fit fully into cores",
> +                       smp_threads, smp_cpus, max_cpus);
> +            cpu_remove_sync(cs);

I'd kind of prefer to reject partial cores at initial startup, rather
than only when we actually attempt to hotplug.

> +            return;
> +        }
> +
> +        spapr_cpu_init(ms, cpu);
> +        spapr_cpu_reset(cpu);
> +        spapr_cpu_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2206,7 +2367,8 @@ static void spapr_machine_device_unplug(HotplugHandler 
> *hotplug_dev,
>  static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
>                                               DeviceState *dev)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>      return NULL;
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 98bf7ae..1901652 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -437,6 +437,9 @@ static void spapr_hotplug_req_event(sPAPRDRConnector 
> *drc, uint8_t hp_action)
>      case SPAPR_DR_CONNECTOR_TYPE_LMB:
>          hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
>          break;
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
> +        break;
>      default:
>          /* we shouldn't be signaling hotplug events for resources
>           * that don't support them
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index e99e25f..160eb07 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -159,6 +159,16 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU 
> *cpu_,
>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>  }
>  
> +static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> +{
> +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> +
> +    if (!pcc->interrupts_big_endian(fcpu)) {
> +        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> +    }
> +}
> +
>  static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>                             uint32_t token, uint32_t nargs,
>                             target_ulong args,
> @@ -195,6 +205,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, 
> sPAPRMachineState *spapr,
>          env->nip = start;
>          env->gpr[3] = r3;
>          cs->halted = 0;
> +        spapr_cpu_set_endianness(cpu);
>  
>          qemu_cpu_kick(cs);
>  
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 16d7b16..c19d630 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -30,6 +30,9 @@
>  #include "qemu/error-report.h"
>  #include "qapi/visitor.h"
>  #include "hw/qdev-properties.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "sysemu/sysemu.h"
> +#endif
>  
>  //#define PPC_DUMP_CPU
>  //#define PPC_DEBUG_SPR
> @@ -8936,6 +8939,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      }
>  
>  #if !defined(CONFIG_USER_ONLY)
> +    if (cs->cpu_index >= max_cpus) {
> +        error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
> +        return;
> +    }
> +
>      cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
>          + (cs->cpu_index % smp_threads);
>  #endif

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpHKU1tDaL0V.pgp
Description: PGP signature