On 17 March 2015 at 19:04, Christoffer Dall [off-list ref] wrote:

On Tue, Mar 17, 2015 at 04:18:16PM +0000, Peter Maydell wrote:

quoted

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...

quoted

quoted

     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.

quoted

         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