KVM: x86: remvoe unnecessary mark_page_dirty

PATCH Applied Xiao Guangrong 2 participants 25 messages Jul 17, 2012 – Jul 23, 2012

Subsystems (5)

x86 KVM PowerPC KVM x86 +1 more subsystems

lore

Share & tools

lore

Xiao Guangrong

author

2012-07-17 13:51 UTC #0 lore raw

PATCH 1/9 KVM: x86: remvoe unnecessary mark_page_dirty

fix:

[  132.474633] 3.5.0-rc1+ #50 Not tainted
[  132.474634] -------------------------------
[  132.474635] include/linux/kvm_host.h:369 suspicious rcu_dereference_check() usage!
[  132.474636]
[  132.474636] other info that might help us debug this:
[  132.474636]
[  132.474638]
[  132.474638] rcu_scheduler_active = 1, debug_locks = 1
[  132.474640] 1 lock held by qemu-kvm/2832:
[  132.474657]  #0:  (&vcpu->mutex){+.+.+.}, at: [<ffffffffa01e1636>] vcpu_load+0x1e/0x91 [kvm]
[  132.474658]
[  132.474658] stack backtrace:
[  132.474660] Pid: 2832, comm: qemu-kvm Not tainted 3.5.0-rc1+ #50
[  132.474661] Call Trace:
[  132.474665]  [<ffffffff81092f40>] lockdep_rcu_suspicious+0xfc/0x105
[  132.474675]  [<ffffffffa01e0c85>] kvm_memslots+0x6d/0x75 [kvm]
[  132.474683]  [<ffffffffa01e0ca1>] gfn_to_memslot+0x14/0x4c [kvm]
[  132.474693]  [<ffffffffa01e3575>] mark_page_dirty+0x17/0x2a [kvm]
[  132.474706]  [<ffffffffa01f21ea>] kvm_arch_vcpu_ioctl+0xbcf/0xc07 [kvm]

Actually, we do not write vcpu->arch.time at this time, mark_page_dirty should be removed

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

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

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

index ff0b487..875b8d8 100644
@@ -2615,7 +2615,6 @@ static int kvm_set_guest_paused(struct kvm_vcpu *vcpu)
 	if (!vcpu->arch.time_page)
 		return -EINVAL;
 	src->flags |= PVCLOCK_GUEST_STOPPED;
-	mark_page_dirty(vcpu->kvm, vcpu->arch.time >> PAGE_SHIFT);
 	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 	return 0;
 }

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-07-17 13:52 UTC #1 lore raw

PATCH 2/9 KVM: x86: simplify read_emulated

No need split mmio read region into 8-bits pieces since we do it in emulator_read_write_onepage

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

---
 arch/x86/kvm/emulate.c |   29 ++++++++++++-----------------
 1 files changed, 12 insertions(+), 17 deletions(-)

arch/x86/kvm/emulate.c +12 ~~-17~~

index 97d9a99..2d1916b 100644
@@ -1166,24 +1166,19 @@ static int read_emulated(struct x86_emulate_ctxt *ctxt,
 	int rc;
 	struct read_cache *mc = &ctxt->mem_read;
 
-	while (size) {
-		int n = min(size, 8u);
-		size -= n;
-		if (mc->pos < mc->end)
-			goto read_cached;
-
-		rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, n,
-					      &ctxt->exception);
-		if (rc != X86EMUL_CONTINUE)
-			return rc;
-		mc->end += n;
+	if (mc->pos < mc->end)
+		goto read_cached;
 
-	read_cached:
-		memcpy(dest, mc->data + mc->pos, n);
-		mc->pos += n;
-		dest += n;
-		addr += n;
-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);
+	mc->pos += size;
 	return X86EMUL_CONTINUE;
 }

Signature · 1 line

1.7.7.6

Marcelo Tosatti

2012-07-20 00:48 UTC #2 lore raw

Re: [PATCH 2/9] KVM: x86: simplify read_emulated

Quoted 35 lines · Xiao Guangrong

No need split mmio read region into 8-bits pieces since we do it in emulator_read_write_onepage

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

---
 arch/x86/kvm/emulate.c |   29 ++++++++++++-----------------
 1 files changed, 12 insertions(+), 17 deletions(-)

arch/x86/kvm/emulate.c +2 ~~-16~~

