Patchwork [1/3] s390/migration: Provide a cpu save for initial life migration work

login
register
mail settings
Submitter Christian Borntraeger
Date Nov. 21, 2012, 2:46 p.m.
Message ID <1353509165-26865-2-git-send-email-borntraeger@de.ibm.com>
Download mbox | patch
Permalink /patch/200786/
State New
Headers show

Comments

Christian Borntraeger - Nov. 21, 2012, 2:46 p.m.
This provides a simple cpu load and save function. With the recent
addition of sync regs we have the crs,acrs, the prefix and the
PSW already up to date. Lets also save the fpu via pre/post hooks.

This patch also changes the license of machine.c to GPLv2 or later.
(The old code was just empty glue code, so there is no need
to go the "contributions after" way).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-s390x/cpu.h     |    1 +
 target-s390x/machine.c |  115 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 103 insertions(+), 13 deletions(-)
Alexander Graf - Nov. 21, 2012, 2:56 p.m.
On 11/21/2012 03:46 PM, Christian Borntraeger wrote:
> This provides a simple cpu load and save function. With the recent
> addition of sync regs we have the crs,acrs, the prefix and the
> PSW already up to date. Lets also save the fpu via pre/post hooks.
>
> This patch also changes the license of machine.c to GPLv2 or later.
> (The old code was just empty glue code, so there is no need
> to go the "contributions after" way).
>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> ---
>   target-s390x/cpu.h     |    1 +
>   target-s390x/machine.c |  115 ++++++++++++++++++++++++++++++++++++++++++------
>   2 files changed, 103 insertions(+), 13 deletions(-)
>
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f9a1f7..ba695dd 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -27,6 +27,7 @@
>   #define ELF_MACHINE	EM_S390
>
>   #define CPUArchState struct CPUS390XState
> +#define CPU_SAVE_VERSION 1
>
>   #include "cpu-defs.h"
>   #define TARGET_PAGE_BITS 12
> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
> index 3e79be6..02706fd 100644
> --- a/target-s390x/machine.c
> +++ b/target-s390x/machine.c
> @@ -2,29 +2,118 @@
>    * QEMU S390x machine definitions
>    *
>    * Copyright (c) 2009 Alexander Graf<agraf@suse.de>
> + * Copyright IBM Corp. 2012
>    *
> - * 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/>.
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
> + * option) any later version.  See the COPYING file in the top-level directory.
>    */
>
>   #include "hw/hw.h"
>   #include "hw/boards.h"
> +#include "cpu.h"
> +#include "kvm.h"
> +
> +#if defined CONFIG_KVM
> +static void cpu_pre_save(void *opaque)
> +{
> +    CPUS390XState *env = opaque;
> +    struct kvm_fpu fpu;
> +    int i, r;
> +
> +    if (!kvm_enabled()) {
> +        return;
> +    }
> +
> +    r = kvm_vcpu_ioctl(env, KVM_GET_FPU,&fpu);
> +    assert(r == 0);
> +    for (i = 0; i<  16; i++) {
> +        env->fregs[i].ll = fpu.fprs[i];
> +    }
> +    env->fpc = fpu.fpc;
> +}
> +
> +static int cpu_post_load(void *opaque, int version_id)
> +{
> +    CPUS390XState *env = opaque;
> +    struct kvm_fpu fpu;
> +    int i, r;
> +
> +    if (!kvm_enabled()) {
> +        return 0;
> +    }
> +
> +    for (i = 0; i<  16; i++) {
> +        fpu.fprs[i] = env->fregs[i].ll;
> +    }
> +    fpu.fpc = env->fpc;
> +
> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
> +    assert(r == 0);
> +
> +    return 0;
> +}

The kvm register sync needs to happen in the kvm register sync function :)


Alex

