From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cyDRF-0002T1-KN for qemu-devel@nongnu.org; Wed, 12 Apr 2017 04:15:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cyDRB-0001px-Hl for qemu-devel@nongnu.org; Wed, 12 Apr 2017 04:15:41 -0400 Received: from szxga03-in.huawei.com ([45.249.212.189]:3363 helo=dggrg03-dlp.huawei.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1cyDRA-0001mK-Mp for qemu-devel@nongnu.org; Wed, 12 Apr 2017 04:15:37 -0400 References: From: "Herongguang (Stephen)" Message-ID: <58EDE1E4.2090003@huawei.com> Date: Wed, 12 Apr 2017 16:14:28 +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 , hrg Cc: 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/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. > > > 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: Wed, 12 Apr 2017 16:14:28 +0800 Message-ID: <58EDE1E4.2090003@huawei.com> References: 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 , hrg 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 List-Id: xen-devel@lists.xenproject.org CgpPbiAyMDE3LzQvMTIgNjozMiwgU3RlZmFubyBTdGFiZWxsaW5pIHdyb3RlOgo+IE9uIFR1ZSwg MTEgQXByIDIwMTcsIGhyZyB3cm90ZToKPj4gT24gVHVlLCBBcHIgMTEsIDIwMTcgYXQgMzo1MCBB TSwgU3RlZmFubyBTdGFiZWxsaW5pCj4+IDxzc3RhYmVsbGluaUBrZXJuZWwub3JnPiB3cm90ZToK Pj4+IE9uIE1vbiwgMTAgQXByIDIwMTcsIFN0ZWZhbm8gU3RhYmVsbGluaSB3cm90ZToKPj4+PiBP biBNb24sIDEwIEFwciAyMDE3LCBocmcgd3JvdGU6Cj4+Pj4+IE9uIFN1biwgQXByIDksIDIwMTcg YXQgMTE6NTUgUE0sIGhyZyA8aHJnc3RlcGhlbkBnbWFpbC5jb20+IHdyb3RlOgo+Pj4+Pj4gT24g U3VuLCBBcHIgOSwgMjAxNyBhdCAxMTo1MiBQTSwgaHJnIDxocmdzdGVwaGVuQGdtYWlsLmNvbT4g d3JvdGU6Cj4+Pj4+Pj4gSGksCj4+Pj4+Pj4KPj4+Pj4+PiBJbiB4ZW5fbWFwX2NhY2hlX3VubG9j a2VkKCksIG1hcCB0byBndWVzdCBtZW1vcnkgbWF5YmUgaW4gZW50cnktPm5leHQKPj4+Pj4+PiBp bnN0ZWFkIG9mIGZpcnN0IGxldmVsIGVudHJ5IChpZiBtYXAgdG8gcm9tIG90aGVyIHRoYW4gZ3Vl c3QgbWVtb3J5Cj4+Pj4+Pj4gY29tZXMgZmlyc3QpLCB3aGlsZSBpbiB4ZW5faW52YWxpZGF0ZV9t YXBfY2FjaGUoKSwgd2hlbiBWTSBiYWxsb29uZWQKPj4+Pj4+PiBvdXQgbWVtb3J5LCBxZW11IGRp ZCBub3QgaW52YWxpZGF0ZSBjYWNoZSBlbnRyaWVzIGluIGxpbmtlZAo+Pj4+Pj4+IGxpc3QoZW50 cnktPm5leHQpLCBzbyB3aGVuIFZNIGJhbGxvb24gYmFjayBpbiBtZW1vcnksIGdmbnMgcHJvYmFi bHkKPj4+Pj4+PiBtYXBwZWQgdG8gZGlmZmVyZW50IG1mbnMsIHRodXMgaWYgZ3Vlc3QgYXNrcyBk ZXZpY2UgdG8gRE1BIHRvIHRoZXNlCj4+Pj4+Pj4gR1BBLCBxZW11IG1heSBETUEgdG8gc3RhbGUg TUZOcy4KPj4+Pj4+Pgo+Pj4+Pj4+IFNvIEkgdGhpbmsgaW4geGVuX2ludmFsaWRhdGVfbWFwX2Nh Y2hlKCkgbGlua2VkIGxpc3RzIHNob3VsZCBhbHNvIGJlCj4+Pj4+Pj4gY2hlY2tlZCBhbmQgaW52 YWxpZGF0ZWQuCj4+Pj4+Pj4KPj4+Pj4+PiBXaGF04oCZcyB5b3VyIG9waW5pb24/IElzIHRoaXMg YSBidWc/IElzIG15IGFuYWx5emUgY29ycmVjdD8KPj4+PiBZZXMsIHlvdSBhcmUgcmlnaHQuIFdl IG5lZWQgdG8gZ28gdGhyb3VnaCB0aGUgbGlzdCBmb3IgZWFjaCBlbGVtZW50IG9mCj4+Pj4gdGhl IGFycmF5IGluIHhlbl9pbnZhbGlkYXRlX21hcF9jYWNoZS4gQ2FuIHlvdSBjb21lIHVwIHdpdGgg YSBwYXRjaD8KPj4+IEkgc3Bva2UgdG9vIHNvb24uIEluIHRoZSByZWd1bGFyIGNhc2UgdGhlcmUg c2hvdWxkIGJlIG5vIGxvY2tlZCBtYXBwaW5ncwo+Pj4gd2hlbiB4ZW5faW52YWxpZGF0ZV9tYXBf Y2FjaGUgaXMgY2FsbGVkIChzZWUgdGhlIERQUklOVEYgd2FybmluZyBhdCB0aGUKPj4+IGJlZ2lu bmluZyBvZiB0aGUgZnVuY3Rpb25zKS4gV2l0aG91dCBsb2NrZWQgbWFwcGluZ3MsIHRoZXJlIHNo b3VsZCBuZXZlcgo+Pj4gYmUgbW9yZSB0aGFuIG9uZSBlbGVtZW50IGluIGVhY2ggbGlzdCAoc2Vl IHhlbl9tYXBfY2FjaGVfdW5sb2NrZWQ6Cj4+PiBlbnRyeS0+bG9jayA9PSB0cnVlIGlzIGEgbmVj ZXNzYXJ5IGNvbmRpdGlvbiB0byBhcHBlbmQgYSBuZXcgZW50cnkgdG8KPj4+IHRoZSBsaXN0LCBv dGhlcndpc2UgaXQgaXMganVzdCByZW1hcHBlZCkuCj4+Pgo+Pj4gQ2FuIHlvdSBjb25maXJtIHRo YXQgd2hhdCB5b3UgYXJlIHNlZWluZyBhcmUgbG9ja2VkIG1hcHBpbmdzCj4+PiB3aGVuIHhlbl9p bnZhbGlkYXRlX21hcF9jYWNoZSBpcyBjYWxsZWQ/IFRvIGZpbmQgb3V0LCBlbmFibGUgdGhlIERQ UklOVEsKPj4+IGJ5IHR1cm5pbmcgaXQgaW50byBhIHByaW50ZiBvciBieSBkZWZpbmluaWcgTUFQ Q0FDSEVfREVCVUcuCj4+IEluIGZhY3QsIEkgdGhpbmsgdGhlIERQUklOVEYgYWJvdmUgaXMgaW5j b3JyZWN0IHRvby4gSW4KPj4gcGNpX2FkZF9vcHRpb25fcm9tKCksIHJ0bDgxMzkgcm9tIGlzIGxv Y2tlZCBtYXBwZWQgaW4KPj4gcGNpX2FkZF9vcHRpb25fcm9tLT5tZW1vcnlfcmVnaW9uX2dldF9y YW1fcHRyIChhZnRlcgo+PiBtZW1vcnlfcmVnaW9uX2luaXRfcmFtKS4gU28gYWN0dWFsbHkgSSB0 aGluayB3ZSBzaG91bGQgcmVtb3ZlIHRoZQo+PiBEUFJJTlRGIHdhcm5pbmcgYXMgaXQgaXMgbm9y bWFsLgo+IExldCBtZSBleHBsYWluIHdoeSB0aGUgRFBSSU5URiB3YXJuaW5nIGlzIHRoZXJlOiBl bXVsYXRlZCBkbWEgb3BlcmF0aW9ucwo+IGNhbiBpbnZvbHZlIGxvY2tlZCBtYXBwaW5ncy4gT25j ZSBhIGRtYSBvcGVyYXRpb24gY29tcGxldGVzLCB0aGUgcmVsYXRlZAo+IG1hcHBpbmcgaXMgdW5s b2NrZWQgYW5kIGNhbiBiZSBzYWZlbHkgZGVzdHJveWVkLiBCdXQgaWYgd2UgZGVzdHJveSBhCj4g bG9ja2VkIG1hcHBpbmcgaW4geGVuX2ludmFsaWRhdGVfbWFwX2NhY2hlLCB3aGlsZSBhIGRtYSBp cyBzdGlsbAo+IG9uZ29pbmcsIFFFTVUgd2lsbCBjcmFzaC4gV2UgY2Fubm90IGhhbmRsZSB0aGF0 IGNhc2UuCj4KPiBIb3dldmVyLCB0aGUgc2NlbmFyaW8geW91IGRlc2NyaWJlZCBpcyBkaWZmZXJl bnQuIEl0IGhhcyBub3RoaW5nIHRvIGRvCj4gd2l0aCBETUEuIEl0IGxvb2tzIGxpa2UgcGNpX2Fk ZF9vcHRpb25fcm9tIGNhbGxzCj4gbWVtb3J5X3JlZ2lvbl9nZXRfcmFtX3B0ciB0byBtYXAgdGhl IHJ0bDgxMzkgcm9tLiBUaGUgbWFwcGluZyBpcyBhCj4gbG9ja2VkIG1hcHBpbmcgYW5kIGl0IGlz IG5ldmVyIHVubG9ja2VkIG9yIGRlc3Ryb3llZC4KPgo+IEl0IGxvb2tzIGxpa2UgInB0ciIgaXMg bm90IHVzZWQgYWZ0ZXIgcGNpX2FkZF9vcHRpb25fcm9tIHJldHVybnMuIERvZXMKPiB0aGUgYXBw ZW5kIHBhdGNoIGZpeCB0aGUgcHJvYmxlbSB5b3UgYXJlIHNlZWluZz8gRm9yIHRoZSBwcm9wZXIg Zml4LCBJCj4gdGhpbmsgd2UgcHJvYmFibHkgbmVlZCBzb21lIHNvcnQgb2YgbWVtb3J5X3JlZ2lv bl91bm1hcCB3cmFwcGVyIG9yIG1heWJlCj4gYSBjYWxsIHRvIGFkZHJlc3Nfc3BhY2VfdW5tYXAu CgpZZXMsIEkgdGhpbmsgc28sIG1heWJlIHRoaXMgaXMgdGhlIHByb3BlciB3YXkgdG8gZml4IHRo aXMuCgo+Cj4KPiBkaWZmIC0tZ2l0IGEvaHcvcGNpL3BjaS5jIGIvaHcvcGNpL3BjaS5jCj4gaW5k ZXggZTZiMDhlMS4uMDRmOThiNyAxMDA2NDQKPiAtLS0gYS9ody9wY2kvcGNpLmMKPiArKysgYi9o dy9wY2kvcGNpLmMKPiBAQCAtMjI0Miw2ICsyMjQyLDcgQEAgc3RhdGljIHZvaWQgcGNpX2FkZF9v cHRpb25fcm9tKFBDSURldmljZSAqcGRldiwgYm9vbCBpc19kZWZhdWx0X3JvbSwKPiAgICAgICB9 Cj4gICAKPiAgICAgICBwY2lfcmVnaXN0ZXJfYmFyKHBkZXYsIFBDSV9ST01fU0xPVCwgMCwgJnBk ZXYtPnJvbSk7Cj4gKyAgICB4ZW5faW52YWxpZGF0ZV9tYXBfY2FjaGVfZW50cnkocHRyKTsKPiAg IH0KPiAgIAo+ICAgc3RhdGljIHZvaWQgcGNpX2RlbF9vcHRpb25fcm9tKFBDSURldmljZSAqcGRl dikKCgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVu LWRldmVsIG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3Rz Lnhlbi5vcmcveGVuLWRldmVsCg==