From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH 3/4] arm/arm64: KVM: Fix migration race in the arch timer Date: Mon, 02 Mar 2015 14:20:33 +0000 Message-ID: <87a8zv32ha.fsf@linaro.org> References: <1424878582-26397-1-git-send-email-alex.bennee@linaro.org> <1424878582-26397-4-git-send-email-alex.bennee@linaro.org> <20150228133003.29d59e96@arm.com> <87twy36avw.fsf@linaro.org> <54F4297A.40905@arm.com> 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 4509A4733A for ; Mon, 2 Mar 2015 09:14:28 -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 R6mLoFsB0gWm for ; Mon, 2 Mar 2015 09:14:26 -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 D6E08472EC for ; Mon, 2 Mar 2015 09:14:26 -0500 (EST) In-reply-to: <54F4297A.40905@arm.com> 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: Marc Zyngier Cc: Russell King , "kvm@vger.kernel.org" , Gleb Natapov , open list , "linux-arm-kernel@lists.infradead.org" , Paolo Bonzini , "kvmarm@lists.cs.columbia.edu" List-Id: kvmarm@lists.cs.columbia.edu Ck1hcmMgWnluZ2llciA8bWFyYy56eW5naWVyQGFybS5jb20+IHdyaXRlczoKCj4gT24gMDIvMDMv MTUgMDg6NTAsIEFsZXggQmVubsOpZSB3cm90ZToKPj4gCj4+IE1hcmMgWnluZ2llciA8bWFyYy56 eW5naWVyQGFybS5jb20+IHdyaXRlczoKPj4gCj4+PiBPbiBXZWQsIDI1IEZlYiAyMDE1IDE1OjM2 OjIxICswMDAwCj4+PiBBbGV4IEJlbm7DqWUgPGFsZXguYmVubmVlQGxpbmFyby5vcmc+IHdyb3Rl Ogo+Pj4KPj4+IEFsZXgsIENocmlzdG9mZmVyLAo+Pj4KPj4gPHNuaXA+Cj4+Pgo+Pj4gU28gdGhl IGZpcnN0IGhhbGYgb2YgdGhlIHBhdGNoIGxvb2tzIHBlcmZlY3RseSBPSyB0byBtZS4uLgo+Pj4K Pj4+PiBkaWZmIC0tZ2l0IGEvdmlydC9rdm0vYXJtL3ZnaWMuYyBiL3ZpcnQva3ZtL2FybS92Z2lj LmMKPj4+PiBpbmRleCBhZjZhNTIxLi4zYjRkZWQyIDEwMDY0NAo+Pj4+IC0tLSBhL3ZpcnQva3Zt L2FybS92Z2ljLmMKPj4+PiArKysgYi92aXJ0L2t2bS9hcm0vdmdpYy5jCj4+Pj4gQEAgLTI2Myw2 ICsyNjMsMTMgQEAgc3RhdGljIGludCB2Z2ljX2lycV9pc19xdWV1ZWQoc3RydWN0IGt2bV92Y3B1 Cj4+Pj4gKnZjcHUsIGludCBpcnEpIHJldHVybiB2Z2ljX2JpdG1hcF9nZXRfaXJxX3ZhbCgmZGlz dC0+aXJxX3F1ZXVlZCwKPj4+PiB2Y3B1LT52Y3B1X2lkLCBpcnEpOyB9Cj4+Pj4gIAo+Pj4+ICtz dGF0aWMgaW50IHZnaWNfaXJxX2lzX2FjdGl2ZShzdHJ1Y3Qga3ZtX3ZjcHUgKnZjcHUsIGludCBp cnEpCj4+Pj4gK3sKPj4+PiArCXN0cnVjdCB2Z2ljX2Rpc3QgKmRpc3QgPSAmdmNwdS0+a3ZtLT5h cmNoLnZnaWM7Cj4+Pj4gKwo+Pj4+ICsJcmV0dXJuIHZnaWNfYml0bWFwX2dldF9pcnFfdmFsKCZk aXN0LT5pcnFfYWN0aXZlLAo+Pj4+IHZjcHUtPnZjcHVfaWQsIGlycSk7ICt9Cj4+Pj4gKwo+Pj4+ ICBzdGF0aWMgdm9pZCB2Z2ljX2lycV9zZXRfcXVldWVkKHN0cnVjdCBrdm1fdmNwdSAqdmNwdSwg aW50IGlycSkKPj4+PiAgewo+Pj4+ICAJc3RydWN0IHZnaWNfZGlzdCAqZGlzdCA9ICZ2Y3B1LT5r dm0tPmFyY2gudmdpYzsKPj4+PiBAQCAtMjg1LDYgKzI5MiwxMyBAQCBzdGF0aWMgdm9pZCB2Z2lj X2lycV9zZXRfYWN0aXZlKHN0cnVjdCBrdm1fdmNwdQo+Pj4+ICp2Y3B1LCBpbnQgaXJxKSB2Z2lj X2JpdG1hcF9zZXRfaXJxX3ZhbCgmZGlzdC0+aXJxX2FjdGl2ZSwKPj4+PiB2Y3B1LT52Y3B1X2lk LCBpcnEsIDEpOyB9Cj4+Pj4gIAo+Pj4+ICtzdGF0aWMgdm9pZCB2Z2ljX2lycV9jbGVhcl9hY3Rp dmUoc3RydWN0IGt2bV92Y3B1ICp2Y3B1LCBpbnQgaXJxKQo+Pj4+ICt7Cj4+Pj4gKwlzdHJ1Y3Qg dmdpY19kaXN0ICpkaXN0ID0gJnZjcHUtPmt2bS0+YXJjaC52Z2ljOwo+Pj4+ICsKPj4+PiArCXZn aWNfYml0bWFwX3NldF9pcnFfdmFsKCZkaXN0LT5pcnFfYWN0aXZlLCB2Y3B1LT52Y3B1X2lkLAo+ Pj4+IGlycSwgMCk7ICt9Cj4+Pj4gKwo+Pj4+ICBzdGF0aWMgaW50IHZnaWNfZGlzdF9pcnFfZ2V0 X2xldmVsKHN0cnVjdCBrdm1fdmNwdSAqdmNwdSwgaW50IGlycSkKPj4+PiAgewo+Pj4+ICAJc3Ry dWN0IHZnaWNfZGlzdCAqZGlzdCA9ICZ2Y3B1LT5rdm0tPmFyY2gudmdpYzsKPj4+PiBAQCAtNjM0 LDE2ICs2NDgsMTIgQEAgYm9vbCB2Z2ljX2hhbmRsZV9jZmdfcmVnKHUzMiAqcmVnLCBzdHJ1Y3QK Pj4+PiBrdm1fZXhpdF9tbWlvICptbWlvLCB9Cj4+Pj4gIAo+Pj4+ICAvKioKPj4+PiAtICogdmdp Y191bnF1ZXVlX2lycXMgLSBtb3ZlIHBlbmRpbmcgSVJRcyBmcm9tIExScyB0byB0aGUgZGlzdHJp YnV0b3IKPj4+PiArICogdmdpY191bnF1ZXVlX2lycXMgLSBtb3ZlIHBlbmRpbmcvYWN0aXZlIElS UXMgZnJvbSBMUnMgdG8gdGhlCj4+Pj4gZGlzdHJpYnV0b3IKPj4+PiAgICogQHZnaWNfY3B1OiBQ b2ludGVyIHRvIHRoZSB2Z2ljX2NwdSBzdHJ1Y3QgaG9sZGluZyB0aGUgTFJzCj4+Pj4gICAqCj4+ Pj4gLSAqIE1vdmUgYW55IHBlbmRpbmcgSVJRcyB0aGF0IGhhdmUgYWxyZWFkeSBiZWVuIGFzc2ln bmVkIHRvIExScyBiYWNrCj4+Pj4gdG8gdGhlCj4+Pj4gKyAqIE1vdmUgYW55IElSUXMgdGhhdCBo YXZlIGFscmVhZHkgYmVlbiBhc3NpZ25lZCB0byBMUnMgYmFjayB0byB0aGUKPj4+PiAgICogZW11 bGF0ZWQgZGlzdHJpYnV0b3Igc3RhdGUgc28gdGhhdCB0aGUgY29tcGxldGUgZW11bGF0ZWQgc3Rh dGUKPj4+PiBjYW4gYmUgcmVhZAo+Pj4+ICAgKiBmcm9tIHRoZSBtYWluIGVtdWxhdGlvbiBzdHJ1 Y3R1cmVzIHdpdGhvdXQgaW52ZXN0aWdhdGluZyB0aGUgTFJzLgo+Pj4+IC0gKgo+Pj4+IC0gKiBO b3RlIHRoYXQgSVJRcyBpbiB0aGUgYWN0aXZlIHN0YXRlIGluIHRoZSBMUnMgZ2V0IHRoZWlyIHBl bmRpbmcKPj4+PiBzdGF0ZSBtb3ZlZAo+Pj4+IC0gKiB0byB0aGUgZGlzdHJpYnV0b3IgYnV0IHRo ZSBhY3RpdmUgc3RhdGUgc3RheXMgaW4gdGhlIExScywgYmVjYXVzZQo+Pj4+IHdlIGRvbid0Cj4+ Pj4gLSAqIHRyYWNrIHRoZSBhY3RpdmUgc3RhdGUgb24gdGhlIGRpc3RyaWJ1dG9yIHNpZGUuCj4+ Pj4gICAqLwo+Pj4+ICB2b2lkIHZnaWNfdW5xdWV1ZV9pcnFzKHN0cnVjdCBrdm1fdmNwdSAqdmNw dSkKPj4+PiAgewo+Pj4+IEBAIC05MTksNyArOTI5LDcgQEAgc3RhdGljIGludCBjb21wdXRlX3Bl bmRpbmdfZm9yX2NwdShzdHJ1Y3QKPj4+PiBrdm1fdmNwdSAqdmNwdSkgCj4+Pj4gIC8qCj4+Pj4g ICAqIFVwZGF0ZSB0aGUgaW50ZXJydXB0IHN0YXRlIGFuZCBkZXRlcm1pbmUgd2hpY2ggQ1BVcyBo YXZlIHBlbmRpbmcKPj4+PiAtICogaW50ZXJydXB0cy4gTXVzdCBiZSBjYWxsZWQgd2l0aCBkaXN0 cmlidXRvciBsb2NrIGhlbGQuCj4+Pj4gKyAqIG9yIGFjdGl2ZSBpbnRlcnJ1cHRzLiBNdXN0IGJl IGNhbGxlZCB3aXRoIGRpc3RyaWJ1dG9yIGxvY2sgaGVsZC4KPj4+PiAgICovCj4+Pj4gIHZvaWQg dmdpY191cGRhdGVfc3RhdGUoc3RydWN0IGt2bSAqa3ZtKQo+Pj4+ICB7Cj4+Pj4gQEAgLTEwMzYs NiArMTA0NiwyNSBAQCBzdGF0aWMgdm9pZCB2Z2ljX3JldGlyZV9kaXNhYmxlZF9pcnFzKHN0cnVj dAo+Pj4+IGt2bV92Y3B1ICp2Y3B1KSB9Cj4+Pj4gIH0KPj4+PiAgCj4+Pj4gK3N0YXRpYyB2b2lk IHZnaWNfcXVldWVfaXJxX3RvX2xyKHN0cnVjdCBrdm1fdmNwdSAqdmNwdSwgaW50IGlycSwKPj4+ PiArCQkJCSBpbnQgbHJfbnIsIHN0cnVjdCB2Z2ljX2xyIHZscikKPj4+PiArewo+Pj4+ICsJaWYg KHZnaWNfaXJxX2lzX2FjdGl2ZSh2Y3B1LCBpcnEpKSB7Cj4+Pj4gKwkJdmxyLnN0YXRlIHw9IExS X1NUQVRFX0FDVElWRTsKPj4+PiArCQlrdm1fZGVidWcoIlNldCBhY3RpdmUsIGNsZWFyIGRpc3Ry aWJ1dG9yOiAweCV4XG4iLAo+Pj4+IHZsci5zdGF0ZSk7Cj4+Pj4gKwkJdmdpY19pcnFfY2xlYXJf YWN0aXZlKHZjcHUsIGlycSk7Cj4+Pj4gKwkJdmdpY191cGRhdGVfc3RhdGUodmNwdS0+a3ZtKTsK Pj4+PiArCX0gZWxzZSBpZiAodmdpY19kaXN0X2lycV9pc19wZW5kaW5nKHZjcHUsIGlycSkpIHsK Pj4+PiArCQl2bHIuc3RhdGUgfD0gTFJfU1RBVEVfUEVORElORzsKPj4+PiArCQlrdm1fZGVidWco IlNldCBwZW5kaW5nOiAweCV4XG4iLCB2bHIuc3RhdGUpOwo+Pj4+ICsJfQo+Pj4+ICsKPj4+PiAr CWlmICghdmdpY19pcnFfaXNfZWRnZSh2Y3B1LCBpcnEpKQo+Pj4+ICsJCXZsci5zdGF0ZSB8PSBM Ul9FT0lfSU5UOwo+Pj4+ICsKPj4+PiArCXZnaWNfc2V0X2xyKHZjcHUsIGxyX25yLCB2bHIpOwo+ Pj4+ICt9Cj4+Pj4gKwo+Pj4+ICAvKgo+Pj4+ICAgKiBRdWV1ZSBhbiBpbnRlcnJ1cHQgdG8gYSBD UFUgdmlydHVhbCBpbnRlcmZhY2UuIFJldHVybiB0cnVlIG9uCj4+Pj4gc3VjY2VzcywKPj4+PiAg ICogb3IgZmFsc2UgaWYgaXQgd2Fzbid0IHBvc3NpYmxlIHRvIHF1ZXVlIGl0Lgo+Pj4+IEBAIC0x MDYzLDggKzEwOTIsNyBAQCBib29sIHZnaWNfcXVldWVfaXJxKHN0cnVjdCBrdm1fdmNwdSAqdmNw dSwgdTgKPj4+PiBzZ2lfc291cmNlX2lkLCBpbnQgaXJxKSBpZiAodmxyLnNvdXJjZSA9PSBzZ2lf c291cmNlX2lkKSB7Cj4+Pj4gIAkJCWt2bV9kZWJ1ZygiTFIlZCBwaWdneWJhY2sgZm9yIElSUSVk XG4iLCBsciwKPj4+PiB2bHIuaXJxKTsgQlVHX09OKCF0ZXN0X2JpdChsciwgdmdpY19jcHUtPmxy X3VzZWQpKTsKPj4+PiAtCQkJdmxyLnN0YXRlIHw9IExSX1NUQVRFX1BFTkRJTkc7Cj4+Pj4gLQkJ CXZnaWNfc2V0X2xyKHZjcHUsIGxyLCB2bHIpOwo+Pj4+ICsJCQl2Z2ljX3F1ZXVlX2lycV90b19s cih2Y3B1LCBpcnEsIGxyLCB2bHIpOwo+Pj4+ICAJCQlyZXR1cm4gdHJ1ZTsKPj4+PiAgCQl9Cj4+ Pj4gIAl9Cj4+Pj4gQEAgLTEwODEsMTEgKzExMDksOCBAQCBib29sIHZnaWNfcXVldWVfaXJxKHN0 cnVjdCBrdm1fdmNwdSAqdmNwdSwgdTgKPj4+PiBzZ2lfc291cmNlX2lkLCBpbnQgaXJxKSAKPj4+ PiAgCXZsci5pcnEgPSBpcnE7Cj4+Pj4gIAl2bHIuc291cmNlID0gc2dpX3NvdXJjZV9pZDsKPj4+ PiAtCXZsci5zdGF0ZSA9IExSX1NUQVRFX1BFTkRJTkc7Cj4+Pj4gLQlpZiAoIXZnaWNfaXJxX2lz X2VkZ2UodmNwdSwgaXJxKSkKPj4+PiAtCQl2bHIuc3RhdGUgfD0gTFJfRU9JX0lOVDsKPj4+PiAt Cj4+Pj4gLQl2Z2ljX3NldF9scih2Y3B1LCBsciwgdmxyKTsKPj4+PiArCXZsci5zdGF0ZSA9IDA7 Cj4+Pj4gKwl2Z2ljX3F1ZXVlX2lycV90b19scih2Y3B1LCBpcnEsIGxyLCB2bHIpOwo+Pj4+ICAK Pj4+PiAgCXJldHVybiB0cnVlOwo+Pj4+ICB9Cj4+Pgo+Pj4KPj4+IC4uLiBidXQgdGhpcyB3aG9s ZSB2Z2ljIHJld29yayBzZWVtcyByYXRoZXIgb3V0IG9mIHBsYWNlLCBhbmQgSSBjYW4ndAo+Pj4g cmVhbGx5IHNlZSBpdHMgY29ubmVjdGlvbiB3aXRoIHRoZSB0aW1lci4gSXNuJ3QgaXQgbG9naWNh bGx5IHBhcnQgb2YgdGhlCj4+PiBwcmV2aW91cyBwYXRjaD8KPj4gCj4+IFByb2JhYmx5IC0gSSB3 YXMgZ29pbmcgdG8gcmUtZmFjdG9yIHRoYXQgY29kZSB3aXRoIHRoZSBvcmlnaW5hbCBwYXRjaAo+ PiBidXQgaXQgd2FzIG9uIHRoZSB0b2RvIGxpc3Qgb25jZSB3ZSBoYWQgaXQgd29ya2luZy4gQ2hy aXN0b2ZmZXIgdGhhbgo+PiBjbGVhbmVkIGl0IHVwIHdoZW4gaGUgZml4ZWQgdGhlIHJhY2UgaGVu Y2UgaXQgYmVpbmcgaGVyZS4KPj4gCj4+IFdvdWxkIHlvdSBsaWtlIGl0IGFzIGEgc2VwYXJhdGUg cGF0Y2ggKGJldHdlZW4gMiBhbmQgMykgb3IganVzdCByb2xsZWQKPj4gaW50byB0aGUgcHJldmlv dXMgcGF0Y2g/Cj4KPiBJbiBnZW5lcmFsLCBJIHByZWZlciBzbWFsbGVyIHBhdGNoZXMgKGtlZXBz IHRoZSBmZXcgYnJhaW4gY2VsbHMgbGVmdAo+IGZyb20gb3ZlcmhlYXRpbmcpLCBzbyBpZiB0aGVz ZSBjaGFuZ2VzIG1ha2Ugc2Vuc2Ugb24gdGhlaXIgb3duLCBwbGVhc2UKPiBwb3N0IHRoZW0gYXMg YSBzZXBhcmF0ZSBwYXRjaC4KCkRvbmUsIG5ldyBzZXJpZXMgcmUtc2VudC4KCj4KPiBUaGFua3Ms Cj4KPiAJTS4KCi0tIApBbGV4IEJlbm7DqWUKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18Ka3ZtYXJtIG1haWxpbmcgbGlzdAprdm1hcm1AbGlzdHMuY3MuY29s dW1iaWEuZWR1Cmh0dHBzOi8vbGlzdHMuY3MuY29sdW1iaWEuZWR1L21haWxtYW4vbGlzdGluZm8v a3ZtYXJtCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.bennee@linaro.org (Alex =?utf-8?Q?Benn=C3=A9e?=) Date: Mon, 02 Mar 2015 14:20:33 +0000 Subject: [PATCH 3/4] arm/arm64: KVM: Fix migration race in the arch timer In-Reply-To: <54F4297A.40905@arm.com> References: <1424878582-26397-1-git-send-email-alex.bennee@linaro.org> <1424878582-26397-4-git-send-email-alex.bennee@linaro.org> <20150228133003.29d59e96@arm.com> <87twy36avw.fsf@linaro.org> <54F4297A.40905@arm.com> Message-ID: <87a8zv32ha.fsf@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Marc Zyngier writes: > On 02/03/15 08:50, Alex Benn?e wrote: >> >> Marc Zyngier writes: >> >>> On Wed, 25 Feb 2015 15:36:21 +0000 >>> Alex Benn?e wrote: >>> >>> Alex, Christoffer, >>> >> >>> >>> So the first half of the patch looks perfectly OK to me... >>> >>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >>>> index af6a521..3b4ded2 100644 >>>> --- a/virt/kvm/arm/vgic.c >>>> +++ b/virt/kvm/arm/vgic.c >>>> @@ -263,6 +263,13 @@ static int vgic_irq_is_queued(struct kvm_vcpu >>>> *vcpu, int irq) return vgic_bitmap_get_irq_val(&dist->irq_queued, >>>> vcpu->vcpu_id, irq); } >>>> >>>> +static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq) >>>> +{ >>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> + >>>> + return vgic_bitmap_get_irq_val(&dist->irq_active, >>>> vcpu->vcpu_id, irq); +} >>>> + >>>> static void vgic_irq_set_queued(struct kvm_vcpu *vcpu, int irq) >>>> { >>>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> @@ -285,6 +292,13 @@ static void vgic_irq_set_active(struct kvm_vcpu >>>> *vcpu, int irq) vgic_bitmap_set_irq_val(&dist->irq_active, >>>> vcpu->vcpu_id, irq, 1); } >>>> >>>> +static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq) >>>> +{ >>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> + >>>> + vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, >>>> irq, 0); +} >>>> + >>>> static int vgic_dist_irq_get_level(struct kvm_vcpu *vcpu, int irq) >>>> { >>>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> @@ -634,16 +648,12 @@ bool vgic_handle_cfg_reg(u32 *reg, struct >>>> kvm_exit_mmio *mmio, } >>>> >>>> /** >>>> - * vgic_unqueue_irqs - move pending IRQs from LRs to the distributor >>>> + * vgic_unqueue_irqs - move pending/active IRQs from LRs to the >>>> distributor >>>> * @vgic_cpu: Pointer to the vgic_cpu struct holding the LRs >>>> * >>>> - * Move any pending IRQs that have already been assigned to LRs back >>>> to the >>>> + * Move any IRQs that have already been assigned to LRs back to the >>>> * emulated distributor state so that the complete emulated state >>>> can be read >>>> * from the main emulation structures without investigating the LRs. >>>> - * >>>> - * Note that IRQs in the active state in the LRs get their pending >>>> state moved >>>> - * to the distributor but the active state stays in the LRs, because >>>> we don't >>>> - * track the active state on the distributor side. >>>> */ >>>> void vgic_unqueue_irqs(struct kvm_vcpu *vcpu) >>>> { >>>> @@ -919,7 +929,7 @@ static int compute_pending_for_cpu(struct >>>> kvm_vcpu *vcpu) >>>> /* >>>> * Update the interrupt state and determine which CPUs have pending >>>> - * interrupts. Must be called with distributor lock held. >>>> + * or active interrupts. Must be called with distributor lock held. >>>> */ >>>> void vgic_update_state(struct kvm *kvm) >>>> { >>>> @@ -1036,6 +1046,25 @@ static void vgic_retire_disabled_irqs(struct >>>> kvm_vcpu *vcpu) } >>>> } >>>> >>>> +static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq, >>>> + int lr_nr, struct vgic_lr vlr) >>>> +{ >>>> + if (vgic_irq_is_active(vcpu, irq)) { >>>> + vlr.state |= LR_STATE_ACTIVE; >>>> + kvm_debug("Set active, clear distributor: 0x%x\n", >>>> vlr.state); >>>> + vgic_irq_clear_active(vcpu, irq); >>>> + vgic_update_state(vcpu->kvm); >>>> + } else if (vgic_dist_irq_is_pending(vcpu, irq)) { >>>> + vlr.state |= LR_STATE_PENDING; >>>> + kvm_debug("Set pending: 0x%x\n", vlr.state); >>>> + } >>>> + >>>> + if (!vgic_irq_is_edge(vcpu, irq)) >>>> + vlr.state |= LR_EOI_INT; >>>> + >>>> + vgic_set_lr(vcpu, lr_nr, vlr); >>>> +} >>>> + >>>> /* >>>> * Queue an interrupt to a CPU virtual interface. Return true on >>>> success, >>>> * or false if it wasn't possible to queue it. >>>> @@ -1063,8 +1092,7 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 >>>> sgi_source_id, int irq) if (vlr.source == sgi_source_id) { >>>> kvm_debug("LR%d piggyback for IRQ%d\n", lr, >>>> vlr.irq); BUG_ON(!test_bit(lr, vgic_cpu->lr_used)); >>>> - vlr.state |= LR_STATE_PENDING; >>>> - vgic_set_lr(vcpu, lr, vlr); >>>> + vgic_queue_irq_to_lr(vcpu, irq, lr, vlr); >>>> return true; >>>> } >>>> } >>>> @@ -1081,11 +1109,8 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 >>>> sgi_source_id, int irq) >>>> vlr.irq = irq; >>>> vlr.source = sgi_source_id; >>>> - vlr.state = LR_STATE_PENDING; >>>> - if (!vgic_irq_is_edge(vcpu, irq)) >>>> - vlr.state |= LR_EOI_INT; >>>> - >>>> - vgic_set_lr(vcpu, lr, vlr); >>>> + vlr.state = 0; >>>> + vgic_queue_irq_to_lr(vcpu, irq, lr, vlr); >>>> >>>> return true; >>>> } >>> >>> >>> ... but this whole vgic rework seems rather out of place, and I can't >>> really see its connection with the timer. Isn't it logically part of the >>> previous patch? >> >> Probably - I was going to re-factor that code with the original patch >> but it was on the todo list once we had it working. Christoffer than >> cleaned it up when he fixed the race hence it being here. >> >> Would you like it as a separate patch (between 2 and 3) or just rolled >> into the previous patch? > > In general, I prefer smaller patches (keeps the few brain cells left > from overheating), so if these changes make sense on their own, please > post them as a separate patch. Done, new series re-sent. > > Thanks, > > M. -- Alex Benn?e From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754365AbbCBOUs (ORCPT ); Mon, 2 Mar 2015 09:20:48 -0500 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:38814 "EHLO socrates.bennee.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751660AbbCBOUq (ORCPT ); Mon, 2 Mar 2015 09:20:46 -0500 References: <1424878582-26397-1-git-send-email-alex.bennee@linaro.org> <1424878582-26397-4-git-send-email-alex.bennee@linaro.org> <20150228133003.29d59e96@arm.com> <87twy36avw.fsf@linaro.org> <54F4297A.40905@arm.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= To: Marc Zyngier Cc: "kvm\@vger.kernel.org" , "linux-arm-kernel\@lists.infradead.org" , "kvmarm\@lists.cs.columbia.edu" , "christoffer.dall\@linaro.org" , Gleb Natapov , Paolo Bonzini , open list , Russell King Subject: Re: [PATCH 3/4] arm/arm64: KVM: Fix migration race in the arch timer In-reply-to: <54F4297A.40905@arm.com> Date: Mon, 02 Mar 2015 14:20:33 +0000 Message-ID: <87a8zv32ha.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 127.0.0.1 X-SA-Exim-Mail-From: alex.bennee@linaro.org X-SA-Exim-Scanned: No (on socrates.bennee.com); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Marc Zyngier writes: > On 02/03/15 08:50, Alex Bennée wrote: >> >> Marc Zyngier writes: >> >>> On Wed, 25 Feb 2015 15:36:21 +0000 >>> Alex Bennée wrote: >>> >>> Alex, Christoffer, >>> >> >>> >>> So the first half of the patch looks perfectly OK to me... >>> >>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >>>> index af6a521..3b4ded2 100644 >>>> --- a/virt/kvm/arm/vgic.c >>>> +++ b/virt/kvm/arm/vgic.c >>>> @@ -263,6 +263,13 @@ static int vgic_irq_is_queued(struct kvm_vcpu >>>> *vcpu, int irq) return vgic_bitmap_get_irq_val(&dist->irq_queued, >>>> vcpu->vcpu_id, irq); } >>>> >>>> +static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq) >>>> +{ >>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> + >>>> + return vgic_bitmap_get_irq_val(&dist->irq_active, >>>> vcpu->vcpu_id, irq); +} >>>> + >>>> static void vgic_irq_set_queued(struct kvm_vcpu *vcpu, int irq) >>>> { >>>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> @@ -285,6 +292,13 @@ static void vgic_irq_set_active(struct kvm_vcpu >>>> *vcpu, int irq) vgic_bitmap_set_irq_val(&dist->irq_active, >>>> vcpu->vcpu_id, irq, 1); } >>>> >>>> +static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq) >>>> +{ >>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> + >>>> + vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, >>>> irq, 0); +} >>>> + >>>> static int vgic_dist_irq_get_level(struct kvm_vcpu *vcpu, int irq) >>>> { >>>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>> @@ -634,16 +648,12 @@ bool vgic_handle_cfg_reg(u32 *reg, struct >>>> kvm_exit_mmio *mmio, } >>>> >>>> /** >>>> - * vgic_unqueue_irqs - move pending IRQs from LRs to the distributor >>>> + * vgic_unqueue_irqs - move pending/active IRQs from LRs to the >>>> distributor >>>> * @vgic_cpu: Pointer to the vgic_cpu struct holding the LRs >>>> * >>>> - * Move any pending IRQs that have already been assigned to LRs back >>>> to the >>>> + * Move any IRQs that have already been assigned to LRs back to the >>>> * emulated distributor state so that the complete emulated state >>>> can be read >>>> * from the main emulation structures without investigating the LRs. >>>> - * >>>> - * Note that IRQs in the active state in the LRs get their pending >>>> state moved >>>> - * to the distributor but the active state stays in the LRs, because >>>> we don't >>>> - * track the active state on the distributor side. >>>> */ >>>> void vgic_unqueue_irqs(struct kvm_vcpu *vcpu) >>>> { >>>> @@ -919,7 +929,7 @@ static int compute_pending_for_cpu(struct >>>> kvm_vcpu *vcpu) >>>> /* >>>> * Update the interrupt state and determine which CPUs have pending >>>> - * interrupts. Must be called with distributor lock held. >>>> + * or active interrupts. Must be called with distributor lock held. >>>> */ >>>> void vgic_update_state(struct kvm *kvm) >>>> { >>>> @@ -1036,6 +1046,25 @@ static void vgic_retire_disabled_irqs(struct >>>> kvm_vcpu *vcpu) } >>>> } >>>> >>>> +static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq, >>>> + int lr_nr, struct vgic_lr vlr) >>>> +{ >>>> + if (vgic_irq_is_active(vcpu, irq)) { >>>> + vlr.state |= LR_STATE_ACTIVE; >>>> + kvm_debug("Set active, clear distributor: 0x%x\n", >>>> vlr.state); >>>> + vgic_irq_clear_active(vcpu, irq); >>>> + vgic_update_state(vcpu->kvm); >>>> + } else if (vgic_dist_irq_is_pending(vcpu, irq)) { >>>> + vlr.state |= LR_STATE_PENDING; >>>> + kvm_debug("Set pending: 0x%x\n", vlr.state); >>>> + } >>>> + >>>> + if (!vgic_irq_is_edge(vcpu, irq)) >>>> + vlr.state |= LR_EOI_INT; >>>> + >>>> + vgic_set_lr(vcpu, lr_nr, vlr); >>>> +} >>>> + >>>> /* >>>> * Queue an interrupt to a CPU virtual interface. Return true on >>>> success, >>>> * or false if it wasn't possible to queue it. >>>> @@ -1063,8 +1092,7 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 >>>> sgi_source_id, int irq) if (vlr.source == sgi_source_id) { >>>> kvm_debug("LR%d piggyback for IRQ%d\n", lr, >>>> vlr.irq); BUG_ON(!test_bit(lr, vgic_cpu->lr_used)); >>>> - vlr.state |= LR_STATE_PENDING; >>>> - vgic_set_lr(vcpu, lr, vlr); >>>> + vgic_queue_irq_to_lr(vcpu, irq, lr, vlr); >>>> return true; >>>> } >>>> } >>>> @@ -1081,11 +1109,8 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 >>>> sgi_source_id, int irq) >>>> vlr.irq = irq; >>>> vlr.source = sgi_source_id; >>>> - vlr.state = LR_STATE_PENDING; >>>> - if (!vgic_irq_is_edge(vcpu, irq)) >>>> - vlr.state |= LR_EOI_INT; >>>> - >>>> - vgic_set_lr(vcpu, lr, vlr); >>>> + vlr.state = 0; >>>> + vgic_queue_irq_to_lr(vcpu, irq, lr, vlr); >>>> >>>> return true; >>>> } >>> >>> >>> ... but this whole vgic rework seems rather out of place, and I can't >>> really see its connection with the timer. Isn't it logically part of the >>> previous patch? >> >> Probably - I was going to re-factor that code with the original patch >> but it was on the todo list once we had it working. Christoffer than >> cleaned it up when he fixed the race hence it being here. >> >> Would you like it as a separate patch (between 2 and 3) or just rolled >> into the previous patch? > > In general, I prefer smaller patches (keeps the few brain cells left > from overheating), so if these changes make sense on their own, please > post them as a separate patch. Done, new series re-sent. > > Thanks, > > M. -- Alex Bennée