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.