Re: [Qemu-devel] [PATCH v3 7/7] spapr: consolidate the logic of core cpu_dt_id

[Greg Kurz <groug@kaod.org>]

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

On Thu, 7 Jul 2016 12:05:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 06, 2016 at 02:14:59PM +0200, Greg Kurz wrote:
> > POWER5 and newer cpus from IBM have a specific numbering scheme for
> > DT ids. This is currently open coded in several places.
> > 
> > This patch consolidates the logic in helpers.
> > 
> > Suggested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> This seems backwards to me.  I think we should be constructing the
> vcpu ids from the core id, not the other way around.  Doing things

Just to be sure: you're talking about the spapr_dt_id_to_*_index() helpers ?

> that way will take a bit longer, but this seems like execessive work
> as an interim cleanup until it's obsoleted by core based numbering.
> 

I agree this patch isn't needed anyway, especially if the numbering is about to
evolve. I'll drop this patch in v4.

> > ---
> >  hw/ppc/spapr.c                  |   11 ++++-------
> >  hw/ppc/spapr_cpu_core.c         |   26 +++++++++++++++++++-------
> >  include/hw/ppc/spapr_cpu_core.h |    9 +++++++++
> >  3 files changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index baefc7bd279c..89e61b976c60 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -199,7 +199,6 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
> >      int ret = 0, offset, cpus_offset;
> >      CPUState *cs;
> >      char cpu_model[32];
> > -    int smt = kvmppc_smt_threads();
> >      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> >  
> >      CPU_FOREACH(cs) {
> > @@ -207,7 +206,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
> >          DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >          int index = ppc_get_vcpu_dt_id(cpu);
> >  
> > -        if ((index % smt) != 0) {
> > +        if (!spapr_core_dt_id_is_valid(index)) {
> >              continue;
> >          }
> >  
> > @@ -735,7 +734,6 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> >      CPUState *cs;
> >      int cpus_offset;
> >      char *nodename;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> >      _FDT(cpus_offset);
> > @@ -753,7 +751,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> >          DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >          int offset;
> >  
> > -        if ((index % smt) != 0) {
> > +        if (!spapr_core_dt_id_is_valid(index)) {
> >              continue;
> >          }
> >  
> > @@ -1822,7 +1820,7 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >          spapr->cores = g_new0(Object *, spapr_max_cores);
> >          for (i = 0; i < spapr_max_cores; i++) {
> > -            int core_dt_id = i * smt;
> > +            int core_dt_id = spapr_core_index_to_dt_id(i);
> >              sPAPRDRConnector *drc =
> >                  spapr_dr_connector_new(OBJECT(spapr),
> >                                         SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> > @@ -2386,7 +2384,6 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
> >      HotpluggableCPUList *head = NULL;
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >      int spapr_max_cores = max_cpus / smp_threads;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      for (i = 0; i < spapr_max_cores; i++) {
> >          HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> > @@ -2396,7 +2393,7 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
> >          cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
> >          cpu_item->vcpus_count = smp_threads;
> >          cpu_props->has_core_id = true;
> > -        cpu_props->core_id = i * smt;
> > +        cpu_props->core_id = spapr_core_index_to_dt_id(i);
> >          /* TODO: add 'has_node/node' here to describe
> >             to which node core belongs */
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index e3a3024baf32..b104778350df 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -103,7 +103,6 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
> >      size_t size = object_type_get_instance_size(typename);
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      CPUCore *cc = CPU_CORE(dev);
> > -    int smt = kvmppc_smt_threads();
> >      int i;
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> > @@ -117,7 +116,7 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
> >          object_unparent(obj);
> >      }
> >  
> > -    spapr->cores[cc->core_id / smt] = NULL;
> > +    spapr->cores[spapr_dt_id_to_core_index(cc->core_id)] = NULL;
> >  
> >      g_free(sc->threads);
> >      object_unparent(OBJECT(dev));
> > @@ -160,10 +159,9 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      void *fdt = NULL;
> >      int fdt_offset = 0;
> >      int index;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
> > -    index = cc->core_id / smt;
> > +    index = spapr_dt_id_to_core_index(cc->core_id);
> >      spapr->cores[index] = OBJECT(dev);
> >  
> >      if (!smc->dr_cpu_enabled) {
> > @@ -217,7 +215,6 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> >      int spapr_max_cores = max_cpus / smp_threads;
> >      int index;
> > -    int smt = kvmppc_smt_threads();
> >      Error *local_err = NULL;
> >      CPUCore *cc = CPU_CORE(dev);
> >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > @@ -238,12 +235,12 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          goto out;
> >      }
> >  
> > -    if (cc->core_id % smt) {
> > +    if (!spapr_core_dt_id_is_valid(cc->core_id)) {
> >          error_setg(&local_err, "invalid core id %d\n", cc->core_id);
> >          goto out;
> >      }
> >  
> > -    index = cc->core_id / smt;
> > +    index = spapr_dt_id_to_core_index(cc->core_id);
> >      if (index < 0 || index >= spapr_max_cores) {
> >          error_setg(&local_err, "core id %d out of range", cc->core_id);
> >          goto out;
> > @@ -331,6 +328,21 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> >      dc->realize = spapr_cpu_core_realize;
> >  }
> >  
> > +unsigned spapr_core_index_to_dt_id(unsigned index)
> > +{
> > +    return index * kvmppc_smt_threads();
> > +}
> > +
> > +unsigned spapr_dt_id_to_core_index(unsigned dt_id)
> > +{
> > +    return dt_id / kvmppc_smt_threads();
> > +}
> > +
> > +unsigned spapr_dt_id_to_thread_index(unsigned dt_id)
> > +{
> > +    return dt_id % kvmppc_smt_threads();
> > +}
> > +
> >  /*
> >   * instance_init routines from different flavours of sPAPR CPU cores.
> >   */
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 1c9b3195cce9..be7be91308ce 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -33,4 +33,13 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                       Error **errp);
> >  void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                         Error **errp);
> > +
> > +unsigned spapr_core_index_to_dt_id(unsigned index);
> > +unsigned spapr_dt_id_to_core_index(unsigned dt_id);
> > +unsigned spapr_dt_id_to_thread_index(unsigned dt_id);
> > +
> > +static inline bool spapr_core_dt_id_is_valid(unsigned dt_id)
> > +{
> > +    return spapr_dt_id_to_thread_index(dt_id) == 0;
> > +}
> >  #endif
> >   
> 


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