On Mon, Mar 9, 2015 at 8:26 AM, Christoffer Dall
<christoffer.dall@xxxxxxxxxx> wrote:
> On Wed, Mar 04, 2015 at 02:35:52PM +0000, Alex Bennée wrote:
>> From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Alex Bennée <alex.bennee@xxxxxxxxxx>
>>
>> ---
>> 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
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index c60e989..45e5c3f 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,27 @@ 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);
>> + /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
>
> not sure about the 'for aarch64' comment; I would say that it's for
> aarch32 support. Also, you can drop the ATM, since this is user space
> ABI that we don't change easily.
>
>
> don't you need to do env->banked_spsr[0] = env->spsr first?
I agree with Christoffer, env->spsr actually has the most current
value so you need to sync up with it before sending it out.
>
>> + env->banked_spsr[1] = env->banked_spsr[0];
>
>
>> + } else {
>> + i = bank_number(env->uncached_cpsr & CPSR_M);
>> + env->banked_spsr[i] = env->spsr;
>
> so here we don't need to worry about banked_spsr[1] = banked_spsr[0]
> because banked_spsr[0] is meaningless for 32-bit state and we only sync
> banked_spsr[1] and up to KVM, correct? I think this is what may deserve
> a comment.
>
>> + }
>> + }
>> +
>> 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 +272,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 +345,35 @@ 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
>
> privilege
>
>> + * there are some transitions possible which can access old state
>> + * so it is worth keeping them all.
>> + */
>
> dubious comment overall
>
>> 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 maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
>
> same as above
If Christoffer's comment is referring to updating env->spsr, it occurs
below based on 'i'.
>
>> + env->banked_spsr[0] = env->banked_spsr[1];
>> + i = aarch64_banked_spsr_index(el);
>> + } else {
>> + i = bank_number(env->uncached_cpsr & CPSR_M);
>
> same potential place for comment as above.
>
>> + }
>> + 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.1
>>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
