[PATCH v4 4/5] target-arm: kvm64 fix save/restore of SPSR regs

[Peter Maydell]

On 17 March 2015 at 19:04, Christoffer Dall <christoffer.dall at linaro.org> wrote:
> On Tue, Mar 17, 2015 at 04:18:16PM +0000, Peter Maydell wrote:
>> Note that this code is implicitly relying on the
>> ordering of register banks defined by the bank_number()
>> function, which is a bit icky.
>
> right, I thought you wrote this code with some deeper intention of doing
> it this way so I tried to stick with the general idea - but now I
> actually looked at git blame and realized that you didn't even write it.
>
> Given all this churn around this, probably it's much cleaner to get rid
> of the loop and have an explicit sync for each architecturally
> implemented register, i.e. the SPSR_EL1 and the various mode-specific
> AArch32 SPSR registers?

Yes, this seems like a good idea. I almost suggested it when
I was writing out my review comments, in fact...

>> >      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];
>>
>> Spaces again.
>>
>> >          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_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);
>>
>> More workarounds for a bug we should just fix, I think.
>>
>
> again, this is just for the loop above to be generic, and then fix
> things up afterwards so that things work both for is_a64() and
> !is_a64().

But the only reason we're fixing anything up is that we're
working around the bug. If we didn't have that bug and the
QEMU definition of where SPSR_EL1's state lived correctly
pointed at banked_spsr[1], then the only thing you'd need
to do for syncing is copy the KVM SPSRs into banked_spsr[1..5].

-- PMM