[PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Takuya Yoshikawa]

Since kvm_arch_prepare_memory_region() is called right after installing
the slot marked invalid, wraparound checking should be there to avoid
zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 This seems to be the simplest solution for fixing the off-by-one issue
 we discussed before.

 arch/x86/kvm/mmu.c |    5 +----
 arch/x86/kvm/x86.c |    7 +++++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 0d094da..bf7af1e 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
 	/*
 	 * The very rare case: if the generation-number is round,
 	 * zap all shadow pages.
-	 *
-	 * The max value is MMIO_MAX_GEN - 1 since it is not called
-	 * when mark memslot invalid.
 	 */
-	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
+	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
 		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
 		kvm_mmu_invalidate_zap_all_pages(kvm);
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7d71c0f..9ddd4ff 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 		memslot->userspace_addr = userspace_addr;
 	}
 
+	/*
+	 * In these cases, slots->generation has been increased for marking the
+	 * slot invalid, so we need wraparound checking here.
+	 */
+	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
+		kvm_mmu_invalidate_mmio_sptes(kvm);
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Paolo Bonzini]

Il 03/07/2013 10:18, Takuya Yoshikawa ha scritto:
> Since kvm_arch_prepare_memory_region() is called right after installing
> the slot marked invalid, wraparound checking should be there to avoid
> zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  This seems to be the simplest solution for fixing the off-by-one issue
>  we discussed before.
> 
>  arch/x86/kvm/mmu.c |    5 +----
>  arch/x86/kvm/x86.c |    7 +++++++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 0d094da..bf7af1e 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>  	/*
>  	 * The very rare case: if the generation-number is round,
>  	 * zap all shadow pages.
> -	 *
> -	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> -	 * when mark memslot invalid.
>  	 */
> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> +	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
>  		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
>  		kvm_mmu_invalidate_zap_all_pages(kvm);
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7d71c0f..9ddd4ff 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  		memslot->userspace_addr = userspace_addr;
>  	}
>  
> +	/*
> +	 * In these cases, slots->generation has been increased for marking the
> +	 * slot invalid, so we need wraparound checking here.
> +	 */
> +	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
> +		kvm_mmu_invalidate_mmio_sptes(kvm);
> +
>  	return 0;
>  }
>  
> 

Applied, thanks.

Paolo

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Xiao Guangrong]

On 07/03/2013 04:28 PM, Paolo Bonzini wrote:
> Il 03/07/2013 10:18, Takuya Yoshikawa ha scritto:
>> Since kvm_arch_prepare_memory_region() is called right after installing
>> the slot marked invalid, wraparound checking should be there to avoid
>> zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.
>>
>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>> ---
>>  This seems to be the simplest solution for fixing the off-by-one issue
>>  we discussed before.
>>
>>  arch/x86/kvm/mmu.c |    5 +----
>>  arch/x86/kvm/x86.c |    7 +++++++
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 0d094da..bf7af1e 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>>  	/*
>>  	 * The very rare case: if the generation-number is round,
>>  	 * zap all shadow pages.
>> -	 *
>> -	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>> -	 * when mark memslot invalid.
>>  	 */
>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
>>  		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
>>  		kvm_mmu_invalidate_zap_all_pages(kvm);
>>  	}
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 7d71c0f..9ddd4ff 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>>  		memslot->userspace_addr = userspace_addr;
>>  	}
>>  
>> +	/*
>> +	 * In these cases, slots->generation has been increased for marking the
>> +	 * slot invalid, so we need wraparound checking here.
>> +	 */
>> +	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
>> +		kvm_mmu_invalidate_mmio_sptes(kvm);
>> +
>>  	return 0;
>>  }
>>  
>>
> 
> Applied, thanks.

Please wait a while. I can not understand it very clearly.

This conditional check will cause caching a overflow value into mmio spte.
The simple case is that kvm adds new slots for many times, the mmio-gen is easily
more than MMIO_MAX_GEN.

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Takuya Yoshikawa]

