KVM: x86: fix vcpu->mmio_fragments overflow

Add the test to trigger the bug that "rep ins" causes vcpu->mmio_fragments overflow overflow while move large data from ioport to MMIO

Signed-off-by Xiao Guangrong <[email protected]>

---
 x86/emulator.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace. After that, we only need two entries to store mmio info for the cross-mmio pages access

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/x86.c       |  127 +++++++++++++++++++++++++++++-----------------
 include/linux/kvm_host.h |   16 +-----
 2 files changed, 84 insertions(+), 59 deletions(-)

arch/x86/kvm/x86.c +81 -46

index 8b90dd5..41ceb51 100644
@@ -3779,9 +3779,6 @@ static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
 static int write_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
 			   void *val, int bytes)
 {
-	struct kvm_mmio_fragment *frag = &vcpu->mmio_fragments[0];
-
-	memcpy(vcpu->run->mmio.data, frag->data, frag->len);
 	return X86EMUL_CONTINUE;
 }
 

@@ -3799,6 +3796,64 @@ static const struct read_write_emulator_ops write_emultor = {
 	.write = true,
 };
 
+static bool get_current_mmio_info(struct kvm_vcpu *vcpu, gpa_t *gpa,
+				  unsigned *len, void **data)
+{
+	struct kvm_mmio_fragment *frag;
+	int cur = vcpu->mmio_cur_fragment;
+
+	if (cur >= vcpu->mmio_nr_fragments)
+		return false;
+
+	frag = &vcpu->mmio_fragments[cur];
+	if (frag->pos >= frag->len) {
+		if (++vcpu->mmio_cur_fragment >= vcpu->mmio_nr_fragments)
+			return false;
+		frag++;
+	}
+
+	*gpa = frag->gpa + frag->pos;
+	*data = frag->data + frag->pos;
+	*len = min(8u, frag->len - frag->pos);
+	return true;
+}
+
+static void complete_current_mmio(struct kvm_vcpu *vcpu)
+{
+	struct kvm_mmio_fragment *frag;
+	gpa_t gpa;
+	unsigned len;
+	void *data;
+
+	get_current_mmio_info(vcpu, &gpa, &len, &data);
+
+	if (!vcpu->mmio_is_write)
+		memcpy(data, vcpu->run->mmio.data, len);
+
+	/* Increase frag->pos to switch to the next mmio. */
+	frag = &vcpu->mmio_fragments[vcpu->mmio_cur_fragment];
+	frag->pos += len;
+}
+
+static bool vcpu_fill_mmio_exit_info(struct kvm_vcpu *vcpu)
+{
+	gpa_t gpa;
+	unsigned len;
+	void *data;
+
+	if (!get_current_mmio_info(vcpu, &gpa, &len, &data))
+		return false;
+
+	vcpu->run->mmio.len = len;
+	vcpu->run->mmio.is_write = vcpu->mmio_is_write;
+	vcpu->run->exit_reason = KVM_EXIT_MMIO;
+	vcpu->run->mmio.phys_addr = gpa;
+
+	if (vcpu->mmio_is_write)
+		memcpy(vcpu->run->mmio.data, data, len);
+	return true;
+}
+
 static int emulator_read_write_onepage(unsigned long addr, void *val,
 				       unsigned int bytes,
 				       struct x86_exception *exception,

@@ -3834,18 +3889,12 @@ mmio:
 	bytes -= handled;
 	val += handled;
 
-	while (bytes) {
-		unsigned now = min(bytes, 8U);
-
-		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
-		frag->gpa = gpa;
-		frag->data = val;
-		frag->len = now;
-
-		gpa += now;
-		val += now;
-		bytes -= now;
-	}
+	WARN_ON(vcpu->mmio_nr_fragments >= KVM_MAX_MMIO_FRAGMENTS);
+	frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
+	frag->pos = 0;
+	frag->gpa = gpa;
+	frag->data = val;
+	frag->len = bytes;
 	return X86EMUL_CONTINUE;
 }
 

