diff mbox

[RFC,03/17] pseries: Always use core objects for CPU construction

Message ID 1477825928-10803-4-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Oct. 30, 2016, 11:11 a.m. UTC 
Currently the pseries machine has two paths for constructing CPUs.  On
newer machine type versions, which support cpu hotplug, it constructs
cpu core objects, which in turn construct CPU threads.  For older machine
versions it individually constructs the CPU threads.

This division is going to make some future changes to the cpu construction
harder, so this patch unifies them.  Now cpu core objects are always
created.  This requires some updates to allow core objects to be created
without a full complement of threads (since older versions allowed a
number of cpus not a multiple of the threads-per-core).  Likewise it needs
some changes to the cpu core hot/cold plug path so as not to choke on the
old machine types without hotplug support.

For good measure, we move the cpu construction to its own subfunction,
spapr_init_cpus().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c          | 125 +++++++++++++++++++++++++++---------------------
 hw/ppc/spapr_cpu_core.c |  30 +++++++-----
 include/hw/ppc/spapr.h  |   1 -
 3 files changed, 89 insertions(+), 67 deletions(-)

Comments

Alexey Kardashevskiy Nov. 3, 2016, 8:11 a.m. UTC | #1 
On 30/10/16 22:11, David Gibson wrote:
> Currently the pseries machine has two paths for constructing CPUs.  On
> newer machine type versions, which support cpu hotplug, it constructs
> cpu core objects, which in turn construct CPU threads.  For older machine
> versions it individually constructs the CPU threads.
> 
> This division is going to make some future changes to the cpu construction
> harder, so this patch unifies them.  Now cpu core objects are always
> created.  This requires some updates to allow core objects to be created
> without a full complement of threads (since older versions allowed a
> number of cpus not a multiple of the threads-per-core).  Likewise it needs
> some changes to the cpu core hot/cold plug path so as not to choke on the
> old machine types without hotplug support.
> 
> For good measure, we move the cpu construction to its own subfunction,
> spapr_init_cpus().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c          | 125 +++++++++++++++++++++++++++---------------------
>  hw/ppc/spapr_cpu_core.c |  30 +++++++-----
>  include/hw/ppc/spapr.h  |   1 -
>  3 files changed, 89 insertions(+), 67 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c8e2921..ad68a9d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1688,11 +1688,80 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
>      }
>  }
>  
> +static void spapr_init_cpus(sPAPRMachineState *spapr)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    char *type = spapr_get_cpu_core_type(machine->cpu_model);
> +    int smt = kvmppc_smt_threads();
> +    int spapr_max_cores, spapr_cores;
> +    int i;
> +
> +    if (!type) {
> +        error_report("Unable to find sPAPR CPU Core definition");
> +        exit(1);
> +    }
> +
> +    if (mc->query_hotpluggable_cpus) {
> +        if (smp_cpus % smp_threads) {
> +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> +                         smp_cpus, smp_threads);
> +            exit(1);
> +        }
> +        if (max_cpus % smp_threads) {
> +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> +                         max_cpus, smp_threads);
> +            exit(1);
> +        }
> +
> +        spapr_max_cores = max_cpus / smp_threads;
> +        spapr_cores = smp_cpus / smp_threads;
> +    } else {
> +        if (max_cpus != smp_cpus) {
> +            error_report("This machine version does not support CPU hotplug");
> +            exit(1);
> +        }
> +
> +        spapr_max_cores = QEMU_ALIGN_UP(smp_cpus, smp_threads) / smp_threads;
> +        spapr_cores = spapr_max_cores;
> +    }
> +
> +    spapr->cores = g_new0(Object *, spapr_max_cores);
> +    for (i = 0; i < spapr_max_cores; i++) {
> +        int core_id = i * smp_threads;
> +
> +        if (mc->query_hotpluggable_cpus) {
> +            sPAPRDRConnector *drc =
> +                spapr_dr_connector_new(OBJECT(spapr),
> +                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
> +                                       (core_id / smp_threads) * smt);
> +
> +            qemu_register_reset(spapr_drc_reset, drc);
> +        }
> +
> +        if (i < spapr_cores) {
> +            Object *core  = object_new(type);
> +            int nr_threads = smp_threads;
> +
> +            /* Handle the partially filled core for older machine types */
> +            if ((i + 1) * smp_threads >= smp_cpus) {
> +                nr_threads = smp_cpus - i * smp_threads;
> +            }


What is this exactly for? Older machines report "qemu-system-ppc64: threads
must be 8" when I do "-smp 12,threads=8 -machine pseries-2.2".



> +
> +            object_property_set_int(core, nr_threads, "nr-threads",
> +                                    &error_fatal);
> +            object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
> +                                    &error_fatal);
> +            object_property_set_bool(core, true, "realized", &error_fatal);
> +        }
> +    }
> +    g_free(type);
> +}
> +
>  /* pSeries LPAR / sPAPR hardware init */
>  static void ppc_spapr_init(MachineState *machine)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> -    MachineClass *mc = MACHINE_GET_CLASS(machine);
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>      const char *kernel_filename = machine->kernel_filename;
>      const char *initrd_filename = machine->initrd_filename;
> @@ -1707,21 +1776,6 @@ static void ppc_spapr_init(MachineState *machine)
>      long load_limit, fw_size;
>      char *filename;
>      int smt = kvmppc_smt_threads();
> -    int spapr_cores = smp_cpus / smp_threads;
> -    int spapr_max_cores = max_cpus / smp_threads;
> -
> -    if (mc->query_hotpluggable_cpus) {
> -        if (smp_cpus % smp_threads) {
> -            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> -                         smp_cpus, smp_threads);
> -            exit(1);
> -        }
> -        if (max_cpus % smp_threads) {
> -            error_report("max_cpus (%u) must be multiple of threads (%u)",
> -                         max_cpus, smp_threads);
> -            exit(1);
> -        }
> -    }
>  
>      msi_nonbroken = true;
>  
> @@ -1801,44 +1855,7 @@ static void ppc_spapr_init(MachineState *machine)
>  
>      ppc_cpu_parse_features(machine->cpu_model);
>  
> -    if (mc->query_hotpluggable_cpus) {
> -        char *type = spapr_get_cpu_core_type(machine->cpu_model);
> -
> -        if (type == NULL) {
> -            error_report("Unable to find sPAPR CPU Core definition");
> -            exit(1);
> -        }
> -
> -        spapr->cores = g_new0(Object *, spapr_max_cores);
> -        for (i = 0; i < spapr_max_cores; i++) {
> -            int core_id = i * smp_threads;
> -            sPAPRDRConnector *drc =
> -                spapr_dr_connector_new(OBJECT(spapr),
> -                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
> -                                       (core_id / smp_threads) * smt);
> -
> -            qemu_register_reset(spapr_drc_reset, drc);
> -
> -            if (i < spapr_cores) {
> -                Object *core  = object_new(type);
> -                object_property_set_int(core, smp_threads, "nr-threads",
> -                                        &error_fatal);
> -                object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
> -                                        &error_fatal);
> -                object_property_set_bool(core, true, "realized", &error_fatal);
> -            }
> -        }
> -        g_free(type);
> -    } else {
> -        for (i = 0; i < smp_cpus; i++) {
> -            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> -            if (cpu == NULL) {
> -                error_report("Unable to find PowerPC CPU definition");
> -                exit(1);
> -            }
> -            spapr_cpu_init(spapr, cpu, &error_fatal);
> -       }
> -    }
> +    spapr_init_cpus(spapr);
>  
>      if (kvm_enabled()) {
>          /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index e0c14f6..1357293 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -46,7 +46,8 @@ static void spapr_cpu_destroy(PowerPCCPU *cpu)
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>  }
>  
> -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> +                           Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> @@ -166,6 +167,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> +    MachineClass *mc = MACHINE_GET_CLASS(spapr);
>      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(dev);
>      CPUState *cs = CPU(core->threads);
> @@ -180,7 +182,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
>      spapr->cores[index] = OBJECT(dev);
>  
> -    g_assert(drc);
> +    g_assert(drc || !mc->query_hotpluggable_cpus);
>  
>      /*
>       * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> @@ -190,13 +192,15 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
>      }
>  
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> -    if (local_err) {
> -        g_free(fdt);
> -        spapr->cores[index] = NULL;
> -        error_propagate(errp, local_err);
> -        return;
> +    if (drc) {
> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> +        if (local_err) {
> +            g_free(fdt);
> +            spapr->cores[index] = NULL;
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
>  
>      if (dev->hotplugged) {
> @@ -209,8 +213,10 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          /*
>           * Set the right DRC states for cold plugged CPU.
>           */
> -        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> -        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +        if (drc) {
> +            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +        }
>      }
>  }
>  
> @@ -227,7 +233,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
>      const char *type = object_get_typename(OBJECT(dev));
>  
> -    if (!mc->query_hotpluggable_cpus) {
> +    if (dev->hotplugged && !mc->query_hotpluggable_cpus) {
>          error_setg(&local_err, "CPU hotplug not supported for this machine");
>          goto out;
>      }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bd5bcf7..f8d444d 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -614,7 +614,6 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                              uint32_t count, uint32_t index);
>  void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                                 uint32_t count, uint32_t index);
> -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
>  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
>                                      sPAPRMachineState *spapr);
>  
>
Greg Kurz Nov. 4, 2016, 9:51 a.m. UTC | #2 
On Thu, 3 Nov 2016 19:11:48 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 30/10/16 22:11, David Gibson wrote:
> > Currently the pseries machine has two paths for constructing CPUs.  On
> > newer machine type versions, which support cpu hotplug, it constructs
> > cpu core objects, which in turn construct CPU threads.  For older machine
> > versions it individually constructs the CPU threads.
> > 
> > This division is going to make some future changes to the cpu construction
> > harder, so this patch unifies them.  Now cpu core objects are always
> > created.  This requires some updates to allow core objects to be created
> > without a full complement of threads (since older versions allowed a
> > number of cpus not a multiple of the threads-per-core).  Likewise it needs
> > some changes to the cpu core hot/cold plug path so as not to choke on the
> > old machine types without hotplug support.
> > 
> > For good measure, we move the cpu construction to its own subfunction,
> > spapr_init_cpus().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c          | 125 +++++++++++++++++++++++++++---------------------
> >  hw/ppc/spapr_cpu_core.c |  30 +++++++-----
> >  include/hw/ppc/spapr.h  |   1 -
> >  3 files changed, 89 insertions(+), 67 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c8e2921..ad68a9d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1688,11 +1688,80 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
> >      }
> >  }
> >  
> > +static void spapr_init_cpus(sPAPRMachineState *spapr)
> > +{
> > +    MachineState *machine = MACHINE(spapr);
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > +    int smt = kvmppc_smt_threads();
> > +    int spapr_max_cores, spapr_cores;
> > +    int i;
> > +
> > +    if (!type) {
> > +        error_report("Unable to find sPAPR CPU Core definition");
> > +        exit(1);
> > +    }
> > +
> > +    if (mc->query_hotpluggable_cpus) {
> > +        if (smp_cpus % smp_threads) {
> > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > +                         smp_cpus, smp_threads);
> > +            exit(1);
> > +        }
> > +        if (max_cpus % smp_threads) {
> > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > +                         max_cpus, smp_threads);
> > +            exit(1);
> > +        }
> > +
> > +        spapr_max_cores = max_cpus / smp_threads;
> > +        spapr_cores = smp_cpus / smp_threads;
> > +    } else {
> > +        if (max_cpus != smp_cpus) {
> > +            error_report("This machine version does not support CPU hotplug");
> > +            exit(1);
> > +        }
> > +
> > +        spapr_max_cores = QEMU_ALIGN_UP(smp_cpus, smp_threads) / smp_threads;
> > +        spapr_cores = spapr_max_cores;
> > +    }
> > +
> > +    spapr->cores = g_new0(Object *, spapr_max_cores);
> > +    for (i = 0; i < spapr_max_cores; i++) {
> > +        int core_id = i * smp_threads;
> > +
> > +        if (mc->query_hotpluggable_cpus) {
> > +            sPAPRDRConnector *drc =
> > +                spapr_dr_connector_new(OBJECT(spapr),
> > +                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
> > +                                       (core_id / smp_threads) * smt);
> > +
> > +            qemu_register_reset(spapr_drc_reset, drc);
> > +        }
> > +
> > +        if (i < spapr_cores) {
> > +            Object *core  = object_new(type);
> > +            int nr_threads = smp_threads;
> > +
> > +            /* Handle the partially filled core for older machine types */
> > +            if ((i + 1) * smp_threads >= smp_cpus) {
> > +                nr_threads = smp_cpus - i * smp_threads;
> > +            }  
> 
> 
> What is this exactly for? Older machines report "qemu-system-ppc64: threads
> must be 8" when I do "-smp 12,threads=8 -machine pseries-2.2".
> 

IIUC, this lowers nr_threads for the last core to end up with the requested
number of vCPUs... but spapr_core_pre_plug() doesn't like partially filled
cores.

    if (cc->nr_threads != smp_threads) {
        error_setg(&local_err, "threads must be %d", smp_threads);
        goto out;
    }

BTW, this error message looks weird when ones has passed "-smp threads=8"...
It should better reads:

    "unsupported partially filled core (%d threads, should have %d)"

If this check is removed, then we hit:

qemu-system-ppc64: core id 8 out of range

because of:

    int spapr_max_cores = max_cpus / smp_threads;

    index = cc->core_id / smp_threads;
    if (index < 0 || index >= spapr_max_cores) {
        error_setg(&local_err, "core id %d out of range", cc->core_id);
        goto out;
    }

Since the cc->core_id / smp_threads pattern is only used on the plug/unplug
paths, maybe these checks in spapr_core_pre_plug() should only be done
when mc->query_hotpluggable_cpus != NULL ?

> 
> 
> > +
> > +            object_property_set_int(core, nr_threads, "nr-threads",
> > +                                    &error_fatal);
> > +            object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
> > +                                    &error_fatal);
> > +            object_property_set_bool(core, true, "realized", &error_fatal);
> > +        }
> > +    }
> > +    g_free(type);
> > +}
> > +
> >  /* pSeries LPAR / sPAPR hardware init */
> >  static void ppc_spapr_init(MachineState *machine)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > -    MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> >      const char *kernel_filename = machine->kernel_filename;
> >      const char *initrd_filename = machine->initrd_filename;
> > @@ -1707,21 +1776,6 @@ static void ppc_spapr_init(MachineState *machine)
> >      long load_limit, fw_size;
> >      char *filename;
> >      int smt = kvmppc_smt_threads();
> > -    int spapr_cores = smp_cpus / smp_threads;
> > -    int spapr_max_cores = max_cpus / smp_threads;
> > -
> > -    if (mc->query_hotpluggable_cpus) {
> > -        if (smp_cpus % smp_threads) {
> > -            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > -                         smp_cpus, smp_threads);
> > -            exit(1);
> > -        }
> > -        if (max_cpus % smp_threads) {
> > -            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > -                         max_cpus, smp_threads);
> > -            exit(1);
> > -        }
> > -    }
> >  
> >      msi_nonbroken = true;
> >  
> > @@ -1801,44 +1855,7 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >      ppc_cpu_parse_features(machine->cpu_model);
> >  
> > -    if (mc->query_hotpluggable_cpus) {
> > -        char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > -
> > -        if (type == NULL) {
> > -            error_report("Unable to find sPAPR CPU Core definition");
> > -            exit(1);
> > -        }
> > -
> > -        spapr->cores = g_new0(Object *, spapr_max_cores);
> > -        for (i = 0; i < spapr_max_cores; i++) {
> > -            int core_id = i * smp_threads;
> > -            sPAPRDRConnector *drc =
> > -                spapr_dr_connector_new(OBJECT(spapr),
> > -                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
> > -                                       (core_id / smp_threads) * smt);
> > -
> > -            qemu_register_reset(spapr_drc_reset, drc);
> > -
> > -            if (i < spapr_cores) {
> > -                Object *core  = object_new(type);
> > -                object_property_set_int(core, smp_threads, "nr-threads",
> > -                                        &error_fatal);
> > -                object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
> > -                                        &error_fatal);
> > -                object_property_set_bool(core, true, "realized", &error_fatal);
> > -            }
> > -        }
> > -        g_free(type);
> > -    } else {
> > -        for (i = 0; i < smp_cpus; i++) {
> > -            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > -            if (cpu == NULL) {
> > -                error_report("Unable to find PowerPC CPU definition");
> > -                exit(1);
> > -            }
> > -            spapr_cpu_init(spapr, cpu, &error_fatal);
> > -       }
> > -    }
> > +    spapr_init_cpus(spapr);
> >  
> >      if (kvm_enabled()) {
> >          /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index e0c14f6..1357293 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -46,7 +46,8 @@ static void spapr_cpu_destroy(PowerPCCPU *cpu)
> >      qemu_unregister_reset(spapr_cpu_reset, cpu);
> >  }
> >  
> > -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > +                           Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      CPUState *cs = CPU(cpu);
> > @@ -166,6 +167,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                       Error **errp)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > +    MachineClass *mc = MACHINE_GET_CLASS(spapr);
> >      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(dev);
> >      CPUState *cs = CPU(core->threads);
> > @@ -180,7 +182,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
> >      spapr->cores[index] = OBJECT(dev);
> >  
> > -    g_assert(drc);
> > +    g_assert(drc || !mc->query_hotpluggable_cpus);
> >  
> >      /*
> >       * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> > @@ -190,13 +192,15 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> >      }
> >  
> > -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > -    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> > -    if (local_err) {
> > -        g_free(fdt);
> > -        spapr->cores[index] = NULL;
> > -        error_propagate(errp, local_err);
> > -        return;
> > +    if (drc) {
> > +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> > +        if (local_err) {
> > +            g_free(fdt);
> > +            spapr->cores[index] = NULL;
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> >      }
> >  
> >      if (dev->hotplugged) {
> > @@ -209,8 +213,10 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          /*
> >           * Set the right DRC states for cold plugged CPU.
> >           */
> > -        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > -        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> > +        if (drc) {
> > +            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > +            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> > +        }
> >      }
> >  }
> >  
> > @@ -227,7 +233,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> >      const char *type = object_get_typename(OBJECT(dev));
> >  
> > -    if (!mc->query_hotpluggable_cpus) {
> > +    if (dev->hotplugged && !mc->query_hotpluggable_cpus) {
> >          error_setg(&local_err, "CPU hotplug not supported for this machine");
> >          goto out;
> >      }
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index bd5bcf7..f8d444d 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -614,7 +614,6 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
> >                                              uint32_t count, uint32_t index);
> >  void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
> >                                                 uint32_t count, uint32_t index);
> > -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> >  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> >                                      sPAPRMachineState *spapr);
> >  
> >   
> 
>
David Gibson Nov. 8, 2016, 5:34 a.m. UTC | #3 
On Fri, Nov 04, 2016 at 10:51:40AM +0100, Greg Kurz wrote:
> On Thu, 3 Nov 2016 19:11:48 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > On 30/10/16 22:11, David Gibson wrote:
> > > Currently the pseries machine has two paths for constructing CPUs.  On
> > > newer machine type versions, which support cpu hotplug, it constructs
> > > cpu core objects, which in turn construct CPU threads.  For older machine
> > > versions it individually constructs the CPU threads.
> > > 
> > > This division is going to make some future changes to the cpu construction
> > > harder, so this patch unifies them.  Now cpu core objects are always
> > > created.  This requires some updates to allow core objects to be created
> > > without a full complement of threads (since older versions allowed a
> > > number of cpus not a multiple of the threads-per-core).  Likewise it needs
> > > some changes to the cpu core hot/cold plug path so as not to choke on the
> > > old machine types without hotplug support.
> > > 
> > > For good measure, we move the cpu construction to its own subfunction,
> > > spapr_init_cpus().
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr.c          | 125 +++++++++++++++++++++++++++---------------------
> > >  hw/ppc/spapr_cpu_core.c |  30 +++++++-----
> > >  include/hw/ppc/spapr.h  |   1 -
> > >  3 files changed, 89 insertions(+), 67 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index c8e2921..ad68a9d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1688,11 +1688,80 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
> > >      }
> > >  }
> > >  
> > > +static void spapr_init_cpus(sPAPRMachineState *spapr)
> > > +{
> > > +    MachineState *machine = MACHINE(spapr);
> > > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > +    char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > > +    int smt = kvmppc_smt_threads();
> > > +    int spapr_max_cores, spapr_cores;
> > > +    int i;
> > > +
> > > +    if (!type) {
> > > +        error_report("Unable to find sPAPR CPU Core definition");
> > > +        exit(1);
> > > +    }
> > > +
> > > +    if (mc->query_hotpluggable_cpus) {
> > > +        if (smp_cpus % smp_threads) {
> > > +            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > +                         smp_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +        if (max_cpus % smp_threads) {
> > > +            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > +                         max_cpus, smp_threads);
> > > +            exit(1);
> > > +        }
> > > +
> > > +        spapr_max_cores = max_cpus / smp_threads;
> > > +        spapr_cores = smp_cpus / smp_threads;
> > > +    } else {
> > > +        if (max_cpus != smp_cpus) {
> > > +            error_report("This machine version does not support CPU hotplug");
> > > +            exit(1);
> > > +        }
> > > +
> > > +        spapr_max_cores = QEMU_ALIGN_UP(smp_cpus, smp_threads) / smp_threads;
> > > +        spapr_cores = spapr_max_cores;
> > > +    }
> > > +
> > > +    spapr->cores = g_new0(Object *, spapr_max_cores);
> > > +    for (i = 0; i < spapr_max_cores; i++) {
> > > +        int core_id = i * smp_threads;
> > > +
> > > +        if (mc->query_hotpluggable_cpus) {
> > > +            sPAPRDRConnector *drc =
> > > +                spapr_dr_connector_new(OBJECT(spapr),
> > > +                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
> > > +                                       (core_id / smp_threads) * smt);
> > > +
> > > +            qemu_register_reset(spapr_drc_reset, drc);
> > > +        }
> > > +
> > > +        if (i < spapr_cores) {
> > > +            Object *core  = object_new(type);
> > > +            int nr_threads = smp_threads;
> > > +
> > > +            /* Handle the partially filled core for older machine types */
> > > +            if ((i + 1) * smp_threads >= smp_cpus) {
> > > +                nr_threads = smp_cpus - i * smp_threads;
> > > +            }  
> > 
> > 
> > What is this exactly for? Older machines report "qemu-system-ppc64: threads
> > must be 8" when I do "-smp 12,threads=8 -machine pseries-2.2".
> > 
> 
> IIUC, this lowers nr_threads for the last core to end up with the requested
> number of vCPUs... but spapr_core_pre_plug() doesn't like partially filled
> cores.
> 
>     if (cc->nr_threads != smp_threads) {
>         error_setg(&local_err, "threads must be %d", smp_threads);
>         goto out;
>     }

Ah, yeah, that's a bug.  I hadn't had a chance to test on real
hardware yet, just TCG, which only supports 1 thread per core, so I
hadn't spotted this.  I'll fix it in the next spin.

> BTW, this error message looks weird when ones has passed "-smp threads=8"...
> It should better reads:
> 
>     "unsupported partially filled core (%d threads, should have %d)"
> 
> If this check is removed, then we hit:
> 
> qemu-system-ppc64: core id 8 out of range
> 
> because of:
> 
>     int spapr_max_cores = max_cpus / smp_threads;
> 
>     index = cc->core_id / smp_threads;
>     if (index < 0 || index >= spapr_max_cores) {
>         error_setg(&local_err, "core id %d out of range", cc->core_id);
>         goto out;
>     }
> 
> Since the cc->core_id / smp_threads pattern is only used on the plug/unplug
> paths, maybe these checks in spapr_core_pre_plug() should only be done
> when mc->query_hotpluggable_cpus != NULL ?
> 
> > 
> > 
> > > +
> > > +            object_property_set_int(core, nr_threads, "nr-threads",
> > > +                                    &error_fatal);
> > > +            object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
> > > +                                    &error_fatal);
> > > +            object_property_set_bool(core, true, "realized", &error_fatal);
> > > +        }
> > > +    }
> > > +    g_free(type);
> > > +}
> > > +
> > >  /* pSeries LPAR / sPAPR hardware init */
> > >  static void ppc_spapr_init(MachineState *machine)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > -    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > >      const char *kernel_filename = machine->kernel_filename;
> > >      const char *initrd_filename = machine->initrd_filename;
> > > @@ -1707,21 +1776,6 @@ static void ppc_spapr_init(MachineState *machine)
> > >      long load_limit, fw_size;
> > >      char *filename;
> > >      int smt = kvmppc_smt_threads();
> > > -    int spapr_cores = smp_cpus / smp_threads;
> > > -    int spapr_max_cores = max_cpus / smp_threads;
> > > -
> > > -    if (mc->query_hotpluggable_cpus) {
> > > -        if (smp_cpus % smp_threads) {
> > > -            error_report("smp_cpus (%u) must be multiple of threads (%u)",
> > > -                         smp_cpus, smp_threads);
> > > -            exit(1);
> > > -        }
> > > -        if (max_cpus % smp_threads) {
> > > -            error_report("max_cpus (%u) must be multiple of threads (%u)",
> > > -                         max_cpus, smp_threads);
> > > -            exit(1);
> > > -        }
> > > -    }
> > >  
> > >      msi_nonbroken = true;
> > >  
> > > @@ -1801,44 +1855,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >  
> > >      ppc_cpu_parse_features(machine->cpu_model);
> > >  
> > > -    if (mc->query_hotpluggable_cpus) {
> > > -        char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > > -
> > > -        if (type == NULL) {
> > > -            error_report("Unable to find sPAPR CPU Core definition");
> > > -            exit(1);
> > > -        }
> > > -
> > > -        spapr->cores = g_new0(Object *, spapr_max_cores);
> > > -        for (i = 0; i < spapr_max_cores; i++) {
> > > -            int core_id = i * smp_threads;
> > > -            sPAPRDRConnector *drc =
> > > -                spapr_dr_connector_new(OBJECT(spapr),
> > > -                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
> > > -                                       (core_id / smp_threads) * smt);
> > > -
> > > -            qemu_register_reset(spapr_drc_reset, drc);
> > > -
> > > -            if (i < spapr_cores) {
> > > -                Object *core  = object_new(type);
> > > -                object_property_set_int(core, smp_threads, "nr-threads",
> > > -                                        &error_fatal);
> > > -                object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
> > > -                                        &error_fatal);
> > > -                object_property_set_bool(core, true, "realized", &error_fatal);
> > > -            }
> > > -        }
> > > -        g_free(type);
> > > -    } else {
> > > -        for (i = 0; i < smp_cpus; i++) {
> > > -            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > > -            if (cpu == NULL) {
> > > -                error_report("Unable to find PowerPC CPU definition");
> > > -                exit(1);
> > > -            }
> > > -            spapr_cpu_init(spapr, cpu, &error_fatal);
> > > -       }
> > > -    }
> > > +    spapr_init_cpus(spapr);
> > >  
> > >      if (kvm_enabled()) {
> > >          /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index e0c14f6..1357293 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -46,7 +46,8 @@ static void spapr_cpu_destroy(PowerPCCPU *cpu)
> > >      qemu_unregister_reset(spapr_cpu_reset, cpu);
> > >  }
> > >  
> > > -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > > +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > +                           Error **errp)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >      CPUState *cs = CPU(cpu);
> > > @@ -166,6 +167,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >                       Error **errp)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > > +    MachineClass *mc = MACHINE_GET_CLASS(spapr);
> > >      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > >      CPUCore *cc = CPU_CORE(dev);
> > >      CPUState *cs = CPU(core->threads);
> > > @@ -180,7 +182,7 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
> > >      spapr->cores[index] = OBJECT(dev);
> > >  
> > > -    g_assert(drc);
> > > +    g_assert(drc || !mc->query_hotpluggable_cpus);
> > >  
> > >      /*
> > >       * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> > > @@ -190,13 +192,15 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > >      }
> > >  
> > > -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > -    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> > > -    if (local_err) {
> > > -        g_free(fdt);
> > > -        spapr->cores[index] = NULL;
> > > -        error_propagate(errp, local_err);
> > > -        return;
> > > +    if (drc) {
> > > +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> > > +        if (local_err) {
> > > +            g_free(fdt);
> > > +            spapr->cores[index] = NULL;
> > > +            error_propagate(errp, local_err);
> > > +            return;
> > > +        }
> > >      }
> > >  
> > >      if (dev->hotplugged) {
> > > @@ -209,8 +213,10 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          /*
> > >           * Set the right DRC states for cold plugged CPU.
> > >           */
> > > -        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > > -        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> > > +        if (drc) {
> > > +            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > > +            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> > > +        }
> > >      }
> > >  }
> > >  
> > > @@ -227,7 +233,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > >      const char *type = object_get_typename(OBJECT(dev));
> > >  
> > > -    if (!mc->query_hotpluggable_cpus) {
> > > +    if (dev->hotplugged && !mc->query_hotpluggable_cpus) {
> > >          error_setg(&local_err, "CPU hotplug not supported for this machine");
> > >          goto out;
> > >      }
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index bd5bcf7..f8d444d 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -614,7 +614,6 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
> > >                                              uint32_t count, uint32_t index);
> > >  void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
> > >                                                 uint32_t count, uint32_t index);
> > > -void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> > >  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> > >                                      sPAPRMachineState *spapr);
> > >  
> > >   
> > 
> > 
>
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c8e2921..ad68a9d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1688,11 +1688,80 @@  static void spapr_validate_node_memory(MachineState *machine, Error **errp)
     }
 }
 
