| Message ID | 1424880159-29348-7-git-send-email-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alex, Seems like you accidentally sent out two copies of this patch, hopefully I'm reviewing the right one... On Wed, Feb 25, 2015 at 04:02:38PM +0000, Alex Bennée wrote: > From: Christoffer Dall <christoffer.dall@linaro.org> > > 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 <christoffer.dall@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > 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 ? 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. > + } > + > 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
Christoffer Dall <christoffer.dall@linaro.org> 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 <christoffer.dall@linaro.org> >> >> 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 <christoffer.dall@linaro.org> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> 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
On Tue, Mar 03, 2015 at 11:28:21AM +0000, Alex Bennée wrote: > > Christoffer Dall <christoffer.dall@linaro.org> 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 <christoffer.dall@linaro.org> > >> > >> 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 <christoffer.dall@linaro.org> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> > >> --- > >> 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. > this function, however, is not used only when migration, but should generally cover the case where you want to synchronize QEMU's state into KVM's state, right? So while it may not be harmful in currently supported use cases, is there ever a situation where (is_a64(env) && el == 0) and env->spsr != banked_spsr[el], and where env->spsr is out-of-date? If that's the case, I think it would be better to have an assert that says "don't call the code in this situation" than relying on limited use cases for callers of this function. > > > > 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); I think that will fail due to the assert in aarch64_banked_spsr_index() for el == 0. -Christoffer
On 9 March 2015 at 21:56, Christoffer Dall <christoffer.dall@linaro.org> wrote: > this function, however, is not used only when migration, but should > generally cover the case where you want to synchronize QEMU's state into > KVM's state, right? So while it may not be harmful in currently > supported use cases, is there ever a situation where (is_a64(env) && el > == 0) and env->spsr != banked_spsr[el], and where env->spsr is > out-of-date? If EL == 0 then you can't access any SPSR, so env->spsr is by definition out of date. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 9 March 2015 at 21:56, Christoffer Dall <christoffer.dall@linaro.org> wrote: >> this function, however, is not used only when migration, but should >> generally cover the case where you want to synchronize QEMU's state into >> KVM's state, right? So while it may not be harmful in currently >> supported use cases, is there ever a situation where (is_a64(env) && el >> == 0) and env->spsr != banked_spsr[el], and where env->spsr is >> out-of-date? > > If EL == 0 then you can't access any SPSR, so env->spsr is by > definition out of date. Indeed and in v2 the whole thing is wrapped in if (el > 0) > > -- PMM
On Mon, Mar 09, 2015 at 10:31:19PM +0900, Peter Maydell wrote: > On 9 March 2015 at 21:56, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > this function, however, is not used only when migration, but should > > generally cover the case where you want to synchronize QEMU's state into > > KVM's state, right? So while it may not be harmful in currently > > supported use cases, is there ever a situation where (is_a64(env) && el > > == 0) and env->spsr != banked_spsr[el], and where env->spsr is > > out-of-date? > > If EL == 0 then you can't access any SPSR, so env->spsr is by > definition out of date. > ok, never mind then. -Christoffer
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; + } + 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]; + } + /* Advanced SIMD and FP registers */ for (i = 0; i < 32; i++) { reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);