From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cyBaj-0004Ur-9M for qemu-devel@nongnu.org; Wed, 12 Apr 2017 02:17:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cyBag-0008AO-43 for qemu-devel@nongnu.org; Wed, 12 Apr 2017 02:17:21 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:34476) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cyBaf-00089v-Oj for qemu-devel@nongnu.org; Wed, 12 Apr 2017 02:17:18 -0400 Received: by mail-lf0-x241.google.com with SMTP id x72so1901958lfb.1 for ; Tue, 11 Apr 2017 23:17:15 -0700 (PDT) Date: Wed, 12 Apr 2017 16:17:04 +1000 From: Alexey G Message-ID: <20170412161704.00003875@gmail.com> In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Xen-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: hrg , wangxinxin.wang@huawei.com, Alexander Graf , qemu-devel@nongnu.org, xen-devel@lists.xensource.com, Jun Nakajima , Anthony PERARD , xen-devel@lists.xenproject.org, xen-devel@lists.xen.org, "Herongguang (Stephen)" On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) Stefano Stabellini wrote: > On Tue, 11 Apr 2017, hrg wrote: > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > > wrote: =20 > > > On Mon, 10 Apr 2017, Stefano Stabellini wrote: =20 > > >> On Mon, 10 Apr 2017, hrg wrote: =20 > > >> > On Sun, Apr 9, 2017 at 11:55 PM, hrg wrote:= =20 > > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg wrot= e: =20 > > >> > >> Hi, > > >> > >> > > >> > >> In xen_map_cache_unlocked(), map to guest memory maybe in > > >> > >> entry->next instead of first level entry (if map to rom other t= han > > >> > >> guest memory comes first), while in xen_invalidate_map_cache(), > > >> > >> when VM ballooned out memory, qemu did not invalidate cache ent= ries > > >> > >> in linked list(entry->next), so when VM balloon back in memory, > > >> > >> gfns probably mapped to different mfns, thus if guest asks devi= ce > > >> > >> to DMA to these GPA, qemu may DMA to stale MFNs. > > >> > >> > > >> > >> So I think in xen_invalidate_map_cache() linked lists should al= so be > > >> > >> checked and invalidated. > > >> > >> > > >> > >> What=E2=80=99s your opinion? Is this a bug? Is my analyze corre= ct? =20 > > >> > > >> Yes, you are right. We need to go through the list for each element = of > > >> the array in xen_invalidate_map_cache. Can you come up with a patch?= =20 > > > > > > I spoke too soon. In the regular case there should be no locked mappi= ngs > > > when xen_invalidate_map_cache is called (see the DPRINTF warning at t= he > > > beginning of the functions). Without locked mappings, there should ne= ver > > > be more than one element in each list (see xen_map_cache_unlocked: > > > entry->lock =3D=3D true is a necessary condition to append a new entr= y to > > > the list, otherwise it is just remapped). > > > > > > Can you confirm that what you are seeing are locked mappings > > > when xen_invalidate_map_cache is called? To find out, enable the DPRI= NTK > > > by turning it into a printf or by defininig MAPCACHE_DEBUG. =20 > >=20 > > In fact, I think the DPRINTF above is incorrect too. In > > pci_add_option_rom(), rtl8139 rom is locked mapped in > > pci_add_option_rom->memory_region_get_ram_ptr (after > > memory_region_init_ram). So actually I think we should remove the > > DPRINTF warning as it is normal. =20 >=20 > Let me explain why the DPRINTF warning is there: emulated dma operations > can involve locked mappings. Once a dma operation completes, the related > mapping is unlocked and can be safely destroyed. But if we destroy a > locked mapping in xen_invalidate_map_cache, while a dma is still > ongoing, QEMU will crash. We cannot handle that case. >=20 > However, the scenario you described is different. It has nothing to do > with DMA. It looks like pci_add_option_rom calls > memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a > locked mapping and it is never unlocked or destroyed. >=20 > It looks like "ptr" is not used after pci_add_option_rom returns. Does > the append patch fix the problem you are seeing? For the proper fix, I > think we probably need some sort of memory_region_unmap wrapper or maybe > a call to address_space_unmap. Hmm, for some reason my message to the Xen-devel list got rejected but was = sent to Qemu-devel instead, without any notice. Sorry if I'm missing something obvious as a list newbie. Stefano, hrg, There is an issue with inconsistency between the list of normal MapCacheEnt= ry's and their 'reverse' counterparts - MapCacheRev's in locked_entries. When bad situation happens, there are multiple (locked) MapCacheEntry entries in the bucket's linked list along with a number of MapCacheRev's. A= nd when it comes to a reverse lookup, xen-mapcache picks the wrong entry from = the first list and calculates a wrong pointer from it which may then be caught = with the "Bad RAM offset" check (or not). Mapcache invalidation might be related= to this issue as well I think. I'll try to provide a test code which can reproduce the issue from the guest side using an emulated IDE controller, though it's much simpler to ac= hieve this result with an AHCI controller using multiple NCQ I/O commands. So far= I've seen this issue only with Windows 7 (and above) guest on AHCI, but any bloc= k I/O DMA should be enough I think. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey G Subject: Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? Date: Wed, 12 Apr 2017 16:17:04 +1000 Message-ID: <20170412161704.00003875@gmail.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Stefano Stabellini Cc: xen-devel@lists.xensource.com, wangxinxin.wang@huawei.com, qemu-devel@nongnu.org, Alexander Graf , Jun Nakajima , Anthony PERARD , xen-devel@lists.xenproject.org, xen-devel@lists.xen.org, "Herongguang (Stephen)" , hrg List-Id: xen-devel@lists.xenproject.org T24gVHVlLCAxMSBBcHIgMjAxNyAxNTozMjowOSAtMDcwMCAoUERUKQpTdGVmYW5vIFN0YWJlbGxp bmkgPHNzdGFiZWxsaW5pQGtlcm5lbC5vcmc+IHdyb3RlOgoKPiBPbiBUdWUsIDExIEFwciAyMDE3 LCBocmcgd3JvdGU6Cj4gPiBPbiBUdWUsIEFwciAxMSwgMjAxNyBhdCAzOjUwIEFNLCBTdGVmYW5v IFN0YWJlbGxpbmkKPiA+IDxzc3RhYmVsbGluaUBrZXJuZWwub3JnPiB3cm90ZTogIAo+ID4gPiBP biBNb24sIDEwIEFwciAyMDE3LCBTdGVmYW5vIFN0YWJlbGxpbmkgd3JvdGU6ICAKPiA+ID4+IE9u IE1vbiwgMTAgQXByIDIwMTcsIGhyZyB3cm90ZTogIAo+ID4gPj4gPiBPbiBTdW4sIEFwciA5LCAy MDE3IGF0IDExOjU1IFBNLCBocmcgPGhyZ3N0ZXBoZW5AZ21haWwuY29tPiB3cm90ZTogIAo+ID4g Pj4gPiA+IE9uIFN1biwgQXByIDksIDIwMTcgYXQgMTE6NTIgUE0sIGhyZyA8aHJnc3RlcGhlbkBn bWFpbC5jb20+IHdyb3RlOiAgCj4gPiA+PiA+ID4+IEhpLAo+ID4gPj4gPiA+Pgo+ID4gPj4gPiA+ PiBJbiB4ZW5fbWFwX2NhY2hlX3VubG9ja2VkKCksIG1hcCB0byBndWVzdCBtZW1vcnkgbWF5YmUg aW4KPiA+ID4+ID4gPj4gZW50cnktPm5leHQgaW5zdGVhZCBvZiBmaXJzdCBsZXZlbCBlbnRyeSAo aWYgbWFwIHRvIHJvbSBvdGhlciB0aGFuCj4gPiA+PiA+ID4+IGd1ZXN0IG1lbW9yeSBjb21lcyBm aXJzdCksIHdoaWxlIGluIHhlbl9pbnZhbGlkYXRlX21hcF9jYWNoZSgpLAo+ID4gPj4gPiA+PiB3 aGVuIFZNIGJhbGxvb25lZCBvdXQgbWVtb3J5LCBxZW11IGRpZCBub3QgaW52YWxpZGF0ZSBjYWNo ZSBlbnRyaWVzCj4gPiA+PiA+ID4+IGluIGxpbmtlZCBsaXN0KGVudHJ5LT5uZXh0KSwgc28gd2hl biBWTSBiYWxsb29uIGJhY2sgaW4gbWVtb3J5LAo+ID4gPj4gPiA+PiBnZm5zIHByb2JhYmx5IG1h cHBlZCB0byBkaWZmZXJlbnQgbWZucywgdGh1cyBpZiBndWVzdCBhc2tzIGRldmljZQo+ID4gPj4g PiA+PiB0byBETUEgdG8gdGhlc2UgR1BBLCBxZW11IG1heSBETUEgdG8gc3RhbGUgTUZOcy4KPiA+ ID4+ID4gPj4KPiA+ID4+ID4gPj4gU28gSSB0aGluayBpbiB4ZW5faW52YWxpZGF0ZV9tYXBfY2Fj aGUoKSBsaW5rZWQgbGlzdHMgc2hvdWxkIGFsc28gYmUKPiA+ID4+ID4gPj4gY2hlY2tlZCBhbmQg aW52YWxpZGF0ZWQuCj4gPiA+PiA+ID4+Cj4gPiA+PiA+ID4+IFdoYXTigJlzIHlvdXIgb3Bpbmlv bj8gSXMgdGhpcyBhIGJ1Zz8gSXMgbXkgYW5hbHl6ZSBjb3JyZWN0PyAgCj4gPiA+Pgo+ID4gPj4g WWVzLCB5b3UgYXJlIHJpZ2h0LiBXZSBuZWVkIHRvIGdvIHRocm91Z2ggdGhlIGxpc3QgZm9yIGVh Y2ggZWxlbWVudCBvZgo+ID4gPj4gdGhlIGFycmF5IGluIHhlbl9pbnZhbGlkYXRlX21hcF9jYWNo ZS4gQ2FuIHlvdSBjb21lIHVwIHdpdGggYSBwYXRjaD8gIAo+ID4gPgo+ID4gPiBJIHNwb2tlIHRv byBzb29uLiBJbiB0aGUgcmVndWxhciBjYXNlIHRoZXJlIHNob3VsZCBiZSBubyBsb2NrZWQgbWFw cGluZ3MKPiA+ID4gd2hlbiB4ZW5faW52YWxpZGF0ZV9tYXBfY2FjaGUgaXMgY2FsbGVkIChzZWUg dGhlIERQUklOVEYgd2FybmluZyBhdCB0aGUKPiA+ID4gYmVnaW5uaW5nIG9mIHRoZSBmdW5jdGlv bnMpLiBXaXRob3V0IGxvY2tlZCBtYXBwaW5ncywgdGhlcmUgc2hvdWxkIG5ldmVyCj4gPiA+IGJl IG1vcmUgdGhhbiBvbmUgZWxlbWVudCBpbiBlYWNoIGxpc3QgKHNlZSB4ZW5fbWFwX2NhY2hlX3Vu bG9ja2VkOgo+ID4gPiBlbnRyeS0+bG9jayA9PSB0cnVlIGlzIGEgbmVjZXNzYXJ5IGNvbmRpdGlv biB0byBhcHBlbmQgYSBuZXcgZW50cnkgdG8KPiA+ID4gdGhlIGxpc3QsIG90aGVyd2lzZSBpdCBp cyBqdXN0IHJlbWFwcGVkKS4KPiA+ID4KPiA+ID4gQ2FuIHlvdSBjb25maXJtIHRoYXQgd2hhdCB5 b3UgYXJlIHNlZWluZyBhcmUgbG9ja2VkIG1hcHBpbmdzCj4gPiA+IHdoZW4geGVuX2ludmFsaWRh dGVfbWFwX2NhY2hlIGlzIGNhbGxlZD8gVG8gZmluZCBvdXQsIGVuYWJsZSB0aGUgRFBSSU5USwo+ ID4gPiBieSB0dXJuaW5nIGl0IGludG8gYSBwcmludGYgb3IgYnkgZGVmaW5pbmlnIE1BUENBQ0hF X0RFQlVHLiAgCj4gPiAKPiA+IEluIGZhY3QsIEkgdGhpbmsgdGhlIERQUklOVEYgYWJvdmUgaXMg aW5jb3JyZWN0IHRvby4gSW4KPiA+IHBjaV9hZGRfb3B0aW9uX3JvbSgpLCBydGw4MTM5IHJvbSBp cyBsb2NrZWQgbWFwcGVkIGluCj4gPiBwY2lfYWRkX29wdGlvbl9yb20tPm1lbW9yeV9yZWdpb25f Z2V0X3JhbV9wdHIgKGFmdGVyCj4gPiBtZW1vcnlfcmVnaW9uX2luaXRfcmFtKS4gU28gYWN0dWFs bHkgSSB0aGluayB3ZSBzaG91bGQgcmVtb3ZlIHRoZQo+ID4gRFBSSU5URiB3YXJuaW5nIGFzIGl0 IGlzIG5vcm1hbC4gIAo+IAo+IExldCBtZSBleHBsYWluIHdoeSB0aGUgRFBSSU5URiB3YXJuaW5n IGlzIHRoZXJlOiBlbXVsYXRlZCBkbWEgb3BlcmF0aW9ucwo+IGNhbiBpbnZvbHZlIGxvY2tlZCBt YXBwaW5ncy4gT25jZSBhIGRtYSBvcGVyYXRpb24gY29tcGxldGVzLCB0aGUgcmVsYXRlZAo+IG1h cHBpbmcgaXMgdW5sb2NrZWQgYW5kIGNhbiBiZSBzYWZlbHkgZGVzdHJveWVkLiBCdXQgaWYgd2Ug ZGVzdHJveSBhCj4gbG9ja2VkIG1hcHBpbmcgaW4geGVuX2ludmFsaWRhdGVfbWFwX2NhY2hlLCB3 aGlsZSBhIGRtYSBpcyBzdGlsbAo+IG9uZ29pbmcsIFFFTVUgd2lsbCBjcmFzaC4gV2UgY2Fubm90 IGhhbmRsZSB0aGF0IGNhc2UuCj4gCj4gSG93ZXZlciwgdGhlIHNjZW5hcmlvIHlvdSBkZXNjcmli ZWQgaXMgZGlmZmVyZW50LiBJdCBoYXMgbm90aGluZyB0byBkbwo+IHdpdGggRE1BLiBJdCBsb29r cyBsaWtlIHBjaV9hZGRfb3B0aW9uX3JvbSBjYWxscwo+IG1lbW9yeV9yZWdpb25fZ2V0X3JhbV9w dHIgdG8gbWFwIHRoZSBydGw4MTM5IHJvbS4gVGhlIG1hcHBpbmcgaXMgYQo+IGxvY2tlZCBtYXBw aW5nIGFuZCBpdCBpcyBuZXZlciB1bmxvY2tlZCBvciBkZXN0cm95ZWQuCj4gCj4gSXQgbG9va3Mg bGlrZSAicHRyIiBpcyBub3QgdXNlZCBhZnRlciBwY2lfYWRkX29wdGlvbl9yb20gcmV0dXJucy4g RG9lcwo+IHRoZSBhcHBlbmQgcGF0Y2ggZml4IHRoZSBwcm9ibGVtIHlvdSBhcmUgc2VlaW5nPyBG b3IgdGhlIHByb3BlciBmaXgsIEkKPiB0aGluayB3ZSBwcm9iYWJseSBuZWVkIHNvbWUgc29ydCBv ZiBtZW1vcnlfcmVnaW9uX3VubWFwIHdyYXBwZXIgb3IgbWF5YmUKPiBhIGNhbGwgdG8gYWRkcmVz c19zcGFjZV91bm1hcC4KCkhtbSwgZm9yIHNvbWUgcmVhc29uIG15IG1lc3NhZ2UgdG8gdGhlIFhl bi1kZXZlbCBsaXN0IGdvdCByZWplY3RlZCBidXQgd2FzIHNlbnQKdG8gUWVtdS1kZXZlbCBpbnN0 ZWFkLCB3aXRob3V0IGFueSBub3RpY2UuIFNvcnJ5IGlmIEknbSBtaXNzaW5nIHNvbWV0aGluZwpv YnZpb3VzIGFzIGEgbGlzdCBuZXdiaWUuCgpTdGVmYW5vLCBocmcsCgpUaGVyZSBpcyBhbiBpc3N1 ZSB3aXRoIGluY29uc2lzdGVuY3kgYmV0d2VlbiB0aGUgbGlzdCBvZiBub3JtYWwgTWFwQ2FjaGVF bnRyeSdzCmFuZCB0aGVpciAncmV2ZXJzZScgY291bnRlcnBhcnRzIC0gTWFwQ2FjaGVSZXYncyBp biBsb2NrZWRfZW50cmllcy4KV2hlbiBiYWQgc2l0dWF0aW9uIGhhcHBlbnMsIHRoZXJlIGFyZSBt dWx0aXBsZSAobG9ja2VkKSBNYXBDYWNoZUVudHJ5CmVudHJpZXMgaW4gdGhlIGJ1Y2tldCdzIGxp bmtlZCBsaXN0IGFsb25nIHdpdGggYSBudW1iZXIgb2YgTWFwQ2FjaGVSZXYncy4gQW5kCndoZW4g aXQgY29tZXMgdG8gYSByZXZlcnNlIGxvb2t1cCwgeGVuLW1hcGNhY2hlIHBpY2tzIHRoZSB3cm9u ZyBlbnRyeSBmcm9tIHRoZQpmaXJzdCBsaXN0IGFuZCBjYWxjdWxhdGVzIGEgd3JvbmcgcG9pbnRl ciBmcm9tIGl0IHdoaWNoIG1heSB0aGVuIGJlIGNhdWdodCB3aXRoCnRoZSAiQmFkIFJBTSBvZmZz ZXQiIGNoZWNrIChvciBub3QpLiBNYXBjYWNoZSBpbnZhbGlkYXRpb24gbWlnaHQgYmUgcmVsYXRl ZCB0bwp0aGlzIGlzc3VlIGFzIHdlbGwgSSB0aGluay4KCkknbGwgdHJ5IHRvIHByb3ZpZGUgYSB0 ZXN0IGNvZGUgd2hpY2ggY2FuIHJlcHJvZHVjZSB0aGUgaXNzdWUgZnJvbSB0aGUKZ3Vlc3Qgc2lk ZSB1c2luZyBhbiBlbXVsYXRlZCBJREUgY29udHJvbGxlciwgdGhvdWdoIGl0J3MgbXVjaCBzaW1w bGVyIHRvIGFjaGlldmUKdGhpcyByZXN1bHQgd2l0aCBhbiBBSENJIGNvbnRyb2xsZXIgdXNpbmcg bXVsdGlwbGUgTkNRIEkvTyBjb21tYW5kcy4gU28gZmFyIEkndmUKc2VlbiB0aGlzIGlzc3VlIG9u bHkgd2l0aCBXaW5kb3dzIDcgKGFuZCBhYm92ZSkgZ3Vlc3Qgb24gQUhDSSwgYnV0IGFueSBibG9j ayBJL08KRE1BIHNob3VsZCBiZSBlbm91Z2ggSSB0aGluay4KCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fClhlbi1kZXZlbCBtYWlsaW5nIGxpc3QKWGVuLWRl dmVsQGxpc3RzLnhlbi5vcmcKaHR0cHM6Ly9saXN0cy54ZW4ub3JnL3hlbi1kZXZlbAo=