From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cyXdO-00068t-61 for qemu-devel@nongnu.org; Thu, 13 Apr 2017 01:49:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cyXdJ-0007BQ-BL for qemu-devel@nongnu.org; Thu, 13 Apr 2017 01:49:34 -0400 Received: from szxga03-in.huawei.com ([45.249.212.189]:3373 helo=dggrg03-dlp.huawei.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1cyXdI-0007Ag-GE for qemu-devel@nongnu.org; Thu, 13 Apr 2017 01:49:29 -0400 References: <58EDE1E4.2090003@huawei.com> From: "Herongguang (Stephen)" Message-ID: <58EF1102.2080802@huawei.com> Date: Thu, 13 Apr 2017 13:47:46 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-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 , Anthony PERARD , xen-devel@lists.xensource.com, qemu-devel@nongnu.org, Jun Nakajima , Alexander Graf , xen-devel@lists.xenproject.org, xen-devel@lists.xen.org, wangxinxin.wang@huawei.com On 2017/4/13 7:51, Stefano Stabellini wrote: > On Wed, 12 Apr 2017, Herongguang (Stephen) wrote: >> On 2017/4/12 6:32, 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. >> >> Yes, I think so, maybe this is the proper way to fix this. > > Would you be up for sending a proper patch and testing it? We cannot call > xen_invalidate_map_cache_entry directly from pci.c though, it would need > to be one of the other functions like address_space_unmap for example. > Yes, I will look into this. > >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index e6b08e1..04f98b7 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -2242,6 +2242,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool >>> is_default_rom, >>> } >>> pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom); >>> + xen_invalidate_map_cache_entry(ptr); >>> } >>> static void pci_del_option_rom(PCIDevice *pdev) 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: Thu, 13 Apr 2017 13:47:46 +0800 Message-ID: <58EF1102.2080802@huawei.com> References: <58EDE1E4.2090003@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" 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, Alexander Graf , qemu-devel@nongnu.org, Jun Nakajima , Anthony PERARD , xen-devel@lists.xenproject.org, xen-devel@lists.xen.org, hrg List-Id: xen-devel@lists.xenproject.org CgpPbiAyMDE3LzQvMTMgNzo1MSwgU3RlZmFubyBTdGFiZWxsaW5pIHdyb3RlOgo+IE9uIFdlZCwg MTIgQXByIDIwMTcsIEhlcm9uZ2d1YW5nIChTdGVwaGVuKSB3cm90ZToKPj4gT24gMjAxNy80LzEy IDY6MzIsIFN0ZWZhbm8gU3RhYmVsbGluaSB3cm90ZToKPj4+IE9uIFR1ZSwgMTEgQXByIDIwMTcs IGhyZyB3cm90ZToKPj4+PiBPbiBUdWUsIEFwciAxMSwgMjAxNyBhdCAzOjUwIEFNLCBTdGVmYW5v IFN0YWJlbGxpbmkKPj4+PiA8c3N0YWJlbGxpbmlAa2VybmVsLm9yZz4gd3JvdGU6Cj4+Pj4+IE9u IE1vbiwgMTAgQXByIDIwMTcsIFN0ZWZhbm8gU3RhYmVsbGluaSB3cm90ZToKPj4+Pj4+IE9uIE1v biwgMTAgQXByIDIwMTcsIGhyZyB3cm90ZToKPj4+Pj4+PiBPbiBTdW4sIEFwciA5LCAyMDE3IGF0 IDExOjU1IFBNLCBocmcgPGhyZ3N0ZXBoZW5AZ21haWwuY29tPiB3cm90ZToKPj4+Pj4+Pj4gT24g U3VuLCBBcHIgOSwgMjAxNyBhdCAxMTo1MiBQTSwgaHJnIDxocmdzdGVwaGVuQGdtYWlsLmNvbT4g d3JvdGU6Cj4+Pj4+Pj4+PiBIaSwKPj4+Pj4+Pj4+Cj4+Pj4+Pj4+PiBJbiB4ZW5fbWFwX2NhY2hl X3VubG9ja2VkKCksIG1hcCB0byBndWVzdCBtZW1vcnkgbWF5YmUgaW4KPj4+Pj4+Pj4+IGVudHJ5 LT5uZXh0Cj4+Pj4+Pj4+PiBpbnN0ZWFkIG9mIGZpcnN0IGxldmVsIGVudHJ5IChpZiBtYXAgdG8g cm9tIG90aGVyIHRoYW4gZ3Vlc3QKPj4+Pj4+Pj4+IG1lbW9yeQo+Pj4+Pj4+Pj4gY29tZXMgZmly c3QpLCB3aGlsZSBpbiB4ZW5faW52YWxpZGF0ZV9tYXBfY2FjaGUoKSwgd2hlbiBWTQo+Pj4+Pj4+ Pj4gYmFsbG9vbmVkCj4+Pj4+Pj4+PiBvdXQgbWVtb3J5LCBxZW11IGRpZCBub3QgaW52YWxpZGF0 ZSBjYWNoZSBlbnRyaWVzIGluIGxpbmtlZAo+Pj4+Pj4+Pj4gbGlzdChlbnRyeS0+bmV4dCksIHNv IHdoZW4gVk0gYmFsbG9vbiBiYWNrIGluIG1lbW9yeSwgZ2Zucwo+Pj4+Pj4+Pj4gcHJvYmFibHkK Pj4+Pj4+Pj4+IG1hcHBlZCB0byBkaWZmZXJlbnQgbWZucywgdGh1cyBpZiBndWVzdCBhc2tzIGRl dmljZSB0byBETUEgdG8KPj4+Pj4+Pj4+IHRoZXNlCj4+Pj4+Pj4+PiBHUEEsIHFlbXUgbWF5IERN QSB0byBzdGFsZSBNRk5zLgo+Pj4+Pj4+Pj4KPj4+Pj4+Pj4+IFNvIEkgdGhpbmsgaW4geGVuX2lu dmFsaWRhdGVfbWFwX2NhY2hlKCkgbGlua2VkIGxpc3RzIHNob3VsZAo+Pj4+Pj4+Pj4gYWxzbyBi ZQo+Pj4+Pj4+Pj4gY2hlY2tlZCBhbmQgaW52YWxpZGF0ZWQuCj4+Pj4+Pj4+Pgo+Pj4+Pj4+Pj4g V2hhdOKAmXMgeW91ciBvcGluaW9uPyBJcyB0aGlzIGEgYnVnPyBJcyBteSBhbmFseXplIGNvcnJl Y3Q/Cj4+Pj4+PiBZZXMsIHlvdSBhcmUgcmlnaHQuIFdlIG5lZWQgdG8gZ28gdGhyb3VnaCB0aGUg bGlzdCBmb3IgZWFjaCBlbGVtZW50IG9mCj4+Pj4+PiB0aGUgYXJyYXkgaW4geGVuX2ludmFsaWRh dGVfbWFwX2NhY2hlLiBDYW4geW91IGNvbWUgdXAgd2l0aCBhIHBhdGNoPwo+Pj4+PiBJIHNwb2tl IHRvbyBzb29uLiBJbiB0aGUgcmVndWxhciBjYXNlIHRoZXJlIHNob3VsZCBiZSBubyBsb2NrZWQg bWFwcGluZ3MKPj4+Pj4gd2hlbiB4ZW5faW52YWxpZGF0ZV9tYXBfY2FjaGUgaXMgY2FsbGVkIChz ZWUgdGhlIERQUklOVEYgd2FybmluZyBhdCB0aGUKPj4+Pj4gYmVnaW5uaW5nIG9mIHRoZSBmdW5j dGlvbnMpLiBXaXRob3V0IGxvY2tlZCBtYXBwaW5ncywgdGhlcmUgc2hvdWxkIG5ldmVyCj4+Pj4+ IGJlIG1vcmUgdGhhbiBvbmUgZWxlbWVudCBpbiBlYWNoIGxpc3QgKHNlZSB4ZW5fbWFwX2NhY2hl X3VubG9ja2VkOgo+Pj4+PiBlbnRyeS0+bG9jayA9PSB0cnVlIGlzIGEgbmVjZXNzYXJ5IGNvbmRp dGlvbiB0byBhcHBlbmQgYSBuZXcgZW50cnkgdG8KPj4+Pj4gdGhlIGxpc3QsIG90aGVyd2lzZSBp dCBpcyBqdXN0IHJlbWFwcGVkKS4KPj4+Pj4KPj4+Pj4gQ2FuIHlvdSBjb25maXJtIHRoYXQgd2hh dCB5b3UgYXJlIHNlZWluZyBhcmUgbG9ja2VkIG1hcHBpbmdzCj4+Pj4+IHdoZW4geGVuX2ludmFs aWRhdGVfbWFwX2NhY2hlIGlzIGNhbGxlZD8gVG8gZmluZCBvdXQsIGVuYWJsZSB0aGUgRFBSSU5U Swo+Pj4+PiBieSB0dXJuaW5nIGl0IGludG8gYSBwcmludGYgb3IgYnkgZGVmaW5pbmlnIE1BUENB Q0hFX0RFQlVHLgo+Pj4+IEluIGZhY3QsIEkgdGhpbmsgdGhlIERQUklOVEYgYWJvdmUgaXMgaW5j b3JyZWN0IHRvby4gSW4KPj4+PiBwY2lfYWRkX29wdGlvbl9yb20oKSwgcnRsODEzOSByb20gaXMg bG9ja2VkIG1hcHBlZCBpbgo+Pj4+IHBjaV9hZGRfb3B0aW9uX3JvbS0+bWVtb3J5X3JlZ2lvbl9n ZXRfcmFtX3B0ciAoYWZ0ZXIKPj4+PiBtZW1vcnlfcmVnaW9uX2luaXRfcmFtKS4gU28gYWN0dWFs bHkgSSB0aGluayB3ZSBzaG91bGQgcmVtb3ZlIHRoZQo+Pj4+IERQUklOVEYgd2FybmluZyBhcyBp dCBpcyBub3JtYWwuCj4+PiBMZXQgbWUgZXhwbGFpbiB3aHkgdGhlIERQUklOVEYgd2FybmluZyBp cyB0aGVyZTogZW11bGF0ZWQgZG1hIG9wZXJhdGlvbnMKPj4+IGNhbiBpbnZvbHZlIGxvY2tlZCBt YXBwaW5ncy4gT25jZSBhIGRtYSBvcGVyYXRpb24gY29tcGxldGVzLCB0aGUgcmVsYXRlZAo+Pj4g bWFwcGluZyBpcyB1bmxvY2tlZCBhbmQgY2FuIGJlIHNhZmVseSBkZXN0cm95ZWQuIEJ1dCBpZiB3 ZSBkZXN0cm95IGEKPj4+IGxvY2tlZCBtYXBwaW5nIGluIHhlbl9pbnZhbGlkYXRlX21hcF9jYWNo ZSwgd2hpbGUgYSBkbWEgaXMgc3RpbGwKPj4+IG9uZ29pbmcsIFFFTVUgd2lsbCBjcmFzaC4gV2Ug Y2Fubm90IGhhbmRsZSB0aGF0IGNhc2UuCj4+Pgo+Pj4gSG93ZXZlciwgdGhlIHNjZW5hcmlvIHlv dSBkZXNjcmliZWQgaXMgZGlmZmVyZW50LiBJdCBoYXMgbm90aGluZyB0byBkbwo+Pj4gd2l0aCBE TUEuIEl0IGxvb2tzIGxpa2UgcGNpX2FkZF9vcHRpb25fcm9tIGNhbGxzCj4+PiBtZW1vcnlfcmVn aW9uX2dldF9yYW1fcHRyIHRvIG1hcCB0aGUgcnRsODEzOSByb20uIFRoZSBtYXBwaW5nIGlzIGEK Pj4+IGxvY2tlZCBtYXBwaW5nIGFuZCBpdCBpcyBuZXZlciB1bmxvY2tlZCBvciBkZXN0cm95ZWQu Cj4+Pgo+Pj4gSXQgbG9va3MgbGlrZSAicHRyIiBpcyBub3QgdXNlZCBhZnRlciBwY2lfYWRkX29w dGlvbl9yb20gcmV0dXJucy4gRG9lcwo+Pj4gdGhlIGFwcGVuZCBwYXRjaCBmaXggdGhlIHByb2Js ZW0geW91IGFyZSBzZWVpbmc/IEZvciB0aGUgcHJvcGVyIGZpeCwgSQo+Pj4gdGhpbmsgd2UgcHJv YmFibHkgbmVlZCBzb21lIHNvcnQgb2YgbWVtb3J5X3JlZ2lvbl91bm1hcCB3cmFwcGVyIG9yIG1h eWJlCj4+PiBhIGNhbGwgdG8gYWRkcmVzc19zcGFjZV91bm1hcC4KPj4KPj4gWWVzLCBJIHRoaW5r IHNvLCBtYXliZSB0aGlzIGlzIHRoZSBwcm9wZXIgd2F5IHRvIGZpeCB0aGlzLgo+Cj4gV291bGQg eW91IGJlIHVwIGZvciBzZW5kaW5nIGEgcHJvcGVyIHBhdGNoIGFuZCB0ZXN0aW5nIGl0PyBXZSBj YW5ub3QgY2FsbAo+IHhlbl9pbnZhbGlkYXRlX21hcF9jYWNoZV9lbnRyeSBkaXJlY3RseSBmcm9t IHBjaS5jIHRob3VnaCwgaXQgd291bGQgbmVlZAo+IHRvIGJlIG9uZSBvZiB0aGUgb3RoZXIgZnVu Y3Rpb25zIGxpa2UgYWRkcmVzc19zcGFjZV91bm1hcCBmb3IgZXhhbXBsZS4KPgoKClllcywgSSB3 aWxsIGxvb2sgaW50byB0aGlzLgoKPgo+Pj4gZGlmZiAtLWdpdCBhL2h3L3BjaS9wY2kuYyBiL2h3 L3BjaS9wY2kuYwo+Pj4gaW5kZXggZTZiMDhlMS4uMDRmOThiNyAxMDA2NDQKPj4+IC0tLSBhL2h3 L3BjaS9wY2kuYwo+Pj4gKysrIGIvaHcvcGNpL3BjaS5jCj4+PiBAQCAtMjI0Miw2ICsyMjQyLDcg QEAgc3RhdGljIHZvaWQgcGNpX2FkZF9vcHRpb25fcm9tKFBDSURldmljZSAqcGRldiwgYm9vbAo+ Pj4gaXNfZGVmYXVsdF9yb20sCj4+PiAgICAgICAgfQo+Pj4gICAgICAgICAgcGNpX3JlZ2lzdGVy X2JhcihwZGV2LCBQQ0lfUk9NX1NMT1QsIDAsICZwZGV2LT5yb20pOwo+Pj4gKyAgICB4ZW5faW52 YWxpZGF0ZV9tYXBfY2FjaGVfZW50cnkocHRyKTsKPj4+ICAgIH0KPj4+ICAgICAgc3RhdGljIHZv aWQgcGNpX2RlbF9vcHRpb25fcm9tKFBDSURldmljZSAqcGRldikKCgpfX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpYZW4tZGV2ZWwgbWFpbGluZyBsaXN0Clhl bi1kZXZlbEBsaXN0cy54ZW4ub3JnCmh0dHBzOi8vbGlzdHMueGVuLm9yZy94ZW4tZGV2ZWwK