On Wed, 03 Jul 2013 16:39:25 +0800
Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> wrote:

> Please wait a while. I can not understand it very clearly.
> 
> This conditional check will cause caching a overflow value into mmio spte.
> The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> more than MMIO_MAX_GEN.

Unconditional checking in commit_memory_region() is still there
to treat such cases.

	Takuya

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Xiao Guangrong]

On 07/03/2013 04:50 PM, Takuya Yoshikawa wrote:
> On Wed, 03 Jul 2013 16:39:25 +0800
> Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> wrote:
> 
>> Please wait a while. I can not understand it very clearly.
>>
>> This conditional check will cause caching a overflow value into mmio spte.
>> The simple case is that kvm adds new slots for many times, the mmio-gen is easily
>> more than MMIO_MAX_GEN.
> 
> Unconditional checking in commit_memory_region() is still there
> to treat such cases.

WHY?

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Paolo Bonzini]

Il 03/07/2013 10:39, Xiao Guangrong ha scritto:
> On 07/03/2013 04:28 PM, Paolo Bonzini wrote:
>> Il 03/07/2013 10:18, Takuya Yoshikawa ha scritto:
>>> Since kvm_arch_prepare_memory_region() is called right after installing
>>> the slot marked invalid, wraparound checking should be there to avoid
>>> zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.
>>>
>>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>> ---
>>>  This seems to be the simplest solution for fixing the off-by-one issue
>>>  we discussed before.
>>>
>>>  arch/x86/kvm/mmu.c |    5 +----
>>>  arch/x86/kvm/x86.c |    7 +++++++
>>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>> index 0d094da..bf7af1e 100644
>>> --- a/arch/x86/kvm/mmu.c
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>>>  	/*
>>>  	 * The very rare case: if the generation-number is round,
>>>  	 * zap all shadow pages.
>>> -	 *
>>> -	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>>> -	 * when mark memslot invalid.
>>>  	 */
>>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
>>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
>>>  		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
>>>  		kvm_mmu_invalidate_zap_all_pages(kvm);
>>>  	}
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 7d71c0f..9ddd4ff 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>>>  		memslot->userspace_addr = userspace_addr;
>>>  	}
>>>  
>>> +	/*
>>> +	 * In these cases, slots->generation has been increased for marking the
>>> +	 * slot invalid, so we need wraparound checking here.
>>> +	 */
>>> +	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
>>> +		kvm_mmu_invalidate_mmio_sptes(kvm);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>>
>>
>> Applied, thanks.
> 
> Please wait a while. I can not understand it very clearly.

I'm only applying to queue anyway until Linus pulls.

> This conditional check will cause caching a overflow value into mmio spte.
> The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> more than MMIO_MAX_GEN.

The mmio generation is masked to MMIO_GEN_MASK:

        return (kvm_memslots(kvm)->generation +
                      MMIO_MAX_GEN - 150) & MMIO_GEN_MASK;

What Takuya's patch does is basically "if __kvm_set_memory_region called
install_new_memslots, call kvm_mmu_invalidate_mmio_sptes".

kvm_arch_prepare_memory_region is preceded by install_new_memslots if
change is KVM_MR_DELETE or KVM_MR_MOVE.  kvm_arch_commit_memory_region
is always preceded by install_new_memslots.  So the logic in x86.c
matches the one in __kvm_set_memory_region.

With this change, each change to the regions is matched by a call to
kvm_mmu_invalidate_mmio_sptes, and there is no need to invalidate twice
before wraparound.

Paolo

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Xiao Guangrong]