index 97d9a99..2d1916b 100644
@@ -1166,24 +1166,19 @@ static int read_emulated(struct x86_emulate_ctxt *ctxt,
 	int rc;
 	struct read_cache *mc = &ctxt->mem_read;
 
-	while (size) {
-		int n = min(size, 8u);
-		size -= n;
-		if (mc->pos < mc->end)
-			goto read_cached;
-
-		rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, n,
-					      &ctxt->exception);
-		if (rc != X86EMUL_CONTINUE)
-			return rc;
-		mc->end += n;
+	if (mc->pos < mc->end)
+		goto read_cached;
 
-	read_cached:
-		memcpy(dest, mc->data + mc->pos, n);
-		mc->pos += n;
-		dest += n;
-		addr += n;

arch/x86/kvm/emulate.c static int read_emulated(struct x86_emulate_ctxt *ctxt, · line 1180

Quoted 10 lines

arch/x86/kvm/emulate.c +9 -1

-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);

What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?

Quoted 3 lines

arch/x86/kvm/emulate.c +1 -0

+	mc->pos += size;
 	return X86EMUL_CONTINUE;
 }

Xiao Guangrong

author

2012-07-20 02:17 UTC #3 lore raw

Re: [PATCH 2/9] KVM: x86: simplify read_emulated

Quoted 5 lines · Marcelo Tosatti

Quoted 5 lines

arch/x86/kvm/emulate.c +4 -1

-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;

Quoted 8 lines

arch/x86/kvm/emulate.c static int read_emulated(struct x86_emulate_ctxt *ctxt, · line 1180
Quoted 5 lines
arch/x86/kvm/emulate.c +5 -0
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);
What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?

Marcelo,

The splitting has been done in emulator_read_write_onepage:

	while (bytes) {
		unsigned now = min(bytes, 8U);

		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
		frag->gpa = gpa;
		frag->data = val;
		frag->len = now;
		frag->write_readonly_mem = (ret == -EPERM);

		gpa += now;
		val += now;
		bytes -= now;
	}

So i think it is safe to remove the splitting in read_emulated.

Marcelo Tosatti

2012-07-20 10:58 UTC #4 lore raw

Re: [PATCH 2/9] KVM: x86: simplify read_emulated

Quoted 34 lines · Xiao Guangrong

Quoted 5 lines · Marcelo Tosatti
Quoted 5 lines
arch/x86/kvm/emulate.c +4 -1
-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
Quoted 8 lines
arch/x86/kvm/emulate.c static int read_emulated(struct x86_emulate_ctxt *ctxt, · line 1180
Quoted 5 lines
arch/x86/kvm/emulate.c +5 -0
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);
What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?
Marcelo,
The splitting has been done in emulator_read_write_onepage:
	while (bytes) {
		unsigned now = min(bytes, 8U);

		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
		frag->gpa = gpa;
		frag->data = val;
		frag->len = now;
		frag->write_readonly_mem = (ret == -EPERM);

		gpa += now;
		val += now;
		bytes -= now;
	}

Quoted 1 line

So i think it is safe to remove the splitting in read_emulated.

Yes, it is fine to remove it.

But splitting in emulate.c prevented the case of _cache read_ with size

Quoted 1 line

8 beyond end of mc->data. Must handle that case in read_emulated.

"What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8) now?"

Xiao Guangrong

author

2012-07-20 13:15 UTC #5 lore raw

Re: [PATCH 2/9] KVM: x86: simplify read_emulated

Quoted 35 lines · Marcelo Tosatti

Quoted 34 lines · Xiao Guangrong
Quoted 5 lines · Marcelo Tosatti
Quoted 5 lines
arch/x86/kvm/emulate.c +4 -1
-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
Quoted 8 lines
arch/x86/kvm/emulate.c static int read_emulated(struct x86_emulate_ctxt *ctxt, · line 1180
Quoted 5 lines
arch/x86/kvm/emulate.c +5 -0
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);
What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?
Marcelo,
The splitting has been done in emulator_read_write_onepage:
	while (bytes) {
		unsigned now = min(bytes, 8U);

		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
		frag->gpa = gpa;
		frag->data = val;
		frag->len = now;
		frag->write_readonly_mem = (ret == -EPERM);

		gpa += now;
		val += now;
		bytes -= now;
	}

Quoted 8 lines

Quoted 1 line
So i think it is safe to remove the splitting in read_emulated.
Yes, it is fine to remove it.
But splitting in emulate.c prevented the case of _cache read_ with size
Quoted 1 line
8 beyond end of mc->data. Must handle that case in read_emulated.
"What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8) now?"

You mean the mmio region is partly cached?

I think it can not happen. Now, we pass the whole size to emulator_read_write_onepage(), after it is finished, it saves the whole data into mc->data[], so, the cache-read can always get the whole data from mc->data[].

Marcelo Tosatti

2012-07-20 19:52 UTC #6 lore raw

Re: [PATCH 2/9] KVM: x86: simplify read_emulated

