* [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-09 15:52 ` hrg 0 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-09 15:52 UTC (permalink / raw) To: stefano.stabellini, anthony.perard, xen-devel, qemu-devel Cc: xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang 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? ^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-09 15:52 ` hrg 0 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-09 15:52 UTC (permalink / raw) To: stefano.stabellini, anthony.perard, xen-devel, qemu-devel Cc: xen-devel, wangxinxin.wang, Herongguang (Stephen), xen-devel 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? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-09 15:52 ` hrg @ 2017-04-09 15:55 ` hrg -1 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-09 15:55 UTC (permalink / raw) To: stefano.stabellini, anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf Cc: xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-09 15:55 ` hrg 0 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-09 15:55 UTC (permalink / raw) To: stefano.stabellini, anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf Cc: xen-devel, wangxinxin.wang, Herongguang (Stephen), xen-devel On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-09 15:55 ` hrg @ 2017-04-09 16:36 ` hrg -1 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-09 16:36 UTC (permalink / raw) To: anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf, sstabellini Cc: xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@gmail.com> wrote: > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-09 16:36 ` hrg 0 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-09 16:36 UTC (permalink / raw) To: anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf, sstabellini Cc: xen-devel, wangxinxin.wang, Herongguang (Stephen), xen-devel On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@gmail.com> wrote: > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-09 16:36 ` hrg @ 2017-04-09 17:52 ` Alexey G -1 siblings, 0 replies; 30+ messages in thread From: Alexey G @ 2017-04-09 17:52 UTC (permalink / raw) To: hrg Cc: anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf, sstabellini, xen-devel, wangxinxin.wang, Herongguang (Stephen), xen-devel On Mon, 10 Apr 2017 00:36:02 +0800 hrg <hrgstephen@gmail.com> wrote: Hi, > On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@gmail.com> wrote: > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-09 17:52 ` Alexey G 0 siblings, 0 replies; 30+ messages in thread From: Alexey G @ 2017-04-09 17:52 UTC (permalink / raw) To: hrg Cc: xen-devel, wangxinxin.wang, qemu-devel, agraf, sstabellini, jun.nakajima, anthony.perard, xen-devel, xen-devel, Herongguang (Stephen) On Mon, 10 Apr 2017 00:36:02 +0800 hrg <hrgstephen@gmail.com> wrote: Hi, > On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@gmail.com> wrote: > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-09 16:36 ` hrg @ 2017-04-10 19:04 ` Stefano Stabellini -1 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-10 19:04 UTC (permalink / raw) To: hrg Cc: anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf, sstabellini, xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang On Mon, 10 Apr 2017, hrg wrote: > On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@gmail.com> wrote: > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-10 19:04 ` Stefano Stabellini 0 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-10 19:04 UTC (permalink / raw) To: hrg Cc: xen-devel, wangxinxin.wang, qemu-devel, agraf, sstabellini, jun.nakajima, anthony.perard, xen-devel, xen-devel, Herongguang (Stephen) [-- Attachment #1: Type: TEXT/PLAIN, Size: 977 bytes --] On Mon, 10 Apr 2017, hrg wrote: > On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@gmail.com> wrote: > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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? [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-10 19:04 ` Stefano Stabellini @ 2017-04-10 19:50 ` Stefano Stabellini -1 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-10 19:50 UTC (permalink / raw) To: Stefano Stabellini Cc: hrg, anthony.perard, xen-devel, qemu-devel, jun.nakajima, agraf, xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang 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 <hrgstephen@gmail.com> wrote: > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-10 19:50 ` Stefano Stabellini 0 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-10 19:50 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, wangxinxin.wang, agraf, qemu-devel, jun.nakajima, anthony.perard, xen-devel, xen-devel, Herongguang (Stephen), hrg [-- Attachment #1: Type: TEXT/PLAIN, Size: 1657 bytes --] 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 <hrgstephen@gmail.com> wrote: > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-10 19:50 ` Stefano Stabellini @ 2017-04-11 4:47 ` hrg -1 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-11 4:47 UTC (permalink / raw) To: Stefano Stabellini Cc: Anthony PERARD, xen-devel, qemu-devel, Jun Nakajima, Alexander Graf, xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-11 4:47 ` hrg 0 siblings, 0 replies; 30+ messages in thread From: hrg @ 2017-04-11 4:47 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, wangxinxin.wang, qemu-devel, Alexander Graf, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, Herongguang (Stephen) On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-11 4:47 ` hrg @ 2017-04-11 22:32 ` Stefano Stabellini -1 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-11 22:32 UTC (permalink / raw) To: hrg Cc: Stefano Stabellini, Anthony PERARD, xen-devel, qemu-devel, Jun Nakajima, Alexander Graf, xen-devel, xen-devel, Herongguang (Stephen), wangxinxin.wang On Tue, 11 Apr 2017, hrg wrote: > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-11 22:32 ` Stefano Stabellini 0 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-11 22:32 UTC (permalink / raw) To: hrg Cc: Stefano Stabellini, wangxinxin.wang, Alexander Graf, qemu-devel, xen-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, Herongguang (Stephen) [-- Attachment #1: Type: TEXT/PLAIN, Size: 3346 bytes --] On Tue, 11 Apr 2017, hrg wrote: > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-11 22:32 ` Stefano Stabellini @ 2017-04-12 6:17 ` Alexey G -1 siblings, 0 replies; 30+ messages in thread From: Alexey G @ 2017-04-12 6:17 UTC (permalink / raw) To: Stefano Stabellini Cc: hrg, wangxinxin.wang, Alexander Graf, qemu-devel, xen-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, Herongguang (Stephen) On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) Stefano Stabellini <sstabellini@kernel.org> wrote: > On Tue, 11 Apr 2017, hrg wrote: > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-12 6:17 ` Alexey G 0 siblings, 0 replies; 30+ messages in thread From: Alexey G @ 2017-04-12 6:17 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, wangxinxin.wang, qemu-devel, Alexander Graf, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, Herongguang (Stephen), hrg On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) Stefano Stabellini <sstabellini@kernel.org> wrote: > On Tue, 11 Apr 2017, hrg wrote: > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-12 6:17 ` [Qemu-devel] " Alexey G @ 2017-04-12 8:28 ` Herongguang (Stephen) -1 siblings, 0 replies; 30+ messages in thread From: Herongguang (Stephen) @ 2017-04-12 8:28 UTC (permalink / raw) To: Alexey G, Stefano Stabellini Cc: hrg, wangxinxin.wang, Alexander Graf, qemu-devel, xen-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel On 2017/4/12 14:17, Alexey G wrote: > On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) > Stefano Stabellini <sstabellini@kernel.org> wrote: > >> On Tue, 11 Apr 2017, hrg wrote: >>> On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini >>> <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >>>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-12 8:28 ` Herongguang (Stephen) 0 siblings, 0 replies; 30+ messages in thread From: Herongguang (Stephen) @ 2017-04-12 8:28 UTC (permalink / raw) To: Alexey G, Stefano Stabellini Cc: xen-devel, wangxinxin.wang, qemu-devel, Alexander Graf, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, hrg On 2017/4/12 14:17, Alexey G wrote: > On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) > Stefano Stabellini <sstabellini@kernel.org> wrote: > >> On Tue, 11 Apr 2017, hrg wrote: >>> On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini >>> <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >>>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-12 6:17 ` [Qemu-devel] " Alexey G @ 2017-04-12 23:51 ` Stefano Stabellini -1 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-12 23:51 UTC (permalink / raw) To: Alexey G Cc: Stefano Stabellini, hrg, wangxinxin.wang, Alexander Graf, qemu-devel, xen-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, Herongguang (Stephen) On Wed, 12 Apr 2017, Alexey G wrote: > On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) > Stefano Stabellini <sstabellini@kernel.org> wrote: > > > On Tue, 11 Apr 2017, hrg wrote: > > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > > > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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). ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-12 23:51 ` Stefano Stabellini 0 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-12 23:51 UTC (permalink / raw) To: Alexey G Cc: xen-devel, wangxinxin.wang, qemu-devel, Alexander Graf, Stefano Stabellini, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, Herongguang (Stephen), hrg [-- Attachment #1: Type: TEXT/PLAIN, Size: 4653 bytes --] On Wed, 12 Apr 2017, Alexey G wrote: > On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) > Stefano Stabellini <sstabellini@kernel.org> wrote: > > > On Tue, 11 Apr 2017, hrg wrote: > > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini > > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > > > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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). [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-11 22:32 ` Stefano Stabellini @ 2017-04-12 8:14 ` Herongguang (Stephen) -1 siblings, 0 replies; 30+ messages in thread From: Herongguang (Stephen) @ 2017-04-12 8:14 UTC (permalink / raw) To: Stefano Stabellini, hrg Cc: Anthony PERARD, xen-devel, qemu-devel, Jun Nakajima, Alexander Graf, xen-devel, xen-devel, wangxinxin.wang 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 >> <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-12 8:14 ` Herongguang (Stephen) 0 siblings, 0 replies; 30+ messages in thread From: Herongguang (Stephen) @ 2017-04-12 8:14 UTC (permalink / raw) To: Stefano Stabellini, hrg Cc: xen-devel, wangxinxin.wang, qemu-devel, Alexander Graf, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel 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 >> <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-12 8:14 ` Herongguang (Stephen) @ 2017-04-12 23:51 ` Stefano Stabellini -1 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-12 23:51 UTC (permalink / raw) To: Herongguang (Stephen) Cc: Stefano Stabellini, hrg, Anthony PERARD, xen-devel, qemu-devel, Jun Nakajima, Alexander Graf, xen-devel, xen-devel, wangxinxin.wang 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 > > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > > > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-12 23:51 ` Stefano Stabellini 0 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-12 23:51 UTC (permalink / raw) To: Herongguang (Stephen) Cc: Stefano Stabellini, wangxinxin.wang, qemu-devel, Alexander Graf, xen-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, hrg [-- Attachment #1: Type: TEXT/PLAIN, Size: 4106 bytes --] 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 > > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: > > > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-12 23:51 ` Stefano Stabellini @ 2017-04-13 5:47 ` Herongguang (Stephen) -1 siblings, 0 replies; 30+ messages in thread From: Herongguang (Stephen) @ 2017-04-13 5:47 UTC (permalink / raw) To: Stefano Stabellini Cc: hrg, Anthony PERARD, xen-devel, qemu-devel, Jun Nakajima, Alexander Graf, xen-devel, xen-devel, wangxinxin.wang 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 >>>> <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >>>>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-13 5:47 ` Herongguang (Stephen) 0 siblings, 0 replies; 30+ messages in thread From: Herongguang (Stephen) @ 2017-04-13 5:47 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, wangxinxin.wang, Alexander Graf, qemu-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, hrg 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 >>>> <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> wrote: >>>>>>>> On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> 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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? 2017-04-13 5:47 ` Herongguang (Stephen) @ 2017-04-28 23:51 ` Stefano Stabellini -1 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-28 23:51 UTC (permalink / raw) To: Herongguang (Stephen) Cc: Stefano Stabellini, hrg, Anthony PERARD, xen-devel, qemu-devel, Jun Nakajima, Alexander Graf, xen-devel, xen-devel, wangxinxin.wang 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 > > > > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> > > > > > > > > wrote: > > > > > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> > > > > > > > > > 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) > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? @ 2017-04-28 23:51 ` Stefano Stabellini 0 siblings, 0 replies; 30+ messages in thread From: Stefano Stabellini @ 2017-04-28 23:51 UTC (permalink / raw) To: Herongguang (Stephen) Cc: Stefano Stabellini, wangxinxin.wang, qemu-devel, Alexander Graf, xen-devel, Jun Nakajima, Anthony PERARD, xen-devel, xen-devel, hrg [-- Attachment #1: Type: TEXT/PLAIN, Size: 4727 bytes --] 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 > > > > > <sstabellini@kernel.org> 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 <hrgstephen@gmail.com> > > > > > > > > wrote: > > > > > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@gmail.com> > > > > > > > > > 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) > [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2017-04-28 23:51 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-09 15:52 [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? hrg 2017-04-09 15:52 ` hrg 2017-04-09 15:55 ` hrg 2017-04-09 15:55 ` hrg 2017-04-09 16:36 ` hrg 2017-04-09 16:36 ` hrg 2017-04-09 17:52 ` [Qemu-devel] [Xen-devel] " Alexey G 2017-04-09 17:52 ` [Qemu-devel] " Alexey G 2017-04-10 19:04 ` Stefano Stabellini 2017-04-10 19:04 ` Stefano Stabellini 2017-04-10 19:50 ` Stefano Stabellini 2017-04-10 19:50 ` Stefano Stabellini 2017-04-11 4:47 ` hrg 2017-04-11 4:47 ` hrg 2017-04-11 22:32 ` Stefano Stabellini 2017-04-11 22:32 ` Stefano Stabellini 2017-04-12 6:17 ` [Qemu-devel] [Xen-devel] " Alexey G 2017-04-12 6:17 ` [Qemu-devel] " Alexey G 2017-04-12 8:28 ` [Qemu-devel] [Xen-devel] " Herongguang (Stephen) 2017-04-12 8:28 ` [Qemu-devel] " Herongguang (Stephen) 2017-04-12 23:51 ` [Qemu-devel] [Xen-devel] " Stefano Stabellini 2017-04-12 23:51 ` [Qemu-devel] " Stefano Stabellini 2017-04-12 8:14 ` Herongguang (Stephen) 2017-04-12 8:14 ` Herongguang (Stephen) 2017-04-12 23:51 ` Stefano Stabellini 2017-04-12 23:51 ` Stefano Stabellini 2017-04-13 5:47 ` Herongguang (Stephen) 2017-04-13 5:47 ` Herongguang (Stephen) 2017-04-28 23:51 ` Stefano Stabellini 2017-04-28 23:51 ` Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.