On 07/03/2013 04:50 PM, Paolo Bonzini wrote:
> Il 03/07/2013 10:39, Xiao Guangrong ha scritto:
>> On 07/03/2013 04:28 PM, Paolo Bonzini wrote:
>>> Il 03/07/2013 10:18, Takuya Yoshikawa ha scritto:
>>>> Since kvm_arch_prepare_memory_region() is called right after installing
>>>> the slot marked invalid, wraparound checking should be there to avoid
>>>> zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.
>>>>
>>>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>>> ---
>>>>  This seems to be the simplest solution for fixing the off-by-one issue
>>>>  we discussed before.
>>>>
>>>>  arch/x86/kvm/mmu.c |    5 +----
>>>>  arch/x86/kvm/x86.c |    7 +++++++
>>>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>>> index 0d094da..bf7af1e 100644
>>>> --- a/arch/x86/kvm/mmu.c
>>>> +++ b/arch/x86/kvm/mmu.c
>>>> @@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>>>>  	/*
>>>>  	 * The very rare case: if the generation-number is round,
>>>>  	 * zap all shadow pages.
>>>> -	 *
>>>> -	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>>>> -	 * when mark memslot invalid.
>>>>  	 */
>>>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
>>>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
>>>>  		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
>>>>  		kvm_mmu_invalidate_zap_all_pages(kvm);
>>>>  	}
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 7d71c0f..9ddd4ff 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>>>>  		memslot->userspace_addr = userspace_addr;
>>>>  	}
>>>>  
>>>> +	/*
>>>> +	 * In these cases, slots->generation has been increased for marking the
>>>> +	 * slot invalid, so we need wraparound checking here.
>>>> +	 */
>>>> +	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
>>>> +		kvm_mmu_invalidate_mmio_sptes(kvm);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>>
>>>
>>> Applied, thanks.
>>
>> Please wait a while. I can not understand it very clearly.
> 
> I'm only applying to queue anyway until Linus pulls.

Okay. :)

> 
>> This conditional check will cause caching a overflow value into mmio spte.
>> The simple case is that kvm adds new slots for many times, the mmio-gen is easily
>> more than MMIO_MAX_GEN.
> 
> The mmio generation is masked to MMIO_GEN_MASK:
> 
>         return (kvm_memslots(kvm)->generation +
>                       MMIO_MAX_GEN - 150) & MMIO_GEN_MASK;
> 
> What Takuya's patch does is basically "if __kvm_set_memory_region called
> install_new_memslots, call kvm_mmu_invalidate_mmio_sptes".
> 
> kvm_arch_prepare_memory_region is preceded by install_new_memslots if
> change is KVM_MR_DELETE or KVM_MR_MOVE.  kvm_arch_commit_memory_region
> is always preceded by install_new_memslots.  So the logic in x86.c
> matches the one in __kvm_set_memory_region.
> 
> With this change, each change to the regions is matched by a call to
> kvm_mmu_invalidate_mmio_sptes, and there is no need to invalidate twice
> before wraparound.

Oh. My mistake, i did not noticed that the check in kvm_arch_commit_memory_region()
is still there. The change is okay to work.

But, the check in two places seems unclean. :(

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Xiao Guangrong]

On 07/03/2013 04:39 PM, Xiao Guangrong wrote:
> On 07/03/2013 04:28 PM, Paolo Bonzini wrote:
>> Il 03/07/2013 10:18, Takuya Yoshikawa ha scritto:
>>> Since kvm_arch_prepare_memory_region() is called right after installing
>>> the slot marked invalid, wraparound checking should be there to avoid
>>> zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.
>>>
>>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>> ---
>>>  This seems to be the simplest solution for fixing the off-by-one issue
>>>  we discussed before.
>>>
>>>  arch/x86/kvm/mmu.c |    5 +----
>>>  arch/x86/kvm/x86.c |    7 +++++++
>>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>> index 0d094da..bf7af1e 100644
>>> --- a/arch/x86/kvm/mmu.c
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>>>  	/*
>>>  	 * The very rare case: if the generation-number is round,
>>>  	 * zap all shadow pages.
>>> -	 *
>>> -	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>>> -	 * when mark memslot invalid.
>>>  	 */
>>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
>>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
>>>  		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
>>>  		kvm_mmu_invalidate_zap_all_pages(kvm);
>>>  	}
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 7d71c0f..9ddd4ff 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>>>  		memslot->userspace_addr = userspace_addr;
>>>  	}
>>>  
>>> +	/*
>>> +	 * In these cases, slots->generation has been increased for marking the
>>> +	 * slot invalid, so we need wraparound checking here.
>>> +	 */
>>> +	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
>>> +		kvm_mmu_invalidate_mmio_sptes(kvm);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>>
>>
>> Applied, thanks.
> 
> Please wait a while. I can not understand it very clearly.
> 
> This conditional check will cause caching a overflow value into mmio spte.
> The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> more than MMIO_MAX_GEN.
> 

Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
the end of install_new_memslots().

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Gleb Natapov]

