From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v4 4/5] target-arm: kvm64 fix save/restore of SPSR regs Date: Mon, 16 Mar 2015 13:52:04 +0100 Message-ID: <20150316125204.GB26480@cbox> References: <1426503716-13931-1-git-send-email-alex.bennee@linaro.org> <1426503716-13931-5-git-send-email-alex.bennee@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, marc.zyngier@arm.com, Peter Maydell To: Alex =?iso-8859-1?Q?Benn=E9e?= Return-path: Received: from mail-lb0-f178.google.com ([209.85.217.178]:33722 "EHLO mail-lb0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752968AbbCPMvl (ORCPT ); Mon, 16 Mar 2015 08:51:41 -0400 Received: by lbbzq9 with SMTP id zq9so29905531lbb.0 for ; Mon, 16 Mar 2015 05:51:40 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1426503716-13931-5-git-send-email-alex.bennee@linaro.org> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Mar 16, 2015 at 11:01:55AM +0000, Alex Benn=E9e wrote: > From: Christoffer Dall >=20 > The current code was negatively indexing the cpu state array and not > synchronizing banked spsr register state with the current mode's spsr > state, causing occasional failures with migration. >=20 > Some munging is done to take care of the aarch64 mapping and also to > ensure the most current value of the spsr is updated to the banked > registers (relevant for KVM<->TCG migration). >=20 > Signed-off-by: Christoffer Dall > Signed-off-by: Alex Benn=E9e >=20 > --- > v2 (ajb) > - minor tweaks and clarifications > v3 > - Use the correct bank index function for setting/getting env->spsr > - only deal with spsrs in elevated exception levels > v4 > - try and make commentary clearer > - ensure env->banked_spsr[0] =3D env->spsr before we sync >=20 > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index 8fd0c8d..7ddb1b1 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int leve= l) > uint64_t val; > int i; > int ret; > + unsigned int el; > =20 > ARMCPU *cpu =3D ARM_CPU(cs); > CPUARMState *env =3D &cpu->env; > @@ -206,9 +207,29 @@ int kvm_arch_put_registers(CPUState *cs, int lev= el) > return ret; > } > =20 > + /* Saved Program State Registers > + * > + * Before we restore from the banked_spsr[] array we need to > + * ensure that any modifications to env->spsr are correctly > + * reflected and map aarch64 exception levels if required. > + */ > + el =3D arm_current_el(env); > + if (el > 0) { > + if (is_a64(env)) { > + g_assert(el =3D=3D 1); > + env->banked_spsr[0] =3D env->spsr; > + /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to > + * KVM_SPSR_SVC for syncing to KVM */ > + env->banked_spsr[1] =3D env->banked_spsr[0]; > + } else { > + i =3D bank_number(env->uncached_cpsr & CPSR_M); > + env->banked_spsr[i] =3D env->spsr; > + } > + } > + > for (i =3D 0; i < KVM_NR_SPSR; i++) { > reg.id =3D AARCH64_CORE_REG(spsr[i]); > - reg.addr =3D (uintptr_t) &env->banked_spsr[i - 1]; > + reg.addr =3D (uintptr_t) &env->banked_spsr[i+1]; > ret =3D kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > if (ret) { > return ret; > @@ -254,6 +275,7 @@ int kvm_arch_get_registers(CPUState *cs) > struct kvm_one_reg reg; > uint64_t val; > uint32_t fpr; > + unsigned int el; > int i; > int ret; > =20 > @@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs) > return ret; > } > =20 > + /* Fetch the SPSR registers > + * > + * KVM has an array of state indexed for all the possible aarch3= 2 > + * privilege levels. These map onto QEMUs aarch32 banks 1 - 4. > + */ > for (i =3D 0; i < KVM_NR_SPSR; i++) { > reg.id =3D AARCH64_CORE_REG(spsr[i]); > - reg.addr =3D (uintptr_t) &env->banked_spsr[i - 1]; > + reg.addr =3D (uintptr_t) &env->banked_spsr[i+1]; > ret =3D kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > if (ret) { > return ret; > } > } > =20 > + el =3D arm_current_el(env); > + if (el > 0) { > + if (is_a64(env)) { > + g_assert(el =3D=3D 1); > + /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU > + * keeps in bank 0 so copy it across. */ > + env->banked_spsr[0] =3D env->banked_spsr[1]; > + i =3D aarch64_banked_spsr_index(el); > + } else { > + i =3D bank_number(env->uncached_cpsr & CPSR_M); > + } > + env->spsr =3D env->banked_spsr[i]; > + } > + > /* Advanced SIMD and FP registers */ > for (i =3D 0; i < 32; i++) { > reg.id =3D AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > --=20 > 2.3.2 >=20 looks good! -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44935) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXUUi-0003qY-2x for qemu-devel@nongnu.org; Mon, 16 Mar 2015 08:51:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXUUf-00017d-90 for qemu-devel@nongnu.org; Mon, 16 Mar 2015 08:51:44 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:34051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXUUe-00017N-UH for qemu-devel@nongnu.org; Mon, 16 Mar 2015 08:51:41 -0400 Received: by lbbsy1 with SMTP id sy1so29919695lbb.1 for ; Mon, 16 Mar 2015 05:51:40 -0700 (PDT) Date: Mon, 16 Mar 2015 13:52:04 +0100 From: Christoffer Dall Message-ID: <20150316125204.GB26480@cbox> References: <1426503716-13931-1-git-send-email-alex.bennee@linaro.org> <1426503716-13931-5-git-send-email-alex.bennee@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1426503716-13931-5-git-send-email-alex.bennee@linaro.org> Subject: Re: [Qemu-devel] [PATCH v4 4/5] target-arm: kvm64 fix save/restore of SPSR regs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex =?iso-8859-1?Q?Benn=E9e?= Cc: Peter Maydell , kvm@vger.kernel.org, marc.zyngier@arm.com, qemu-devel@nongnu.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org On Mon, Mar 16, 2015 at 11:01:55AM +0000, Alex Bennée wrote: > From: Christoffer Dall > > The current code was negatively indexing the cpu state array and not > synchronizing banked spsr register state with the current mode's spsr > state, causing occasional failures with migration. > > Some munging is done to take care of the aarch64 mapping and also to > ensure the most current value of the spsr is updated to the banked > registers (relevant for KVM<->TCG migration). > > Signed-off-by: Christoffer Dall > Signed-off-by: Alex Bennée > > --- > v2 (ajb) > - minor tweaks and clarifications > v3 > - Use the correct bank index function for setting/getting env->spsr > - only deal with spsrs in elevated exception levels > v4 > - try and make commentary clearer > - ensure env->banked_spsr[0] = env->spsr before we sync > > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index 8fd0c8d..7ddb1b1 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level) > uint64_t val; > int i; > int ret; > + unsigned int el; > > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > @@ -206,9 +207,29 @@ int kvm_arch_put_registers(CPUState *cs, int level) > return ret; > } > > + /* Saved Program State Registers > + * > + * Before we restore from the banked_spsr[] array we need to > + * ensure that any modifications to env->spsr are correctly > + * reflected and map aarch64 exception levels if required. > + */ > + el = arm_current_el(env); > + if (el > 0) { > + if (is_a64(env)) { > + g_assert(el == 1); > + env->banked_spsr[0] = env->spsr; > + /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to > + * KVM_SPSR_SVC for syncing to KVM */ > + env->banked_spsr[1] = env->banked_spsr[0]; > + } else { > + i = bank_number(env->uncached_cpsr & CPSR_M); > + env->banked_spsr[i] = env->spsr; > + } > + } > + > for (i = 0; i < KVM_NR_SPSR; i++) { > reg.id = AARCH64_CORE_REG(spsr[i]); > - reg.addr = (uintptr_t) &env->banked_spsr[i - 1]; > + reg.addr = (uintptr_t) &env->banked_spsr[i+1]; > ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > if (ret) { > return ret; > @@ -254,6 +275,7 @@ int kvm_arch_get_registers(CPUState *cs) > struct kvm_one_reg reg; > uint64_t val; > uint32_t fpr; > + unsigned int el; > int i; > int ret; > > @@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs) > return ret; > } > > + /* Fetch the SPSR registers > + * > + * KVM has an array of state indexed for all the possible aarch32 > + * privilege levels. These map onto QEMUs aarch32 banks 1 - 4. > + */ > for (i = 0; i < KVM_NR_SPSR; i++) { > reg.id = AARCH64_CORE_REG(spsr[i]); > - reg.addr = (uintptr_t) &env->banked_spsr[i - 1]; > + reg.addr = (uintptr_t) &env->banked_spsr[i+1]; > ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > if (ret) { > return ret; > } > } > > + el = arm_current_el(env); > + if (el > 0) { > + if (is_a64(env)) { > + g_assert(el == 1); > + /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU > + * keeps in bank 0 so copy it across. */ > + env->banked_spsr[0] = env->banked_spsr[1]; > + i = aarch64_banked_spsr_index(el); > + } else { > + i = bank_number(env->uncached_cpsr & CPSR_M); > + } > + env->spsr = env->banked_spsr[i]; > + } > + > /* Advanced SIMD and FP registers */ > for (i = 0; i < 32; i++) { > reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > -- > 2.3.2 > looks good! -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Mon, 16 Mar 2015 13:52:04 +0100 Subject: [PATCH v4 4/5] target-arm: kvm64 fix save/restore of SPSR regs In-Reply-To: <1426503716-13931-5-git-send-email-alex.bennee@linaro.org> References: <1426503716-13931-1-git-send-email-alex.bennee@linaro.org> <1426503716-13931-5-git-send-email-alex.bennee@linaro.org> Message-ID: <20150316125204.GB26480@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Mar 16, 2015 at 11:01:55AM +0000, Alex Benn?e wrote: > From: Christoffer Dall > > The current code was negatively indexing the cpu state array and not > synchronizing banked spsr register state with the current mode's spsr > state, causing occasional failures with migration. > > Some munging is done to take care of the aarch64 mapping and also to > ensure the most current value of the spsr is updated to the banked > registers (relevant for KVM<->TCG migration). > > Signed-off-by: Christoffer Dall > Signed-off-by: Alex Benn?e > > --- > v2 (ajb) > - minor tweaks and clarifications > v3 > - Use the correct bank index function for setting/getting env->spsr > - only deal with spsrs in elevated exception levels > v4 > - try and make commentary clearer > - ensure env->banked_spsr[0] = env->spsr before we sync > > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index 8fd0c8d..7ddb1b1 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level) > uint64_t val; > int i; > int ret; > + unsigned int el; > > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > @@ -206,9 +207,29 @@ int kvm_arch_put_registers(CPUState *cs, int level) > return ret; > } > > + /* Saved Program State Registers > + * > + * Before we restore from the banked_spsr[] array we need to > + * ensure that any modifications to env->spsr are correctly > + * reflected and map aarch64 exception levels if required. > + */ > + el = arm_current_el(env); > + if (el > 0) { > + if (is_a64(env)) { > + g_assert(el == 1); > + env->banked_spsr[0] = env->spsr; > + /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to > + * KVM_SPSR_SVC for syncing to KVM */ > + env->banked_spsr[1] = env->banked_spsr[0]; > + } else { > + i = bank_number(env->uncached_cpsr & CPSR_M); > + env->banked_spsr[i] = env->spsr; > + } > + } > + > for (i = 0; i < KVM_NR_SPSR; i++) { > reg.id = AARCH64_CORE_REG(spsr[i]); > - reg.addr = (uintptr_t) &env->banked_spsr[i - 1]; > + reg.addr = (uintptr_t) &env->banked_spsr[i+1]; > ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > if (ret) { > return ret; > @@ -254,6 +275,7 @@ int kvm_arch_get_registers(CPUState *cs) > struct kvm_one_reg reg; > uint64_t val; > uint32_t fpr; > + unsigned int el; > int i; > int ret; > > @@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs) > return ret; > } > > + /* Fetch the SPSR registers > + * > + * KVM has an array of state indexed for all the possible aarch32 > + * privilege levels. These map onto QEMUs aarch32 banks 1 - 4. > + */ > for (i = 0; i < KVM_NR_SPSR; i++) { > reg.id = AARCH64_CORE_REG(spsr[i]); > - reg.addr = (uintptr_t) &env->banked_spsr[i - 1]; > + reg.addr = (uintptr_t) &env->banked_spsr[i+1]; > ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > if (ret) { > return ret; > } > } > > + el = arm_current_el(env); > + if (el > 0) { > + if (is_a64(env)) { > + g_assert(el == 1); > + /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU > + * keeps in bank 0 so copy it across. */ > + env->banked_spsr[0] = env->banked_spsr[1]; > + i = aarch64_banked_spsr_index(el); > + } else { > + i = bank_number(env->uncached_cpsr & CPSR_M); > + } > + env->spsr = env->banked_spsr[i]; > + } > + > /* Advanced SIMD and FP registers */ > for (i = 0; i < 32; i++) { > reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > -- > 2.3.2 > looks good! -Christoffer