> +#else
> +static int cpu_post_load(void *opaque, int version_id)
> +{
> +    return 0;
> +}
> +
> +static void cpu_pre_save(void *opaque)
> +{
> +}
> +#endif
> +
> +
> +static const VMStateDescription vmstate_cpu = {
> +    .name = "cpu",
> +    .version_id = CPU_SAVE_VERSION,
> +    .pre_save = cpu_pre_save,
> +    .post_load = cpu_post_load,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT64(fregs[0].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[1].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[2].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[3].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[4].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[5].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[6].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[7].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[8].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[9].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[10].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[11].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[12].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[13].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[14].ll, CPUS390XState),
> +        VMSTATE_UINT64(fregs[15].ll, CPUS390XState),
> +        VMSTATE_UINT64_ARRAY(regs, CPUS390XState, 16),
> +        VMSTATE_UINT64(psw.mask, CPUS390XState),
> +        VMSTATE_UINT64(psw.addr, CPUS390XState),
> +        VMSTATE_UINT64(psa, CPUS390XState),
> +        VMSTATE_UINT32(fpc, CPUS390XState),
> +        VMSTATE_UINT32_ARRAY(aregs, CPUS390XState, 16),
> +        VMSTATE_UINT64_ARRAY(cregs, CPUS390XState, 16),
> +        VMSTATE_END_OF_LIST()
> +     },
> +    .subsections = (VMStateSubsection[]) {
> +        {
> +            /* empty */
> +        }
> +    }
> +
> +};
> +
> +
> +
>
>   void cpu_save(QEMUFile *f, void *opaque)
>   {
> +    vmstate_save_state(f,&vmstate_cpu, opaque);
>   }
>
>   int cpu_load(QEMUFile *f, void *opaque, int version_id)
>   {
> -    return 0;
> +    return vmstate_load_state(f,&vmstate_cpu, opaque, version_id);
>   }
Christian Borntraeger - Nov. 21, 2012, 2:59 p.m.
On 21/11/12 15:56, Alexander Graf wrote:
> On 11/21/2012 03:46 PM, Christian Borntraeger wrote:
>> This provides a simple cpu load and save function. With the recent
>> addition of sync regs we have the crs,acrs, the prefix and the
>> PSW already up to date. Lets also save the fpu via pre/post hooks.
>>
>> This patch also changes the license of machine.c to GPLv2 or later.
>> (The old code was just empty glue code, so there is no need
>> to go the "contributions after" way).
>>
>> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
>> ---
>>   target-s390x/cpu.h     |    1 +
>>   target-s390x/machine.c |  115 ++++++++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 103 insertions(+), 13 deletions(-)
>>
>> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
>> index 0f9a1f7..ba695dd 100644
>> --- a/target-s390x/cpu.h
>> +++ b/target-s390x/cpu.h
>> @@ -27,6 +27,7 @@
>>   #define ELF_MACHINE    EM_S390
>>
>>   #define CPUArchState struct CPUS390XState
>> +#define CPU_SAVE_VERSION 1
>>
>>   #include "cpu-defs.h"
>>   #define TARGET_PAGE_BITS 12
>> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
>> index 3e79be6..02706fd 100644
>> --- a/target-s390x/machine.c
>> +++ b/target-s390x/machine.c
>> @@ -2,29 +2,118 @@
>>    * QEMU S390x machine definitions
>>    *
>>    * Copyright (c) 2009 Alexander Graf<agraf@suse.de>
>> + * Copyright IBM Corp. 2012
>>    *
>> - * 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/>.
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
>> + * option) any later version.  See the COPYING file in the top-level directory.
>>    */
>>
>>   #include "hw/hw.h"
>>   #include "hw/boards.h"
>> +#include "cpu.h"
>> +#include "kvm.h"
>> +
>> +#if defined CONFIG_KVM
>> +static void cpu_pre_save(void *opaque)
>> +{
>> +    CPUS390XState *env = opaque;
>> +    struct kvm_fpu fpu;
>> +    int i, r;
>> +
>> +    if (!kvm_enabled()) {
>> +        return;
>> +    }
>> +
>> +    r = kvm_vcpu_ioctl(env, KVM_GET_FPU,&fpu);
>> +    assert(r == 0);
>> +    for (i = 0; i<  16; i++) {
>> +        env->fregs[i].ll = fpu.fprs[i];
>> +    }
>> +    env->fpc = fpu.fpc;
>> +}
>> +
>> +static int cpu_post_load(void *opaque, int version_id)
>> +{
>> +    CPUS390XState *env = opaque;
>> +    struct kvm_fpu fpu;
>> +    int i, r;
>> +
>> +    if (!kvm_enabled()) {
>> +        return 0;
>> +    }
>> +
>> +    for (i = 0; i<  16; i++) {
>> +        fpu.fprs[i] = env->fregs[i].ll;
>> +    }
>> +    fpu.fpc = env->fpc;
>> +
>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>> +    assert(r == 0);
>> +
>> +    return 0;
>> +}
> 
> The kvm register sync needs to happen in the kvm register sync function :)

That would eliminate the whole purpose of sync regs and forces us to have an
expensive ioctl on lots of exits (again). I would prefer to sync the registers 
that we never need in qemu just here.
Alexander Graf - Nov. 21, 2012, 3:02 p.m.
On 11/21/2012 03:59 PM, Christian Borntraeger wrote:
> On 21/11/12 15:56, Alexander Graf wrote:
>> On 11/21/2012 03:46 PM, Christian Borntraeger wrote:
>>> This provides a simple cpu load and save function. With the recent
>>> addition of sync regs we have the crs,acrs, the prefix and the
>>> PSW already up to date. Lets also save the fpu via pre/post hooks.
>>>
>>> This patch also changes the license of machine.c to GPLv2 or later.
>>> (The old code was just empty glue code, so there is no need
>>> to go the "contributions after" way).
>>>
>>> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
>>> ---
>>>    target-s390x/cpu.h     |    1 +
>>>    target-s390x/machine.c |  115 ++++++++++++++++++++++++++++++++++++++++++------
>>>    2 files changed, 103 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
>>> index 0f9a1f7..ba695dd 100644
>>> --- a/target-s390x/cpu.h
>>> +++ b/target-s390x/cpu.h
>>> @@ -27,6 +27,7 @@
>>>    #define ELF_MACHINE    EM_S390
>>>
>>>    #define CPUArchState struct CPUS390XState
>>> +#define CPU_SAVE_VERSION 1
>>>
>>>    #include "cpu-defs.h"
>>>    #define TARGET_PAGE_BITS 12
>>> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
>>> index 3e79be6..02706fd 100644
>>> --- a/target-s390x/machine.c
>>> +++ b/target-s390x/machine.c
>>> @@ -2,29 +2,118 @@
>>>     * QEMU S390x machine definitions
>>>     *
>>>     * Copyright (c) 2009 Alexander Graf<agraf@suse.de>
>>> + * Copyright IBM Corp. 2012
>>>     *
>>> - * 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/>.
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
>>> + * option) any later version.  See the COPYING file in the top-level directory.
>>>     */
>>>
>>>    #include "hw/hw.h"
>>>    #include "hw/boards.h"
>>> +#include "cpu.h"
>>> +#include "kvm.h"
>>> +
>>> +#if defined CONFIG_KVM
>>> +static void cpu_pre_save(void *opaque)
>>> +{
>>> +    CPUS390XState *env = opaque;
>>> +    struct kvm_fpu fpu;
>>> +    int i, r;
>>> +
>>> +    if (!kvm_enabled()) {
>>> +        return;
>>> +    }
>>> +
>>> +    r = kvm_vcpu_ioctl(env, KVM_GET_FPU,&fpu);
>>> +    assert(r == 0);
>>> +    for (i = 0; i<   16; i++) {
>>> +        env->fregs[i].ll = fpu.fprs[i];
>>> +    }
>>> +    env->fpc = fpu.fpc;
>>> +}
>>> +
>>> +static int cpu_post_load(void *opaque, int version_id)
>>> +{
>>> +    CPUS390XState *env = opaque;
>>> +    struct kvm_fpu fpu;
>>> +    int i, r;
>>> +
>>> +    if (!kvm_enabled()) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    for (i = 0; i<   16; i++) {
>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>> +    }
>>> +    fpu.fpc = env->fpc;
>>> +
>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>> +    assert(r == 0);
>>> +
>>> +    return 0;
>>> +}
>> The kvm register sync needs to happen in the kvm register sync function :)
> That would eliminate the whole purpose of sync regs and forces us to have an
> expensive ioctl on lots of exits (again). I would prefer to sync the registers
> that we never need in qemu just here.

That's why the register sync has different stages.


Alex
Christian Borntraeger - Nov. 21, 2012, 3:03 p.m.
On 21/11/12 16:02, Alexander Graf wrote:
> On 11/21/2012 03:59 PM, Christian Borntraeger wrote:
>> On 21/11/12 15:56, Alexander Graf wrote:
>>> On 11/21/2012 03:46 PM, Christian Borntraeger wrote:
>>>> This provides a simple cpu load and save function. With the recent
>>>> addition of sync regs we have the crs,acrs, the prefix and the
>>>> PSW already up to date. Lets also save the fpu via pre/post hooks.
>>>>
>>>> This patch also changes the license of machine.c to GPLv2 or later.
>>>> (The old code was just empty glue code, so there is no need
>>>> to go the "contributions after" way).
>>>>
>>>> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
>>>> ---
>>>>    target-s390x/cpu.h     |    1 +
>>>>    target-s390x/machine.c |  115 ++++++++++++++++++++++++++++++++++++++++++------
>>>>    2 files changed, 103 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
>>>> index 0f9a1f7..ba695dd 100644
>>>> --- a/target-s390x/cpu.h
>>>> +++ b/target-s390x/cpu.h
>>>> @@ -27,6 +27,7 @@
>>>>    #define ELF_MACHINE    EM_S390
>>>>
>>>>    #define CPUArchState struct CPUS390XState
>>>> +#define CPU_SAVE_VERSION 1
>>>>
>>>>    #include "cpu-defs.h"
>>>>    #define TARGET_PAGE_BITS 12
>>>> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
>>>> index 3e79be6..02706fd 100644
>>>> --- a/target-s390x/machine.c
>>>> +++ b/target-s390x/machine.c
>>>> @@ -2,29 +2,118 @@
>>>>     * QEMU S390x machine definitions
>>>>     *
>>>>     * Copyright (c) 2009 Alexander Graf<agraf@suse.de>
>>>> + * Copyright IBM Corp. 2012
>>>>     *
>>>> - * 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/>.
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
>>>> + * option) any later version.  See the COPYING file in the top-level directory.
>>>>     */
>>>>
>>>>    #include "hw/hw.h"
>>>>    #include "hw/boards.h"
>>>> +#include "cpu.h"
>>>> +#include "kvm.h"
>>>> +
>>>> +#if defined CONFIG_KVM
>>>> +static void cpu_pre_save(void *opaque)
>>>> +{
>>>> +    CPUS390XState *env = opaque;
>>>> +    struct kvm_fpu fpu;
>>>> +    int i, r;
>>>> +
>>>> +    if (!kvm_enabled()) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    r = kvm_vcpu_ioctl(env, KVM_GET_FPU,&fpu);
>>>> +    assert(r == 0);
>>>> +    for (i = 0; i<   16; i++) {
>>>> +        env->fregs[i].ll = fpu.fprs[i];
>>>> +    }
>>>> +    env->fpc = fpu.fpc;
>>>> +}
>>>> +
>>>> +static int cpu_post_load(void *opaque, int version_id)
>>>> +{
>>>> +    CPUS390XState *env = opaque;
>>>> +    struct kvm_fpu fpu;
>>>> +    int i, r;
>>>> +
>>>> +    if (!kvm_enabled()) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i<   16; i++) {
>>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>>> +    }
>>>> +    fpu.fpc = env->fpc;
>>>> +
>>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>>> +    assert(r == 0);
>>>> +
>>>> +    return 0;
>>>> +}
>>> The kvm register sync needs to happen in the kvm register sync function :)
>> That would eliminate the whole purpose of sync regs and forces us to have an
>> expensive ioctl on lots of exits (again). I would prefer to sync the registers
>> that we never need in qemu just here.
> 
> That's why the register sync has different stages.

Not the get_register. Which is called on every synchronize_state. Which happen quite often
on s390.
Alexander Graf - Nov. 21, 2012, 3:06 p.m.
On 11/21/2012 04:03 PM, Christian Borntraeger wrote:
> On 21/11/12 16:02, Alexander Graf wrote:
>> On 11/21/2012 03:59 PM, Christian Borntraeger wrote:
>>> On 21/11/12 15:56, Alexander Graf wrote:
>>>> On 11/21/2012 03:46 PM, Christian Borntraeger wrote:
>>>>> This provides a simple cpu load and save function. With the recent
>>>>> addition of sync regs we have the crs,acrs, the prefix and the
>>>>> PSW already up to date. Lets also save the fpu via pre/post hooks.
>>>>>
>>>>> This patch also changes the license of machine.c to GPLv2 or later.
>>>>> (The old code was just empty glue code, so there is no need
>>>>> to go the "contributions after" way).
>>>>>
>>>>> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
>>>>> ---
>>>>>     target-s390x/cpu.h     |    1 +
>>>>>     target-s390x/machine.c |  115 ++++++++++++++++++++++++++++++++++++++++++------
>>>>>     2 files changed, 103 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
>>>>> index 0f9a1f7..ba695dd 100644
>>>>> --- a/target-s390x/cpu.h
>>>>> +++ b/target-s390x/cpu.h
>>>>> @@ -27,6 +27,7 @@
>>>>>     #define ELF_MACHINE    EM_S390
>>>>>
>>>>>     #define CPUArchState struct CPUS390XState
>>>>> +#define CPU_SAVE_VERSION 1
>>>>>
>>>>>     #include "cpu-defs.h"
>>>>>     #define TARGET_PAGE_BITS 12
>>>>> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
>>>>> index 3e79be6..02706fd 100644
>>>>> --- a/target-s390x/machine.c
>>>>> +++ b/target-s390x/machine.c
>>>>> @@ -2,29 +2,118 @@
>>>>>      * QEMU S390x machine definitions
>>>>>      *
>>>>>      * Copyright (c) 2009 Alexander Graf<agraf@suse.de>
>>>>> + * Copyright IBM Corp. 2012
>>>>>      *
>>>>> - * 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/>.
>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
>>>>> + * option) any later version.  See the COPYING file in the top-level directory.
>>>>>      */
>>>>>
>>>>>     #include "hw/hw.h"
>>>>>     #include "hw/boards.h"
>>>>> +#include "cpu.h"
>>>>> +#include "kvm.h"
>>>>> +
>>>>> +#if defined CONFIG_KVM
>>>>> +static void cpu_pre_save(void *opaque)
>>>>> +{
>>>>> +    CPUS390XState *env = opaque;
>>>>> +    struct kvm_fpu fpu;
>>>>> +    int i, r;
>>>>> +
>>>>> +    if (!kvm_enabled()) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    r = kvm_vcpu_ioctl(env, KVM_GET_FPU,&fpu);
>>>>> +    assert(r == 0);
>>>>> +    for (i = 0; i<    16; i++) {
>>>>> +        env->fregs[i].ll = fpu.fprs[i];
>>>>> +    }
>>>>> +    env->fpc = fpu.fpc;
>>>>> +}
>>>>> +
>>>>> +static int cpu_post_load(void *opaque, int version_id)
>>>>> +{
>>>>> +    CPUS390XState *env = opaque;
>>>>> +    struct kvm_fpu fpu;
>>>>> +    int i, r;
>>>>> +
>>>>> +    if (!kvm_enabled()) {
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    for (i = 0; i<    16; i++) {
>>>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>>>> +    }
>>>>> +    fpu.fpc = env->fpc;
>>>>> +
>>>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>>>> +    assert(r == 0);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>> The kvm register sync needs to happen in the kvm register sync function :)
>>> That would eliminate the whole purpose of sync regs and forces us to have an
>>> expensive ioctl on lots of exits (again). I would prefer to sync the registers
>>> that we never need in qemu just here.
>> That's why the register sync has different stages.
> Not the get_register. Which is called on every synchronize_state. Which happen quite often
> on s390.

Sounds like bad design then :).

Maybe we should explicitly tell the register synchronization which 
register sets to sync, so that we don't waste time getting _all_ the 
state every time we sync registers?

Jan, ideas?


Alex
Christian Borntraeger - Nov. 21, 2012, 3:08 p.m.
On 21/11/12 16:06, Alexander Graf wrote:
[...]
>>>>>> +static int cpu_post_load(void *opaque, int version_id)
>>>>>> +{
>>>>>> +    CPUS390XState *env = opaque;
>>>>>> +    struct kvm_fpu fpu;
>>>>>> +    int i, r;
>>>>>> +
>>>>>> +    if (!kvm_enabled()) {
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +
>>>>>> +    for (i = 0; i<    16; i++) {
>>>>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>>>>> +    }
>>>>>> +    fpu.fpc = env->fpc;
>>>>>> +
>>>>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>>>>> +    assert(r == 0);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>> The kvm register sync needs to happen in the kvm register sync function :)
>>>> That would eliminate the whole purpose of sync regs and forces us to have an
>>>> expensive ioctl on lots of exits (again). I would prefer to sync the registers
>>>> that we never need in qemu just here.
>>> That's why the register sync has different stages.
>> Not the get_register. Which is called on every synchronize_state. Which happen quite often
>> on s390.
> 
> Sounds like bad design then :).
> 
> Maybe we should explicitly tell the register synchronization which register sets to sync, so that we don't waste time getting _all_ the state every time we sync registers?

Yes, a level statement for kvm_arch_get_registers would be good.
Jan Kiszka - Nov. 21, 2012, 3:22 p.m.
On 2012-11-21 16:08, Christian Borntraeger wrote:
> On 21/11/12 16:06, Alexander Graf wrote:
> [...]
>>>>>>> +static int cpu_post_load(void *opaque, int version_id)
>>>>>>> +{
>>>>>>> +    CPUS390XState *env = opaque;
>>>>>>> +    struct kvm_fpu fpu;
>>>>>>> +    int i, r;
>>>>>>> +
>>>>>>> +    if (!kvm_enabled()) {
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    for (i = 0; i<    16; i++) {
>>>>>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>>>>>> +    }
>>>>>>> +    fpu.fpc = env->fpc;
>>>>>>> +
>>>>>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>>>>>> +    assert(r == 0);
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>> The kvm register sync needs to happen in the kvm register sync function :)
>>>>> That would eliminate the whole purpose of sync regs and forces us to have an
>>>>> expensive ioctl on lots of exits (again). I would prefer to sync the registers
>>>>> that we never need in qemu just here.
>>>> That's why the register sync has different stages.
>>> Not the get_register. Which is called on every synchronize_state. Which happen quite often
>>> on s390.
>>
>> Sounds like bad design then :).
>>
>> Maybe we should explicitly tell the register synchronization which register sets to sync, so that we don't waste time getting _all_ the state every time we sync registers?
> 
> Yes, a level statement for kvm_arch_get_registers would be good.
> 

The challenge is defining those levels generically - as it is also
generic code that calls cpu_synchronize_state. What levels do you have
in mind? And where would they be applied?

Jan
Christian Borntraeger - Nov. 21, 2012, 3:27 p.m.
On 21/11/12 16:22, Jan Kiszka wrote:
> On 2012-11-21 16:08, Christian Borntraeger wrote:
>> On 21/11/12 16:06, Alexander Graf wrote:
>> [...]
>>>>>>>> +static int cpu_post_load(void *opaque, int version_id)
>>>>>>>> +{
>>>>>>>> +    CPUS390XState *env = opaque;
>>>>>>>> +    struct kvm_fpu fpu;
>>>>>>>> +    int i, r;
>>>>>>>> +
>>>>>>>> +    if (!kvm_enabled()) {
>>>>>>>> +        return 0;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    for (i = 0; i<    16; i++) {
>>>>>>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>>>>>>> +    }
>>>>>>>> +    fpu.fpc = env->fpc;
>>>>>>>> +
>>>>>>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>>>>>>> +    assert(r == 0);
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>> The kvm register sync needs to happen in the kvm register sync function :)
>>>>>> That would eliminate the whole purpose of sync regs and forces us to have an
>>>>>> expensive ioctl on lots of exits (again). I would prefer to sync the registers
>>>>>> that we never need in qemu just here.
>>>>> That's why the register sync has different stages.
>>>> Not the get_register. Which is called on every synchronize_state. Which happen quite often
>>>> on s390.
>>>
>>> Sounds like bad design then :).
>>>
>>> Maybe we should explicitly tell the register synchronization which register sets to sync, so that we don't waste time getting _all_ the state every time we sync registers?
>>
>> Yes, a level statement for kvm_arch_get_registers would be good.
>>
> 
> The challenge is defining those levels generically - as it is also
> generic code that calls cpu_synchronize_state. What levels do you have
> in mind? And where would they be applied?

I think that RUNTIME_STATE and FULL_STATE would be sufficient for the needs
that I have. The registers that I need during runtime can be accessed quite
fast, but for life migration I also need those registers that are accessed
via ONE_REG or other ioctls.

Christian
Jan Kiszka - Nov. 21, 2012, 3:32 p.m.
On 2012-11-21 16:27, Christian Borntraeger wrote:
> On 21/11/12 16:22, Jan Kiszka wrote:
>> On 2012-11-21 16:08, Christian Borntraeger wrote:
>>> On 21/11/12 16:06, Alexander Graf wrote:
>>> [...]
>>>>>>>>> +static int cpu_post_load(void *opaque, int version_id)
>>>>>>>>> +{
>>>>>>>>> +    CPUS390XState *env = opaque;
>>>>>>>>> +    struct kvm_fpu fpu;
>>>>>>>>> +    int i, r;
>>>>>>>>> +
>>>>>>>>> +    if (!kvm_enabled()) {
>>>>>>>>> +        return 0;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    for (i = 0; i<    16; i++) {
>>>>>>>>> +        fpu.fprs[i] = env->fregs[i].ll;
>>>>>>>>> +    }
>>>>>>>>> +    fpu.fpc = env->fpc;
>>>>>>>>> +
>>>>>>>>> +    r = kvm_vcpu_ioctl(env, KVM_SET_FPU,&fpu);
>>>>>>>>> +    assert(r == 0);
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>> The kvm register sync needs to happen in the kvm register sync function :)
>>>>>>> That would eliminate the whole purpose of sync regs and forces us to have an
>>>>>>> expensive ioctl on lots of exits (again). I would prefer to sync the registers
>>>>>>> that we never need in qemu just here.
>>>>>> That's why the register sync has different stages.
>>>>> Not the get_register. Which is called on every synchronize_state. Which happen quite often
>>>>> on s390.
>>>>
>>>> Sounds like bad design then :).
>>>>
>>>> Maybe we should explicitly tell the register synchronization which register sets to sync, so that we don't waste time getting _all_ the state every time we sync registers?
>>>
>>> Yes, a level statement for kvm_arch_get_registers would be good.
>>>
>>
>> The challenge is defining those levels generically - as it is also
>> generic code that calls cpu_synchronize_state. What levels do you have
>> in mind? And where would they be applied?
> 
> I think that RUNTIME_STATE and FULL_STATE would be sufficient for the needs
> that I have. The registers that I need during runtime can be accessed quite
> fast, but for life migration I also need those registers that are accessed
> via ONE_REG or other ioctls.

OK, if all existing synchronization points remain FULL_STATE and only
s390-specific points become RUNTIME_STATE, I'm fine with it. Other archs
could then do their optimizations as the like (and actually need) to.

Jan

Patch

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 0f9a1f7..ba695dd 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -27,6 +27,7 @@ 
 #define ELF_MACHINE	EM_S390
 
 #define CPUArchState struct CPUS390XState
+#define CPU_SAVE_VERSION 1
 
 #include "cpu-defs.h"
 #define TARGET_PAGE_BITS 12
diff --git a/target-s390x/machine.c b/target-s390x/machine.c
index 3e79be6..02706fd 100644
--- a/target-s390x/machine.c
+++ b/target-s390x/machine.c
@@ -2,29 +2,118 @@ 
  * QEMU S390x machine definitions
  *
  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ * Copyright IBM Corp. 2012
  *
- * 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/>.
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version.  See the COPYING file in the top-level directory.
  */
 
 #include "hw/hw.h"
 #include "hw/boards.h"
+#include "cpu.h"
+#include "kvm.h"
+
+#if defined CONFIG_KVM
+static void cpu_pre_save(void *opaque)
+{
+    CPUS390XState *env = opaque;
+    struct kvm_fpu fpu;
+    int i, r;
+
+    if (!kvm_enabled()) {
+        return;
+    }
+
+    r = kvm_vcpu_ioctl(env, KVM_GET_FPU, &fpu);
+    assert(r == 0);
+    for (i = 0; i < 16; i++) {
+        env->fregs[i].ll = fpu.fprs[i];
+    }
+    env->fpc = fpu.fpc;
+}
+
+static int cpu_post_load(void *opaque, int version_id)
+{
+    CPUS390XState *env = opaque;
+    struct kvm_fpu fpu;
+    int i, r;
+
+    if (!kvm_enabled()) {
+        return 0;
+    }
+
+    for (i = 0; i < 16; i++) {
+        fpu.fprs[i] = env->fregs[i].ll;
+    }
+    fpu.fpc = env->fpc;
+
+    r = kvm_vcpu_ioctl(env, KVM_SET_FPU, &fpu);
+    assert(r == 0);
+
+    return 0;
+}
+#else
+static int cpu_post_load(void *opaque, int version_id)
+{
+    return 0;
+}
+
+static void cpu_pre_save(void *opaque)
+{
+}
+#endif
+
+
+static const VMStateDescription vmstate_cpu = {
+    .name = "cpu",
+    .version_id = CPU_SAVE_VERSION,
+    .pre_save = cpu_pre_save,
+    .post_load = cpu_post_load,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT64(fregs[0].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[1].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[2].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[3].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[4].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[5].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[6].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[7].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[8].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[9].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[10].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[11].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[12].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[13].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[14].ll, CPUS390XState),
+        VMSTATE_UINT64(fregs[15].ll, CPUS390XState),
+        VMSTATE_UINT64_ARRAY(regs, CPUS390XState, 16),
+        VMSTATE_UINT64(psw.mask, CPUS390XState),
+        VMSTATE_UINT64(psw.addr, CPUS390XState),
+        VMSTATE_UINT64(psa, CPUS390XState),
+        VMSTATE_UINT32(fpc, CPUS390XState),
+        VMSTATE_UINT32_ARRAY(aregs, CPUS390XState, 16),
+        VMSTATE_UINT64_ARRAY(cregs, CPUS390XState, 16),
+        VMSTATE_END_OF_LIST()
+     },
+    .subsections = (VMStateSubsection[]) {
+        {
+            /* empty */
+        }
+    }
+
+};
+
+
+
 
 void cpu_save(QEMUFile *f, void *opaque)
 {
+    vmstate_save_state(f, &vmstate_cpu, opaque);
 }
 
 int cpu_load(QEMUFile *f, void *opaque, int version_id)
 {
-    return 0;
+    return vmstate_load_state(f, &vmstate_cpu, opaque, version_id);
 }