On Wed, Jul 03, 2013 at 04:50:36PM +0800, Xiao Guangrong wrote:
> On 07/03/2013 04:39 PM, Xiao Guangrong wrote:
> > On 07/03/2013 04:28 PM, Paolo Bonzini wrote:
> >> Il 03/07/2013 10:18, Takuya Yoshikawa ha scritto:
> >>> Since kvm_arch_prepare_memory_region() is called right after installing
> >>> the slot marked invalid, wraparound checking should be there to avoid
> >>> zapping mmio sptes when mmio generation is still MMIO_MAX_GEN - 1.
> >>>
> >>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> >>> ---
> >>>  This seems to be the simplest solution for fixing the off-by-one issue
> >>>  we discussed before.
> >>>
> >>>  arch/x86/kvm/mmu.c |    5 +----
> >>>  arch/x86/kvm/x86.c |    7 +++++++
> >>>  2 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> >>> index 0d094da..bf7af1e 100644
> >>> --- a/arch/x86/kvm/mmu.c
> >>> +++ b/arch/x86/kvm/mmu.c
> >>> @@ -4383,11 +4383,8 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> >>>  	/*
> >>>  	 * The very rare case: if the generation-number is round,
> >>>  	 * zap all shadow pages.
> >>> -	 *
> >>> -	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> >>> -	 * when mark memslot invalid.
> >>>  	 */
> >>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> >>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= MMIO_MAX_GEN)) {
> >>>  		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
> >>>  		kvm_mmu_invalidate_zap_all_pages(kvm);
> >>>  	}
> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >>> index 7d71c0f..9ddd4ff 100644
> >>> --- a/arch/x86/kvm/x86.c
> >>> +++ b/arch/x86/kvm/x86.c
> >>> @@ -7046,6 +7046,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
> >>>  		memslot->userspace_addr = userspace_addr;
> >>>  	}
> >>>  
> >>> +	/*
> >>> +	 * In these cases, slots->generation has been increased for marking the
> >>> +	 * slot invalid, so we need wraparound checking here.
> >>> +	 */
> >>> +	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))
> >>> +		kvm_mmu_invalidate_mmio_sptes(kvm);
> >>> +
> >>>  	return 0;
> >>>  }
> >>>  
> >>>
> >>
> >> Applied, thanks.
> > 
> > Please wait a while. I can not understand it very clearly.
> > 
> > This conditional check will cause caching a overflow value into mmio spte.
> > The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> > more than MMIO_MAX_GEN.
> > 
> 
> Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
> the end of install_new_memslots().
> 
Exactly. Why should we hide it in obscure functions?

--
			Gleb.

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Paolo Bonzini]

Il 03/07/2013 10:53, Gleb Natapov ha scritto:
>>> > > Please wait a while. I can not understand it very clearly.
>>> > > 
>>> > > This conditional check will cause caching a overflow value into mmio spte.
>>> > > The simple case is that kvm adds new slots for many times, the mmio-gen is easily
>>> > > more than MMIO_MAX_GEN.
>>> > > 
>> > 
>> > Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
>> > the end of install_new_memslots().
>> > 
> Exactly. Why should we hide it in obscure functions?

