From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Maydell Subject: Re: [PATCH v2 3/6] hw/char: pl011 don't keep setting the IRQ if nothing changed Date: Thu, 12 Mar 2015 15:51:50 +0000 Message-ID: References: <1425479753-18349-1-git-send-email-alex.bennee@linaro.org> <1425479753-18349-4-git-send-email-alex.bennee@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: Marc Zyngier , "kvmarm@lists.cs.columbia.edu" , QEMU Developers , kvm-devel , arm-mail-list To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Return-path: In-Reply-To: <1425479753-18349-4-git-send-email-alex.bennee@linaro.org> 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 List-Id: kvm.vger.kernel.org T24gNCBNYXJjaCAyMDE1IGF0IDE0OjM1LCBBbGV4IEJlbm7DqWUgPGFsZXguYmVubmVlQGxpbmFy by5vcmc+IHdyb3RlOgo+IFdoaWxlIG9ic2VydmluZyBLVk0gdHJhY2VzIEkgY2FuIHNlZSBhZGRp dGlvbmFsIElSUSBjYWxscyBvbiBwcmV0dHkgbXVjaAo+IGV2ZXJ5IE1NSU8gYWNjZXNzIHdoaWNo IGlzIGp1c3QgcGxhaW4gaW5lZmZpY2llbnQuIE9ubHkgdXBkYXRlIHRoZSBRRU1VCj4gSVJRIGxl dmVsIGlmIHNvbWV0aGluZyBoYXMgYWN0dWFsbHkgY2hhbmdlZCBmcm9tIGxhc3QgdGltZS4gT3Ro ZXJ3aXNlIHdlCj4gbWF5IGJlIHBhcGVyaW5nIG92ZXIgb3RoZXIgZmFpbHVyZSBtb2Rlcy4KPgo+ IFNpZ25lZC1vZmYtYnk6IEFsZXggQmVubsOpZSA8YWxleC5iZW5uZWVAbGluYXJvLm9yZz4KPgo+ IGRpZmYgLS1naXQgYS9ody9jaGFyL3BsMDExLmMgYi9ody9jaGFyL3BsMDExLmMKPiBpbmRleCAw YTQ1MTE1Li5iYjU1NGJjIDEwMDY0NAo+IC0tLSBhL2h3L2NoYXIvcGwwMTEuYwo+ICsrKyBiL2h3 L2NoYXIvcGwwMTEuYwo+IEBAIC0zNiw2ICszNiw5IEBAIHR5cGVkZWYgc3RydWN0IFBMMDExU3Rh dGUgewo+ICAgICAgQ2hhckRyaXZlclN0YXRlICpjaHI7Cj4gICAgICBxZW11X2lycSBpcnE7Cj4g ICAgICBjb25zdCB1bnNpZ25lZCBjaGFyICppZDsKPiArCj4gKyAgICAvKiBub3Qgc2VyaWFsaXNl ZCwgcHJldmVudHMgcGwwMTFfdXBkYXRlIGRvaW5nIGV4dHJhIHNldF9pcnFzICovCj4gKyAgICB1 aW50MzJfdCBjdXJyZW50X2lycTsKPiAgfSBQTDAxMVN0YXRlOwo+Cj4gICNkZWZpbmUgUEwwMTFf SU5UX1RYIDB4MjAKPiBAQCAtNTMsMTAgKzU2LDExIEBAIHN0YXRpYyBjb25zdCB1bnNpZ25lZCBj aGFyIHBsMDExX2lkX2x1bWluYXJ5WzhdID0KPgo+ICBzdGF0aWMgdm9pZCBwbDAxMV91cGRhdGUo UEwwMTFTdGF0ZSAqcykKPiAgewo+IC0gICAgdWludDMyX3QgZmxhZ3M7Cj4gLQo+IC0gICAgZmxh Z3MgPSBzLT5pbnRfbGV2ZWwgJiBzLT5pbnRfZW5hYmxlZDsKPiAtICAgIHFlbXVfc2V0X2lycShz LT5pcnEsIGZsYWdzICE9IDApOwo+ICsgICAgdWludDMyX3QgZmxhZ3MgPSBzLT5pbnRfbGV2ZWwg JiBzLT5pbnRfZW5hYmxlZDsKPiArICAgIGlmIChmbGFncyAhPSBzLT5jdXJyZW50X2lycSkgewo+ ICsgICAgICAgIHMtPmN1cnJlbnRfaXJxID0gZmxhZ3M7Cj4gKyAgICAgICAgcWVtdV9zZXRfaXJx KHMtPmlycSwgcy0+Y3VycmVudF9pcnEgIT0gMCk7Cj4gKyAgICB9Cj4gIH0KCkNvbnNpZGVyIHRo aXMgc2VxdWVuY2Ugb2YgZXZlbnRzOgoKICogdGhlIGd1ZXN0IGRvZXMgc29tZXRoaW5nIGNhdXNp bmcgdGhlIGludGVycnVwdCB0bwogICBiZSBhc3NlcnRlZDsgaW50X2xldmVsIGFuZCBpbnRfZW5h YmxlZCBhcmUgMSwgYW5kCiAgIGN1cnJlbnRfaXJxIGlzIGFsc28gbm93IDEuIFdlIGNhbGwgcWVt dV9zZXRfaXJxKCkKICAgdG8gcmFpc2UgdGhlIGludGVycnVwdCB3aXRoIHRoZSBHSUMKICogd2Ug bWlncmF0ZSB0aGUgZ3Vlc3QgdG8gYW5vdGhlciBob3N0CiAqIG9uIHRoZSByZWNlaXZpbmcgZW5k LCBRRU1VIGlzIGluIGEgY2xlYW5seSByZXNldAogICBzdGF0ZSwgYW5kIHNvIGN1cnJlbnRfaXJx LCBpbnRfbGV2ZWwgYW5kIGludF9lbmFibGVkCiAgIGFyZSBhbGwgemVybyBiZWZvcmUgaW5jb21p bmcgZGF0YSBhcnJpdmVzCiAqIGludF9sZXZlbCBhbmQgaW50X2VuYWJsZWQgYXJlIGJvdGggc2V0 IHRvIDEgZnJvbQogICB0aGUgaW5jb21pbmcgZGF0YSBzdHJlYW0KICogdGhlIEdJQyBpdHNlbGYg aXMgc2V0IHRvIHRoZSAiaW50ZXJydXB0IGlzCiAgIGFzc2VydGVkIiBzdGF0ZSBieSBpdHMgb3du IGluY29taW5nIGRhdGEKICogY3VycmVudF9pcnEgcmVtYWlucyB6ZXJvLCBiZWNhdXNlIGl0J3Mg bm90IG1pZ3JhdGVkCiAqIHRoZSBndWVzdCBpcyByZXN1bWVkLCBhbmQgZG9lcyBzb21ldGhpbmcg dG8gZGVhc3NlcnQKICAgdGhlIGludGVycnVwdC4gdGhlIG5ldyAnZmxhZ3MnIHZhbHVlIGlzIHpl cm8KICogYmVjYXVzZSBmbGFncyA9PSBzLT5jdXJyZW50X2lycSwgd2UgZG9uJ3QgY2FsbAogICBx ZW11X3NldF9pcnEsIGFuZCBzbyB3ZSd2ZSBqdXN0IGRyb3BwZWQgdGhlIGRlYXNzZXJ0CiAgIG9m IHRoaXMgaW50ZXJydXB0IG9uIHRoZSBmbG9vci4KCi0tIFBNTQpfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwprdm1hcm0gbWFpbGluZyBsaXN0Cmt2bWFybUBs aXN0cy5jcy5jb2x1bWJpYS5lZHUKaHR0cHM6Ly9saXN0cy5jcy5jb2x1bWJpYS5lZHUvbWFpbG1h bi9saXN0aW5mby9rdm1hcm0K From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34790) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YW5PD-0004iD-HB for qemu-devel@nongnu.org; Thu, 12 Mar 2015 11:52:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YW5PA-0001rU-2R for qemu-devel@nongnu.org; Thu, 12 Mar 2015 11:52:15 -0400 Received: from mail-lb0-f182.google.com ([209.85.217.182]:39282) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YW5P9-0001rQ-RY for qemu-devel@nongnu.org; Thu, 12 Mar 2015 11:52:12 -0400 Received: by lbiw7 with SMTP id w7so16930945lbi.6 for ; Thu, 12 Mar 2015 08:52:10 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1425479753-18349-4-git-send-email-alex.bennee@linaro.org> References: <1425479753-18349-1-git-send-email-alex.bennee@linaro.org> <1425479753-18349-4-git-send-email-alex.bennee@linaro.org> From: Peter Maydell Date: Thu, 12 Mar 2015 15:51:50 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 3/6] hw/char: pl011 don't keep setting the IRQ if nothing changed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: Marc Zyngier , "kvmarm@lists.cs.columbia.edu" , QEMU Developers , kvm-devel , arm-mail-list On 4 March 2015 at 14:35, Alex Benn=C3=A9e wrote: > While observing KVM traces I can see additional IRQ calls on pretty much > every MMIO access which is just plain inefficient. Only update the QEMU > IRQ level if something has actually changed from last time. Otherwise we > may be papering over other failure modes. > > Signed-off-by: Alex Benn=C3=A9e > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 0a45115..bb554bc 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -36,6 +36,9 @@ typedef struct PL011State { > CharDriverState *chr; > qemu_irq irq; > const unsigned char *id; > + > + /* not serialised, prevents pl011_update doing extra set_irqs */ > + uint32_t current_irq; > } PL011State; > > #define PL011_INT_TX 0x20 > @@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] =3D > > static void pl011_update(PL011State *s) > { > - uint32_t flags; > - > - flags =3D s->int_level & s->int_enabled; > - qemu_set_irq(s->irq, flags !=3D 0); > + uint32_t flags =3D s->int_level & s->int_enabled; > + if (flags !=3D s->current_irq) { > + s->current_irq =3D flags; > + qemu_set_irq(s->irq, s->current_irq !=3D 0); > + } > } Consider this sequence of events: * the guest does something causing the interrupt to be asserted; int_level and int_enabled are 1, and current_irq is also now 1. We call qemu_set_irq() to raise the interrupt with the GIC * we migrate the guest to another host * on the receiving end, QEMU is in a cleanly reset state, and so current_irq, int_level and int_enabled are all zero before incoming data arrives * int_level and int_enabled are both set to 1 from the incoming data stream * the GIC itself is set to the "interrupt is asserted" state by its own incoming data * current_irq remains zero, because it's not migrated * the guest is resumed, and does something to deassert the interrupt. the new 'flags' value is zero * because flags =3D=3D s->current_irq, we don't call qemu_set_irq, and so we've just dropped the deassert of this interrupt on the floor. -- PMM From mboxrd@z Thu Jan 1 00:00:00 1970 From: peter.maydell@linaro.org (Peter Maydell) Date: Thu, 12 Mar 2015 15:51:50 +0000 Subject: [PATCH v2 3/6] hw/char: pl011 don't keep setting the IRQ if nothing changed In-Reply-To: <1425479753-18349-4-git-send-email-alex.bennee@linaro.org> References: <1425479753-18349-1-git-send-email-alex.bennee@linaro.org> <1425479753-18349-4-git-send-email-alex.bennee@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 4 March 2015 at 14:35, Alex Benn?e wrote: > While observing KVM traces I can see additional IRQ calls on pretty much > every MMIO access which is just plain inefficient. Only update the QEMU > IRQ level if something has actually changed from last time. Otherwise we > may be papering over other failure modes. > > Signed-off-by: Alex Benn?e > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 0a45115..bb554bc 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -36,6 +36,9 @@ typedef struct PL011State { > CharDriverState *chr; > qemu_irq irq; > const unsigned char *id; > + > + /* not serialised, prevents pl011_update doing extra set_irqs */ > + uint32_t current_irq; > } PL011State; > > #define PL011_INT_TX 0x20 > @@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] = > > static void pl011_update(PL011State *s) > { > - uint32_t flags; > - > - flags = s->int_level & s->int_enabled; > - qemu_set_irq(s->irq, flags != 0); > + uint32_t flags = s->int_level & s->int_enabled; > + if (flags != s->current_irq) { > + s->current_irq = flags; > + qemu_set_irq(s->irq, s->current_irq != 0); > + } > } Consider this sequence of events: * the guest does something causing the interrupt to be asserted; int_level and int_enabled are 1, and current_irq is also now 1. We call qemu_set_irq() to raise the interrupt with the GIC * we migrate the guest to another host * on the receiving end, QEMU is in a cleanly reset state, and so current_irq, int_level and int_enabled are all zero before incoming data arrives * int_level and int_enabled are both set to 1 from the incoming data stream * the GIC itself is set to the "interrupt is asserted" state by its own incoming data * current_irq remains zero, because it's not migrated * the guest is resumed, and does something to deassert the interrupt. the new 'flags' value is zero * because flags == s->current_irq, we don't call qemu_set_irq, and so we've just dropped the deassert of this interrupt on the floor. -- PMM