+static void spapr_init_cpus(sPAPRMachineState *spapr)
+{
+    MachineState *machine = MACHINE(spapr);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    char *type = spapr_get_cpu_core_type(machine->cpu_model);
+    int smt = kvmppc_smt_threads();
+    int spapr_max_cores, spapr_cores;
+    int i;
+
+    if (!type) {
+        error_report("Unable to find sPAPR CPU Core definition");
+        exit(1);
+    }
+
+    if (mc->query_hotpluggable_cpus) {
+        if (smp_cpus % smp_threads) {
+            error_report("smp_cpus (%u) must be multiple of threads (%u)",
+                         smp_cpus, smp_threads);
+            exit(1);
+        }
+        if (max_cpus % smp_threads) {
+            error_report("max_cpus (%u) must be multiple of threads (%u)",
+                         max_cpus, smp_threads);
+            exit(1);
+        }
+
+        spapr_max_cores = max_cpus / smp_threads;
+        spapr_cores = smp_cpus / smp_threads;
+    } else {
+        if (max_cpus != smp_cpus) {
+            error_report("This machine version does not support CPU hotplug");
+            exit(1);
+        }
+
+        spapr_max_cores = QEMU_ALIGN_UP(smp_cpus, smp_threads) / smp_threads;
+        spapr_cores = spapr_max_cores;
+    }
+
+    spapr->cores = g_new0(Object *, spapr_max_cores);
+    for (i = 0; i < spapr_max_cores; i++) {
+        int core_id = i * smp_threads;
+
+        if (mc->query_hotpluggable_cpus) {
+            sPAPRDRConnector *drc =
+                spapr_dr_connector_new(OBJECT(spapr),
+                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
+                                       (core_id / smp_threads) * smt);
+
+            qemu_register_reset(spapr_drc_reset, drc);
+        }
+
+        if (i < spapr_cores) {
+            Object *core  = object_new(type);
+            int nr_threads = smp_threads;
+
+            /* Handle the partially filled core for older machine types */
+            if ((i + 1) * smp_threads >= smp_cpus) {
+                nr_threads = smp_cpus - i * smp_threads;
+            }
+
+            object_property_set_int(core, nr_threads, "nr-threads",
+                                    &error_fatal);
+            object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
+                                    &error_fatal);
+            object_property_set_bool(core, true, "realized", &error_fatal);
+        }
+    }
+    g_free(type);
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(MachineState *machine)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
@@ -1707,21 +1776,6 @@  static void ppc_spapr_init(MachineState *machine)
     long load_limit, fw_size;
     char *filename;
     int smt = kvmppc_smt_threads();