Quoted 43 lines · Xiao Guangrong

Quoted 42 lines · Marcelo Tosatti
Quoted 34 lines · Xiao Guangrong
Quoted 5 lines · Marcelo Tosatti
Quoted 5 lines
arch/x86/kvm/emulate.c +4 -1
-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
Quoted 8 lines
arch/x86/kvm/emulate.c static int read_emulated(struct x86_emulate_ctxt *ctxt, · line 1180
Quoted 5 lines
arch/x86/kvm/emulate.c +5 -0
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);
What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?
Marcelo,
The splitting has been done in emulator_read_write_onepage:
	while (bytes) {
		unsigned now = min(bytes, 8U);

		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
		frag->gpa = gpa;
		frag->data = val;
		frag->len = now;
		frag->write_readonly_mem = (ret == -EPERM);

		gpa += now;
		val += now;
		bytes -= now;
	}
Quoted 1 line
So i think it is safe to remove the splitting in read_emulated.
Yes, it is fine to remove it.
But splitting in emulate.c prevented the case of _cache read_ with size
Quoted 1 line
8 beyond end of mc->data. Must handle that case in read_emulated.

Quoted 7 lines

Quoted 1 line
"What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8) now?"
You mean the mmio region is partly cached?
I think it can not happen. Now, we pass the whole size to emulator_read_write_onepage(), after it is finished, it saves the whole data into mc->data[], so, the cache-read can always get the whole data from mc->data[].

I mean that nothing prevents a caller from reading beyond the end of mc->data array (but then again this was the previous behavior).

ACK

Xiao Guangrong

author

2012-07-23 04:23 UTC #7 lore raw

Re: [PATCH 2/9] KVM: x86: simplify read_emulated

Quoted 46 lines · Marcelo Tosatti

Quoted 45 lines · Xiao Guangrong
Quoted 43 lines · Marcelo Tosatti
Quoted 34 lines · Xiao Guangrong
Quoted 5 lines · Marcelo Tosatti
Quoted 5 lines
arch/x86/kvm/emulate.c +4 -1
-	}
+	rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+				      &ctxt->exception);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
Quoted 8 lines
arch/x86/kvm/emulate.c static int read_emulated(struct x86_emulate_ctxt *ctxt, · line 1180
Quoted 5 lines
arch/x86/kvm/emulate.c +5 -0
+
+	mc->end += size;
+
+read_cached:
+	memcpy(dest, mc->data + mc->pos, size);
What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?
Marcelo,
The splitting has been done in emulator_read_write_onepage:
	while (bytes) {
		unsigned now = min(bytes, 8U);

		frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
		frag->gpa = gpa;
		frag->data = val;
		frag->len = now;
		frag->write_readonly_mem = (ret == -EPERM);

		gpa += now;
		val += now;
		bytes -= now;
	}
Quoted 1 line
So i think it is safe to remove the splitting in read_emulated.
Yes, it is fine to remove it.
But splitting in emulate.c prevented the case of _cache read_ with size
Quoted 1 line
8 beyond end of mc->data. Must handle that case in read_emulated.
"What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8) now?"

Quoted 8 lines

Quoted 5 lines
You mean the mmio region is partly cached?
I think it can not happen. Now, we pass the whole size to emulator_read_write_onepage(), after it is finished, it saves the whole data into mc->data[], so, the cache-read can always get the whole data from mc->data[].
I mean that nothing prevents a caller from reading beyond the end of mc->data array (but then again this was the previous behavior).

1024 bytes should be enough for instructions, may be we can add a WARN_ON to check buffer-overflow.

Quoted 3 lines

ACK

Thank you, Marcelo!

Xiao Guangrong

author

2012-07-17 13:52 UTC #8 lore raw

PATCH 3/9 KVM: x86: introduce set_mmio_exit_info

In reply to Xiao Guangrong

Introduce set_mmio_exit_info to cleanup the common code

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

---
 arch/x86/kvm/x86.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

arch/x86/kvm/x86.c +17 ~~-16~~

index 875b8d8..8171836 100644
@@ -3759,9 +3759,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;
 }

@@ -3829,6 +3826,20 @@ mmio:
 	return X86EMUL_CONTINUE;
 }
 
+static void set_mmio_exit_info(struct kvm_vcpu *vcpu,
+			       struct kvm_mmio_fragment *frag, bool write)
+{
+	struct kvm_run *run = vcpu->run;
+
+	run->exit_reason = KVM_EXIT_MMIO;
+	run->mmio.phys_addr = frag->gpa;
+	run->mmio.len = frag->len;
+	run->mmio.is_write = vcpu->mmio_is_write = write;
+
+	if (write)
+		memcpy(run->mmio.data, frag->data, frag->len);
+}
+
 int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 			void *val, unsigned int bytes,
 			struct x86_exception *exception,

