Re: [Qemu-ppc] [PATCH v4 0/3] Split cpu_exec_init() into an init and a realize part

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 09:22:50PM +0200, Laurent Vivier wrote:
> Since commit 42ecaba ("target-i386: Call cpu_exec_init() on realize"),
> , commit 6dd0f83 ("target-ppc: Move cpu_exec_init() call to realize 
> function"),
> and commit c6644fc ("s390x/cpu: Get rid of side effects when creating a 
> vcpu"),
> cpu_exec_init() has been moved to realize function for some architectures
> to implement CPU htoplug. This allows any failures from cpu_exec_init() to be
> handled appropriately.
> 
> This series tries to do the same work for all the other CPUs.
> 
> But as the ARM Virtual Machine ("virt") needs the "memory" property of the CPU
> in the machine init function (the "memory" property is created in
> cpu_exec_init() we want to move to the realize part), split cpu_exec_init() in
> two parts: a realize part (cpu_exec_realizefn(), adding the CPU in the
> environment) and an init part (cpu_exec_initfn(), initializing the CPU, like
> adding the "memory" property). To mirror the realize part, add an unrealize
> part, and remove the cpu_exec_exit() call from the finalize part.
> 
> This also allows to remove all the "cannot_destroy_with_object_finalize_yet"
> properties from the CPU device class.
> 
> v4:
> - arm: update mp_affinity in realize only if it s not set by the user
> - i386 and ppc: add an parent_unrealize to call the Device unrealize
>   because it is overwritten by the CPU unrealize
> - update commit messages

Series:

Reviewed-by: Eduardo Habkost <address@hidden>

But I will wait for Drew or other ARM people to review the ARM
hunks on patch 2/3 before applying.

v3 -> v4 diff, for reference:

  diff --git a/target-arm/cpu.c b/target-arm/cpu.c
  index 364a45d..f0d2074 100644
  --- a/target-arm/cpu.c
  +++ b/target-arm/cpu.c
  @@ -33,6 +33,8 @@
   #include "sysemu/kvm.h"
   #include "kvm_arm.h"
   
  +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
  +
   static void arm_cpu_set_pc(CPUState *cs, vaddr value)
   {
       ARMCPU *cpu = ARM_CPU(cs);
  @@ -633,9 +635,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
        * in later ARM ARM versions), or any of the higher affinity level 
fields,
        * so these bits always RAZ.
        */
  -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
  -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
  -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
  +    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
  +        Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
  +        Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
  +        cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
  +    }
   
       if (cpu->reset_hivecs) {
               cpu->reset_sctlr |= (1 << 13);
  @@ -1467,7 +1471,7 @@ static Property arm_cpu_properties[] = {
       DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
       DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
       DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
  -    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
  +    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 
MP_AFFINITY_INVALID),
       DEFINE_PROP_END_OF_LIST()
   };
   
  diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
  index 5dde658..07030d2 100644
  --- a/target-i386/cpu-qom.h
  +++ b/target-i386/cpu-qom.h
  @@ -64,6 +64,7 @@ typedef struct X86CPUClass {
       bool kvm_required;
   
       DeviceRealize parent_realize;
  +    DeviceUnrealize parent_unrealize;
       void (*parent_reset)(CPUState *cpu);
   } X86CPUClass;
   
  diff --git a/target-i386/cpu.c b/target-i386/cpu.c
  index 399a3e4..95ce067 100644
  --- a/target-i386/cpu.c
  +++ b/target-i386/cpu.c
  @@ -3243,6 +3243,8 @@ out:
   static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
   {
       X86CPU *cpu = X86_CPU(dev);
  +    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
  +    Error *local_err = NULL;
   
   #ifndef CONFIG_USER_ONLY
       cpu_remove_sync(CPU(dev));
  @@ -3254,7 +3256,11 @@ static void x86_cpu_unrealizefn(DeviceState *dev, 
Error **errp)
           cpu->apic_state = NULL;
       }
   
  -    cpu_exec_unrealizefn(CPU(dev));
  +    xcc->parent_unrealize(dev, &local_err);
  +    if (local_err != NULL) {
  +        error_propagate(errp, local_err);
  +        return;
  +    }
   }
   
   typedef struct BitProperty {
  @@ -3504,6 +3510,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, 
void *data)
       DeviceClass *dc = DEVICE_CLASS(oc);
   
       xcc->parent_realize = dc->realize;
  +    xcc->parent_unrealize = dc->unrealize;
       dc->realize = x86_cpu_realizefn;
       dc->unrealize = x86_cpu_unrealizefn;
       dc->props = x86_cpu_properties;
  diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
  index 713deef..d46c31a 100644
  --- a/target-ppc/cpu-qom.h
  +++ b/target-ppc/cpu-qom.h
  @@ -174,6 +174,7 @@ typedef struct PowerPCCPUClass {
       /*< public >*/
   
       DeviceRealize parent_realize;
  +    DeviceUnrealize parent_unrealize;
       void (*parent_reset)(CPUState *cpu);
   
       uint32_t pvr;
  diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
  index 2de6a06..208fa1e 100644
  --- a/target-ppc/translate_init.c
  +++ b/target-ppc/translate_init.c
  @@ -9906,11 +9906,17 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error 
**errp)
   static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
   {
       PowerPCCPU *cpu = POWERPC_CPU(dev);
  +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
       CPUPPCState *env = &cpu->env;
  +    Error *local_err = NULL;
       opc_handler_t **table, **table_2;
       int i, j, k;
   
  -    cpu_exec_unrealizefn(CPU(dev));
  +    pcc->parent_unrealize(dev, &local_err);
  +    if (local_err != NULL) {
  +        error_propagate(errp, local_err);
  +        return;
  +    }
   
       for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
           if (env->opcodes[i] == &invalid_handler) {
  @@ -10521,6 +10527,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
*data)
       DeviceClass *dc = DEVICE_CLASS(oc);
   
       pcc->parent_realize = dc->realize;
  +    pcc->parent_unrealize = dc->unrealize;
       pcc->pvr_match = ppc_pvr_match_default;
       pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
       dc->realize = ppc_cpu_realizefn;

> 
> v3:
> - reorganize the series in 3 patches:
>   1- split cpu_exec_init() and call the init part from
>      cpu_common_initfn()
>   2- move all the cpu_exec_realize() calls to the
>      XXX_cpu_realizefn() functions
>   3- create a cpu_common_unrealizefn() function
>       and call the cpu_exec_unrealize() function from here,
>       except for ppc64 and i386 that have their own XX_cpu_unrealizefn()
>       functions
> - rename cpu_exec_init(), cpu_exec_realize(), cpu_exec_unrealize()
>   to cpu_exec_initfn(), cpu_exec_realizefn() and cpu_exec_unrealizefn(),
> - move cpu_exec_unrealizefn() to the device class unrealize
>   and declare it in qom/cpu.h instead of exec/exec-all.h
> 
> v2:
> - rename cpu_exec_exit() as cpu_exec_unrealize(),
>   as it un-does what cpu_exec_realize() does,
> - remove cpu_exec_exit() from ppc_cpu_unrealizefn() as
>   it is called from cpu_common_finalize(),
> - add a patch to move all cpu_exec_init() from
>   all XX_cpu_initfn() to cpu_common_initfn(),
> - arm: move setting of cpu->mp_affinity to
>   arm_cpu_realizefn() as the cpu_index is now set in
>   cpu_exec_realizefn().
> - update some commit messages
> 
> Laurent Vivier (3):
>   exec: split cpu_exec_init()
>   exec: move cpu_exec_init() calls to realize functions
>   exec: call cpu_exec_exit() from a CPU unrealize common function
> 
>  exec.c                      | 12 +++++++-----
>  include/exec/exec-all.h     |  1 -
>  include/qom/cpu.h           |  4 +++-
>  qom/cpu.c                   | 10 +++++++++-
>  target-alpha/cpu.c          | 15 +++++++--------
>  target-arm/cpu.c            | 45 
> ++++++++++++++++++++++-----------------------
>  target-cris/cpu.c           | 15 +++++++--------
>  target-i386/cpu-qom.h       |  1 +
>  target-i386/cpu.c           | 20 ++++++++++++++------
>  target-lm32/cpu.c           | 15 +++++++--------
>  target-m68k/cpu.c           | 15 +++++++--------
>  target-microblaze/cpu.c     | 14 +++++++-------
>  target-mips/cpu.c           | 15 +++++++--------
>  target-moxie/cpu.c          | 15 +++++++--------
>  target-openrisc/cpu.c       | 15 +++++++--------
>  target-ppc/cpu-qom.h        |  1 +
>  target-ppc/translate_init.c | 11 +++++++++--
>  target-s390x/cpu.c          |  8 +-------
>  target-sh4/cpu.c            | 15 +++++++--------
>  target-sparc/cpu.c          | 18 +++++++++---------
>  target-tilegx/cpu.c         | 15 +++++++--------
>  target-tricore/cpu.c        | 15 +++++++--------
>  target-unicore32/cpu.c      | 18 +++++++++---------
>  target-xtensa/cpu.c         | 15 +++++++--------
>  24 files changed, 169 insertions(+), 159 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
Eduardo