Re: [Qemu-devel] [PULL 7/8] s390x/migration: migrate CPU state

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Thu, 9 Oct 2014 17:28:57 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 9 October 2014 14:36, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > From: Thomas Huth <thuth@linux.vnet.ibm.com>
> >
> > This patch provides the cpu save information for dumps and later life
> > migration and enables migration of the CPU state. The code is based on
> > earlier work from Christian Borntraeger and Jason Herne.
> >
> > Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > [provide cpu_post_load()]
> > Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> > CC: Andreas Faerber <afaerber@suse.de>
> > CC: Christian Borntraeger <borntraeger@de.ibm.com>
> > CC: Jason J. Herne <jjherne@us.ibm.com>
> > Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> >  target-s390x/cpu.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 57 insertions(+), 2 deletions(-)
> >
> > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > index ec7df90..c9c237f 100644
> > --- a/target-s390x/cpu.c
> > +++ b/target-s390x/cpu.c
> 
> I think the migration code should live in machine.c like
> it does for our other targets. (Among other useful things,
> this means you can have the makefile say
>    obj-$(CONFIG_SOFTMMU) += machine.o
> so it doesn't try to build it for the linux-user target :-))

Probably. Thomas, can you look into that (and the other comments)?

> 
> > @@ -292,9 +292,64 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
> >  }
> >  #endif
> >
> > +static int cpu_post_load(void *opaque, int version_id)
> > +{
> > +    S390CPU *cpu = opaque;
> > +
> > +    /* the cpu state is fine for QEMU - we just need to push it to kvm */
> > +    if (kvm_enabled()) {
> > +        kvm_s390_set_cpu_state(cpu, cpu->env.cpu_state);
> > +    }
> 
> Haven't looked at the detail but I'm vaguely surprised
> this has to be done manually rather than it just
> being automatically resynced when we next try to
> run the vCPU.

We also need to propagate state to vcpus that have not been yet run, as
code targeting other vcpus may need to check it.

> 
> > +
> > +    return 0;
> > +}
> > +
> >  static const VMStateDescription vmstate_s390_cpu = {
> >      .name = "cpu",
> > -    .unmigratable = 1,
> > +    .post_load = cpu_post_load,
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> 
> You don't need minimum_version_id_old any more.
> 
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_UINT64(env.fregs[0].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[1].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[2].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[3].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[4].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[5].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[6].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[7].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[8].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[9].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[10].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[11].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[12].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[13].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[14].ll, S390CPU),
> > +        VMSTATE_UINT64(env.fregs[15].ll, S390CPU),
> > +        VMSTATE_UINT64_ARRAY(env.regs, S390CPU, 16),
> > +        VMSTATE_UINT64(env.psw.mask, S390CPU),
> > +        VMSTATE_UINT64(env.psw.addr, S390CPU),
> > +        VMSTATE_UINT64(env.psa, S390CPU),
> > +        VMSTATE_UINT32(env.fpc, S390CPU),
> > +        VMSTATE_UINT32(env.todpr, S390CPU),
> > +        VMSTATE_UINT64(env.pfault_token, S390CPU),
> > +        VMSTATE_UINT64(env.pfault_compare, S390CPU),
> > +        VMSTATE_UINT64(env.pfault_select, S390CPU),
> > +        VMSTATE_UINT64(env.cputm, S390CPU),
> > +        VMSTATE_UINT64(env.ckc, S390CPU),
> > +        VMSTATE_UINT64(env.gbea, S390CPU),
> > +        VMSTATE_UINT64(env.pp, S390CPU),
> > +        VMSTATE_UINT32_ARRAY(env.aregs, S390CPU, 16),
> > +        VMSTATE_UINT64_ARRAY(env.cregs, S390CPU, 16),
> > +        VMSTATE_UINT8(env.cpu_state, S390CPU),
> > +        VMSTATE_END_OF_LIST()
> > +     },
> > +    .subsections = (VMStateSubsection[]) {
> > +        {
> > +            /* empty */
> > +        }
> 
> Why the empty subsections list?
> 
> > +    }
> >  };
> 
> thanks
> -- PMM
>