Because kvm_mmu_invalidate_mmio_sptes is an x86-specific function, and
we already have a good arch API to hook into __kvm_set_memory_region.

Another possible implementation is to cache the last memslot generation,
check it in kvm_arch_prepare/commit_memory_region, and call
kvm_mmu_invalidate_mmio_sptes if it changed.  This would avoid the need
to keep "((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))" in sync
between the two places.  But I don't think it is a substantial improvement.

Paolo

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Gleb Natapov]

On Wed, Jul 03, 2013 at 10:57:35AM +0200, Paolo Bonzini wrote:
> Il 03/07/2013 10:53, Gleb Natapov ha scritto:
> >>> > > Please wait a while. I can not understand it very clearly.
> >>> > > 
> >>> > > This conditional check will cause caching a overflow value into mmio spte.
> >>> > > The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> >>> > > more than MMIO_MAX_GEN.
> >>> > > 
> >> > 
> >> > Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
> >> > the end of install_new_memslots().
> >> > 
> > Exactly. Why should we hide it in obscure functions?
> 
> Because kvm_mmu_invalidate_mmio_sptes is an x86-specific function, and
> we already have a good arch API to hook into __kvm_set_memory_region.
> 
>From this patch it can be seen that the API is not good enough.
Generation update API is missing.

> Another possible implementation is to cache the last memslot generation,
> check it in kvm_arch_prepare/commit_memory_region, and call
> kvm_mmu_invalidate_mmio_sptes if it changed.  This would avoid the need
> to keep "((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))" in sync
> between the two places.  But I don't think it is a substantial improvement.
> 
Why are you trying to overcome API shortcomings instead of fixing the API.
 
--
			Gleb.

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Paolo Bonzini]

Il 03/07/2013 10:50, Xiao Guangrong ha scritto:
>> > Please wait a while. I can not understand it very clearly.
>> > 
>> > This conditional check will cause caching a overflow value into mmio spte.
>> > The simple case is that kvm adds new slots for many times, the mmio-gen is easily
>> > more than MMIO_MAX_GEN.
>> > 
> Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
> the end of install_new_memslots().
> 
> 

Yes, the actual operation would be the same as this patch.  You can
rename kvm_mmu_invalidate_mmio_sptes to kvm_arch_memslots_installed, or
something like that.  But it would have to touch all architectures.

Paolo

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Takuya Yoshikawa]

On Wed, 03 Jul 2013 10:53:51 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 03/07/2013 10:50, Xiao Guangrong ha scritto:
> >> > Please wait a while. I can not understand it very clearly.
> >> > 
> >> > This conditional check will cause caching a overflow value into mmio spte.
> >> > The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> >> > more than MMIO_MAX_GEN.
> >> > 
> > Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
> > the end of install_new_memslots().
> > 
> > 
> 
> Yes, the actual operation would be the same as this patch.  You can
> rename kvm_mmu_invalidate_mmio_sptes to kvm_arch_memslots_installed, or
> something like that.  But it would have to touch all architectures.

I tried to avoid introducing x86-centric code into the generic one.

If another arch can gain something by such function, I'm willing to
touch all arch code.

	Takuya

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Gleb Natapov]

On Wed, Jul 03, 2013 at 06:05:00PM +0900, Takuya Yoshikawa wrote:
> On Wed, 03 Jul 2013 10:53:51 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 03/07/2013 10:50, Xiao Guangrong ha scritto:
> > >> > Please wait a while. I can not understand it very clearly.
> > >> > 
> > >> > This conditional check will cause caching a overflow value into mmio spte.
> > >> > The simple case is that kvm adds new slots for many times, the mmio-gen is easily
> > >> > more than MMIO_MAX_GEN.
> > >> > 
> > > Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to
> > > the end of install_new_memslots().
> > > 
> > > 
> > 
> > Yes, the actual operation would be the same as this patch.  You can
> > rename kvm_mmu_invalidate_mmio_sptes to kvm_arch_memslots_installed, or
> > something like that.  But it would have to touch all architectures.
> 
> I tried to avoid introducing x86-centric code into the generic one.
> 
> If another arch can gain something by such function, I'm willing to
> touch all arch code.
> 
Please do. X86 is the most optimized one so it does things other arches
do not yet. Slot generation update hook sounds generic enough.

