[PATCH 1/9] KVM: x86: remvoe unnecessary mark_page_dirty

[PATCH 1/9] KVM: x86: remvoe unnecessary mark_page_dirty

[Xiao Guangrong]

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 <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/x86.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ff0b487..875b8d8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
 }
-- 
1.7.7.6

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

[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 <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/emulate.c |   29 ++++++++++++-----------------
 1 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 97d9a99..2d1916b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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;
 }

-- 
1.7.7.6

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

[Marcelo Tosatti]

On Tue, Jul 17, 2012 at 09:51:34PM +0800, Xiao Guangrong wrote:
> No need split mmio read region into 8-bits pieces since we do it in
> emulator_read_write_onepage
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  arch/x86/kvm/emulate.c |   29 ++++++++++++-----------------
>  1 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 97d9a99..2d1916b 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -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);

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

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

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

[Xiao Guangrong]

On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:

>> -	}
>> +	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?

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.

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

[Marcelo Tosatti]

On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
> 
> >> -	}
> >> +	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?
> 
> 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.

Yes, it is fine to remove it.

But splitting in emulate.c prevented the case of _cache read_ with size
> 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?"

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

[Xiao Guangrong]

On 07/20/2012 06:58 PM, Marcelo Tosatti wrote:
> On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
>> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
>>
>>>> -	}
>>>> +	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?
>>
>> 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.
> 
> Yes, it is fine to remove it.
> 
> But splitting in emulate.c prevented the case of _cache read_ with size
>> 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[].

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

[Marcelo Tosatti]

On Fri, Jul 20, 2012 at 09:15:44PM +0800, Xiao Guangrong wrote:
> On 07/20/2012 06:58 PM, Marcelo Tosatti wrote:
> > On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
> >> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
> >>
> >>>> -	}
> >>>> +	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?
> >>
> >> 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.
> > 
> > Yes, it is fine to remove it.
> > 
> > But splitting in emulate.c prevented the case of _cache read_ with size
> >> 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[].

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

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

[Xiao Guangrong]

On 07/21/2012 03:52 AM, Marcelo Tosatti wrote:
> On Fri, Jul 20, 2012 at 09:15:44PM +0800, Xiao Guangrong wrote:
>> On 07/20/2012 06:58 PM, Marcelo Tosatti wrote:
>>> On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
>>>> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
>>>>
>>>>>> -	}
>>>>>> +	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?
>>>>
>>>> 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.
>>>
>>> Yes, it is fine to remove it.
>>>
>>> But splitting in emulate.c prevented the case of _cache read_ with size
>>>> 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[].
> 
> 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.

> 
> ACK
> 

Thank you, Marcelo!

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

[Xiao Guangrong]

Introduce set_mmio_exit_info to cleanup the common code

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/x86.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 875b8d8..8171836 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;

 	}
-- 
1.7.7.6

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

[Marcelo Tosatti]

On Tue, Jul 17, 2012 at 09:52:13PM +0800, Xiao Guangrong wrote:
> Introduce set_mmio_exit_info to cleanup the common code
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  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.

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

[Xiao Guangrong]

On 07/20/2012 08:03 AM, Marcelo Tosatti wrote:
> On Tue, Jul 17, 2012 at 09:52:13PM +0800, Xiao Guangrong wrote:
>> Introduce set_mmio_exit_info to cleanup the common code
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  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. :)

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

[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 <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 28c8fbc..28b12e2 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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))
-- 
1.7.7.6

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

[Marcelo Tosatti]

On Tue, Jul 17, 2012 at 09:52:52PM +0800, Xiao Guangrong wrote:
> 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 <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  arch/x86/kvm/mmu.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)

Applied, thanks.

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

[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 <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 28b12e2..a846a9c 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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;
-- 
1.7.7.6

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

[Marcelo Tosatti]

On Tue, Jul 17, 2012 at 09:53:29PM +0800, Xiao Guangrong wrote:
> 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 <xiaoguangrong@linux.vnet.ibm.com>

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?

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

[Xiao Guangrong]

On 07/20/2012 08:39 AM, Marcelo Tosatti wrote:
> On Tue, Jul 17, 2012 at 09:53:29PM +0800, Xiao Guangrong wrote:
>> 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 <xiaoguangrong@linux.vnet.ibm.com>
> 
> 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.

> 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.

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

[Marcelo Tosatti]

On Fri, Jul 20, 2012 at 10:34:28AM +0800, Xiao Guangrong wrote:
> On 07/20/2012 08:39 AM, Marcelo Tosatti wrote:
> > On Tue, Jul 17, 2012 at 09:53:29PM +0800, Xiao Guangrong wrote:
> >> 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 <xiaoguangrong@linux.vnet.ibm.com>
> > 
> > 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.

> I will post the Number after I do the performance test.
> 
> > 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.

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

[Xiao Guangrong]

On 07/20/2012 07:09 PM, Marcelo Tosatti wrote:
> On Fri, Jul 20, 2012 at 10:34:28AM +0800, Xiao Guangrong wrote:
>> On 07/20/2012 08:39 AM, Marcelo Tosatti wrote:
>>> On Tue, Jul 17, 2012 at 09:53:29PM +0800, Xiao Guangrong wrote:
>>>> 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 <xiaoguangrong@linux.vnet.ibm.com>
>>>
>>> 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.
> 
>> I will post the Number after I do the performance test.
>>
>>> 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? 
> 

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

> 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

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

[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.

On 07/20/2012 08:39 AM, Marcelo Tosatti wrote:can
> On Tue, Jul 17, 2012 at 09:53:29PM +0800, Xiao Guangrong wrote:
>> 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 <xiaoguangrong@linux.vnet.ibm.com>
> 
> 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?
> 
> 
> 

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

[Marcelo Tosatti]

On Fri, Jul 20, 2012 at 11:45:59AM +0800, Xiao Guangrong wrote:
> 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

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

[Xiao Guangrong]

Using get_fault_pfn to cleanup the code

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 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(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a846a9c..e23cdae 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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);

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e3c86f8..258b35a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 60af030..feef72e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)
-- 
1.7.7.6

[PATCH 7/9] KVM: mark do not extern bad_pfn

[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 <xiaoguangrong@linux.vnet.ibm.com>
---
 include/linux/kvm_host.h |    1 -
 virt/kvm/kvm_main.c      |    6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 258b35a..bafcc56 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index feef72e..7082e33 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)
 {
-- 
1.7.7.6

[PATCH 8/9] KVM: remove is_error_hpa

[Xiao Guangrong]

Remove them since they are not used anymore

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 include/linux/kvm_host.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index bafcc56..f1ef5fc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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);
-- 
1.7.7.6

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

[Xiao Guangrong]

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

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 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(-)

diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c
index c510fc9..c8f6c58 100644
--- a/arch/powerpc/kvm/e500_tlb.c
+++ b/arch/powerpc/kvm/e500_tlb.c
@@ -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);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index e23cdae..4ed543a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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,
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f1ef5fc..14bfde4 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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);
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index e9fff98..c03f1fb 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -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;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7082e33..78cf42f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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,
-- 
1.7.7.6

Re: [PATCH 1/9] KVM: x86: remvoe unnecessary mark_page_dirty

[Marcelo Tosatti]

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