@@ -3868,14 +3879,10 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
 		return rc;
 
 	gpa = vcpu->mmio_fragments[0].gpa;
-
 	vcpu->mmio_needed = 1;
 	vcpu->mmio_cur_fragment = 0;
 
-	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;
+	set_mmio_exit_info(vcpu, &vcpu->mmio_fragments[0], ops->write);
 
 	return ops->read_write_exit_mmio(vcpu, gpa, val, bytes);
 }

@@ -5485,7 +5492,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
  */
 static int complete_mmio(struct kvm_vcpu *vcpu)
 {
-	struct kvm_run *run = vcpu->run;
 	struct kvm_mmio_fragment *frag;
 	int r;

@@ -5496,7 +5502,7 @@ static int complete_mmio(struct kvm_vcpu *vcpu)
 		/* Complete previous fragment */
 		frag = &vcpu->mmio_fragments[vcpu->mmio_cur_fragment++];
 		if (!vcpu->mmio_is_write)
-			memcpy(frag->data, run->mmio.data, frag->len);
+			memcpy(frag->data, vcpu->run->mmio.data, frag->len);
 		if (vcpu->mmio_cur_fragment == vcpu->mmio_nr_fragments) {
 			vcpu->mmio_needed = 0;
 			if (vcpu->mmio_is_write)

@@ -5506,12 +5512,7 @@ static int complete_mmio(struct kvm_vcpu *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;
+		set_mmio_exit_info(vcpu, frag, vcpu->mmio_is_write);
 		return 0;
 
 	}

Signature · 1 line

1.7.7.6

Marcelo Tosatti

2012-07-20 00:48 UTC #9 lore raw

Re: [PATCH 3/9] KVM: x86: introduce set_mmio_exit_info

Quoted 6 lines · Xiao Guangrong

Introduce set_mmio_exit_info to cleanup the common code
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/x86.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

This makes the code less readable IMO. The fragments are small enough already.

Xiao Guangrong

author

2012-07-20 02:24 UTC #10 lore raw

Re: [PATCH 3/9] KVM: x86: introduce set_mmio_exit_info

Quoted 11 lines · Marcelo Tosatti

Quoted 6 lines · Xiao Guangrong
Introduce set_mmio_exit_info to cleanup the common code
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/x86.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)
This makes the code less readable IMO. The fragments are small enough already.

I think the function name set_mmio_exit_info has the better explanation - it generates a mmio-exit from the specified fragment.

Actually, the later patchset, readonly memeslot will set a new member in the mmio-exit-info:

	run->mmio.write_readonly_mem = frag->write_readonly_mem;

If we wrap this operation up, i think the code should be neater. :)

Xiao Guangrong

author

2012-07-17 13:52 UTC #11 lore raw

PATCH 4/9 KVM: MMU: track the refcount when unmap the page

In reply to Xiao Guangrong

It will trigger a WARN_ON if the page has been freed but it is still used in mmu, it can help us to detect mm bug early

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

---
 arch/x86/kvm/mmu.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

arch/x86/kvm/mmu.c +8 -0

index 28c8fbc..28b12e2 100644
@@ -556,6 +556,14 @@ static int mmu_spte_clear_track_bits(u64 *sptep)
 		return 0;
 
 	pfn = spte_to_pfn(old_spte);
+
+	/*
+	 * KVM does not hold the refcount of the page used by
+	 * kvm mmu, before reclaiming the page, we should
+	 * unmap it from mmu first.
+	 */
+	WARN_ON(!kvm_is_mmio_pfn(pfn) && !page_count(pfn_to_page(pfn)));
+
 	if (!shadow_accessed_mask || old_spte & shadow_accessed_mask)
 		kvm_set_pfn_accessed(pfn);
 	if (!shadow_dirty_mask || (old_spte & shadow_dirty_mask))

Signature · 1 line

1.7.7.6

Marcelo Tosatti

Applied

2012-07-20 00:09 UTC #12 lore raw

Re: [PATCH 4/9] KVM: MMU: track the refcount when unmap the page

Quoted 7 lines · Xiao Guangrong

It will trigger a WARN_ON if the page has been freed but it is still used in mmu, it can help us to detect mm bug early
Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

Applied, thanks.

Xiao Guangrong

author

2012-07-17 13:53 UTC #13 lore raw

PATCH 5/9 KVM: MMU: fask check write-protect for direct mmu

In reply to Xiao Guangrong

If it have no indirect shadow pages we need not protect any gfn, this is always true for direct mmu without nested

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