-    int spapr_cores = smp_cpus / smp_threads;
-    int spapr_max_cores = max_cpus / smp_threads;
-
-    if (mc->query_hotpluggable_cpus) {
-        if (smp_cpus % smp_threads) {
-            error_report("smp_cpus (%u) must be multiple of threads (%u)",
-                         smp_cpus, smp_threads);
-            exit(1);
-        }
-        if (max_cpus % smp_threads) {
-            error_report("max_cpus (%u) must be multiple of threads (%u)",
-                         max_cpus, smp_threads);
-            exit(1);
-        }
-    }
 
     msi_nonbroken = true;
 
@@ -1801,44 +1855,7 @@  static void ppc_spapr_init(MachineState *machine)
 
     ppc_cpu_parse_features(machine->cpu_model);
 
-    if (mc->query_hotpluggable_cpus) {
-        char *type = spapr_get_cpu_core_type(machine->cpu_model);
-
-        if (type == NULL) {
-            error_report("Unable to find sPAPR CPU Core definition");
-            exit(1);
-        }
-
-        spapr->cores = g_new0(Object *, spapr_max_cores);
-        for (i = 0; i < spapr_max_cores; i++) {
-            int core_id = i * smp_threads;
-            sPAPRDRConnector *drc =
-                spapr_dr_connector_new(OBJECT(spapr),
-                                       SPAPR_DR_CONNECTOR_TYPE_CPU,
-                                       (core_id / smp_threads) * smt);
-
-            qemu_register_reset(spapr_drc_reset, drc);
-
-            if (i < spapr_cores) {
-                Object *core  = object_new(type);
-                object_property_set_int(core, smp_threads, "nr-threads",
-                                        &error_fatal);
-                object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
-                                        &error_fatal);
-                object_property_set_bool(core, true, "realized", &error_fatal);
-            }
-        }
-        g_free(type);
-    } else {
-        for (i = 0; i < smp_cpus; i++) {
-            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
-            if (cpu == NULL) {
-                error_report("Unable to find PowerPC CPU definition");
-                exit(1);
-            }
-            spapr_cpu_init(spapr, cpu, &error_fatal);
-       }
-    }
+    spapr_init_cpus(spapr);
 
     if (kvm_enabled()) {
         /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index e0c14f6..1357293 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -46,7 +46,8 @@  static void spapr_cpu_destroy(PowerPCCPU *cpu)
     qemu_unregister_reset(spapr_cpu_reset, cpu);
 }
 
