Re: [Qemu-devel] [PATCH 0/8] ppc: compute cpu_dt_id in the machine code

[Greg Kurz <groug@kaod.org>]

On Thu, 30 Jun 2016 09:34:35 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 29 Jun 2016 22:50:06 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > This series is a sequel to the discussion on a patch from Ben's powernv
> > patchset:
> > 
> >         http://patchwork.ozlabs.org/patch/597153/
> > 
> > Indeed, since the DT is a machine abstraction, it should definitely sit
> > under hw/ppc and not in the target code:
> > - all machine types are forced to share the same numbering logic
> > - user mode does not need that => there are #ifdef everywhere
> > 
> > So this series moves all the current numbering logic to the machine
> > code. It also provides the ability for each machine type to provide
> > its own numbering logic instead of using the legacy one.
> > 
> > The current code computes cpu_dt_id out of cpu_index, both getting
> > their values during cpu realization.
> >
> > The idea is to compute cpu_index at cpu initialization time, so
> > that the machine can compute cpu_dt_id before realizing the cpu.  
> Series goes to considerable lengths to reuse cpu_index,

I fully agree ! 

> maybe other way around would be better (like we did for x86) where
> the end goal was the same as here i.e. make machine code assign/manage
> apic_id (x86 equivalent of cpu_dt_id). So what we did to make it happen.
> 
>  - do not try to reuse cpu_index and leave it alone

Looking again at patches 5/8 and 6/8, I'm more than convinced, yeah :)

>  - do not use cpu_init() in machine code, leave it for creating a CPU only for usermode

Makes sense, cpu_ppc_init() should only be used by usermode. I'll start with
that I guess.

>  - make target code use dt_id_property instead of cpu_index internally

Agreed.

>  - make machine code to construct cpu manually
>      cpu = object_new(CPU)
>      set_dt_id_property(cpu)
>      set other properties if needed
>      realize(cpu)
>    see pc_new_cpu() as example

With the recent addition of CPU hotplug, the latest pseries machine type is
now constructing cpus that way. The idea in patches 7-8/8 was to provide a
helper to do the same for other machines.

>  - make machine code do mgmt/bookkeeping of DT_IDs
>      possibly you could reuse CPUArchIdList for it,
>      see pc_cpus_init() + usage of pcms->possible_cpus
> 

I'll have look.

Thanks a lot for your feedback !

Cheers.

--
Greg

> > I had to fix some issues and to do some more structural changes to have
> > this working, hence the RFC tag. The first 4 patches are bug fixes. The
> > next ones do what the subject says.
> > 
> > This patchset is based on David's ppc-for-2.7 branch. I could test it plays
> > nicely with Bharata's CPU hotplug code for sPAPR.
> > 
> > There are some more places where the cpu_dt_id logic is open coded in the
> > sPAPR code. It may be worth to consolidate that in follow-up patches. Also
> > maybe other machine types may provide their own numbering logic if the
> > current one is not appropriate.
> > 
> > Please comment.
> > 
> > ---
> > 
> > Greg Kurz (8):
> >       spapr: drop reference on child object during core realization
> >       spapr: do proper error propagation in spapr_cpu_core_realize_child()
> >       spapr: drop duplicate variable in spapr_core_release()
> >       exec: add missing conditional compilation
> >       exec: move cpu_index init and exit to their own function
> >       ppc: move cpu index setup to instance_init/finalize
> >       cpu: add initialization helper without realize
> >       hw/ppc: move DT cpu id generation to machine code
> > 
> > 
> >  exec.c                      |   72 +++++++++++++++++++++++++++++++++----------
> >  hw/ppc/e500.c               |    2 +
> >  hw/ppc/mac_newworld.c       |    2 +
> >  hw/ppc/mac_oldworld.c       |    2 +
> >  hw/ppc/ppc.c                |   60 ++++++++++++++++++++++++++++++++++++
> >  hw/ppc/ppc440_bamboo.c      |    2 +
> >  hw/ppc/ppc4xx_devs.c        |    2 +
> >  hw/ppc/prep.c               |    2 +
> >  hw/ppc/spapr.c              |    2 +
> >  hw/ppc/spapr_cpu_core.c     |   23 ++++++++++----
> >  hw/ppc/virtex_ml507.c       |    2 +
> >  include/exec/exec-all.h     |    2 +
> >  include/hw/ppc/ppc.h        |    4 ++
> >  include/qom/cpu.h           |   14 ++++++++
> >  qom/cpu.c                   |   19 ++++++++++-
> >  target-ppc/translate_init.c |   42 ++++++-------------------
> >  16 files changed, 185 insertions(+), 67 deletions(-)
> > 
> > --
> > Greg
> > 
> >   
>