@@ -3855,7 +3904,6 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 			const struct read_write_emulator_ops *ops)
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
-	gpa_t gpa;
 	int rc;
 
 	if (ops->read_write_prepare &&

@@ -3887,17 +3935,13 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 	if (!vcpu->mmio_nr_fragments)
 		return rc;
 
-	gpa = vcpu->mmio_fragments[0].gpa;
-
 	vcpu->mmio_needed = 1;
 	vcpu->mmio_cur_fragment = 0;
+	vcpu->mmio_is_write = ops->write;
 
-	vcpu->run->mmio.len = vcpu->mmio_fragments[0].len;
-	vcpu->run->mmio.is_write = vcpu->mmio_is_write = ops->write;
-	vcpu->run->exit_reason = KVM_EXIT_MMIO;
-	vcpu->run->mmio.phys_addr = gpa;
-
-	return ops->read_write_exit_mmio(vcpu, gpa, val, bytes);
+	vcpu_fill_mmio_exit_info(vcpu);
+	return ops->read_write_exit_mmio(vcpu, vcpu->mmio_fragments[0].gpa,
+					 val, bytes);
 }
 
 static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,

@@ -5524,43 +5568,34 @@ static int complete_emulated_pio(struct kvm_vcpu *vcpu)
  *
  * read:
  *   for each fragment
- *     write gpa, len
- *     exit
- *     copy data
+ *      for each mmio piece in the fragment
+ *        write gpa, len
+ *        exit
+ *        copy data
  *   execute insn
  *
  * write:
  *   for each fragment
