From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59987) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cyDeO-00069f-70 for qemu-devel@nongnu.org; Wed, 12 Apr 2017 04:29:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cyDeK-0005xN-Vo for qemu-devel@nongnu.org; Wed, 12 Apr 2017 04:29:16 -0400 Received: from szxga03-in.huawei.com ([45.249.212.189]:3364 helo=dggrg03-dlp.huawei.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1cyDeK-0005wa-1R for qemu-devel@nongnu.org; Wed, 12 Apr 2017 04:29:12 -0400 References: <20170412161704.00003875@gmail.com> From: "Herongguang (Stephen)" Message-ID: <58EDE525.4050905@huawei.com> Date: Wed, 12 Apr 2017 16:28:21 +0800 MIME-Version: 1.0 In-Reply-To: <20170412161704.00003875@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit 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: Alexey G , 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 On 2017/4/12 14:17, Alexey G wrote: > 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: >>>> On Mon, 10 Apr 2017, Stefano Stabellini wrote: >>>>> On Mon, 10 Apr 2017, hrg wrote: >>>>>> On Sun, Apr 9, 2017 at 11:55 PM, hrg wrote: >>>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> In xen_map_cache_unlocked(), map to guest memory maybe in >>>>>>>> entry->next instead of first level entry (if map to rom other than >>>>>>>> guest memory comes first), while in xen_invalidate_map_cache(), >>>>>>>> when VM ballooned out memory, qemu did not invalidate cache entries >>>>>>>> in linked list(entry->next), so when VM balloon back in memory, >>>>>>>> gfns probably mapped to different mfns, thus if guest asks device >>>>>>>> to DMA to these GPA, qemu may DMA to stale MFNs. >>>>>>>> >>>>>>>> So I think in xen_invalidate_map_cache() linked lists should also be >>>>>>>> checked and invalidated. >>>>>>>> >>>>>>>> What’s your opinion? Is this a bug? Is my analyze correct? >>>>> >>>>> 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? >>>> >>>> I spoke too soon. In the regular case there should be no locked mappings >>>> when xen_invalidate_map_cache is called (see the DPRINTF warning at the >>>> beginning of the functions). Without locked mappings, there should never >>>> be more than one element in each list (see xen_map_cache_unlocked: >>>> entry->lock == true is a necessary condition to append a new entry 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 DPRINTK >>>> by turning it into a printf or by defininig MAPCACHE_DEBUG. >>> >>> 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. >> >> 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. >> >> 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. >> >> 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 MapCacheEntry'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. And > 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 achieve > 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 block I/O > DMA should be enough I think. > Yes, I think there may be other bugs lurking, considering the complexity, though we need to reproduce it if we want to delve into it. From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Herongguang (Stephen)" Subject: Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? Date: Wed, 12 Apr 2017 16:28:21 +0800 Message-ID: <58EDE525.4050905@huawei.com> References: <20170412161704.00003875@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170412161704.00003875@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Alexey G , 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, hrg List-Id: xen-devel@lists.xenproject.org CgpPbiAyMDE3LzQvMTIgMTQ6MTcsIEFsZXhleSBHIHdyb3RlOgo+IE9uIFR1ZSwgMTEgQXByIDIw MTcgMTU6MzI6MDkgLTA3MDAgKFBEVCkKPiBTdGVmYW5vIFN0YWJlbGxpbmkgPHNzdGFiZWxsaW5p QGtlcm5lbC5vcmc+IHdyb3RlOgo+Cj4+IE9uIFR1ZSwgMTEgQXByIDIwMTcsIGhyZyB3cm90ZToK Pj4+IE9uIFR1ZSwgQXByIDExLCAyMDE3IGF0IDM6NTAgQU0sIFN0ZWZhbm8gU3RhYmVsbGluaQo+ Pj4gPHNzdGFiZWxsaW5pQGtlcm5lbC5vcmc+IHdyb3RlOgo+Pj4+IE9uIE1vbiwgMTAgQXByIDIw MTcsIFN0ZWZhbm8gU3RhYmVsbGluaSB3cm90ZToKPj4+Pj4gT24gTW9uLCAxMCBBcHIgMjAxNywg aHJnIHdyb3RlOgo+Pj4+Pj4gT24gU3VuLCBBcHIgOSwgMjAxNyBhdCAxMTo1NSBQTSwgaHJnIDxo cmdzdGVwaGVuQGdtYWlsLmNvbT4gd3JvdGU6Cj4+Pj4+Pj4gT24gU3VuLCBBcHIgOSwgMjAxNyBh dCAxMTo1MiBQTSwgaHJnIDxocmdzdGVwaGVuQGdtYWlsLmNvbT4gd3JvdGU6Cj4+Pj4+Pj4+IEhp LAo+Pj4+Pj4+Pgo+Pj4+Pj4+PiBJbiB4ZW5fbWFwX2NhY2hlX3VubG9ja2VkKCksIG1hcCB0byBn dWVzdCBtZW1vcnkgbWF5YmUgaW4KPj4+Pj4+Pj4gZW50cnktPm5leHQgaW5zdGVhZCBvZiBmaXJz dCBsZXZlbCBlbnRyeSAoaWYgbWFwIHRvIHJvbSBvdGhlciB0aGFuCj4+Pj4+Pj4+IGd1ZXN0IG1l bW9yeSBjb21lcyBmaXJzdCksIHdoaWxlIGluIHhlbl9pbnZhbGlkYXRlX21hcF9jYWNoZSgpLAo+ Pj4+Pj4+PiB3aGVuIFZNIGJhbGxvb25lZCBvdXQgbWVtb3J5LCBxZW11IGRpZCBub3QgaW52YWxp ZGF0ZSBjYWNoZSBlbnRyaWVzCj4+Pj4+Pj4+IGluIGxpbmtlZCBsaXN0KGVudHJ5LT5uZXh0KSwg c28gd2hlbiBWTSBiYWxsb29uIGJhY2sgaW4gbWVtb3J5LAo+Pj4+Pj4+PiBnZm5zIHByb2JhYmx5 IG1hcHBlZCB0byBkaWZmZXJlbnQgbWZucywgdGh1cyBpZiBndWVzdCBhc2tzIGRldmljZQo+Pj4+ Pj4+PiB0byBETUEgdG8gdGhlc2UgR1BBLCBxZW11IG1heSBETUEgdG8gc3RhbGUgTUZOcy4KPj4+ Pj4+Pj4KPj4+Pj4+Pj4gU28gSSB0aGluayBpbiB4ZW5faW52YWxpZGF0ZV9tYXBfY2FjaGUoKSBs aW5rZWQgbGlzdHMgc2hvdWxkIGFsc28gYmUKPj4+Pj4+Pj4gY2hlY2tlZCBhbmQgaW52YWxpZGF0 ZWQuCj4+Pj4+Pj4+Cj4+Pj4+Pj4+IFdoYXTigJlzIHlvdXIgb3Bpbmlvbj8gSXMgdGhpcyBhIGJ1 Zz8gSXMgbXkgYW5hbHl6ZSBjb3JyZWN0Pwo+Pj4+Pgo+Pj4+PiBZZXMsIHlvdSBhcmUgcmlnaHQu IFdlIG5lZWQgdG8gZ28gdGhyb3VnaCB0aGUgbGlzdCBmb3IgZWFjaCBlbGVtZW50IG9mCj4+Pj4+ IHRoZSBhcnJheSBpbiB4ZW5faW52YWxpZGF0ZV9tYXBfY2FjaGUuIENhbiB5b3UgY29tZSB1cCB3 aXRoIGEgcGF0Y2g/Cj4+Pj4KPj4+PiBJIHNwb2tlIHRvbyBzb29uLiBJbiB0aGUgcmVndWxhciBj YXNlIHRoZXJlIHNob3VsZCBiZSBubyBsb2NrZWQgbWFwcGluZ3MKPj4+PiB3aGVuIHhlbl9pbnZh bGlkYXRlX21hcF9jYWNoZSBpcyBjYWxsZWQgKHNlZSB0aGUgRFBSSU5URiB3YXJuaW5nIGF0IHRo ZQo+Pj4+IGJlZ2lubmluZyBvZiB0aGUgZnVuY3Rpb25zKS4gV2l0aG91dCBsb2NrZWQgbWFwcGlu Z3MsIHRoZXJlIHNob3VsZCBuZXZlcgo+Pj4+IGJlIG1vcmUgdGhhbiBvbmUgZWxlbWVudCBpbiBl YWNoIGxpc3QgKHNlZSB4ZW5fbWFwX2NhY2hlX3VubG9ja2VkOgo+Pj4+IGVudHJ5LT5sb2NrID09 IHRydWUgaXMgYSBuZWNlc3NhcnkgY29uZGl0aW9uIHRvIGFwcGVuZCBhIG5ldyBlbnRyeSB0bwo+ Pj4+IHRoZSBsaXN0LCBvdGhlcndpc2UgaXQgaXMganVzdCByZW1hcHBlZCkuCj4+Pj4KPj4+PiBD YW4geW91IGNvbmZpcm0gdGhhdCB3aGF0IHlvdSBhcmUgc2VlaW5nIGFyZSBsb2NrZWQgbWFwcGlu Z3MKPj4+PiB3aGVuIHhlbl9pbnZhbGlkYXRlX21hcF9jYWNoZSBpcyBjYWxsZWQ/IFRvIGZpbmQg b3V0LCBlbmFibGUgdGhlIERQUklOVEsKPj4+PiBieSB0dXJuaW5nIGl0IGludG8gYSBwcmludGYg b3IgYnkgZGVmaW5pbmlnIE1BUENBQ0hFX0RFQlVHLgo+Pj4KPj4+IEluIGZhY3QsIEkgdGhpbmsg dGhlIERQUklOVEYgYWJvdmUgaXMgaW5jb3JyZWN0IHRvby4gSW4KPj4+IHBjaV9hZGRfb3B0aW9u X3JvbSgpLCBydGw4MTM5IHJvbSBpcyBsb2NrZWQgbWFwcGVkIGluCj4+PiBwY2lfYWRkX29wdGlv bl9yb20tPm1lbW9yeV9yZWdpb25fZ2V0X3JhbV9wdHIgKGFmdGVyCj4+PiBtZW1vcnlfcmVnaW9u X2luaXRfcmFtKS4gU28gYWN0dWFsbHkgSSB0aGluayB3ZSBzaG91bGQgcmVtb3ZlIHRoZQo+Pj4g RFBSSU5URiB3YXJuaW5nIGFzIGl0IGlzIG5vcm1hbC4KPj4KPj4gTGV0IG1lIGV4cGxhaW4gd2h5 IHRoZSBEUFJJTlRGIHdhcm5pbmcgaXMgdGhlcmU6IGVtdWxhdGVkIGRtYSBvcGVyYXRpb25zCj4+ IGNhbiBpbnZvbHZlIGxvY2tlZCBtYXBwaW5ncy4gT25jZSBhIGRtYSBvcGVyYXRpb24gY29tcGxl dGVzLCB0aGUgcmVsYXRlZAo+PiBtYXBwaW5nIGlzIHVubG9ja2VkIGFuZCBjYW4gYmUgc2FmZWx5 IGRlc3Ryb3llZC4gQnV0IGlmIHdlIGRlc3Ryb3kgYQo+PiBsb2NrZWQgbWFwcGluZyBpbiB4ZW5f aW52YWxpZGF0ZV9tYXBfY2FjaGUsIHdoaWxlIGEgZG1hIGlzIHN0aWxsCj4+IG9uZ29pbmcsIFFF TVUgd2lsbCBjcmFzaC4gV2UgY2Fubm90IGhhbmRsZSB0aGF0IGNhc2UuCj4+Cj4+IEhvd2V2ZXIs IHRoZSBzY2VuYXJpbyB5b3UgZGVzY3JpYmVkIGlzIGRpZmZlcmVudC4gSXQgaGFzIG5vdGhpbmcg dG8gZG8KPj4gd2l0aCBETUEuIEl0IGxvb2tzIGxpa2UgcGNpX2FkZF9vcHRpb25fcm9tIGNhbGxz Cj4+IG1lbW9yeV9yZWdpb25fZ2V0X3JhbV9wdHIgdG8gbWFwIHRoZSBydGw4MTM5IHJvbS4gVGhl IG1hcHBpbmcgaXMgYQo+PiBsb2NrZWQgbWFwcGluZyBhbmQgaXQgaXMgbmV2ZXIgdW5sb2NrZWQg b3IgZGVzdHJveWVkLgo+Pgo+PiBJdCBsb29rcyBsaWtlICJwdHIiIGlzIG5vdCB1c2VkIGFmdGVy IHBjaV9hZGRfb3B0aW9uX3JvbSByZXR1cm5zLiBEb2VzCj4+IHRoZSBhcHBlbmQgcGF0Y2ggZml4 IHRoZSBwcm9ibGVtIHlvdSBhcmUgc2VlaW5nPyBGb3IgdGhlIHByb3BlciBmaXgsIEkKPj4gdGhp bmsgd2UgcHJvYmFibHkgbmVlZCBzb21lIHNvcnQgb2YgbWVtb3J5X3JlZ2lvbl91bm1hcCB3cmFw cGVyIG9yIG1heWJlCj4+IGEgY2FsbCB0byBhZGRyZXNzX3NwYWNlX3VubWFwLgo+Cj4gSG1tLCBm b3Igc29tZSByZWFzb24gbXkgbWVzc2FnZSB0byB0aGUgWGVuLWRldmVsIGxpc3QgZ290IHJlamVj dGVkIGJ1dCB3YXMgc2VudAo+IHRvIFFlbXUtZGV2ZWwgaW5zdGVhZCwgd2l0aG91dCBhbnkgbm90 aWNlLiBTb3JyeSBpZiBJJ20gbWlzc2luZyBzb21ldGhpbmcKPiBvYnZpb3VzIGFzIGEgbGlzdCBu ZXdiaWUuCj4KPiBTdGVmYW5vLCBocmcsCj4KPiBUaGVyZSBpcyBhbiBpc3N1ZSB3aXRoIGluY29u c2lzdGVuY3kgYmV0d2VlbiB0aGUgbGlzdCBvZiBub3JtYWwgTWFwQ2FjaGVFbnRyeSdzCj4gYW5k IHRoZWlyICdyZXZlcnNlJyBjb3VudGVycGFydHMgLSBNYXBDYWNoZVJldidzIGluIGxvY2tlZF9l bnRyaWVzLgo+IFdoZW4gYmFkIHNpdHVhdGlvbiBoYXBwZW5zLCB0aGVyZSBhcmUgbXVsdGlwbGUg KGxvY2tlZCkgTWFwQ2FjaGVFbnRyeQo+IGVudHJpZXMgaW4gdGhlIGJ1Y2tldCdzIGxpbmtlZCBs aXN0IGFsb25nIHdpdGggYSBudW1iZXIgb2YgTWFwQ2FjaGVSZXYncy4gQW5kCj4gd2hlbiBpdCBj b21lcyB0byBhIHJldmVyc2UgbG9va3VwLCB4ZW4tbWFwY2FjaGUgcGlja3MgdGhlIHdyb25nIGVu dHJ5IGZyb20gdGhlCj4gZmlyc3QgbGlzdCBhbmQgY2FsY3VsYXRlcyBhIHdyb25nIHBvaW50ZXIg ZnJvbSBpdCB3aGljaCBtYXkgdGhlbiBiZSBjYXVnaHQgd2l0aAo+IHRoZSAiQmFkIFJBTSBvZmZz ZXQiIGNoZWNrIChvciBub3QpLiBNYXBjYWNoZSBpbnZhbGlkYXRpb24gbWlnaHQgYmUgcmVsYXRl ZCB0bwo+IHRoaXMgaXNzdWUgYXMgd2VsbCBJIHRoaW5rLgo+Cj4gSSdsbCB0cnkgdG8gcHJvdmlk ZSBhIHRlc3QgY29kZSB3aGljaCBjYW4gcmVwcm9kdWNlIHRoZSBpc3N1ZSBmcm9tIHRoZQo+IGd1 ZXN0IHNpZGUgdXNpbmcgYW4gZW11bGF0ZWQgSURFIGNvbnRyb2xsZXIsIHRob3VnaCBpdCdzIG11 Y2ggc2ltcGxlciB0byBhY2hpZXZlCj4gdGhpcyByZXN1bHQgd2l0aCBhbiBBSENJIGNvbnRyb2xs ZXIgdXNpbmcgbXVsdGlwbGUgTkNRIEkvTyBjb21tYW5kcy4gU28gZmFyIEkndmUKPiBzZWVuIHRo aXMgaXNzdWUgb25seSB3aXRoIFdpbmRvd3MgNyAoYW5kIGFib3ZlKSBndWVzdCBvbiBBSENJLCBi dXQgYW55IGJsb2NrIEkvTwo+IERNQSBzaG91bGQgYmUgZW5vdWdoIEkgdGhpbmsuCj4KClllcywg SSB0aGluayB0aGVyZSBtYXkgYmUgb3RoZXIgYnVncyBsdXJraW5nLCBjb25zaWRlcmluZyB0aGUg Y29tcGxleGl0eSwgdGhvdWdoIHdlIG5lZWQgdG8gcmVwcm9kdWNlIGl0IGlmIHdlIHdhbnQgdG8g ZGVsdmUgaW50byBpdC4KCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fXwpYZW4tZGV2ZWwgbWFpbGluZyBsaXN0Clhlbi1kZXZlbEBsaXN0cy54ZW4ub3JnCmh0 dHBzOi8vbGlzdHMueGVuLm9yZy94ZW4tZGV2ZWwK