| Message ID | 1353509165-26865-2-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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); > }
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.
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
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.
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
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.
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
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
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
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); }
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(-)