-void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
+static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
+                           Error **errp)
 {
     CPUPPCState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
@@ -166,6 +167,7 @@  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                      Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
+    MachineClass *mc = MACHINE_GET_CLASS(spapr);
     sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
     CPUState *cs = CPU(core->threads);
@@ -180,7 +182,7 @@  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
     spapr->cores[index] = OBJECT(dev);
 
-    g_assert(drc);
+    g_assert(drc || !mc->query_hotpluggable_cpus);
 
     /*
      * Setup CPU DT entries only for hotplugged CPUs. For boot time or
@@ -190,13 +192,15 @@  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
     }
 
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
-    if (local_err) {
-        g_free(fdt);
-        spapr->cores[index] = NULL;
-        error_propagate(errp, local_err);
-        return;
+    if (drc) {
+        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
+        if (local_err) {
+            g_free(fdt);
+            spapr->cores[index] = NULL;
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (dev->hotplugged) {
@@ -209,8 +213,10 @@  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         /*
          * Set the right DRC states for cold plugged CPU.
          */
-        drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
-        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
+        if (drc) {
+            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
+            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
+        }
     }
 }
 
@@ -227,7 +233,7 @@  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
     const char *type = object_get_typename(OBJECT(dev));
 
-    if (!mc->query_hotpluggable_cpus) {
+    if (dev->hotplugged && !mc->query_hotpluggable_cpus) {
         error_setg(&local_err, "CPU hotplug not supported for this machine");
         goto out;
     }
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bd5bcf7..f8d444d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -614,7 +614,6 @@  void spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
                                             uint32_t count, uint32_t index);
 void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
                                                uint32_t count, uint32_t index);
-void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
 void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
                                     sPAPRMachineState *spapr);