---
 arch/x86/kvm/mmu.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

arch/x86/kvm/mmu.c +3 -0

index 28b12e2..a846a9c 100644
@@ -2294,6 +2294,9 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 	struct hlist_node *node;
 	bool need_unsync = false;
 
+	if (!vcpu->kvm->arch.indirect_shadow_pages)
+		return 0;
+
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn, node) {
 		if (!can_unsync)
 			return 1;

Signature · 1 line

1.7.7.6

Marcelo Tosatti

2012-07-20 00:39 UTC #14 lore raw

Re: [PATCH 5/9] KVM: MMU: fask check write-protect for direct mmu

Quoted 4 lines · Xiao Guangrong

If it have no indirect shadow pages we need not protect any gfn, this is always true for direct mmu without nested
Signed-off-by Xiao Guangrong <[email protected]>

Xiao,

What is the motivation? Numbers please.

In fact, what case was the original indirect_shadow_pages conditional in kvm_mmu_pte_write optimizing again?

Xiao Guangrong

author

2012-07-20 02:34 UTC #15 lore raw

Re: [PATCH 5/9] KVM: MMU: fask check write-protect for direct mmu

Quoted 10 lines · Marcelo Tosatti

Quoted 4 lines · Xiao Guangrong
If it have no indirect shadow pages we need not protect any gfn, this is always true for direct mmu without nested
Signed-off-by Xiao Guangrong <[email protected]>
Xiao,
What is the motivation? Numbers please.

mmu_need_write_protect is the common path for both soft-mmu and hard-mmu, checking indirect_shadow_pages can skip hash-table walking for the case which is tdp is enabled without nested guest.

I will post the Number after I do the performance test.

Quoted 3 lines

In fact, what case was the original indirect_shadow_pages conditional in kvm_mmu_pte_write optimizing again?

They are the different paths, mmu_need_write_protect is the real page fault path, and kvm_mmu_pte_write is caused by mmio emulation.

Marcelo Tosatti

2012-07-20 12:02 UTC #16 lore raw

Re: [PATCH 5/9] KVM: MMU: fask check write-protect for direct mmu

Quoted 7 lines · Xiao Guangrong

Quoted 6 lines · Marcelo Tosatti
Quoted 4 lines · Xiao Guangrong
If it have no indirect shadow pages we need not protect any gfn, this is always true for direct mmu without nested
Signed-off-by Xiao Guangrong <[email protected]>

Quoted 8 lines

Quoted 4 lines
Xiao,
What is the motivation? Numbers please.
mmu_need_write_protect is the common path for both soft-mmu and hard-mmu, checking indirect_shadow_pages can skip hash-table walking for the case which is tdp is enabled without nested guest.

I mean motivation as observation that it is a bottleneck.

Quoted 8 lines

I will post the Number after I do the performance test.
Quoted 3 lines
In fact, what case was the original indirect_shadow_pages conditional in kvm_mmu_pte_write optimizing again?
They are the different paths, mmu_need_write_protect is the real page fault path, and kvm_mmu_pte_write is caused by mmio emulation.

Sure. What i am asking is, what use case is the indirect_shadow_pages optimizing? What scenario, what workload?

See the "When to optimize" section of
http://en.wikipedia.org/wiki/Program_optimization.

Can't remember why indirect_shadow_pages was introduced in kvm_mmu_pte_write.

Xiao Guangrong

author

2012-07-20 13:33 UTC #17 lore raw

Re: [PATCH 5/9] KVM: MMU: fask check write-protect for direct mmu

Quoted 25 lines · Marcelo Tosatti

Quoted 7 lines · Xiao Guangrong
Quoted 6 lines · Marcelo Tosatti
Quoted 4 lines · Xiao Guangrong
If it have no indirect shadow pages we need not protect any gfn, this is always true for direct mmu without nested
Signed-off-by Xiao Guangrong <[email protected]>
Quoted 8 lines
Quoted 4 lines
Xiao,
What is the motivation? Numbers please.
mmu_need_write_protect is the common path for both soft-mmu and hard-mmu, checking indirect_shadow_pages can skip hash-table walking for the case which is tdp is enabled without nested guest.
I mean motivation as observation that it is a bottleneck.
Quoted 6 lines
I will post the Number after I do the performance test.
Quoted 3 lines
In fact, what case was the original indirect_shadow_pages conditional in kvm_mmu_pte_write optimizing again?

Quoted 6 lines

Quoted 2 lines
They are the different paths, mmu_need_write_protect is the real page fault path, and kvm_mmu_pte_write is caused by mmio emulation.
Sure. What i am asking is, what use case is the indirect_shadow_pages optimizing? What scenario, what workload?

Sorry, Marcelo, i do know why i completely misunderstood your mail. :(

I am not sure whether this is a bottleneck, i just got it from code review, i will measure it to see if we can get benefit from it. :p

Quoted 6 lines

See the "When to optimize" section of
http://en.wikipedia.org/wiki/Program_optimization.
Can't remember why indirect_shadow_pages was introduced in kvm_mmu_pte_write.

Please refer to:

	https://lkml.org/lkml/2011/5/18/174

Xiao Guangrong

author

2012-07-20 03:45 UTC #18 lore raw

Re: [PATCH 5/9] KVM: MMU: fask check write-protect for direct mmu

In reply to Marcelo Tosatti

BTW, they are some bug fix patches on -master branch, but it is not existed on -next branch:
commit: f411930442e01f9cf1bf4df41ff7e89476575c4d
commit: 85b7059169e128c57a3a8a3e588fb89cb2031da1

It causes code conflict if we do the development on -next.

On 07/20/2012 08:39 AM, Marcelo Tosatti wrote:can

Quoted 15 lines · Xiao Guangrong

Quoted 4 lines
If it have no indirect shadow pages we need not protect any gfn, this is always true for direct mmu without nested
Signed-off-by Xiao Guangrong <[email protected]>
Xiao,
What is the motivation? Numbers please.
In fact, what case was the original indirect_shadow_pages conditional in kvm_mmu_pte_write optimizing again?

Marcelo Tosatti

2012-07-20 11:45 UTC #19 lore raw

Re: [PATCH 5/9] KVM: MMU: fask check write-protect for direct mmu

Quoted 6 lines · Xiao Guangrong

BTW, they are some bug fix patches on -master branch, but it is not existed on -next branch:
commit: f411930442e01f9cf1bf4df41ff7e89476575c4d
commit: 85b7059169e128c57a3a8a3e588fb89cb2031da1
It causes code conflict if we do the development on -next.

See auto-next branch.

http://www.linux-kvm.org/page/Kvm-Git-Workflow

Xiao Guangrong

author

2012-07-17 13:54 UTC #20 lore raw

PATCH 6/9 KVM: using get_fault_pfn to get the fault pfn

In reply to Xiao Guangrong

Using get_fault_pfn to cleanup the code

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

---
 arch/x86/kvm/mmu.c       |    6 ++----
 include/linux/kvm_host.h |    5 +----
 virt/kvm/kvm_main.c      |   13 ++++---------
 3 files changed, 7 insertions(+), 17 deletions(-)

arch/x86/kvm/mmu.c +2 -4

index a846a9c..e23cdae 100644
@@ -2483,10 +2483,8 @@ static pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
 	unsigned long hva;
 
 	slot = gfn_to_memslot_dirty_bitmap(vcpu, gfn, no_dirty_log);
-	if (!slot) {
-		get_page(fault_page);
-		return page_to_pfn(fault_page);
-	}
+	if (!slot)
+		return get_fault_pfn();
 
 	hva = gfn_to_hva_memslot(slot, gfn);

include/linux/kvm_host.h +1 -4

index e3c86f8..258b35a 100644
@@ -383,15 +383,11 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 static inline int is_error_hpa(hpa_t hpa) { return hpa >> HPA_MSB; }
 
 extern struct page *bad_page;
-extern struct page *fault_page;
-
 extern pfn_t bad_pfn;
-extern pfn_t fault_pfn;
 
 int is_error_page(struct page *page);
 int is_error_pfn(pfn_t pfn);
 int is_hwpoison_pfn(pfn_t pfn);
-int is_fault_pfn(pfn_t pfn);
 int is_noslot_pfn(pfn_t pfn);
 int is_invalid_pfn(pfn_t pfn);
 int kvm_is_error_hva(unsigned long addr);

@@ -441,6 +437,7 @@ void kvm_release_pfn_clean(pfn_t pfn);
 void kvm_set_pfn_dirty(pfn_t pfn);
 void kvm_set_pfn_accessed(pfn_t pfn);
 void kvm_get_pfn(pfn_t pfn);
+pfn_t get_fault_pfn(void);
 
 int kvm_read_guest_page(struct kvm *kvm, gfn_t gfn, void *data, int offset,
 			int len);

virt/kvm/kvm_main.c +4 -9

index 60af030..feef72e 100644
@@ -103,8 +103,8 @@ static bool largepages_enabled = true;
 static struct page *hwpoison_page;
 static pfn_t hwpoison_pfn;
 
-struct page *fault_page;
-pfn_t fault_pfn;
+static struct page *fault_page;
+static pfn_t fault_pfn;
 
 inline int kvm_is_mmio_pfn(pfn_t pfn)
 {

@@ -950,12 +950,6 @@ int is_hwpoison_pfn(pfn_t pfn)
 }
 EXPORT_SYMBOL_GPL(is_hwpoison_pfn);
 
-int is_fault_pfn(pfn_t pfn)
-{
-	return pfn == fault_pfn;
-}
-EXPORT_SYMBOL_GPL(is_fault_pfn);
-
 int is_noslot_pfn(pfn_t pfn)
 {
 	return pfn == bad_pfn;

@@ -1039,11 +1033,12 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
 }
 EXPORT_SYMBOL_GPL(gfn_to_hva);
 
-static pfn_t get_fault_pfn(void)
+pfn_t get_fault_pfn(void)
 {
 	get_page(fault_page);
 	return fault_pfn;
 }
+EXPORT_SYMBOL_GPL(get_fault_pfn);
 
 int get_user_page_nowait(struct task_struct *tsk, struct mm_struct *mm,
 	unsigned long start, int write, struct page **page)

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-07-17 13:55 UTC #21 lore raw

PATCH 7/9 KVM: mark do not extern bad_pfn

In reply to Xiao Guangrong

bad_pfn is not used out of kvm_main.c, so mark it static, also move it near hwpoison_pfn and fault_pfn

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

---
 include/linux/kvm_host.h |    1 -
 virt/kvm/kvm_main.c      |    6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

include/linux/kvm_host.h +0 -1

index 258b35a..bafcc56 100644
@@ -383,7 +383,6 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 static inline int is_error_hpa(hpa_t hpa) { return hpa >> HPA_MSB; }
 
 extern struct page *bad_page;
-extern pfn_t bad_pfn;
 
 int is_error_page(struct page *page);
 int is_error_pfn(pfn_t pfn);

virt/kvm/kvm_main.c +3 -3

index feef72e..7082e33 100644
@@ -100,6 +100,9 @@ EXPORT_SYMBOL_GPL(kvm_rebooting);
 
 static bool largepages_enabled = true;
 
+struct page *bad_page;
+static pfn_t bad_pfn;
+
 static struct page *hwpoison_page;
 static pfn_t hwpoison_pfn;

@@ -2692,9 +2695,6 @@ static struct syscore_ops kvm_syscore_ops = {
 	.resume = kvm_resume,
 };
 
-struct page *bad_page;
-pfn_t bad_pfn;
-
 static inline
 struct kvm_vcpu *preempt_notifier_to_vcpu(struct preempt_notifier *pn)
 {

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-07-17 13:55 UTC #22 lore raw

PATCH 8/9 KVM: remove is_error_hpa

In reply to Xiao Guangrong

Remove them since they are not used anymore

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

---
 include/linux/kvm_host.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

include/linux/kvm_host.h +0 -4

index bafcc56..f1ef5fc 100644
@@ -378,10 +378,6 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 	return slot;
 }
 
-#define HPA_MSB ((sizeof(hpa_t) * 8) - 1)
-#define HPA_ERR_MASK ((hpa_t)1 << HPA_MSB)
-static inline int is_error_hpa(hpa_t hpa) { return hpa >> HPA_MSB; }
-
 extern struct page *bad_page;
 
 int is_error_page(struct page *page);

Signature · 1 line

1.7.7.6

Xiao Guangrong

author

2012-07-17 13:56 UTC #23 lore raw

PATCH 9/9 KVM: remove the unused parameter of gfn_to_pfn_memslot

In reply to Xiao Guangrong

The parameter, 'kvm', is not used in gfn_to_pfn_memslot, we can happily remove it

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

---
 arch/powerpc/kvm/e500_tlb.c |    2 +-
 arch/x86/kvm/mmu.c          |    2 +-
 include/linux/kvm_host.h    |    5 ++---
 virt/kvm/iommu.c            |   10 +++++-----
 virt/kvm/kvm_main.c         |   15 +++++++--------
 5 files changed, 16 insertions(+), 18 deletions(-)

arch/powerpc/kvm/e500_tlb.c +1 -1

index c510fc9..c8f6c58 100644
@@ -520,7 +520,7 @@ static inline void kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 
 	if (likely(!pfnmap)) {
 		unsigned long tsize_pages = 1 << (tsize + 10 - PAGE_SHIFT);
-		pfn = gfn_to_pfn_memslot(vcpu_e500->vcpu.kvm, slot, gfn);
+		pfn = gfn_to_pfn_memslot(slot, gfn);
 		if (is_error_pfn(pfn)) {
 			printk(KERN_ERR "Couldn't get real page for gfn %lx!\n",
 					(long)gfn);

arch/x86/kvm/mmu.c +1 -1

index e23cdae..4ed543a 100644
@@ -2488,7 +2488,7 @@ static pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
 
 	hva = gfn_to_hva_memslot(slot, gfn);
 
-	return hva_to_pfn_atomic(vcpu->kvm, hva);
+	return hva_to_pfn_atomic(hva);
 }
 
 static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,

include/linux/kvm_host.h +2 -3

index f1ef5fc..14bfde4 100644
@@ -418,15 +418,14 @@ void kvm_release_page_dirty(struct page *page);
 void kvm_set_page_dirty(struct page *page);
 void kvm_set_page_accessed(struct page *page);
 
-pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr);
+pfn_t hva_to_pfn_atomic(unsigned long addr);
 pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn);
 pfn_t gfn_to_pfn_async(struct kvm *kvm, gfn_t gfn, bool *async,
 		       bool write_fault, bool *writable);
 pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn);
 pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
 		      bool *writable);
-pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
-			 struct kvm_memory_slot *slot, gfn_t gfn);
+pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn);
 void kvm_release_pfn_dirty(pfn_t);
 void kvm_release_pfn_clean(pfn_t pfn);
 void kvm_set_pfn_dirty(pfn_t pfn);

virt/kvm/iommu.c +5 -5

index e9fff98..c03f1fb 100644
@@ -42,13 +42,13 @@ static int kvm_iommu_unmap_memslots(struct kvm *kvm);
 static void kvm_iommu_put_pages(struct kvm *kvm,
 				gfn_t base_gfn, unsigned long npages);
 
-static pfn_t kvm_pin_pages(struct kvm *kvm, struct kvm_memory_slot *slot,
-			   gfn_t gfn, unsigned long size)
+static pfn_t kvm_pin_pages(struct kvm_memory_slot *slot, gfn_t gfn,
+			   unsigned long size)
 {
 	gfn_t end_gfn;
 	pfn_t pfn;
 
-	pfn     = gfn_to_pfn_memslot(kvm, slot, gfn);
+	pfn     = gfn_to_pfn_memslot(slot, gfn);
 	end_gfn = gfn + (size >> PAGE_SHIFT);
 	gfn    += 1;

@@ -56,7 +56,7 @@ static pfn_t kvm_pin_pages(struct kvm *kvm, struct kvm_memory_slot *slot,
 		return pfn;
 
 	while (gfn < end_gfn)
-		gfn_to_pfn_memslot(kvm, slot, gfn++);
+		gfn_to_pfn_memslot(slot, gfn++);
 
 	return pfn;
 }

@@ -105,7 +105,7 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
 		 * Pin all pages we are about to map in memory. This is
 		 * important because we unmap and unpin in 4kb steps later.
 		 */
-		pfn = kvm_pin_pages(kvm, slot, gfn, page_size);
+		pfn = kvm_pin_pages(slot, gfn, page_size);
 		if (is_error_pfn(pfn)) {
 			gfn += 1;
 			continue;

virt/kvm/kvm_main.c +7 -8

index 7082e33..78cf42f 100644
@@ -1063,8 +1063,8 @@ static inline int check_user_page_hwpoison(unsigned long addr)
 	return rc == -EHWPOISON;
 }
 
-static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
-			bool *async, bool write_fault, bool *writable)
+static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
+			bool write_fault, bool *writable)
 {
 	struct page *page[1];
 	int npages = 0;

@@ -1144,9 +1144,9 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic,
 	return pfn;
 }
 
-pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr)
+pfn_t hva_to_pfn_atomic(unsigned long addr)
 {
-	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
+	return hva_to_pfn(addr, true, NULL, true, NULL);
 }
 EXPORT_SYMBOL_GPL(hva_to_pfn_atomic);

@@ -1164,7 +1164,7 @@ static pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic, bool *async,
 		return page_to_pfn(bad_page);
 	}
 
-	return hva_to_pfn(kvm, addr, atomic, async, write_fault, writable);
+	return hva_to_pfn(addr, atomic, async, write_fault, writable);
 }
 
 pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)

@@ -1193,11 +1193,10 @@ pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn_prot);
 
-pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
-			 struct kvm_memory_slot *slot, gfn_t gfn)
+pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
-	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
+	return hva_to_pfn(addr, false, NULL, true, NULL);
 }
 
 int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn, struct page **pages,

Signature · 1 line

1.7.7.6

Marcelo Tosatti

Applied

2012-07-20 00:48 UTC #24 lore raw

In reply to Xiao Guangrong

Applied all patches except 2, 3 and 5, thanks.