--
			Gleb.

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Paolo Bonzini]

Il 03/07/2013 11:05, Gleb Natapov ha scritto:
>>> Yes, the actual operation would be the same as this patch.  You can
>>> rename kvm_mmu_invalidate_mmio_sptes to kvm_arch_memslots_installed, or
>>> something like that.  But it would have to touch all architectures.
>>
>> I tried to avoid introducing x86-centric code into the generic one.
>>
>> If another arch can gain something by such function, I'm willing to
>> touch all arch code.
>>
> Please do. X86 is the most optimized one so it does things other arches
> do not yet. Slot generation update hook sounds generic enough.

Yes, makes sense.  However, this patch is still an improvement because
the current code is too easily mistaken for an off-by-one bug.

Any improvements to the API can go on top.

Paolo

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Gleb Natapov]

On Wed, Jul 03, 2013 at 11:08:18AM +0200, Paolo Bonzini wrote:
> Il 03/07/2013 11:05, Gleb Natapov ha scritto:
> >>> Yes, the actual operation would be the same as this patch.  You can
> >>> rename kvm_mmu_invalidate_mmio_sptes to kvm_arch_memslots_installed, or
> >>> something like that.  But it would have to touch all architectures.
> >>
> >> I tried to avoid introducing x86-centric code into the generic one.
> >>
> >> If another arch can gain something by such function, I'm willing to
> >> touch all arch code.
> >>
> > Please do. X86 is the most optimized one so it does things other arches
> > do not yet. Slot generation update hook sounds generic enough.
> 
> Yes, makes sense.  However, this patch is still an improvement because
> the current code is too easily mistaken for an off-by-one bug.
> 
> Any improvements to the API can go on top.
> 
If Takuya will send the proper fix shortly I do not see any reason to
apply this one. It does not fix any bug.

--
			Gleb.

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Takuya Yoshikawa]

On Wed, 3 Jul 2013 12:10:57 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> > Yes, makes sense.  However, this patch is still an improvement because
> > the current code is too easily mistaken for an off-by-one bug.
> > 
> > Any improvements to the API can go on top.
> > 
> If Takuya will send the proper fix shortly I do not see any reason to
> apply this one. It does not fix any bug.

No problem.

Please give me a day or so.
Thank you everyone for revewing this patch!

	Takuya

Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound

[Paolo Bonzini]

Il 03/07/2013 11:10, Gleb Natapov ha scritto:
> On Wed, Jul 03, 2013 at 11:08:18AM +0200, Paolo Bonzini wrote:
>> Il 03/07/2013 11:05, Gleb Natapov ha scritto:
>>>>> Yes, the actual operation would be the same as this patch.  You can
>>>>> rename kvm_mmu_invalidate_mmio_sptes to kvm_arch_memslots_installed, or
>>>>> something like that.  But it would have to touch all architectures.
>>>>
>>>> I tried to avoid introducing x86-centric code into the generic one.
>>>>
>>>> If another arch can gain something by such function, I'm willing to
>>>> touch all arch code.
>>>>
>>> Please do. X86 is the most optimized one so it does things other arches
>>> do not yet. Slot generation update hook sounds generic enough.
>>
>> Yes, makes sense.  However, this patch is still an improvement because
>> the current code is too easily mistaken for an off-by-one bug.
>>
>> Any improvements to the API can go on top.
>>
> If Takuya will send the proper fix shortly I do not see any reason to
> apply this one. It does not fix any bug.

It is still a small bug to do two zaps when only one is needed...

Paolo