Am 02.03.2020 um 13:59 hat Pavel Dovgalyuk geschrieben:
Windows guest sometimes makes DMA requests with overlapping
target addresses. This leads to the following structure of iov for
the block driver:
addr size1
addr size2
addr size3
It means that three adjacent disk blocks should be read into the same
memory buffer. Windows does not expects anything from these bytes
(should it be data from the first block, or the last one, or some
mix),
but uses them somehow. It leads to non-determinism of the guest
execution,
because block driver does not preserve any order of reading.
This situation was discusses in the mailing list at least twice:
https://lists.gnu.org/archive/html/qemu-devel/2010-09/msg01996.html
https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg05185.html
This patch makes such disk reads deterministic in icount mode.
It skips SG parts that were already affected by prior reads
within the same request. Parts that are non identical, but are just
overlapped, are trimmed.
Examples for different SG part sequences:
1)
A1 1000
A1 1000
->
A1 1000
2)
A1 1000
A2 1000
A1 1000
A3 1000
->
Two requests with different offsets, because second A1/1000 should be
skipped.
A1 1000
A2 1000
--
A3 1000
How is the "--" line represented in the code?
3)
A1 800
A2 1000
A1 1000
->
First 800 bytes of third SG are skipped.
A1 800
A2 1000
--
A1+800 800
Signed-off-by: Pavel Dovgalyuk <address@hidden>
---
dma-helpers.c | 57
+++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 53 insertions(+), 4 deletions(-)
diff --git a/dma-helpers.c b/dma-helpers.c
index e8a26e81e1..d71512f707 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -13,6 +13,7 @@
#include "trace-root.h"
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+#include "sysemu/cpus.h"
/* #define DEBUG_IOMMU */
@@ -139,17 +140,65 @@ static void dma_blk_cb(void *opaque, int ret)
dma_blk_unmap(dbs);
while (dbs->sg_cur_index < dbs->sg->nsg) {
+ bool skip = false;
cur_addr = dbs->sg->sg[dbs->sg_cur_index].base +
dbs->sg_cur_byte;
cur_len = dbs->sg->sg[dbs->sg_cur_index].len -
dbs->sg_cur_byte;
- mem = dma_memory_map(dbs->sg->as, cur_addr, &cur_len,
dbs->dir);
- if (!mem)
- break;
- qemu_iovec_add(&dbs->iov, mem, cur_len);
+
+ /*
+ * Make reads deterministic in icount mode.
+ * Windows sometimes issues disk read requests with
+ * overlapping SGs. It leads to non-determinism, because
+ * resulting buffer contents may be mixed from several
+ * sectors.
+ * This code crops SGs that were already read in this
request.
+ */
Please make use of the full line length for the commit text, and add
empty lines between paragraphs.
+ }
+
dbs->sg_cur_byte += cur_len;
if (dbs->sg_cur_byte == dbs->sg->sg[dbs->sg_cur_index].len) {
dbs->sg_cur_byte = 0;
++dbs->sg_cur_index;
}
+
+ /*
+ * All remaining SGs were skipped.
+ * This is not reschedule case, because we already
+ * performed the reads, and the last SGs were skipped.
+ */
+ if (dbs->sg_cur_index == dbs->sg->nsg && dbs->iov.size == 0)
{
+ dma_complete(dbs, ret);
+ return;
+ }
}
I think the concept of skipping SG list entries makes this patch
relatively complex. Maybe one of these would work better:
1. Instead of skipping, add a temporary bounce buffer to the iovec.
2. Instead of skipping, just exit the loop and effectively split the
request in multiple parts (like you already do in one case). Then
the
memory will still be written to twice, but deterministically so that
the later SG list entry always wins.
I think 2. sounds quite attractive because you don't have to manage any
additional state. You can even simplify the loop to use
ranges_overlap()