From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59854) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSl06-0004uR-39 for qemu-devel@nongnu.org; Tue, 03 Mar 2015 06:28:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSl00-00019R-Pg for qemu-devel@nongnu.org; Tue, 03 Mar 2015 06:28:34 -0500 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:49662 helo=socrates.bennee.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSl00-00019G-Gf for qemu-devel@nongnu.org; Tue, 03 Mar 2015 06:28:28 -0500 References: <1424880159-29348-1-git-send-email-alex.bennee@linaro.org> <1424880159-29348-7-git-send-email-alex.bennee@linaro.org> <20150302172212.GB10137@lvm> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <20150302172212.GB10137@lvm> Date: Tue, 03 Mar 2015 11:28:21 +0000 Message-ID: <874mq27222.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoffer Dall 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 Christoffer Dall writes: > Hi Alex, > > Seems like you accidentally sent out two copies of this patch, hopefully > I'm reviewing the right one... Oops, perils of not wiping your output directory. I think it was just a title tweak! > On Wed, Feb 25, 2015 at 04:02:38PM +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 >> >> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c >> index c60e989..1e36b0a 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,25 @@ 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 (is_a64(env) && el > 0) { >> + g_assert(el == 1); >> + /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */ >> + env->banked_spsr[1] = env->banked_spsr[0]; >> + env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr; >> + } else { >> + env->banked_spsr[el] = env->spsr; > > is this valid if (is_a64(env) && el == 0)) ? I thought that if you're > in el == 0, then env->banked_spsr[x] is the most up-to-date one, not > env->spsr ? Actually they will both be the same (at least for KVM). In the end both: VMSTATE_UINT32(env.spsr, ARMCPU), VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8), get serialised in the stream and when we save the stream out we make sure everything is in sync (i.e. env->spsr is correct). In reality this makes more sense for TCG than KVM which is the only reason env->spsr exists. > > for !is_a64(env) this looks wrong, because of the same as above if el == > 0, but also because I think you need > bank_number(env->uncached_cpsr & CPSR_M) to index into the array. > Good catch. For some reason I was treating the number from arm_current_el() as equivalent. How about: el = arm_current_el(env); if (is_a64(env) && el > 0) { g_assert(el == 1); /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */ env->banked_spsr[1] = env->banked_spsr[0]; } i = is_a64(env) ? aarch64_banked_spsr_index(el) : bank_number(env->unached_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; >> @@ -253,6 +270,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; >> >> @@ -325,15 +343,32 @@ 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 >> + * privilage levels. Although not all are valid at all points >> + * there are some transitions possible which can access old state >> + * so it is worth keeping them all. >> + */ >> 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 (is_a64(env) && el > 0) { >> + g_assert(el == 1); >> + /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */ >> + env->banked_spsr[0] = env->banked_spsr[1]; >> + env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)]; >> + } else { >> + env->spsr = env->banked_spsr[el]; > > same concern with bank_number as above. > >> + } >> + >> /* Advanced SIMD and FP registers */ >> for (i = 0; i < 32; i++) { >> reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); >> -- >> 2.3.0 >> > > Thanks, > -Christoffer -- Alex Bennée