From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs Date: Tue, 03 Mar 2015 11:28:21 +0000 Message-ID: <874mq27222.fsf@linaro.org> References: <1424880159-29348-1-git-send-email-alex.bennee@linaro.org> <1424880159-29348-7-git-send-email-alex.bennee@linaro.org> <20150302172212.GB10137@lvm> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 14C3746972 for ; Tue, 3 Mar 2015 06:22:25 -0500 (EST) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id z7aFxqE+O3lX for ; Tue, 3 Mar 2015 06:22:23 -0500 (EST) Received: from socrates.bennee.com (static.88-198-71-155.clients.your-server.de [88.198.71.155]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id CAAB3477EB for ; Tue, 3 Mar 2015 06:22:23 -0500 (EST) In-reply-to: <20150302172212.GB10137@lvm> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Christoffer Dall Cc: kvm@vger.kernel.org, marc.zyngier@arm.com, qemu-devel@nongnu.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu CkNocmlzdG9mZmVyIERhbGwgPGNocmlzdG9mZmVyLmRhbGxAbGluYXJvLm9yZz4gd3JpdGVzOgoK PiBIaSBBbGV4LAo+Cj4gU2VlbXMgbGlrZSB5b3UgYWNjaWRlbnRhbGx5IHNlbnQgb3V0IHR3byBj b3BpZXMgb2YgdGhpcyBwYXRjaCwgaG9wZWZ1bGx5Cj4gSSdtIHJldmlld2luZyB0aGUgcmlnaHQg b25lLi4uCgpPb3BzLCBwZXJpbHMgb2Ygbm90IHdpcGluZyB5b3VyIG91dHB1dCBkaXJlY3Rvcnku IEkgdGhpbmsgaXQgd2FzIGp1c3QgYQp0aXRsZSB0d2VhayEKCj4gT24gV2VkLCBGZWIgMjUsIDIw MTUgYXQgMDQ6MDI6MzhQTSArMDAwMCwgQWxleCBCZW5uw6llIHdyb3RlOgo+PiBGcm9tOiBDaHJp c3RvZmZlciBEYWxsIDxjaHJpc3RvZmZlci5kYWxsQGxpbmFyby5vcmc+Cj4+IAo+PiBUaGUgY3Vy cmVudCBjb2RlIHdhcyBuZWdhdGl2ZWx5IGluZGV4aW5nIHRoZSBjcHUgc3RhdGUgYXJyYXkgYW5k IG5vdAo+PiBzeW5jaHJvbml6aW5nIGJhbmtlZCBzcHNyIHJlZ2lzdGVyIHN0YXRlIHdpdGggdGhl IGN1cnJlbnQgbW9kZSdzIHNwc3IKPj4gc3RhdGUsIGNhdXNpbmcgb2NjYXNpb25hbCBmYWlsdXJl cyB3aXRoIG1pZ3JhdGlvbi4KPj4gCj4+IFNvbWUgbXVuZ2luZyBpcyBkb25lIHRvIHRha2UgY2Fy ZSBvZiB0aGUgYWFyY2g2NCBtYXBwaW5nIGFuZCBhbHNvIHRvCj4+IGVuc3VyZSB0aGUgbW9zdCBj dXJyZW50IHZhbHVlIG9mIHRoZSBzcHNyIGlzIHVwZGF0ZWQgdG8gdGhlIGJhbmtlZAo+PiByZWdp c3RlcnMgKHJlbGV2YW50IGZvciBLVk08LT5UQ0cgbWlncmF0aW9uKS4KPj4gCj4+IFNpZ25lZC1v ZmYtYnk6IENocmlzdG9mZmVyIERhbGwgPGNocmlzdG9mZmVyLmRhbGxAbGluYXJvLm9yZz4KPj4g U2lnbmVkLW9mZi1ieTogQWxleCBCZW5uw6llIDxhbGV4LmJlbm5lZUBsaW5hcm8ub3JnPgo+PiAK Pj4gLS0tCj4+IHYyIChhamIpCj4+ICAgLSBtaW5vciB0d2Vha3MgYW5kIGNsYXJpZmljYXRpb25z Cj4+IAo+PiBkaWZmIC0tZ2l0IGEvdGFyZ2V0LWFybS9rdm02NC5jIGIvdGFyZ2V0LWFybS9rdm02 NC5jCj4+IGluZGV4IGM2MGU5ODkuLjFlMzZiMGEgMTAwNjQ0Cj4+IC0tLSBhL3RhcmdldC1hcm0v a3ZtNjQuYwo+PiArKysgYi90YXJnZXQtYXJtL2t2bTY0LmMKPj4gQEAgLTE0MCw2ICsxNDAsNyBA QCBpbnQga3ZtX2FyY2hfcHV0X3JlZ2lzdGVycyhDUFVTdGF0ZSAqY3MsIGludCBsZXZlbCkKPj4g ICAgICB1aW50NjRfdCB2YWw7Cj4+ICAgICAgaW50IGk7Cj4+ICAgICAgaW50IHJldDsKPj4gKyAg ICB1bnNpZ25lZCBpbnQgZWw7Cj4+ICAKPj4gICAgICBBUk1DUFUgKmNwdSA9IEFSTV9DUFUoY3Mp Owo+PiAgICAgIENQVUFSTVN0YXRlICplbnYgPSAmY3B1LT5lbnY7Cj4+IEBAIC0yMDYsOSArMjA3 LDI1IEBAIGludCBrdm1fYXJjaF9wdXRfcmVnaXN0ZXJzKENQVVN0YXRlICpjcywgaW50IGxldmVs KQo+PiAgICAgICAgICByZXR1cm4gcmV0Owo+PiAgICAgIH0KPj4gIAo+PiArICAgIC8qIFNhdmVk IFByb2dyYW0gU3RhdGUgUmVnaXN0ZXJzCj4+ICsgICAgICoKPj4gKyAgICAgKiBCZWZvcmUgd2Ug cmVzdG9yZSBmcm9tIHRoZSBiYW5rZWRfc3BzcltdIGFycmF5IHdlIG5lZWQgdG8KPj4gKyAgICAg KiBlbnN1cmUgdGhhdCBhbnkgbW9kaWZpY2F0aW9ucyB0byBlbnYtPnNwc3IgYXJlIGNvcnJlY3Rs eQo+PiArICAgICAqIHJlZmxlY3RlZCBhbmQgbWFwIGFhcmNoNjQgZXhjZXB0aW9uIGxldmVscyBp ZiByZXF1aXJlZC4KPj4gKyAgICAgKi8KPj4gKyAgICBlbCA9IGFybV9jdXJyZW50X2VsKGVudik7 Cj4+ICsgICAgaWYgKGlzX2E2NChlbnYpICYmIGVsID4gMCkgewo+PiArICAgICAgICBnX2Fzc2Vy dChlbCA9PSAxKTsKPj4gKyAgICAgICAgLyogS1ZNIG1hcHMgS1ZNX1NQU1JfU1ZDIHRvIEtWTV9T UFNSX0VMMSBmb3IgYWFyY2g2NCAqLwo+PiArICAgICAgICBlbnYtPmJhbmtlZF9zcHNyWzFdID0g ZW52LT5iYW5rZWRfc3BzclswXTsKPj4gKyAgICAgICAgZW52LT5iYW5rZWRfc3BzclthYXJjaDY0 X2JhbmtlZF9zcHNyX2luZGV4KGVsKV0gPSBlbnYtPnNwc3I7Cj4+ICsgICAgfSBlbHNlIHsKPj4g KyAgICAgICAgZW52LT5iYW5rZWRfc3BzcltlbF0gPSBlbnYtPnNwc3I7Cj4KPiBpcyB0aGlzIHZh bGlkIGlmIChpc19hNjQoZW52KSAmJiBlbCA9PSAwKSkgPyAgSSB0aG91Z2h0IHRoYXQgaWYgeW91 J3JlCj4gaW4gZWwgPT0gMCwgdGhlbiBlbnYtPmJhbmtlZF9zcHNyW3hdIGlzIHRoZSBtb3N0IHVw LXRvLWRhdGUgb25lLCBub3QKPiBlbnYtPnNwc3IgPwoKQWN0dWFsbHkgdGhleSB3aWxsIGJvdGgg YmUgdGhlIHNhbWUgKGF0IGxlYXN0IGZvciBLVk0pLiBJbiB0aGUgZW5kIGJvdGg6CgogICAgICAg IFZNU1RBVEVfVUlOVDMyKGVudi5zcHNyLCBBUk1DUFUpLAogICAgICAgIFZNU1RBVEVfVUlOVDY0 X0FSUkFZKGVudi5iYW5rZWRfc3BzciwgQVJNQ1BVLCA4KSwKCmdldCBzZXJpYWxpc2VkIGluIHRo ZSBzdHJlYW0gYW5kIHdoZW4gd2Ugc2F2ZSB0aGUgc3RyZWFtIG91dCB3ZSBtYWtlCnN1cmUgZXZl cnl0aGluZyBpcyBpbiBzeW5jIChpLmUuIGVudi0+c3BzciBpcyBjb3JyZWN0KS4gSW4gcmVhbGl0 eSB0aGlzCm1ha2VzIG1vcmUgc2Vuc2UgZm9yIFRDRyB0aGFuIEtWTSB3aGljaCBpcyB0aGUgb25s eSByZWFzb24gZW52LT5zcHNyCmV4aXN0cy4KCj4KPiBmb3IgIWlzX2E2NChlbnYpIHRoaXMgbG9v a3Mgd3JvbmcsIGJlY2F1c2Ugb2YgdGhlIHNhbWUgYXMgYWJvdmUgaWYgZWwgPT0KPiAwLCBidXQg YWxzbyBiZWNhdXNlIEkgdGhpbmsgeW91IG5lZWQKPiBiYW5rX251bWJlcihlbnYtPnVuY2FjaGVk X2Nwc3IgJiBDUFNSX00pIHRvIGluZGV4IGludG8gdGhlIGFycmF5Lgo+CgpHb29kIGNhdGNoLiBG b3Igc29tZSByZWFzb24gSSB3YXMgdHJlYXRpbmcgdGhlIG51bWJlciBmcm9tCmFybV9jdXJyZW50 X2VsKCkgYXMgZXF1aXZhbGVudC4gSG93IGFib3V0OgoKICAgIGVsID0gYXJtX2N1cnJlbnRfZWwo ZW52KTsKICAgIGlmIChpc19hNjQoZW52KSAmJiBlbCA+IDApIHsKICAgICAgICBnX2Fzc2VydChl bCA9PSAxKTsKICAgICAgICAvKiBLVk0gb25seSBtYXBzIEtWTV9TUFNSX1NWQyB0byBLVk1fU1BT Ul9FTDEgZm9yIGFhcmNoNjQgQVRNICovCiAgICAgICAgZW52LT5iYW5rZWRfc3BzclsxXSA9IGVu di0+YmFua2VkX3Nwc3JbMF07CiAgICB9CiAgICBpID0gaXNfYTY0KGVudikgPwogICAgICAgIGFh cmNoNjRfYmFua2VkX3Nwc3JfaW5kZXgoZWwpIDogYmFua19udW1iZXIoZW52LT51bmFjaGVkX2Nw c3IgJiBDUFNSX00pOwogICAgZW52LT5iYW5rZWRfc3BzcltpXSA9IGVudi0+c3BzcjsKCgo+PiAr ICAgIH0KPj4gKwo+PiAgICAgIGZvciAoaSA9IDA7IGkgPCBLVk1fTlJfU1BTUjsgaSsrKSB7Cj4+ ICAgICAgICAgIHJlZy5pZCA9IEFBUkNINjRfQ09SRV9SRUcoc3BzcltpXSk7Cj4+IC0gICAgICAg IHJlZy5hZGRyID0gKHVpbnRwdHJfdCkgJmVudi0+YmFua2VkX3Nwc3JbaSAtIDFdOwo+PiArICAg ICAgICByZWcuYWRkciA9ICh1aW50cHRyX3QpICZlbnYtPmJhbmtlZF9zcHNyW2krMV07Cj4+ICAg ICAgICAgIHJldCA9IGt2bV92Y3B1X2lvY3RsKGNzLCBLVk1fU0VUX09ORV9SRUcsICZyZWcpOwo+ PiAgICAgICAgICBpZiAocmV0KSB7Cj4+ICAgICAgICAgICAgICByZXR1cm4gcmV0Owo+PiBAQCAt MjUzLDYgKzI3MCw3IEBAIGludCBrdm1fYXJjaF9nZXRfcmVnaXN0ZXJzKENQVVN0YXRlICpjcykK Pj4gICAgICBzdHJ1Y3Qga3ZtX29uZV9yZWcgcmVnOwo+PiAgICAgIHVpbnQ2NF90IHZhbDsKPj4g ICAgICB1aW50MzJfdCBmcHI7Cj4+ICsgICAgdW5zaWduZWQgaW50IGVsOwo+PiAgICAgIGludCBp Owo+PiAgICAgIGludCByZXQ7Cj4+ICAKPj4gQEAgLTMyNSwxNSArMzQzLDMyIEBAIGludCBrdm1f YXJjaF9nZXRfcmVnaXN0ZXJzKENQVVN0YXRlICpjcykKPj4gICAgICAgICAgcmV0dXJuIHJldDsK Pj4gICAgICB9Cj4+ICAKPj4gKyAgICAvKiBGZXRjaCB0aGUgU1BTUiByZWdpc3RlcnMKPj4gKyAg ICAgKgo+PiArICAgICAqIEtWTSBoYXMgYW4gYXJyYXkgb2Ygc3RhdGUgaW5kZXhlZCBmb3IgYWxs IHRoZSBwb3NzaWJsZSBhYXJjaDMyCj4+ICsgICAgICogcHJpdmlsYWdlIGxldmVscy4gQWx0aG91 Z2ggbm90IGFsbCBhcmUgdmFsaWQgYXQgYWxsIHBvaW50cwo+PiArICAgICAqIHRoZXJlIGFyZSBz b21lIHRyYW5zaXRpb25zIHBvc3NpYmxlIHdoaWNoIGNhbiBhY2Nlc3Mgb2xkIHN0YXRlCj4+ICsg ICAgICogc28gaXQgaXMgd29ydGgga2VlcGluZyB0aGVtIGFsbC4KPj4gKyAgICAgKi8KPj4gICAg ICBmb3IgKGkgPSAwOyBpIDwgS1ZNX05SX1NQU1I7IGkrKykgewo+PiAgICAgICAgICByZWcuaWQg PSBBQVJDSDY0X0NPUkVfUkVHKHNwc3JbaV0pOwo+PiAtICAgICAgICByZWcuYWRkciA9ICh1aW50 cHRyX3QpICZlbnYtPmJhbmtlZF9zcHNyW2kgLSAxXTsKPj4gKyAgICAgICAgcmVnLmFkZHIgPSAo dWludHB0cl90KSAmZW52LT5iYW5rZWRfc3BzcltpKzFdOwo+PiAgICAgICAgICByZXQgPSBrdm1f dmNwdV9pb2N0bChjcywgS1ZNX0dFVF9PTkVfUkVHLCAmcmVnKTsKPj4gICAgICAgICAgaWYgKHJl dCkgewo+PiAgICAgICAgICAgICAgcmV0dXJuIHJldDsKPj4gICAgICAgICAgfQo+PiAgICAgIH0K Pj4gIAo+PiArICAgIGVsID0gYXJtX2N1cnJlbnRfZWwoZW52KTsKPj4gKyAgICBpZiAoaXNfYTY0 KGVudikgJiYgZWwgPiAwKSB7Cj4+ICsgICAgICAgIGdfYXNzZXJ0KGVsID09IDEpOwo+PiArICAg ICAgICAvKiBLVk0gbWFwcyBLVk1fU1BTUl9TVkMgdG8gS1ZNX1NQU1JfRUwxIGZvciBhYXJjaDY0 ICovCj4+ICsgICAgICAgIGVudi0+YmFua2VkX3Nwc3JbMF0gPSBlbnYtPmJhbmtlZF9zcHNyWzFd Owo+PiArICAgICAgICBlbnYtPnNwc3IgPSBlbnYtPmJhbmtlZF9zcHNyW2FhcmNoNjRfYmFua2Vk X3Nwc3JfaW5kZXgoZWwpXTsKPj4gKyAgICB9IGVsc2Ugewo+PiArICAgICAgICBlbnYtPnNwc3Ig PSBlbnYtPmJhbmtlZF9zcHNyW2VsXTsKPgo+IHNhbWUgY29uY2VybiB3aXRoIGJhbmtfbnVtYmVy IGFzIGFib3ZlLgo+Cj4+ICsgICAgfQo+PiArCj4+ICAgICAgLyogQWR2YW5jZWQgU0lNRCBhbmQg RlAgcmVnaXN0ZXJzICovCj4+ICAgICAgZm9yIChpID0gMDsgaSA8IDMyOyBpKyspIHsKPj4gICAg ICAgICAgcmVnLmlkID0gQUFSQ0g2NF9TSU1EX0NPUkVfUkVHKGZwX3JlZ3MudnJlZ3NbaV0pOwo+ PiAtLSAKPj4gMi4zLjAKPj4gCj4KPiBUaGFua3MsCj4gLUNocmlzdG9mZmVyCgotLSAKQWxleCBC ZW5uw6llCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmt2 bWFybSBtYWlsaW5nIGxpc3QKa3ZtYXJtQGxpc3RzLmNzLmNvbHVtYmlhLmVkdQpodHRwczovL2xp c3RzLmNzLmNvbHVtYmlhLmVkdS9tYWlsbWFuL2xpc3RpbmZvL2t2bWFybQo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.bennee@linaro.org (Alex =?utf-8?Q?Benn=C3=A9e?=) Date: Tue, 03 Mar 2015 11:28:21 +0000 Subject: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs In-Reply-To: <20150302172212.GB10137@lvm> References: <1424880159-29348-1-git-send-email-alex.bennee@linaro.org> <1424880159-29348-7-git-send-email-alex.bennee@linaro.org> <20150302172212.GB10137@lvm> Message-ID: <874mq27222.fsf@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Christoffer Dall writes: > Hi Alex, > > Seems like you accidentally sent out two copies of this patch, hopefully > I'm reviewing the right one... Oops, perils of not wiping your output directory. I think it was just a title tweak! > On Wed, Feb 25, 2015 at 04:02:38PM +0000, Alex Benn?e wrote: >> From: Christoffer Dall >> >> 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 >> Signed-off-by: Alex Benn?e >> >> --- >> v2 (ajb) >> - minor tweaks and clarifications >> >> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c >> index c60e989..1e36b0a 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,25 @@ 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 (is_a64(env) && el > 0) { >> + g_assert(el == 1); >> + /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */ >> + env->banked_spsr[1] = env->banked_spsr[0]; >> + env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr; >> + } else { >> + env->banked_spsr[el] = env->spsr; > > is this valid if (is_a64(env) && el == 0)) ? I thought that if you're > in el == 0, then env->banked_spsr[x] is the most up-to-date one, not > env->spsr ? Actually they will both be the same (at least for KVM). In the end both: VMSTATE_UINT32(env.spsr, ARMCPU), VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8), get serialised in the stream and when we save the stream out we make sure everything is in sync (i.e. env->spsr is correct). In reality this makes more sense for TCG than KVM which is the only reason env->spsr exists. > > for !is_a64(env) this looks wrong, because of the same as above if el == > 0, but also because I think you need > bank_number(env->uncached_cpsr & CPSR_M) to index into the array. > Good catch. For some reason I was treating the number from arm_current_el() as equivalent. How about: el = arm_current_el(env); if (is_a64(env) && el > 0) { g_assert(el == 1); /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */ env->banked_spsr[1] = env->banked_spsr[0]; } i = is_a64(env) ? aarch64_banked_spsr_index(el) : bank_number(env->unached_cpsr & CPSR_M); env->banked_spsr[i] = env->spsr; >> + } >> + >> 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 +270,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 +343,32 @@ 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 >> + * there are some transitions possible which can access old state >> + * so it is worth keeping them all. >> + */ >> 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 (is_a64(env) && el > 0) { >> + g_assert(el == 1); >> + /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */ >> + env->banked_spsr[0] = env->banked_spsr[1]; >> + env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)]; >> + } else { >> + env->spsr = env->banked_spsr[el]; > > same concern with bank_number as above. > >> + } >> + >> /* Advanced SIMD and FP registers */ >> for (i = 0; i < 32; i++) { >> reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); >> -- >> 2.3.0 >> > > Thanks, > -Christoffer -- Alex Benn?e From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59854) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSl06-0004uR-39 for qemu-devel@nongnu.org; Tue, 03 Mar 2015 06:28:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSl00-00019R-Pg for qemu-devel@nongnu.org; Tue, 03 Mar 2015 06:28:34 -0500 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:49662 helo=socrates.bennee.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSl00-00019G-Gf for qemu-devel@nongnu.org; Tue, 03 Mar 2015 06:28:28 -0500 References: <1424880159-29348-1-git-send-email-alex.bennee@linaro.org> <1424880159-29348-7-git-send-email-alex.bennee@linaro.org> <20150302172212.GB10137@lvm> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <20150302172212.GB10137@lvm> Date: Tue, 03 Mar 2015 11:28:21 +0000 Message-ID: <874mq27222.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoffer Dall Cc: Peter Maydell , kvm@vger.kernel.org, marc.zyngier@arm.com, qemu-devel@nongnu.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Christoffer Dall writes: > Hi Alex, > > Seems like you accidentally sent out two copies of this patch, hopefully > I'm reviewing the right one... Oops, perils of not wiping your output directory. I think it was just a title tweak! > On Wed, Feb 25, 2015 at 04:02:38PM +0000, Alex Bennée wrote: >> From: Christoffer Dall >> >> 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 >> Signed-off-by: Alex Bennée >> >> --- >> v2 (ajb) >> - minor tweaks and clarifications >> >> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c >> index c60e989..1e36b0a 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,25 @@ 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 (is_a64(env) && el > 0) { >> + g_assert(el == 1); >> + /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */ >> + env->banked_spsr[1] = env->banked_spsr[0]; >> + env->banked_spsr[aarch64_banked_spsr_index(el)] = env->spsr; >> + } else { >> + env->banked_spsr[el] = env->spsr; > > is this valid if (is_a64(env) && el == 0)) ? I thought that if you're > in el == 0, then env->banked_spsr[x] is the most up-to-date one, not > env->spsr ? Actually they will both be the same (at least for KVM). In the end both: VMSTATE_UINT32(env.spsr, ARMCPU), VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8), get serialised in the stream and when we save the stream out we make sure everything is in sync (i.e. env->spsr is correct). In reality this makes more sense for TCG than KVM which is the only reason env->spsr exists. > > for !is_a64(env) this looks wrong, because of the same as above if el == > 0, but also because I think you need > bank_number(env->uncached_cpsr & CPSR_M) to index into the array. > Good catch. For some reason I was treating the number from arm_current_el() as equivalent. How about: el = arm_current_el(env); if (is_a64(env) && el > 0) { g_assert(el == 1); /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */ env->banked_spsr[1] = env->banked_spsr[0]; } i = is_a64(env) ? aarch64_banked_spsr_index(el) : bank_number(env->unached_cpsr & CPSR_M); env->banked_spsr[i] = env->spsr; >> + } >> + >> 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 +270,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 +343,32 @@ 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 >> + * there are some transitions possible which can access old state >> + * so it is worth keeping them all. >> + */ >> 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 (is_a64(env) && el > 0) { >> + g_assert(el == 1); >> + /* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */ >> + env->banked_spsr[0] = env->banked_spsr[1]; >> + env->spsr = env->banked_spsr[aarch64_banked_spsr_index(el)]; >> + } else { >> + env->spsr = env->banked_spsr[el]; > > same concern with bank_number as above. > >> + } >> + >> /* Advanced SIMD and FP registers */ >> for (i = 0; i < 32; i++) { >> reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); >> -- >> 2.3.0 >> > > Thanks, > -Christoffer -- Alex Bennée