Re: [PATCH v2 05/12] target/microblaze: Fill in VMStateDescription for cpu

[Edgar E. Iglesias]

On Thu, Sep 03, 2020 at 12:26:43AM -0700, Richard Henderson wrote:

Hi Richard,


> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/microblaze/cpu.h       |   4 ++
>  target/microblaze/cpu.c       |   8 +--
>  target/microblaze/machine.c   | 112 ++++++++++++++++++++++++++++++++++
>  target/microblaze/meson.build |   5 +-
>  4 files changed, 121 insertions(+), 8 deletions(-)
>  create mode 100644 target/microblaze/machine.c
> 
> diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
> index 20c2979396..133ebaa4d4 100644
> --- a/target/microblaze/cpu.h
> +++ b/target/microblaze/cpu.h
> @@ -419,4 +419,8 @@ static inline int cpu_mmu_index(CPUMBState *env, bool 
> ifetch)
>      return MMU_KERNEL_IDX;
>  }
>  
> +#ifndef CONFIG_USER_ONLY
> +extern const VMStateDescription vmstate_mb_cpu;
> +#endif
> +
>  #endif
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 6392524135..388605ccca 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -26,7 +26,6 @@
>  #include "cpu.h"
>  #include "qemu/module.h"
>  #include "hw/qdev-properties.h"
> -#include "migration/vmstate.h"
>  #include "exec/exec-all.h"
>  #include "fpu/softfloat-helpers.h"
>  
> @@ -254,11 +253,6 @@ static void mb_cpu_initfn(Object *obj)
>  #endif
>  }
>  
> -static const VMStateDescription vmstate_mb_cpu = {
> -    .name = "cpu",
> -    .unmigratable = 1,
> -};
> -
>  static Property mb_properties[] = {
>      DEFINE_PROP_UINT32("base-vectors", MicroBlazeCPU, cfg.base_vectors, 0),
>      DEFINE_PROP_BOOL("use-stack-protection", MicroBlazeCPU, cfg.stackprot,
> @@ -338,8 +332,8 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>  #ifndef CONFIG_USER_ONLY
>      cc->do_transaction_failed = mb_cpu_transaction_failed;
>      cc->get_phys_page_debug = mb_cpu_get_phys_page_debug;
> -#endif
>      dc->vmsd = &vmstate_mb_cpu;
> +#endif
>      device_class_set_props(dc, mb_properties);
>      cc->gdb_num_core_regs = 32 + 27;
>  
> diff --git a/target/microblaze/machine.c b/target/microblaze/machine.c
> new file mode 100644
> index 0000000000..aad3c5d1d3
> --- /dev/null
> +++ b/target/microblaze/machine.c
> @@ -0,0 +1,112 @@
> +/*
> + *  Microblaze VMState for qemu.
> + *
> + *  Copyright (c) 2020 Linaro, Ltd.
> + *
> + * 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
> + * Lesser 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/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "migration/cpu.h"
> +
> +
> +static VMStateField vmstate_mmu_fields[] = {
> +    VMSTATE_UINT64_2DARRAY(rams, MicroBlazeMMU, 2, TLB_ENTRIES),
> +    VMSTATE_UINT8_ARRAY(tids, MicroBlazeMMU, TLB_ENTRIES),
> +    VMSTATE_UINT32_ARRAY(regs, MicroBlazeMMU, 3),
> +    VMSTATE_INT32(c_mmu, MicroBlazeMMU),
> +    VMSTATE_INT32(c_mmu_tlb_access, MicroBlazeMMU),
> +    VMSTATE_INT32(c_mmu_zones, MicroBlazeMMU),
> +    VMSTATE_UINT64(c_addr_mask, MicroBlazeMMU),

These last 4 entries are elaboration-time settings, i.e they will not
change during VM runtime. I don't think we need to transfer these since
we expect the peer to setup the same kind of microblaze?



> +    VMSTATE_END_OF_LIST()
> +};
> +
> +static const VMStateDescription vmstate_mmu = {
> +    .name = "mmu",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = vmstate_mmu_fields,
> +};
> +
> +static int get_msr(QEMUFile *f, void *opaque, size_t size,
> +                   const VMStateField *field)
> +{
> +    CPUMBState *env = container_of(opaque, CPUMBState, msr);
> +
> +    mb_cpu_write_msr(env, qemu_get_be32(f));
> +    return 0;
> +}
> +
> +static int put_msr(QEMUFile *f, void *opaque, size_t size,
> +                   const VMStateField *field, QJSON *vmdesc)
> +{
> +    CPUMBState *env = container_of(opaque, CPUMBState, msr);
> +
> +    qemu_put_be32(f, mb_cpu_read_msr(env));
> +    return 0;
> +}
> +
> +static const VMStateInfo vmstate_msr = {
> +    .name = "msr",
> +    .get = get_msr,
> +    .put = put_msr,
> +};
> +
> +static VMStateField vmstate_env_fields[] = {
> +    VMSTATE_UINT32_ARRAY(regs, CPUMBState, 32),
> +
> +    VMSTATE_UINT32(pc, CPUMBState),
> +    VMSTATE_SINGLE(msr, CPUMBState, 0, vmstate_msr, uint32_t),
> +    VMSTATE_UINT32(esr, CPUMBState),
> +    VMSTATE_UINT32(fsr, CPUMBState),
> +    VMSTATE_UINT32(btr, CPUMBState),
> +    VMSTATE_UINT32(edr, CPUMBState),
> +    VMSTATE_UINT32(slr, CPUMBState),
> +    VMSTATE_UINT32(shr, CPUMBState),
> +    VMSTATE_UINT64(ear, CPUMBState),
> +
> +    VMSTATE_UINT32(btarget, CPUMBState),
> +    VMSTATE_UINT32(imm, CPUMBState),
> +    VMSTATE_UINT32(iflags, CPUMBState),
> +
> +    VMSTATE_UINT32(res_val, CPUMBState),
> +    VMSTATE_UINTTL(res_addr, CPUMBState),
> +
> +    VMSTATE_UINT32_ARRAY(pvr.regs, CPUMBState, 13),

pvr.regs are also elaboration time setup and should be read-only during
the VM's lifetime.

If you fix those, this looks good to me.:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>