On 17 March 2015 at 19:04, Christoffer Dall <christoffer.dall@xxxxxxxxxx> 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, ®);
>> > 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
--
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
