Re: [Qemu-devel] [PATCH v2] target-cris: update CPU state save/load to use VMStateDescription

[Edgar E. Iglesias]

On Fri, Aug 07, 2015 at 05:02:14PM +0100, Peter Maydell wrote:
> From: Juan Quintela <address@hidden>
> 
> Update the CRIS CPU state save/load to use a VMStateDescription struct
> rather than cpu_save/cpu_load functions.
> 
> Have to define TLBSet struct.
> Multidimensional arrays in C are a mess, just unroll them.
> 
> Signed-off-by: Juan Quintela <address@hidden>


Acked-by: Edgar E. Iglesias <address@hidden>



> [PMM:
>  * expand commit message a little since it's no longer one patch in
>    a 35-patch series
>  * add header/copyright comment to machine.c; credited copyright is
>    Red Hat and author is Juan, since this commit gives the file all-new
>    contents; license is LGPL-2-or-later, to match other target-cris code
>  * remove hardcoded tab
>  * add fields for locked_irq, interrupt_vector, fault_vector, trap_vector
>  * drop minimum_version_id_old fields
>  * bump version_id to 2 as we are not compatible with old state format
>  * remove unnecessary hw/boards.h include
>  * update to register via dc->vmsd]
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> 
> This is a patch of Juan's from way back in 2012, which I am resurrecting
> because we now have only two CPUs which still use old-style non-VMState
> save/load (CRIS and SPARC). If we can update them both we can drop the
> machinery in the common code which supports this.
> 
> Notes:
>  * CPUCRISState indent style is somewhat mismatched (cf load_info);
>    I took the "minimal make checkpatch happy" approach, but am
>    happy to do something else with the line changed here
>  * I have added a copyright header to target-cris/machine.c, because it
>    did not have one at all, and this commit effectively gives the
>    file all-new contents. I have set it up as LGPLv2-or-later,
>    copyright Red Hat, author Juan. Please let me know if you would
>    prefer something else!
>  * I added vmstate entries for four fields which did not previously
>    get saved and restored, which is presumably a bug fix...
>  * vmsave/vmload on the axis-dev88 board does not currently seem to
>    work (among other obvious problems, there is no vmstate support
>    in the interrupt controller), so we're limited to "looks good
>    on code review" here.
> 
> Changes v1->v2:
>  * actually register the vmstate struct, by setting dc->vmsd
>    (discussion on IRC with Andreas a few weeks back indicated that
>    dc->vmsd is preferred over cc->vmsd for new CPUs)
>  * update it to be the right format for a dc->vmsd struct
> 
> Testing: I checked with a host gdb that we seem to be filling in
> the registers with the right values. Since none of the devices
> in the axis model (including the interrupt controller) support
> vmstate save/load, the run after load from snapshot goes wrong pretty
> quickly, obviously.
> 
>  target-cris/cpu-qom.h |   4 ++
>  target-cris/cpu.c     |   1 +
>  target-cris/cpu.h     |  13 ++--
>  target-cris/machine.c | 167 
> +++++++++++++++++++++++++-------------------------
>  4 files changed, 95 insertions(+), 90 deletions(-)
> 
> diff --git a/target-cris/cpu-qom.h b/target-cris/cpu-qom.h
> index 6fc30c2..df4c0b5 100644
> --- a/target-cris/cpu-qom.h
> +++ b/target-cris/cpu-qom.h
> @@ -73,6 +73,10 @@ static inline CRISCPU *cris_env_get_cpu(CPUCRISState *env)
>  
>  #define ENV_OFFSET offsetof(CRISCPU, env)
>  
> +#ifndef CONFIG_USER_ONLY
> +extern const struct VMStateDescription vmstate_cris_cpu;
> +#endif
> +
>  void cris_cpu_do_interrupt(CPUState *cpu);
>  void crisv10_cpu_do_interrupt(CPUState *cpu);
>  bool cris_cpu_exec_interrupt(CPUState *cpu, int int_req);
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index b17e849..d461e07 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -302,6 +302,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->handle_mmu_fault = cris_cpu_handle_mmu_fault;
>  #else
>      cc->get_phys_page_debug = cris_cpu_get_phys_page_debug;
> +    dc->vmsd = &vmstate_cris_cpu;
>  #endif
>  
>      cc->gdb_num_core_regs = 49;
> diff --git a/target-cris/cpu.h b/target-cris/cpu.h
> index d422e35..6760dc6 100644
> --- a/target-cris/cpu.h
> +++ b/target-cris/cpu.h
> @@ -108,6 +108,11 @@
>  
>  #define NB_MMU_MODES 2
>  
> +typedef struct {
> +    uint32_t hi;
> +    uint32_t lo;
> +} TLBSet;
> +
>  typedef struct CPUCRISState {
>       uint32_t regs[16];
>       /* P0 - P15 are referred to as special registers in the docs.  */
> @@ -161,11 +166,7 @@ typedef struct CPUCRISState {
>        *
>        * One for I and another for D.
>        */
> -     struct
> -     {
> -             uint32_t hi;
> -             uint32_t lo;
> -     } tlbsets[2][4][16];
> +        TLBSet tlbsets[2][4][16];
>  
>       CPU_COMMON
>  
> @@ -227,8 +228,6 @@ enum {
>  #define cpu_gen_code cpu_cris_gen_code
>  #define cpu_signal_handler cpu_cris_signal_handler
>  
> -#define CPU_SAVE_VERSION 1
> -
>  /* MMU modes definitions */
>  #define MMU_MODE0_SUFFIX _kernel
>  #define MMU_MODE1_SUFFIX _user
> diff --git a/target-cris/machine.c b/target-cris/machine.c
> index 8f9c0dd..983b67c 100644
> --- a/target-cris/machine.c
> +++ b/target-cris/machine.c
> @@ -1,90 +1,91 @@
> -#include "hw/hw.h"
> -#include "hw/boards.h"
> -
> -void cpu_save(QEMUFile *f, void *opaque)
> -{
> -    CPUCRISState *env = opaque;
> -    int i;
> -    int s;
> -    int mmu;
> -
> -    for (i = 0; i < 16; i++)
> -        qemu_put_be32(f, env->regs[i]);
> -    for (i = 0; i < 16; i++)
> -        qemu_put_be32(f, env->pregs[i]);
> -
> -    qemu_put_be32(f, env->pc);
> -    qemu_put_be32(f, env->ksp);
> +/*
> + *  CRIS virtual CPU state save/load support
> + *
> + *  Copyright (c) 2012 Red Hat, Inc.
> + *  Written by Juan Quintela <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + */
>  
> -    qemu_put_be32(f, env->dslot);
> -    qemu_put_be32(f, env->btaken);
> -    qemu_put_be32(f, env->btarget);
> -
> -    qemu_put_be32(f, env->cc_op);
> -    qemu_put_be32(f, env->cc_mask);
> -    qemu_put_be32(f, env->cc_dest);
> -    qemu_put_be32(f, env->cc_src);
> -    qemu_put_be32(f, env->cc_result);
> -    qemu_put_be32(f, env->cc_size);
> -    qemu_put_be32(f, env->cc_x);
> -
> -    for (s = 0; s < 4; s++) {
> -        for (i = 0; i < 16; i++)
> -            qemu_put_be32(f, env->sregs[s][i]);
> -    }
> +#include "hw/hw.h"
>  
> -    qemu_put_be32(f, env->mmu_rand_lfsr);
> -    for (mmu = 0; mmu < 2; mmu++) {
> -        for (s = 0; s < 4; s++) {
> -            for (i = 0; i < 16; i++) {
> -                qemu_put_be32(f, env->tlbsets[mmu][s][i].lo);
> -                qemu_put_be32(f, env->tlbsets[mmu][s][i].hi);
> -            }
> -        }
> +static const VMStateDescription vmstate_tlbset = {
> +    .name = "cpu/tlbset",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(lo, TLBSet),
> +        VMSTATE_UINT32(hi, TLBSet),
> +        VMSTATE_END_OF_LIST()
>      }
> -}
> -
> -int cpu_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -     CPUCRISState *env = opaque;
> -    int i;
> -    int s;
> -    int mmu;
> -
> -    for (i = 0; i < 16; i++)
> -        env->regs[i] = qemu_get_be32(f);
> -    for (i = 0; i < 16; i++)
> -        env->pregs[i] = qemu_get_be32(f);
> -
> -    env->pc = qemu_get_be32(f);
> -    env->ksp = qemu_get_be32(f);
> +};
>  
> -    env->dslot = qemu_get_be32(f);
> -    env->btaken = qemu_get_be32(f);
> -    env->btarget = qemu_get_be32(f);
> -
> -    env->cc_op = qemu_get_be32(f);
> -    env->cc_mask = qemu_get_be32(f);
> -    env->cc_dest = qemu_get_be32(f);
> -    env->cc_src = qemu_get_be32(f);
> -    env->cc_result = qemu_get_be32(f);
> -    env->cc_size = qemu_get_be32(f);
> -    env->cc_x = qemu_get_be32(f);
> -
> -    for (s = 0; s < 4; s++) {
> -        for (i = 0; i < 16; i++)
> -            env->sregs[s][i] = qemu_get_be32(f);
> +static const VMStateDescription vmstate_cris_env = {
> +    .name = "env",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, CPUCRISState, 16),
> +        VMSTATE_UINT32_ARRAY(pregs, CPUCRISState, 16),
> +        VMSTATE_UINT32(pc, CPUCRISState),
> +        VMSTATE_UINT32(ksp, CPUCRISState),
> +        VMSTATE_INT32(dslot, CPUCRISState),
> +        VMSTATE_INT32(btaken, CPUCRISState),
> +        VMSTATE_UINT32(btarget, CPUCRISState),
> +        VMSTATE_UINT32(cc_op, CPUCRISState),
> +        VMSTATE_UINT32(cc_mask, CPUCRISState),
> +        VMSTATE_UINT32(cc_dest, CPUCRISState),
> +        VMSTATE_UINT32(cc_src, CPUCRISState),
> +        VMSTATE_UINT32(cc_result, CPUCRISState),
> +        VMSTATE_INT32(cc_size, CPUCRISState),
> +        VMSTATE_INT32(cc_x, CPUCRISState),
> +        VMSTATE_INT32(locked_irq, CPUCRISState),
> +        VMSTATE_INT32(interrupt_vector, CPUCRISState),
> +        VMSTATE_INT32(fault_vector, CPUCRISState),
> +        VMSTATE_INT32(trap_vector, CPUCRISState),
> +        VMSTATE_UINT32_ARRAY(sregs[0], CPUCRISState, 16),
> +        VMSTATE_UINT32_ARRAY(sregs[1], CPUCRISState, 16),
> +        VMSTATE_UINT32_ARRAY(sregs[2], CPUCRISState, 16),
> +        VMSTATE_UINT32_ARRAY(sregs[3], CPUCRISState, 16),
> +        VMSTATE_UINT32(mmu_rand_lfsr, CPUCRISState),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[0][0], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[0][1], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[0][2], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[0][3], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[1][0], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[1][1], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[1][2], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_STRUCT_ARRAY(tlbsets[1][3], CPUCRISState, 16, 0,
> +                             vmstate_tlbset, TLBSet),
> +        VMSTATE_END_OF_LIST()
>      }
> +};
>  
> -    env->mmu_rand_lfsr = qemu_get_be32(f);
> -    for (mmu = 0; mmu < 2; mmu++) {
> -        for (s = 0; s < 4; s++) {
> -            for (i = 0; i < 16; i++) {
> -                env->tlbsets[mmu][s][i].lo = qemu_get_be32(f);
> -                env->tlbsets[mmu][s][i].hi = qemu_get_be32(f);
> -            }
> -        }
> +const VMStateDescription vmstate_cris_cpu = {
> +    .name = "cpu",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_CPU(),
> +        VMSTATE_STRUCT(env, CRISCPU, 1, vmstate_cris_env, CPUCRISState),
> +        VMSTATE_END_OF_LIST()
>      }
> -
> -    return 0;
> -}
> +};
> -- 
> 1.9.1
>