other: 0.945 assembly: 0.938 graphic: 0.933 socket: 0.929 semantic: 0.928 instruction: 0.928 device: 0.919 boot: 0.918 network: 0.916 vnc: 0.916 KVM: 0.894 mistranslation: 0.841 [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 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? 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? Added Jun Nakajima and Alexander Graf 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? > > Added Jun Nakajima and Alexander Graf And correct Stefano Stabellini's email address. On Mon, 10 Apr 2017 00:36:02 +0800 hrg wrote: Hi, > 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? > > > > Added Jun Nakajima and Alexander Graf > And correct Stefano Stabellini's email address. There is a real issue with the xen-mapcache corruption in fact. I encountered it a few months ago while experimenting with Q35 support on Xen. Q35 emulation uses an AHCI controller by default, along with NCQ mode enabled. The issue can be (somewhat) easily reproduced there, though using a normal i440 emulation might possibly allow to reproduce the issue as well, using a dedicated test code from a guest side. In case of Q35+NCQ the issue can be reproduced "as is". The issue occurs when a guest domain performs an intensive disk I/O, ex. while guest OS booting. QEMU crashes with "Bad ram offset 980aa000" message logged, where the address is different each time. The hard thing with this issue is that it has a very low reproducibility rate. The corruption happens when there are multiple I/O commands in the NCQ queue. So there are overlapping emulated DMA operations in flight and QEMU uses a sequence of mapcache actions which can be executed in the "wrong" order thus leading to an inconsistent xen-mapcache - so a bad address from the wrong entry is returned. The bad thing with this issue is that QEMU crash due to "Bad ram offset" appearance is a relatively good situation in the sense that this is a caught error. But there might be a much worse (artificial) situation where the returned address looks valid but points to a different mapped memory. The fix itself is not hard (ex. an additional checked field in MapCacheEntry), but there is a need of some reliable way to test it considering the low reproducibility rate. Regards, Alex 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? 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. 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. 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. 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) 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. 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. On Wed, 12 Apr 2017, 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. That would be helpful. Please see if you can reproduce it after fixing the other issue ( http://marc.info/?l=qemu-devel&m=149195042500707&w=2 ). 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) 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. > > 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) 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) On Thu, 13 Apr 2017, Herongguang (Stephen) wrote: > 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. Any updates? > > > > 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) >