- *      write gpa, len
- *      copy data
- *      exit
+ *      for each mmio piece in the fragment
+ *         write gpa, len
+ *         copy data
+ *   exit
  */
 static int complete_emulated_mmio(struct kvm_vcpu *vcpu)
 {
-	struct kvm_run *run = vcpu->run;
-	struct kvm_mmio_fragment *frag;
-
 	BUG_ON(!vcpu->mmio_needed);
 
-	/* Complete previous fragment */
-	frag = &vcpu->mmio_fragments[vcpu->mmio_cur_fragment++];
-	if (!vcpu->mmio_is_write)
-		memcpy(frag->data, run->mmio.data, frag->len);
-	if (vcpu->mmio_cur_fragment == vcpu->mmio_nr_fragments) {
+	complete_current_mmio(vcpu);
+
+	/* Initiate next fragment */
+	if (!vcpu_fill_mmio_exit_info(vcpu)) {
 		vcpu->mmio_needed = 0;
 		if (vcpu->mmio_is_write)
 			return 1;
 		vcpu->mmio_read_completed = 1;
 		return complete_emulated_io(vcpu);
 	}
-	/* Initiate next fragment */
-	++frag;
-	run->exit_reason = KVM_EXIT_MMIO;
-	run->mmio.phys_addr = frag->gpa;
-	if (vcpu->mmio_is_write)
-		memcpy(run->mmio.data, frag->data, frag->len);
-	run->mmio.len = frag->len;
-	run->mmio.is_write = vcpu->mmio_is_write;
+
 	vcpu->arch.complete_userspace_io = complete_emulated_mmio;
 	return 0;
 }

include/linux/kvm_host.h +3 -13

index e235068..f6c3c2f 100644
@@ -42,19 +42,8 @@
  */
 #define KVM_MEMSLOT_INVALID	(1UL << 16)
 
-/*
- * If we support unaligned MMIO, at most one fragment will be split into two:
- */
-#ifdef KVM_UNALIGNED_MMIO
-#  define KVM_EXTRA_MMIO_FRAGMENTS 1
-#else
-#  define KVM_EXTRA_MMIO_FRAGMENTS 0
-#endif
-
-#define KVM_USER_MMIO_SIZE 8
-
-#define KVM_MAX_MMIO_FRAGMENTS \
-	(KVM_MMIO_SIZE / KVM_USER_MMIO_SIZE + KVM_EXTRA_MMIO_FRAGMENTS)
+/* Two fragments for cross MMIO pages. */
+#define KVM_MAX_MMIO_FRAGMENTS	2
 
 /*
  * For the normal pfn, the highest 12 bits should be zero,

@@ -203,6 +192,7 @@ struct kvm_mmio_fragment {
 	gpa_t gpa;
 	void *data;
 	unsigned len;
+	unsigned pos;
 };
 
 struct kvm_vcpu {

Signature · 1 line

1.7.7.6

Quoted 18 lines · Xiao Guangrong

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Quoted 5 lines

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace. After that, we only need two entries to store mmio info for the cross-mmio pages access

I wonder can we put the data into coalesced mmio buffer instead of

exiting for each 8 bytes? Is it worth the complexity?

Quoted 24 lines · Gleb Natapov

Quoted 23 lines · Xiao Guangrong

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace. After that, we only need two entries to store mmio info for the cross-mmio pages access

Quoted 1 line

I wonder can we put the data into coalesced mmio buffer instead of

If we put all mmio data into coalesced buffer, we should:
- ensure the userspace program uses KVM_REGISTER_COALESCED_MMIO to register all mmio regions.

- even if the MMIO region is not used by emulated-device, it also need to be registered.

It will breaks old version userspace program.

Quoted 1 line

exiting for each 8 bytes? Is it worth the complexity?

Simpler way is always better but i failed, so i appreciate your guys comments.

Quoted 27 lines · Xiao Guangrong

Quoted 25 lines · Gleb Natapov

Quoted 18 lines · Xiao Guangrong

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Quoted 5 lines

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace. After that, we only need two entries to store mmio info for the cross-mmio pages access

I wonder can we put the data into coalesced mmio buffer instead of

Quoted 4 lines

If we put all mmio data into coalesced buffer, we should:
- ensure the userspace program uses KVM_REGISTER_COALESCED_MMIO to register all mmio regions.

It appears to not be so.
Userspace calls kvm_flush_coalesced_mmio_buffer() after returning from KVM_RUN which looks like this:

Quoted 32 lines · Gleb Natapov

Quoted 31 lines · Xiao Guangrong

Quoted 19 lines · Gleb Natapov

Quoted 18 lines · Xiao Guangrong

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Quoted 6 lines

Quoted 5 lines

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace. After that, we only need two entries to store mmio info for the cross-mmio pages access

I wonder can we put the data into coalesced mmio buffer instead of

If we put all mmio data into coalesced buffer, we should:
- ensure the userspace program uses KVM_REGISTER_COALESCED_MMIO to register all mmio regions.

Quoted 3 lines

It appears to not be so.
Userspace calls kvm_flush_coalesced_mmio_buffer() after returning from KVM_RUN which looks like this:

Nope, no longer, only on accesses to devices that actually use such regions (and there are only two ATM). The current design of a global coalesced mmio ring is horrible /wrt latency.

Quoted 37 lines · Jan Kiszka

Quoted 35 lines · Gleb Natapov

Quoted 25 lines · Xiao Guangrong

Quoted 24 lines · Gleb Natapov

Quoted 23 lines · Xiao Guangrong

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace. After that, we only need two entries to store mmio info for the cross-mmio pages access

Quoted 6 lines

Quoted 1 line

I wonder can we put the data into coalesced mmio buffer instead of

If we put all mmio data into coalesced buffer, we should:
- ensure the userspace program uses KVM_REGISTER_COALESCED_MMIO to register all mmio regions.

It appears to not be so.
Userspace calls kvm_flush_coalesced_mmio_buffer() after returning from KVM_RUN which looks like this:

Quoted 4 lines

Nope, no longer, only on accesses to devices that actually use such regions (and there are only two ATM). The current design of a global coalesced mmio ring is horrible /wrt latency.

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 3 lines

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 6 lines

Quoted 3 lines

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

A) At least right now, we do not differentiate between the VGA modes and if flushing is needed. So that device is generally taboo for RT cores of the VM.
B) We need to disable coalescing in E1000 as well - if we want to use that model.
C) Gleb seems to propose using coalescing far beyond those two use cases.

Quoted 14 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 6 lines

Quoted 3 lines

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

Quoted 7 lines

A) At least right now, we do not differentiate between the VGA modes and if flushing is needed. So that device is generally taboo for RT cores of the VM.
B) We need to disable coalescing in E1000 as well - if we want to use that model.
C) Gleb seems to propose using coalescing far beyond those two use cases.

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Quoted 7 lines · Jan Kiszka

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

Quoted 3 lines

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Any mmio exit has to flush everything. For example a DMA caused by an e1000 write has to see any writes to the framebuffer, in case the guest is transmitting its framebuffer to the outside world.

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Quoted 3 lines

Any mmio exit has to flush everything. For example a DMA caused by an e1000 write has to see any writes to the framebuffer, in case the guest is transmitting its framebuffer to the outside world.

We already flush when that crazy guest actually accesses the region, no need to do this unconditionally.

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Quoted 7 lines

Quoted 3 lines

Any mmio exit has to flush everything. For example a DMA caused by an e1000 write has to see any writes to the framebuffer, in case the guest is transmitting its framebuffer to the outside world.

We already flush when that crazy guest actually accesses the region, no need to do this unconditionally.

What if framebuffer is accessed from inside the kernel? Is this case handled?

Quoted 18 lines · Gleb Natapov

Quoted 17 lines · Jan Kiszka

Quoted 15 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Any mmio exit has to flush everything. For example a DMA caused by an e1000 write has to see any writes to the framebuffer, in case the guest is transmitting its framebuffer to the outside world.

Quoted 4 lines

Quoted 3 lines

We already flush when that crazy guest actually accesses the region, no need to do this unconditionally.

What if framebuffer is accessed from inside the kernel? Is this case handled?

Unless I miss a case now, there is no direct access to the framebuffer possible when we are also doing coalescing. Everything needs to go through userspace.

Quoted 24 lines · Jan Kiszka

Quoted 22 lines · Gleb Natapov

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Quoted 7 lines

Quoted 3 lines

Any mmio exit has to flush everything. For example a DMA caused by an e1000 write has to see any writes to the framebuffer, in case the guest is transmitting its framebuffer to the outside world.

We already flush when that crazy guest actually accesses the region, no need to do this unconditionally.

What if framebuffer is accessed from inside the kernel? Is this case handled?

Quoted 4 lines

Unless I miss a case now, there is no direct access to the framebuffer possible when we are also doing coalescing. Everything needs to go through userspace.

Yes, with frame buffer is seems to be the case. One can imagine ROMD device that is MMIO on write but still can be accessed for read from kernel, but it cannot be coalesced even if coalesced buffer is flushed on every exit.

Yes, with frame buffer is seems to be the case. One can imagine ROMD device that is MMIO on write but still can be accessed for read from kernel, but it cannot be coalesced even if coalesced buffer is flushed on every exit.

Quoted 22 lines · Gleb Natapov

Quoted 14 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 6 lines

Quoted 3 lines

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

Quoted 7 lines

A) At least right now, we do not differentiate between the VGA modes and if flushing is needed. So that device is generally taboo for RT cores of the VM.
B) We need to disable coalescing in E1000 as well - if we want to use that model.
C) Gleb seems to propose using coalescing far beyond those two use cases.

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

When taking an exit for A, I'm not interesting in flushing stuff for B unless I have a dependency. Thus, buffers would have to be per device before extending their use.

Quoted 14 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 6 lines

Quoted 3 lines

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

Quoted 7 lines

A) At least right now, we do not differentiate between the VGA modes and if flushing is needed. So that device is generally taboo for RT cores of the VM.
B) We need to disable coalescing in E1000 as well - if we want to use that model.
C) Gleb seems to propose using coalescing far beyond those two use cases.

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

Quoted 22 lines · Gleb Natapov

Quoted 14 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Quoted 6 lines

Quoted 3 lines

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

Quoted 7 lines

A) At least right now, we do not differentiate between the VGA modes and if flushing is needed. So that device is generally taboo for RT cores of the VM.
B) We need to disable coalescing in E1000 as well - if we want to use that model.
C) Gleb seems to propose using coalescing far beyond those two use cases.

Quoted 6 lines

Since the userspace change is needed the idea is dead, but if we could implement it I do not see how it can hurt the latency if it would be the only mechanism to use coalesced mmio buffer. Checking that the ring buffer is empty is cheap and if it is not empty it means that kernel just saved you a lot of 8 bytes exists so even after iterating over all the entries there you still saved a lot of time.

It's time where the guest cannot take interrupts, and time in a high priority guest thread that is spent processing low guest priority requests.

Quoted 3 lines

It's time where the guest cannot take interrupts, and time in a high priority guest thread that is spent processing low guest priority requests.

Proposed fix has exactly same issue. Until all data is transfered to userspace no interrupt will be served.

Quoted 3 lines · Gleb Natapov

Quoted 3 lines

It's time where the guest cannot take interrupts, and time in a high priority guest thread that is spent processing low guest priority requests.

Quoted 2 lines

Proposed fix has exactly same issue. Until all data is transfered to userspace no interrupt will be served.

For mmio_fragments that is okay. It's the same guest instruction, and it's still O(1).

It's not okay for general mmio coalescing.

Quoted 10 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Quoted 3 lines

Indeed. git pull, recheck and call for kvm_flush_coalesced_mmio_buffer() is gone. So this will break new userspace, not old. By global you mean shared between devices (or memory regions)?

Yes. We only have a single ring per VM, so we cannot flush multi-second VGA access separately from other devices. In theory solvable by introducing per-region rings that can be driven separately.

Quoted 2 lines

But in practice unneeded. Real time VMs can disable coalescing and not use planar VGA modes.

A) At least right now, we do not differentiate between the VGA modes and if flushing is needed. So that device is generally taboo for RT cores of the VM.

B) We need to disable coalescing in E1000 as well - if we want to use that model.

After commit b3356bf0dbb349 (KVM: emulator: optimize "rep ins" handling), the pieces of io data can be collected and write them to the guest memory or MMIO together.

Unfortunately, kvm splits the mmio access into 8 bytes and store them to vcpu->mmio_fragments. If the guest uses "rep ins" to move large data, it will cause vcpu->mmio_fragments overflow

The bug can be exposed by isapc (-M isapc):

[23154.818733] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ ......]
[23154.858083] Call Trace:
[23154.859874]  [<ffffffffa04f0e17>] kvm_get_cr8+0x1d/0x28 [kvm]
[23154.861677]  [<ffffffffa04fa6d4>] kvm_arch_vcpu_ioctl_run+0xcda/0xe45 [kvm]
[23154.863604]  [<ffffffffa04f5a1a>] ? kvm_arch_vcpu_load+0x17b/0x180 [kvm]

Actually, we can use one mmio_fragment to store a large mmio access for the mmio access is always continuous then split it when we pass the mmio-exit-info to userspace.

After that, we only need two entries to store mmio info for the cross-mmio pages access

arch/x86/kvm/x86.c +5 -3

index 8b90dd5..41ceb51 100644
@@ -3779,9 +3779,6 @@ static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
 static int write_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa,
 			   void *val, int bytes)
 {
-	struct kvm_mmio_fragment *frag = &vcpu->mmio_fragments[0];
-
-	memcpy(vcpu->run->mmio.data, frag->data, frag->len);
 	return X86EMUL_CONTINUE;
 }
 

@@ -3799,6 +3796,64 @@ static const struct read_write_emulator_ops write_emultor = {
 	.write = true,
 };
 
+static bool get_current_mmio_info(struct kvm_vcpu *vcpu, gpa_t *gpa,
+				  unsigned *len, void **data)
+{
+	struct kvm_mmio_fragment *frag;
+	int cur = vcpu->mmio_cur_fragment;

arch/x86/kvm/x86.c +14 -0

+
+	*gpa = frag->gpa + frag->pos;
+	*data = frag->data + frag->pos;
+	*len = min(8u, frag->len - frag->pos);
+	return true;
+}
+
+static void complete_current_mmio(struct kvm_vcpu *vcpu)
+{
+	struct kvm_mmio_fragment *frag;
+	gpa_t gpa;
+	unsigned len;
+	void *data;
+