[Qemu-devel] [PATCH v2 5/6] target-arm: kvm64 fix save/restore of SPSR regs

[Greg Bellows]

On Mon, Mar 9, 2015 at 8:26 AM, Christoffer Dall
<christoffer.dall at linaro.org> wrote:
> On Wed, Mar 04, 2015 at 02:35:52PM +0000, Alex Bennée wrote:
>> From: Christoffer Dall <christoffer.dall at 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 at linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee at linaro.org>
>>
>> ---
>> 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, &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, &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
>>
>