Re: [Qemu-devel] [PATCH 03/13] target-ppc: Convert ppc cpu savevm to VMStateDescription

["Andreas Färber" <afaerber@suse.de>]

Am 04.06.2013 14:20, schrieb Alexey Kardashevskiy:
> Author: David Gibson <david@gibson.dropbear.id.au>

From:

> 
> The savevm code for the powerpc cpu emulation is currently based around
> the old register_savevm() rather than register_vmstate() method.  It's also
> rather broken, missing some important state on some CPU models.
> 
> This patch completely rewrites the savevm for target-ppc, using the new
> VMStateDescription approach.  Exactly what needs to be saved in what
> configurations has been more carefully examined, too.  This introduces a
> new version (5) of the cpu save format.  The old load function is retained
> to support version 4 images.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  target-ppc/cpu-qom.h        |    4 +
>  target-ppc/cpu.h            |    8 +-
>  target-ppc/machine.c        |  539 ++++++++++++++++++++++++++++++++++++-------
>  target-ppc/translate_init.c |    2 +
>  4 files changed, 460 insertions(+), 93 deletions(-)
> 
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index eb03a00..2b96b04 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -102,4 +102,8 @@ PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr);
>  
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  
> +#ifndef CONFIG_USER_ONLY
> +extern const struct VMStateDescription vmstate_ppc_cpu;
> +#endif
> +
>  #endif
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index aa1d013..e47853a 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -946,7 +946,7 @@ struct CPUPPCState {
>  #if defined(TARGET_PPC64)
>      /* PowerPC 64 SLB area */
>      ppc_slb_t slb[64];
> -    int slb_nr;
> +    int32_t slb_nr;
>  #endif
>      /* segment registers */
>      hwaddr htab_base;
> @@ -955,11 +955,11 @@ struct CPUPPCState {
>      /* externally stored hash table */
>      uint8_t *external_htab;
>      /* BATs */
> -    int nb_BATs;
> +    uint32_t nb_BATs;

Here you changed to uint32_t but above and below to int32_t?

>      target_ulong DBAT[2][8];
>      target_ulong IBAT[2][8];
>      /* PowerPC TLB registers (for 4xx, e500 and 60x software driven TLBs) */
> -    int nb_tlb;      /* Total number of TLB                                  */
> +    int32_t nb_tlb;      /* Total number of TLB                              */
>      int tlb_per_way; /* Speed-up helper: used to avoid divisions at run time */
>      int nb_ways;     /* Number of ways in the TLB set                        */
>      int last_way;    /* Last used way used to allocate TLB in a LRU way      */
> @@ -1174,8 +1174,6 @@ static inline CPUPPCState *cpu_init(const char *cpu_model)
>  #define cpu_signal_handler cpu_ppc_signal_handler
>  #define cpu_list ppc_cpu_list
>  
> -#define CPU_SAVE_VERSION 4
> -
>  /* MMU modes definitions */
>  #define MMU_MODE0_SUFFIX _user
>  #define MMU_MODE1_SUFFIX _kernel
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 2d10adb..d58e652 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
[...]
> +static bool fpu_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return (env->insns_flags & PPC_FLOAT);

If you only need env once, you could just access cpu->env.foo directly,
saving a line. Applies to most such helpers below except for those that
access env two times.

> +}
[...]
> +static bool altivec_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return (env->insns_flags & PPC_ALTIVEC);
> +}
[...]
> +static bool vsx_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return (env->insns_flags2 & PPC2_VSX);
> +}
[...]
> +static bool sr_needed(void *opaque)
> +{
> +#ifdef TARGET_PPC64
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return !(env->mmu_model & POWERPC_MMU_64);
> +#else
> +    return true;
> +#endif
> +}
[...]
> +static bool slb_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    /* We don't support any of the old segment table based 64-bit CPUs */
> +    return (env->mmu_model & POWERPC_MMU_64);
> +}
[...]
> +static bool tlb6xx_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return env->nb_tlb && (env->tlb_type == TLB_6XX);
> +}
[...]
> +static bool tlbemb_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return env->nb_tlb && (env->tlb_type == TLB_EMB);
> +}
> +
> +static bool pbr403_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +    uint32_t pvr = env->spr[SPR_PVR];
> +
> +    return (pvr & 0xffff0000) == 0x00200000;
> +}
[...]
> +static bool tlbmas_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    return env->nb_tlb && (env->tlb_type == TLB_MAS);
> +}
[...]
> +const VMStateDescription vmstate_ppc_cpu = {
> +    .name = "cpu",
> +    .version_id = 5,
> +    .minimum_version_id = 5,
> +    .minimum_version_id_old = 4,
> +    .load_state_old = cpu_load_old,
> +    .pre_save = cpu_pre_save,
> +    .post_load = cpu_post_load,
> +    .fields      = (VMStateField []) {
[...]
> +};
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 021a31e..4f89b14 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8309,6 +8309,8 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  
>      cc->class_by_name = ppc_cpu_class_by_name;
>      cc->do_interrupt = ppc_cpu_do_interrupt;
> +
> +    cpu_class_set_vmsd(cc, &vmstate_ppc_cpu);
>  }
>  
>  static const TypeInfo ppc_cpu_type_info = {

The way to hook it up looks fine now. Device-based CPU VMState is still
not ready and would be incompatible from what I understand, so with or
without my minor comments addressed this looks good to go to me now!

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg