Re: [Qemu-devel] [PATCH 1/3] s390/migration: Provide a cpu save for initial life migration work

[Alexander Graf <agraf@suse.de>]

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