diff options
Diffstat (limited to 'classification_output/04/other/55247116')
| -rw-r--r-- | classification_output/04/other/55247116 | 1318 |
1 files changed, 0 insertions, 1318 deletions
diff --git a/classification_output/04/other/55247116 b/classification_output/04/other/55247116 deleted file mode 100644 index a42111a5e..000000000 --- a/classification_output/04/other/55247116 +++ /dev/null @@ -1,1318 +0,0 @@ -other: 0.945 -assembly: 0.938 -graphic: 0.933 -socket: 0.929 -semantic: 0.928 -instruction: 0.928 -device: 0.919 -boot: 0.918 -network: 0.916 -vnc: 0.916 -KVM: 0.894 -mistranslation: 0.841 - -[Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache? - -Hi, - -In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -instead of first level entry (if map to rom other than guest memory -comes first), while in xen_invalidate_map_cache(), when VM ballooned -out memory, qemu did not invalidate cache entries in linked -list(entry->next), so when VM balloon back in memory, gfns probably -mapped to different mfns, thus if guest asks device to DMA to these -GPA, qemu may DMA to stale MFNs. - -So I think in xen_invalidate_map_cache() linked lists should also be -checked and invalidated. - -Whatâs your opinion? Is this a bug? Is my analyze correct? - -On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -Hi, -> -> -In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> -instead of first level entry (if map to rom other than guest memory -> -comes first), while in xen_invalidate_map_cache(), when VM ballooned -> -out memory, qemu did not invalidate cache entries in linked -> -list(entry->next), so when VM balloon back in memory, gfns probably -> -mapped to different mfns, thus if guest asks device to DMA to these -> -GPA, qemu may DMA to stale MFNs. -> -> -So I think in xen_invalidate_map_cache() linked lists should also be -> -checked and invalidated. -> -> -Whatâs your opinion? Is this a bug? Is my analyze correct? -Added Jun Nakajima and Alexander Graf - -On Sun, Apr 9, 2017 at 11:55 PM, hrg <address@hidden> wrote: -> -On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -> Hi, -> -> -> -> In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> -> instead of first level entry (if map to rom other than guest memory -> -> comes first), while in xen_invalidate_map_cache(), when VM ballooned -> -> out memory, qemu did not invalidate cache entries in linked -> -> list(entry->next), so when VM balloon back in memory, gfns probably -> -> mapped to different mfns, thus if guest asks device to DMA to these -> -> GPA, qemu may DMA to stale MFNs. -> -> -> -> So I think in xen_invalidate_map_cache() linked lists should also be -> -> checked and invalidated. -> -> -> -> Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> -Added Jun Nakajima and Alexander Graf -And correct Stefano Stabellini's email address. - -On Mon, 10 Apr 2017 00:36:02 +0800 -hrg <address@hidden> wrote: - -Hi, - -> -On Sun, Apr 9, 2017 at 11:55 PM, hrg <address@hidden> wrote: -> -> On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> ->> Hi, -> ->> -> ->> In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> ->> instead of first level entry (if map to rom other than guest memory -> ->> comes first), while in xen_invalidate_map_cache(), when VM ballooned -> ->> out memory, qemu did not invalidate cache entries in linked -> ->> list(entry->next), so when VM balloon back in memory, gfns probably -> ->> mapped to different mfns, thus if guest asks device to DMA to these -> ->> GPA, qemu may DMA to stale MFNs. -> ->> -> ->> So I think in xen_invalidate_map_cache() linked lists should also be -> ->> checked and invalidated. -> ->> -> ->> Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> -> -> Added Jun Nakajima and Alexander Graf -> -And correct Stefano Stabellini's email address. -There is a real issue with the xen-mapcache corruption in fact. I encountered -it a few months ago while experimenting with Q35 support on Xen. Q35 emulation -uses an AHCI controller by default, along with NCQ mode enabled. The issue can -be (somewhat) easily reproduced there, though using a normal i440 emulation -might possibly allow to reproduce the issue as well, using a dedicated test -code from a guest side. In case of Q35+NCQ the issue can be reproduced "as is". - -The issue occurs when a guest domain performs an intensive disk I/O, ex. while -guest OS booting. QEMU crashes with "Bad ram offset 980aa000" -message logged, where the address is different each time. The hard thing with -this issue is that it has a very low reproducibility rate. - -The corruption happens when there are multiple I/O commands in the NCQ queue. -So there are overlapping emulated DMA operations in flight and QEMU uses a -sequence of mapcache actions which can be executed in the "wrong" order thus -leading to an inconsistent xen-mapcache - so a bad address from the wrong -entry is returned. - -The bad thing with this issue is that QEMU crash due to "Bad ram offset" -appearance is a relatively good situation in the sense that this is a caught -error. But there might be a much worse (artificial) situation where the returned -address looks valid but points to a different mapped memory. - -The fix itself is not hard (ex. an additional checked field in MapCacheEntry), -but there is a need of some reliable way to test it considering the low -reproducibility rate. - -Regards, -Alex - -On Mon, 10 Apr 2017, hrg wrote: -> -On Sun, Apr 9, 2017 at 11:55 PM, hrg <address@hidden> wrote: -> -> On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> ->> Hi, -> ->> -> ->> In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> ->> instead of first level entry (if map to rom other than guest memory -> ->> comes first), while in xen_invalidate_map_cache(), when VM ballooned -> ->> out memory, qemu did not invalidate cache entries in linked -> ->> list(entry->next), so when VM balloon back in memory, gfns probably -> ->> mapped to different mfns, thus if guest asks device to DMA to these -> ->> GPA, qemu may DMA to stale MFNs. -> ->> -> ->> So I think in xen_invalidate_map_cache() linked lists should also be -> ->> checked and invalidated. -> ->> -> ->> Whatâs your opinion? Is this a bug? Is my analyze correct? -Yes, you are right. We need to go through the list for each element of -the array in xen_invalidate_map_cache. Can you come up with a patch? - -On Mon, 10 Apr 2017, Stefano Stabellini wrote: -> -On Mon, 10 Apr 2017, hrg wrote: -> -> On Sun, Apr 9, 2017 at 11:55 PM, hrg <address@hidden> wrote: -> -> > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -> >> Hi, -> -> >> -> -> >> In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> -> >> instead of first level entry (if map to rom other than guest memory -> -> >> comes first), while in xen_invalidate_map_cache(), when VM ballooned -> -> >> out memory, qemu did not invalidate cache entries in linked -> -> >> list(entry->next), so when VM balloon back in memory, gfns probably -> -> >> mapped to different mfns, thus if guest asks device to DMA to these -> -> >> GPA, qemu may DMA to stale MFNs. -> -> >> -> -> >> So I think in xen_invalidate_map_cache() linked lists should also be -> -> >> checked and invalidated. -> -> >> -> -> >> Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> -Yes, you are right. We need to go through the list for each element of -> -the array in xen_invalidate_map_cache. Can you come up with a patch? -I spoke too soon. In the regular case there should be no locked mappings -when xen_invalidate_map_cache is called (see the DPRINTF warning at the -beginning of the functions). Without locked mappings, there should never -be more than one element in each list (see xen_map_cache_unlocked: -entry->lock == true is a necessary condition to append a new entry to -the list, otherwise it is just remapped). - -Can you confirm that what you are seeing are locked mappings -when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -by turning it into a printf or by defininig MAPCACHE_DEBUG. - -On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -<address@hidden> 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 <address@hidden> wrote: -> -> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -> > >> Hi, -> -> > >> -> -> > >> In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> -> > >> instead of first level entry (if map to rom other than guest memory -> -> > >> comes first), while in xen_invalidate_map_cache(), when VM ballooned -> -> > >> out memory, qemu did not invalidate cache entries in linked -> -> > >> list(entry->next), so when VM balloon back in memory, gfns probably -> -> > >> mapped to different mfns, thus if guest asks device to DMA to these -> -> > >> GPA, qemu may DMA to stale MFNs. -> -> > >> -> -> > >> So I think in xen_invalidate_map_cache() linked lists should also be -> -> > >> checked and invalidated. -> -> > >> -> -> > >> Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> -> -> Yes, you are right. We need to go through the list for each element of -> -> the array in xen_invalidate_map_cache. Can you come up with a patch? -> -> -I spoke too soon. In the regular case there should be no locked mappings -> -when xen_invalidate_map_cache is called (see the DPRINTF warning at the -> -beginning of the functions). Without locked mappings, there should never -> -be more than one element in each list (see xen_map_cache_unlocked: -> -entry->lock == true is a necessary condition to append a new entry to -> -the list, otherwise it is just remapped). -> -> -Can you confirm that what you are seeing are locked mappings -> -when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -> -by turning it into a printf or by defininig MAPCACHE_DEBUG. -In fact, I think the DPRINTF above is incorrect too. In -pci_add_option_rom(), rtl8139 rom is locked mapped in -pci_add_option_rom->memory_region_get_ram_ptr (after -memory_region_init_ram). So actually I think we should remove the -DPRINTF warning as it is normal. - -On Tue, 11 Apr 2017, hrg wrote: -> -On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -> -<address@hidden> 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 <address@hidden> wrote: -> ->> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> ->> > >> Hi, -> ->> > >> -> ->> > >> In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -> ->> > >> instead of first level entry (if map to rom other than guest memory -> ->> > >> comes first), while in xen_invalidate_map_cache(), when VM ballooned -> ->> > >> out memory, qemu did not invalidate cache entries in linked -> ->> > >> list(entry->next), so when VM balloon back in memory, gfns probably -> ->> > >> mapped to different mfns, thus if guest asks device to DMA to these -> ->> > >> GPA, qemu may DMA to stale MFNs. -> ->> > >> -> ->> > >> So I think in xen_invalidate_map_cache() linked lists should also be -> ->> > >> checked and invalidated. -> ->> > >> -> ->> > >> Whatâs your opinion? Is this a bug? Is my analyze correct? -> ->> -> ->> Yes, you are right. We need to go through the list for each element of -> ->> the array in xen_invalidate_map_cache. Can you come up with a patch? -> -> -> -> I spoke too soon. In the regular case there should be no locked mappings -> -> when xen_invalidate_map_cache is called (see the DPRINTF warning at the -> -> beginning of the functions). Without locked mappings, there should never -> -> be more than one element in each list (see xen_map_cache_unlocked: -> -> entry->lock == true is a necessary condition to append a new entry to -> -> the list, otherwise it is just remapped). -> -> -> -> Can you confirm that what you are seeing are locked mappings -> -> when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -> -> by turning it into a printf or by defininig MAPCACHE_DEBUG. -> -> -In fact, I think the DPRINTF above is incorrect too. In -> -pci_add_option_rom(), rtl8139 rom is locked mapped in -> -pci_add_option_rom->memory_region_get_ram_ptr (after -> -memory_region_init_ram). So actually I think we should remove the -> -DPRINTF warning as it is normal. -Let me explain why the DPRINTF warning is there: emulated dma operations -can involve locked mappings. Once a dma operation completes, the related -mapping is unlocked and can be safely destroyed. But if we destroy a -locked mapping in xen_invalidate_map_cache, while a dma is still -ongoing, QEMU will crash. We cannot handle that case. - -However, the scenario you described is different. It has nothing to do -with DMA. It looks like pci_add_option_rom calls -memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -locked mapping and it is never unlocked or destroyed. - -It looks like "ptr" is not used after pci_add_option_rom returns. Does -the append patch fix the problem you are seeing? For the proper fix, I -think we probably need some sort of memory_region_unmap wrapper or maybe -a call to address_space_unmap. - - -diff --git a/hw/pci/pci.c b/hw/pci/pci.c -index e6b08e1..04f98b7 100644 ---- a/hw/pci/pci.c -+++ b/hw/pci/pci.c -@@ -2242,6 +2242,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool -is_default_rom, - } - - pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom); -+ xen_invalidate_map_cache_entry(ptr); - } - - static void pci_del_option_rom(PCIDevice *pdev) - -On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) -Stefano Stabellini <address@hidden> wrote: - -> -On Tue, 11 Apr 2017, hrg wrote: -> -> On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -> -> <address@hidden> 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 <address@hidden> wrote: -> -> >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -> >> > >> Hi, -> -> >> > >> -> -> >> > >> In xen_map_cache_unlocked(), map to guest memory maybe in -> -> >> > >> entry->next instead of first level entry (if map to rom other than -> -> >> > >> guest memory comes first), while in xen_invalidate_map_cache(), -> -> >> > >> when VM ballooned out memory, qemu did not invalidate cache entries -> -> >> > >> in linked list(entry->next), so when VM balloon back in memory, -> -> >> > >> gfns probably mapped to different mfns, thus if guest asks device -> -> >> > >> to DMA to these GPA, qemu may DMA to stale MFNs. -> -> >> > >> -> -> >> > >> So I think in xen_invalidate_map_cache() linked lists should also be -> -> >> > >> checked and invalidated. -> -> >> > >> -> -> >> > >> Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> >> -> -> >> Yes, you are right. We need to go through the list for each element of -> -> >> the array in xen_invalidate_map_cache. Can you come up with a patch? -> -> > -> -> > I spoke too soon. In the regular case there should be no locked mappings -> -> > when xen_invalidate_map_cache is called (see the DPRINTF warning at the -> -> > beginning of the functions). Without locked mappings, there should never -> -> > be more than one element in each list (see xen_map_cache_unlocked: -> -> > entry->lock == true is a necessary condition to append a new entry to -> -> > the list, otherwise it is just remapped). -> -> > -> -> > Can you confirm that what you are seeing are locked mappings -> -> > when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -> -> > by turning it into a printf or by defininig MAPCACHE_DEBUG. -> -> -> -> In fact, I think the DPRINTF above is incorrect too. In -> -> pci_add_option_rom(), rtl8139 rom is locked mapped in -> -> pci_add_option_rom->memory_region_get_ram_ptr (after -> -> memory_region_init_ram). So actually I think we should remove the -> -> DPRINTF warning as it is normal. -> -> -Let me explain why the DPRINTF warning is there: emulated dma operations -> -can involve locked mappings. Once a dma operation completes, the related -> -mapping is unlocked and can be safely destroyed. But if we destroy a -> -locked mapping in xen_invalidate_map_cache, while a dma is still -> -ongoing, QEMU will crash. We cannot handle that case. -> -> -However, the scenario you described is different. It has nothing to do -> -with DMA. It looks like pci_add_option_rom calls -> -memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -> -locked mapping and it is never unlocked or destroyed. -> -> -It looks like "ptr" is not used after pci_add_option_rom returns. Does -> -the append patch fix the problem you are seeing? For the proper fix, I -> -think we probably need some sort of memory_region_unmap wrapper or maybe -> -a call to address_space_unmap. -Hmm, for some reason my message to the Xen-devel list got rejected but was sent -to Qemu-devel instead, without any notice. Sorry if I'm missing something -obvious as a list newbie. - -Stefano, hrg, - -There is an issue with inconsistency between the list of normal MapCacheEntry's -and their 'reverse' counterparts - MapCacheRev's in locked_entries. -When bad situation happens, there are multiple (locked) MapCacheEntry -entries in the bucket's linked list along with a number of MapCacheRev's. And -when it comes to a reverse lookup, xen-mapcache picks the wrong entry from the -first list and calculates a wrong pointer from it which may then be caught with -the "Bad RAM offset" check (or not). Mapcache invalidation might be related to -this issue as well I think. - -I'll try to provide a test code which can reproduce the issue from the -guest side using an emulated IDE controller, though it's much simpler to achieve -this result with an AHCI controller using multiple NCQ I/O commands. So far I've -seen this issue only with Windows 7 (and above) guest on AHCI, but any block I/O -DMA should be enough I think. - -On 2017/4/12 14:17, Alexey G wrote: -On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) -Stefano Stabellini <address@hidden> wrote: -On Tue, 11 Apr 2017, hrg wrote: -On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -<address@hidden> 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 <address@hidden> wrote: -On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -Hi, - -In xen_map_cache_unlocked(), map to guest memory maybe in -entry->next instead of first level entry (if map to rom other than -guest memory comes first), while in xen_invalidate_map_cache(), -when VM ballooned out memory, qemu did not invalidate cache entries -in linked list(entry->next), so when VM balloon back in memory, -gfns probably mapped to different mfns, thus if guest asks device -to DMA to these GPA, qemu may DMA to stale MFNs. - -So I think in xen_invalidate_map_cache() linked lists should also be -checked and invalidated. - -Whatâs your opinion? Is this a bug? Is my analyze correct? -Yes, you are right. We need to go through the list for each element of -the array in xen_invalidate_map_cache. Can you come up with a patch? -I spoke too soon. In the regular case there should be no locked mappings -when xen_invalidate_map_cache is called (see the DPRINTF warning at the -beginning of the functions). Without locked mappings, there should never -be more than one element in each list (see xen_map_cache_unlocked: -entry->lock == true is a necessary condition to append a new entry to -the list, otherwise it is just remapped). - -Can you confirm that what you are seeing are locked mappings -when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -by turning it into a printf or by defininig MAPCACHE_DEBUG. -In fact, I think the DPRINTF above is incorrect too. In -pci_add_option_rom(), rtl8139 rom is locked mapped in -pci_add_option_rom->memory_region_get_ram_ptr (after -memory_region_init_ram). So actually I think we should remove the -DPRINTF warning as it is normal. -Let me explain why the DPRINTF warning is there: emulated dma operations -can involve locked mappings. Once a dma operation completes, the related -mapping is unlocked and can be safely destroyed. But if we destroy a -locked mapping in xen_invalidate_map_cache, while a dma is still -ongoing, QEMU will crash. We cannot handle that case. - -However, the scenario you described is different. It has nothing to do -with DMA. It looks like pci_add_option_rom calls -memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -locked mapping and it is never unlocked or destroyed. - -It looks like "ptr" is not used after pci_add_option_rom returns. Does -the append patch fix the problem you are seeing? For the proper fix, I -think we probably need some sort of memory_region_unmap wrapper or maybe -a call to address_space_unmap. -Hmm, for some reason my message to the Xen-devel list got rejected but was sent -to Qemu-devel instead, without any notice. Sorry if I'm missing something -obvious as a list newbie. - -Stefano, hrg, - -There is an issue with inconsistency between the list of normal MapCacheEntry's -and their 'reverse' counterparts - MapCacheRev's in locked_entries. -When bad situation happens, there are multiple (locked) MapCacheEntry -entries in the bucket's linked list along with a number of MapCacheRev's. And -when it comes to a reverse lookup, xen-mapcache picks the wrong entry from the -first list and calculates a wrong pointer from it which may then be caught with -the "Bad RAM offset" check (or not). Mapcache invalidation might be related to -this issue as well I think. - -I'll try to provide a test code which can reproduce the issue from the -guest side using an emulated IDE controller, though it's much simpler to achieve -this result with an AHCI controller using multiple NCQ I/O commands. So far I've -seen this issue only with Windows 7 (and above) guest on AHCI, but any block I/O -DMA should be enough I think. -Yes, I think there may be other bugs lurking, considering the complexity, -though we need to reproduce it if we want to delve into it. - -On Wed, 12 Apr 2017, Alexey G wrote: -> -On Tue, 11 Apr 2017 15:32:09 -0700 (PDT) -> -Stefano Stabellini <address@hidden> wrote: -> -> -> On Tue, 11 Apr 2017, hrg wrote: -> -> > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -> -> > <address@hidden> 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 <address@hidden> wrote: -> -> > >> > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -> > >> > >> Hi, -> -> > >> > >> -> -> > >> > >> In xen_map_cache_unlocked(), map to guest memory maybe in -> -> > >> > >> entry->next instead of first level entry (if map to rom other than -> -> > >> > >> guest memory comes first), while in xen_invalidate_map_cache(), -> -> > >> > >> when VM ballooned out memory, qemu did not invalidate cache -> -> > >> > >> entries -> -> > >> > >> in linked list(entry->next), so when VM balloon back in memory, -> -> > >> > >> gfns probably mapped to different mfns, thus if guest asks device -> -> > >> > >> to DMA to these GPA, qemu may DMA to stale MFNs. -> -> > >> > >> -> -> > >> > >> So I think in xen_invalidate_map_cache() linked lists should also -> -> > >> > >> be -> -> > >> > >> checked and invalidated. -> -> > >> > >> -> -> > >> > >> Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> > >> -> -> > >> Yes, you are right. We need to go through the list for each element of -> -> > >> the array in xen_invalidate_map_cache. Can you come up with a patch? -> -> > > -> -> > > I spoke too soon. In the regular case there should be no locked mappings -> -> > > when xen_invalidate_map_cache is called (see the DPRINTF warning at the -> -> > > beginning of the functions). Without locked mappings, there should never -> -> > > be more than one element in each list (see xen_map_cache_unlocked: -> -> > > entry->lock == true is a necessary condition to append a new entry to -> -> > > the list, otherwise it is just remapped). -> -> > > -> -> > > Can you confirm that what you are seeing are locked mappings -> -> > > when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -> -> > > by turning it into a printf or by defininig MAPCACHE_DEBUG. -> -> > -> -> > In fact, I think the DPRINTF above is incorrect too. In -> -> > pci_add_option_rom(), rtl8139 rom is locked mapped in -> -> > pci_add_option_rom->memory_region_get_ram_ptr (after -> -> > memory_region_init_ram). So actually I think we should remove the -> -> > DPRINTF warning as it is normal. -> -> -> -> Let me explain why the DPRINTF warning is there: emulated dma operations -> -> can involve locked mappings. Once a dma operation completes, the related -> -> mapping is unlocked and can be safely destroyed. But if we destroy a -> -> locked mapping in xen_invalidate_map_cache, while a dma is still -> -> ongoing, QEMU will crash. We cannot handle that case. -> -> -> -> However, the scenario you described is different. It has nothing to do -> -> with DMA. It looks like pci_add_option_rom calls -> -> memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -> -> locked mapping and it is never unlocked or destroyed. -> -> -> -> It looks like "ptr" is not used after pci_add_option_rom returns. Does -> -> the append patch fix the problem you are seeing? For the proper fix, I -> -> think we probably need some sort of memory_region_unmap wrapper or maybe -> -> a call to address_space_unmap. -> -> -Hmm, for some reason my message to the Xen-devel list got rejected but was -> -sent -> -to Qemu-devel instead, without any notice. Sorry if I'm missing something -> -obvious as a list newbie. -> -> -Stefano, hrg, -> -> -There is an issue with inconsistency between the list of normal -> -MapCacheEntry's -> -and their 'reverse' counterparts - MapCacheRev's in locked_entries. -> -When bad situation happens, there are multiple (locked) MapCacheEntry -> -entries in the bucket's linked list along with a number of MapCacheRev's. And -> -when it comes to a reverse lookup, xen-mapcache picks the wrong entry from the -> -first list and calculates a wrong pointer from it which may then be caught -> -with -> -the "Bad RAM offset" check (or not). Mapcache invalidation might be related to -> -this issue as well I think. -> -> -I'll try to provide a test code which can reproduce the issue from the -> -guest side using an emulated IDE controller, though it's much simpler to -> -achieve -> -this result with an AHCI controller using multiple NCQ I/O commands. So far -> -I've -> -seen this issue only with Windows 7 (and above) guest on AHCI, but any block -> -I/O -> -DMA should be enough I think. -That would be helpful. Please see if you can reproduce it after fixing -the other issue ( -http://marc.info/?l=qemu-devel&m=149195042500707&w=2 -). - -On 2017/4/12 6:32, Stefano Stabellini wrote: -On Tue, 11 Apr 2017, hrg wrote: -On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -<address@hidden> 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 <address@hidden> wrote: -On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -Hi, - -In xen_map_cache_unlocked(), map to guest memory maybe in entry->next -instead of first level entry (if map to rom other than guest memory -comes first), while in xen_invalidate_map_cache(), when VM ballooned -out memory, qemu did not invalidate cache entries in linked -list(entry->next), so when VM balloon back in memory, gfns probably -mapped to different mfns, thus if guest asks device to DMA to these -GPA, qemu may DMA to stale MFNs. - -So I think in xen_invalidate_map_cache() linked lists should also be -checked and invalidated. - -Whatâs your opinion? Is this a bug? Is my analyze correct? -Yes, you are right. We need to go through the list for each element of -the array in xen_invalidate_map_cache. Can you come up with a patch? -I spoke too soon. In the regular case there should be no locked mappings -when xen_invalidate_map_cache is called (see the DPRINTF warning at the -beginning of the functions). Without locked mappings, there should never -be more than one element in each list (see xen_map_cache_unlocked: -entry->lock == true is a necessary condition to append a new entry to -the list, otherwise it is just remapped). - -Can you confirm that what you are seeing are locked mappings -when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -by turning it into a printf or by defininig MAPCACHE_DEBUG. -In fact, I think the DPRINTF above is incorrect too. In -pci_add_option_rom(), rtl8139 rom is locked mapped in -pci_add_option_rom->memory_region_get_ram_ptr (after -memory_region_init_ram). So actually I think we should remove the -DPRINTF warning as it is normal. -Let me explain why the DPRINTF warning is there: emulated dma operations -can involve locked mappings. Once a dma operation completes, the related -mapping is unlocked and can be safely destroyed. But if we destroy a -locked mapping in xen_invalidate_map_cache, while a dma is still -ongoing, QEMU will crash. We cannot handle that case. - -However, the scenario you described is different. It has nothing to do -with DMA. It looks like pci_add_option_rom calls -memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -locked mapping and it is never unlocked or destroyed. - -It looks like "ptr" is not used after pci_add_option_rom returns. Does -the append patch fix the problem you are seeing? For the proper fix, I -think we probably need some sort of memory_region_unmap wrapper or maybe -a call to address_space_unmap. -Yes, I think so, maybe this is the proper way to fix this. -diff --git a/hw/pci/pci.c b/hw/pci/pci.c -index e6b08e1..04f98b7 100644 ---- a/hw/pci/pci.c -+++ b/hw/pci/pci.c -@@ -2242,6 +2242,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool -is_default_rom, - } -pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom); -+ xen_invalidate_map_cache_entry(ptr); - } -static void pci_del_option_rom(PCIDevice *pdev) - -On Wed, 12 Apr 2017, Herongguang (Stephen) wrote: -> -On 2017/4/12 6:32, Stefano Stabellini wrote: -> -> On Tue, 11 Apr 2017, hrg wrote: -> -> > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -> -> > <address@hidden> 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 <address@hidden> wrote: -> -> > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -> -> > > > > > > Hi, -> -> > > > > > > -> -> > > > > > > In xen_map_cache_unlocked(), map to guest memory maybe in -> -> > > > > > > entry->next -> -> > > > > > > instead of first level entry (if map to rom other than guest -> -> > > > > > > memory -> -> > > > > > > comes first), while in xen_invalidate_map_cache(), when VM -> -> > > > > > > ballooned -> -> > > > > > > out memory, qemu did not invalidate cache entries in linked -> -> > > > > > > list(entry->next), so when VM balloon back in memory, gfns -> -> > > > > > > probably -> -> > > > > > > mapped to different mfns, thus if guest asks device to DMA to -> -> > > > > > > these -> -> > > > > > > GPA, qemu may DMA to stale MFNs. -> -> > > > > > > -> -> > > > > > > So I think in xen_invalidate_map_cache() linked lists should -> -> > > > > > > also be -> -> > > > > > > checked and invalidated. -> -> > > > > > > -> -> > > > > > > Whatâs your opinion? Is this a bug? Is my analyze correct? -> -> > > > Yes, you are right. We need to go through the list for each element of -> -> > > > the array in xen_invalidate_map_cache. Can you come up with a patch? -> -> > > I spoke too soon. In the regular case there should be no locked mappings -> -> > > when xen_invalidate_map_cache is called (see the DPRINTF warning at the -> -> > > beginning of the functions). Without locked mappings, there should never -> -> > > be more than one element in each list (see xen_map_cache_unlocked: -> -> > > entry->lock == true is a necessary condition to append a new entry to -> -> > > the list, otherwise it is just remapped). -> -> > > -> -> > > Can you confirm that what you are seeing are locked mappings -> -> > > when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -> -> > > by turning it into a printf or by defininig MAPCACHE_DEBUG. -> -> > In fact, I think the DPRINTF above is incorrect too. In -> -> > pci_add_option_rom(), rtl8139 rom is locked mapped in -> -> > pci_add_option_rom->memory_region_get_ram_ptr (after -> -> > memory_region_init_ram). So actually I think we should remove the -> -> > DPRINTF warning as it is normal. -> -> Let me explain why the DPRINTF warning is there: emulated dma operations -> -> can involve locked mappings. Once a dma operation completes, the related -> -> mapping is unlocked and can be safely destroyed. But if we destroy a -> -> locked mapping in xen_invalidate_map_cache, while a dma is still -> -> ongoing, QEMU will crash. We cannot handle that case. -> -> -> -> However, the scenario you described is different. It has nothing to do -> -> with DMA. It looks like pci_add_option_rom calls -> -> memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -> -> locked mapping and it is never unlocked or destroyed. -> -> -> -> It looks like "ptr" is not used after pci_add_option_rom returns. Does -> -> the append patch fix the problem you are seeing? For the proper fix, I -> -> think we probably need some sort of memory_region_unmap wrapper or maybe -> -> a call to address_space_unmap. -> -> -Yes, I think so, maybe this is the proper way to fix this. -Would you be up for sending a proper patch and testing it? We cannot call -xen_invalidate_map_cache_entry directly from pci.c though, it would need -to be one of the other functions like address_space_unmap for example. - - -> -> diff --git a/hw/pci/pci.c b/hw/pci/pci.c -> -> index e6b08e1..04f98b7 100644 -> -> --- a/hw/pci/pci.c -> -> +++ b/hw/pci/pci.c -> -> @@ -2242,6 +2242,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool -> -> is_default_rom, -> -> } -> -> pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom); -> -> + xen_invalidate_map_cache_entry(ptr); -> -> } -> -> static void pci_del_option_rom(PCIDevice *pdev) - -On 2017/4/13 7:51, Stefano Stabellini wrote: -On Wed, 12 Apr 2017, Herongguang (Stephen) wrote: -On 2017/4/12 6:32, Stefano Stabellini wrote: -On Tue, 11 Apr 2017, hrg wrote: -On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -<address@hidden> 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 <address@hidden> wrote: -On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> wrote: -Hi, - -In xen_map_cache_unlocked(), map to guest memory maybe in -entry->next -instead of first level entry (if map to rom other than guest -memory -comes first), while in xen_invalidate_map_cache(), when VM -ballooned -out memory, qemu did not invalidate cache entries in linked -list(entry->next), so when VM balloon back in memory, gfns -probably -mapped to different mfns, thus if guest asks device to DMA to -these -GPA, qemu may DMA to stale MFNs. - -So I think in xen_invalidate_map_cache() linked lists should -also be -checked and invalidated. - -Whatâs your opinion? Is this a bug? Is my analyze correct? -Yes, you are right. We need to go through the list for each element of -the array in xen_invalidate_map_cache. Can you come up with a patch? -I spoke too soon. In the regular case there should be no locked mappings -when xen_invalidate_map_cache is called (see the DPRINTF warning at the -beginning of the functions). Without locked mappings, there should never -be more than one element in each list (see xen_map_cache_unlocked: -entry->lock == true is a necessary condition to append a new entry to -the list, otherwise it is just remapped). - -Can you confirm that what you are seeing are locked mappings -when xen_invalidate_map_cache is called? To find out, enable the DPRINTK -by turning it into a printf or by defininig MAPCACHE_DEBUG. -In fact, I think the DPRINTF above is incorrect too. In -pci_add_option_rom(), rtl8139 rom is locked mapped in -pci_add_option_rom->memory_region_get_ram_ptr (after -memory_region_init_ram). So actually I think we should remove the -DPRINTF warning as it is normal. -Let me explain why the DPRINTF warning is there: emulated dma operations -can involve locked mappings. Once a dma operation completes, the related -mapping is unlocked and can be safely destroyed. But if we destroy a -locked mapping in xen_invalidate_map_cache, while a dma is still -ongoing, QEMU will crash. We cannot handle that case. - -However, the scenario you described is different. It has nothing to do -with DMA. It looks like pci_add_option_rom calls -memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a -locked mapping and it is never unlocked or destroyed. - -It looks like "ptr" is not used after pci_add_option_rom returns. Does -the append patch fix the problem you are seeing? For the proper fix, I -think we probably need some sort of memory_region_unmap wrapper or maybe -a call to address_space_unmap. -Yes, I think so, maybe this is the proper way to fix this. -Would you be up for sending a proper patch and testing it? We cannot call -xen_invalidate_map_cache_entry directly from pci.c though, it would need -to be one of the other functions like address_space_unmap for example. -Yes, I will look into this. -diff --git a/hw/pci/pci.c b/hw/pci/pci.c -index e6b08e1..04f98b7 100644 ---- a/hw/pci/pci.c -+++ b/hw/pci/pci.c -@@ -2242,6 +2242,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool -is_default_rom, - } - pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom); -+ xen_invalidate_map_cache_entry(ptr); - } - static void pci_del_option_rom(PCIDevice *pdev) - -On Thu, 13 Apr 2017, Herongguang (Stephen) wrote: -> -On 2017/4/13 7:51, Stefano Stabellini wrote: -> -> On Wed, 12 Apr 2017, Herongguang (Stephen) wrote: -> -> > On 2017/4/12 6:32, Stefano Stabellini wrote: -> -> > > On Tue, 11 Apr 2017, hrg wrote: -> -> > > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini -> -> > > > <address@hidden> 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 <address@hidden> -> -> > > > > > > wrote: -> -> > > > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <address@hidden> -> -> > > > > > > > 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) -> - |