From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Maydell Subject: Re: [PATCH v4 3/5] target-arm: kvm64 sync FP register state Date: Tue, 17 Mar 2015 15:29:15 +0000 Message-ID: References: <1426503716-13931-1-git-send-email-alex.bennee@linaro.org> <1426503716-13931-4-git-send-email-alex.bennee@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: kvm-devel , Marc Zyngier , QEMU Developers , Christoffer Dall , "kvmarm@lists.cs.columbia.edu" , arm-mail-list To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Return-path: In-Reply-To: <1426503716-13931-4-git-send-email-alex.bennee@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org On 16 March 2015 at 11:01, Alex Benn=C3=A9e wrote: > For migration to work we need to sync all of the register state. This is > especially noticeable when GCC starts using FP registers as spill > registers even with integer programs. > > Signed-off-by: Alex Benn=C3=A9e > > --- > > v4: > - fixed merge conflicts > - rm superfluous reg.id++ > > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index fed03f2..8fd0c8d 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -126,9 +126,17 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regid= x) > #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \ > KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > > +/* The linux headers don't define a 128 bit wide SIMD macro for us */ I'm not clear what this comment is trying to tell me... > +#define AARCH64_SIMD_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | = \ > + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > + > +#define AARCH64_SIMD_CTRL_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \ > + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > + > int kvm_arch_put_registers(CPUState *cs, int level) > { > struct kvm_one_reg reg; > + uint32_t fpr; > uint64_t val; > int i; > int ret; > @@ -207,15 +215,37 @@ int kvm_arch_put_registers(CPUState *cs, int level) > } > } > > + /* Advanced SIMD and FP registers */ > + for (i =3D 0; i < 32; i++) { > + reg.id =3D AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > + reg.addr =3D (uintptr_t)(&env->vfp.regs[i]); env->vfp.regs[] is a 64 entry array, but here we are only dealing with the first 32 entries (and furthermore trying to write 128 bits into a 64 bit element...) > + ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); Does this work right for bigendian? (ie do the kernel and QEMU agree on which order the two halves of the 128 bit value go in? I suspect not...) > + if (ret) { > + return ret; > + } > + } > + > + reg.addr =3D (uintptr_t)(&fpr); > + fpr =3D vfp_get_fpsr(env); > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > + fpr =3D vfp_get_fpcr(env); > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > if (!write_list_to_kvmstate(cpu)) { > return EINVAL; > } > > kvm_arm_sync_mpstate_to_kvm(cpu); > > - /* TODO: > - * FP state > - */ > return ret; > } > > @@ -223,6 +253,7 @@ int kvm_arch_get_registers(CPUState *cs) > { > struct kvm_one_reg reg; > uint64_t val; > + uint32_t fpr; > int i; > int ret; > > @@ -304,6 +335,31 @@ int kvm_arch_get_registers(CPUState *cs) > } > } > > + /* Advanced SIMD and FP registers */ > + for (i =3D 0; i < 32; i++) { > + reg.id =3D AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > + reg.addr =3D (uintptr_t)(&env->vfp.regs[i]); > + ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); Same remarks apply here as for the set loop. > + if (ret) { > + return ret; > + } > + } > + > + reg.addr =3D (uintptr_t)(&fpr); > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + vfp_set_fpsr(env, fpr); > + > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + vfp_set_fpcr(env, fpr); > + > if (!write_kvmstate_to_list(cpu)) { > return EINVAL; > } > -- > 2.3.2 > -- PMM From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51688) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXtR6-0000ff-CT for qemu-devel@nongnu.org; Tue, 17 Mar 2015 11:29:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXtR2-0002vM-CC for qemu-devel@nongnu.org; Tue, 17 Mar 2015 11:29:40 -0400 Received: from mail-ie0-f171.google.com ([209.85.223.171]:36662) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXtR2-0002vH-6Q for qemu-devel@nongnu.org; Tue, 17 Mar 2015 11:29:36 -0400 Received: by iegc3 with SMTP id c3so13713894ieg.3 for ; Tue, 17 Mar 2015 08:29:35 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1426503716-13931-4-git-send-email-alex.bennee@linaro.org> References: <1426503716-13931-1-git-send-email-alex.bennee@linaro.org> <1426503716-13931-4-git-send-email-alex.bennee@linaro.org> From: Peter Maydell Date: Tue, 17 Mar 2015 15:29:15 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 3/5] target-arm: kvm64 sync FP register state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: kvm-devel , Marc Zyngier , QEMU Developers , Christoffer Dall , "kvmarm@lists.cs.columbia.edu" , arm-mail-list On 16 March 2015 at 11:01, Alex Benn=C3=A9e wrote: > For migration to work we need to sync all of the register state. This is > especially noticeable when GCC starts using FP registers as spill > registers even with integer programs. > > Signed-off-by: Alex Benn=C3=A9e > > --- > > v4: > - fixed merge conflicts > - rm superfluous reg.id++ > > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index fed03f2..8fd0c8d 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -126,9 +126,17 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regid= x) > #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \ > KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > > +/* The linux headers don't define a 128 bit wide SIMD macro for us */ I'm not clear what this comment is trying to tell me... > +#define AARCH64_SIMD_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | = \ > + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > + > +#define AARCH64_SIMD_CTRL_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \ > + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > + > int kvm_arch_put_registers(CPUState *cs, int level) > { > struct kvm_one_reg reg; > + uint32_t fpr; > uint64_t val; > int i; > int ret; > @@ -207,15 +215,37 @@ int kvm_arch_put_registers(CPUState *cs, int level) > } > } > > + /* Advanced SIMD and FP registers */ > + for (i =3D 0; i < 32; i++) { > + reg.id =3D AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > + reg.addr =3D (uintptr_t)(&env->vfp.regs[i]); env->vfp.regs[] is a 64 entry array, but here we are only dealing with the first 32 entries (and furthermore trying to write 128 bits into a 64 bit element...) > + ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); Does this work right for bigendian? (ie do the kernel and QEMU agree on which order the two halves of the 128 bit value go in? I suspect not...) > + if (ret) { > + return ret; > + } > + } > + > + reg.addr =3D (uintptr_t)(&fpr); > + fpr =3D vfp_get_fpsr(env); > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > + fpr =3D vfp_get_fpcr(env); > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > if (!write_list_to_kvmstate(cpu)) { > return EINVAL; > } > > kvm_arm_sync_mpstate_to_kvm(cpu); > > - /* TODO: > - * FP state > - */ > return ret; > } > > @@ -223,6 +253,7 @@ int kvm_arch_get_registers(CPUState *cs) > { > struct kvm_one_reg reg; > uint64_t val; > + uint32_t fpr; > int i; > int ret; > > @@ -304,6 +335,31 @@ int kvm_arch_get_registers(CPUState *cs) > } > } > > + /* Advanced SIMD and FP registers */ > + for (i =3D 0; i < 32; i++) { > + reg.id =3D AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > + reg.addr =3D (uintptr_t)(&env->vfp.regs[i]); > + ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); Same remarks apply here as for the set loop. > + if (ret) { > + return ret; > + } > + } > + > + reg.addr =3D (uintptr_t)(&fpr); > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + vfp_set_fpsr(env, fpr); > + > + reg.id =3D AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); > + ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + vfp_set_fpcr(env, fpr); > + > if (!write_kvmstate_to_list(cpu)) { > return EINVAL; > } > -- > 2.3.2 > -- PMM From mboxrd@z Thu Jan 1 00:00:00 1970 From: peter.maydell@linaro.org (Peter Maydell) Date: Tue, 17 Mar 2015 15:29:15 +0000 Subject: [PATCH v4 3/5] target-arm: kvm64 sync FP register state In-Reply-To: <1426503716-13931-4-git-send-email-alex.bennee@linaro.org> References: <1426503716-13931-1-git-send-email-alex.bennee@linaro.org> <1426503716-13931-4-git-send-email-alex.bennee@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 16 March 2015 at 11:01, Alex Benn?e wrote: > For migration to work we need to sync all of the register state. This is > especially noticeable when GCC starts using FP registers as spill > registers even with integer programs. > > Signed-off-by: Alex Benn?e > > --- > > v4: > - fixed merge conflicts > - rm superfluous reg.id++ > > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index fed03f2..8fd0c8d 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -126,9 +126,17 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx) > #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \ > KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > > +/* The linux headers don't define a 128 bit wide SIMD macro for us */ I'm not clear what this comment is trying to tell me... > +#define AARCH64_SIMD_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | \ > + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > + > +#define AARCH64_SIMD_CTRL_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \ > + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > + > int kvm_arch_put_registers(CPUState *cs, int level) > { > struct kvm_one_reg reg; > + uint32_t fpr; > uint64_t val; > int i; > int ret; > @@ -207,15 +215,37 @@ int kvm_arch_put_registers(CPUState *cs, int level) > } > } > > + /* Advanced SIMD and FP registers */ > + for (i = 0; i < 32; i++) { > + reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > + reg.addr = (uintptr_t)(&env->vfp.regs[i]); env->vfp.regs[] is a 64 entry array, but here we are only dealing with the first 32 entries (and furthermore trying to write 128 bits into a 64 bit element...) > + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); Does this work right for bigendian? (ie do the kernel and QEMU agree on which order the two halves of the 128 bit value go in? I suspect not...) > + if (ret) { > + return ret; > + } > + } > + > + reg.addr = (uintptr_t)(&fpr); > + fpr = vfp_get_fpsr(env); > + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); > + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > + fpr = vfp_get_fpcr(env); > + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); > + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + > if (!write_list_to_kvmstate(cpu)) { > return EINVAL; > } > > kvm_arm_sync_mpstate_to_kvm(cpu); > > - /* TODO: > - * FP state > - */ > return ret; > } > > @@ -223,6 +253,7 @@ int kvm_arch_get_registers(CPUState *cs) > { > struct kvm_one_reg reg; > uint64_t val; > + uint32_t fpr; > int i; > int ret; > > @@ -304,6 +335,31 @@ int kvm_arch_get_registers(CPUState *cs) > } > } > > + /* Advanced SIMD and FP registers */ > + for (i = 0; i < 32; i++) { > + reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > + reg.addr = (uintptr_t)(&env->vfp.regs[i]); > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); Same remarks apply here as for the set loop. > + if (ret) { > + return ret; > + } > + } > + > + reg.addr = (uintptr_t)(&fpr); > + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr); > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + vfp_set_fpsr(env, fpr); > + > + reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr); > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + vfp_set_fpcr(env, fpr); > + > if (!write_kvmstate_to_list(cpu)) { > return EINVAL; > } > -- > 2.3.2 > -- PMM