Re: [PATCH v2 05/15] KVM: MMU: flush tlb out of mmu lock when write-protect the sptes

[Xiao Guangrong]

On Oct 1, 2013, at 7:05 AM, Marcelo Tosatti  wrote:

> On Thu, Sep 05, 2013 at 06:29:08PM +0800, Xiao Guangrong wrote:
>> Now we can flush all the TLBs out of the mmu lock without TLB corruption when
>> write-proect the sptes, it is because:
>> - we have marked large sptes readonly instead of dropping them that means we
>>  just change the spte from writable to readonly so that we only need to care
>>  the case of changing spte from present to present (changing the spte from
>>  present to nonpresent will flush all the TLBs immediately), in other words,
>>  the only case we need to care is mmu_spte_update()
>> 
>> - in mmu_spte_update(), we have checked
>>  SPTE_HOST_WRITEABLE | PTE_MMU_WRITEABLE instead of PT_WRITABLE_MASK, that
>>  means it does not depend on PT_WRITABLE_MASK anymore
>> 
>> Signed-off-by: Xiao Guangrong 
>> ---
>> arch/x86/kvm/mmu.c | 18 ++
>> arch/x86/kvm/x86.c |  9 +++--
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 7488229..a983570 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -4320,15 +4320,25 @@ void kvm_mmu_slot_remove_write_access(struct kvm 
>> *kvm, int slot)
>>  if (*rmapp)
>>  __rmap_write_protect(kvm, rmapp, false);
>> 
>> -if (need_resched() || spin_needbreak(>mmu_lock)) {
>> -kvm_flush_remote_tlbs(kvm);
>> +if (need_resched() || spin_needbreak(>mmu_lock))
>>  cond_resched_lock(>mmu_lock);
>> -}
>>  }
>>  }
>> 
>> -kvm_flush_remote_tlbs(kvm);
>>  spin_unlock(>mmu_lock);
>> +
>> +/*
>> + * We can flush all the TLBs out of the mmu lock without TLB
>> + * corruption since we just change the spte from writable to
>> + * readonly so that we only need to care the case of changing
>> + * spte from present to present (changing the spte from present
>> + * to nonpresent will flush all the TLBs immediately), in other
>> + * words, the only case we care is mmu_spte_update() where we
>> + * haved checked SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE
>> + * instead of PT_WRITABLE_MASK, that means it does not depend
>> + * on PT_WRITABLE_MASK anymore.
>> + */
>> +kvm_flush_remote_tlbs(kvm);
>> }
> 
> What about need_remote_flush? 

It is safe because before calling need_remote_flush mmu_pte_write_new_pte is 
called to
update the spte which will finally call set_spte() where the tlb flush has been 
properly checked.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 05/15] KVM: MMU: flush tlb out of mmu lock when write-protect the sptes

[Xiao Guangrong]

On Oct 1, 2013, at 7:05 AM, Marcelo Tosatti mtosa...@redhat.com wrote:

 On Thu, Sep 05, 2013 at 06:29:08PM +0800, Xiao Guangrong wrote:
 Now we can flush all the TLBs out of the mmu lock without TLB corruption when
 write-proect the sptes, it is because:
 - we have marked large sptes readonly instead of dropping them that means we
  just change the spte from writable to readonly so that we only need to care
  the case of changing spte from present to present (changing the spte from
  present to nonpresent will flush all the TLBs immediately), in other words,
  the only case we need to care is mmu_spte_update()
 
 - in mmu_spte_update(), we have checked
  SPTE_HOST_WRITEABLE | PTE_MMU_WRITEABLE instead of PT_WRITABLE_MASK, that
  means it does not depend on PT_WRITABLE_MASK anymore
 
 Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
 ---
 arch/x86/kvm/mmu.c | 18 ++
 arch/x86/kvm/x86.c |  9 +++--
 2 files changed, 21 insertions(+), 6 deletions(-)
 
 diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
 index 7488229..a983570 100644
 --- a/arch/x86/kvm/mmu.c
 +++ b/arch/x86/kvm/mmu.c
 @@ -4320,15 +4320,25 @@ void kvm_mmu_slot_remove_write_access(struct kvm 
 *kvm, int slot)
  if (*rmapp)
  __rmap_write_protect(kvm, rmapp, false);
 
 -if (need_resched() || spin_needbreak(kvm-mmu_lock)) {
 -kvm_flush_remote_tlbs(kvm);
 +if (need_resched() || spin_needbreak(kvm-mmu_lock))
  cond_resched_lock(kvm-mmu_lock);
 -}
  }
  }
 
 -kvm_flush_remote_tlbs(kvm);
  spin_unlock(kvm-mmu_lock);
 +
 +/*
 + * We can flush all the TLBs out of the mmu lock without TLB
 + * corruption since we just change the spte from writable to
 + * readonly so that we only need to care the case of changing
 + * spte from present to present (changing the spte from present
 + * to nonpresent will flush all the TLBs immediately), in other
 + * words, the only case we care is mmu_spte_update() where we
 + * haved checked SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE
 + * instead of PT_WRITABLE_MASK, that means it does not depend
 + * on PT_WRITABLE_MASK anymore.
 + */
 +kvm_flush_remote_tlbs(kvm);
 }
 
 What about need_remote_flush? 

It is safe because before calling need_remote_flush mmu_pte_write_new_pte is 
called to
update the spte which will finally call set_spte() where the tlb flush has been 
properly checked.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 05/15] KVM: MMU: flush tlb out of mmu lock when write-protect the sptes

[Marcelo Tosatti]

On Thu, Sep 05, 2013 at 06:29:08PM +0800, Xiao Guangrong wrote:
> Now we can flush all the TLBs out of the mmu lock without TLB corruption when
> write-proect the sptes, it is because:
> - we have marked large sptes readonly instead of dropping them that means we
>   just change the spte from writable to readonly so that we only need to care
>   the case of changing spte from present to present (changing the spte from
>   present to nonpresent will flush all the TLBs immediately), in other words,
>   the only case we need to care is mmu_spte_update()
> 
> - in mmu_spte_update(), we have checked
>   SPTE_HOST_WRITEABLE | PTE_MMU_WRITEABLE instead of PT_WRITABLE_MASK, that
>   means it does not depend on PT_WRITABLE_MASK anymore
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  arch/x86/kvm/mmu.c | 18 ++
>  arch/x86/kvm/x86.c |  9 +++--
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 7488229..a983570 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4320,15 +4320,25 @@ void kvm_mmu_slot_remove_write_access(struct kvm 
> *kvm, int slot)
>   if (*rmapp)
>   __rmap_write_protect(kvm, rmapp, false);
>  
> - if (need_resched() || spin_needbreak(>mmu_lock)) {
> - kvm_flush_remote_tlbs(kvm);
> + if (need_resched() || spin_needbreak(>mmu_lock))
>   cond_resched_lock(>mmu_lock);
> - }
>   }
>   }
>  
> - kvm_flush_remote_tlbs(kvm);
>   spin_unlock(>mmu_lock);
> +
> + /*
> +  * We can flush all the TLBs out of the mmu lock without TLB
> +  * corruption since we just change the spte from writable to
> +  * readonly so that we only need to care the case of changing
> +  * spte from present to present (changing the spte from present
> +  * to nonpresent will flush all the TLBs immediately), in other
> +  * words, the only case we care is mmu_spte_update() where we
> +  * haved checked SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE
> +  * instead of PT_WRITABLE_MASK, that means it does not depend
> +  * on PT_WRITABLE_MASK anymore.
> +  */
> + kvm_flush_remote_tlbs(kvm);
>  }

What about need_remote_flush? 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 05/15] KVM: MMU: flush tlb out of mmu lock when write-protect the sptes

[Marcelo Tosatti]

On Thu, Sep 05, 2013 at 06:29:08PM +0800, Xiao Guangrong wrote:
 Now we can flush all the TLBs out of the mmu lock without TLB corruption when
 write-proect the sptes, it is because:
 - we have marked large sptes readonly instead of dropping them that means we
   just change the spte from writable to readonly so that we only need to care
   the case of changing spte from present to present (changing the spte from
   present to nonpresent will flush all the TLBs immediately), in other words,
   the only case we need to care is mmu_spte_update()
 
 - in mmu_spte_update(), we have checked
   SPTE_HOST_WRITEABLE | PTE_MMU_WRITEABLE instead of PT_WRITABLE_MASK, that
   means it does not depend on PT_WRITABLE_MASK anymore
 
 Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
 ---
  arch/x86/kvm/mmu.c | 18 ++
  arch/x86/kvm/x86.c |  9 +++--
  2 files changed, 21 insertions(+), 6 deletions(-)
 
 diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
 index 7488229..a983570 100644
 --- a/arch/x86/kvm/mmu.c
 +++ b/arch/x86/kvm/mmu.c
 @@ -4320,15 +4320,25 @@ void kvm_mmu_slot_remove_write_access(struct kvm 
 *kvm, int slot)
   if (*rmapp)
   __rmap_write_protect(kvm, rmapp, false);
  
 - if (need_resched() || spin_needbreak(kvm-mmu_lock)) {
 - kvm_flush_remote_tlbs(kvm);
 + if (need_resched() || spin_needbreak(kvm-mmu_lock))
   cond_resched_lock(kvm-mmu_lock);
 - }
   }
   }
  
 - kvm_flush_remote_tlbs(kvm);
   spin_unlock(kvm-mmu_lock);
 +
 + /*
 +  * We can flush all the TLBs out of the mmu lock without TLB
 +  * corruption since we just change the spte from writable to
 +  * readonly so that we only need to care the case of changing
 +  * spte from present to present (changing the spte from present
 +  * to nonpresent will flush all the TLBs immediately), in other
 +  * words, the only case we care is mmu_spte_update() where we
 +  * haved checked SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE
 +  * instead of PT_WRITABLE_MASK, that means it does not depend
 +  * on PT_WRITABLE_MASK anymore.
 +  */
 + kvm_flush_remote_tlbs(kvm);
  }

What about need_remote_flush? 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 05/15] KVM: MMU: flush tlb out of mmu lock when write-protect the sptes

[Xiao Guangrong]

Now we can flush all the TLBs out of the mmu lock without TLB corruption when
write-proect the sptes, it is because:
- we have marked large sptes readonly instead of dropping them that means we
  just change the spte from writable to readonly so that we only need to care
  the case of changing spte from present to present (changing the spte from
  present to nonpresent will flush all the TLBs immediately), in other words,
  the only case we need to care is mmu_spte_update()

- in mmu_spte_update(), we have checked
  SPTE_HOST_WRITEABLE | PTE_MMU_WRITEABLE instead of PT_WRITABLE_MASK, that
  means it does not depend on PT_WRITABLE_MASK anymore

Signed-off-by: Xiao Guangrong 
---
 arch/x86/kvm/mmu.c | 18 ++
 arch/x86/kvm/x86.c |  9 +++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 7488229..a983570 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4320,15 +4320,25 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, 
int slot)
if (*rmapp)
__rmap_write_protect(kvm, rmapp, false);
 
-   if (need_resched() || spin_needbreak(>mmu_lock)) {
-   kvm_flush_remote_tlbs(kvm);
+   if (need_resched() || spin_needbreak(>mmu_lock))
cond_resched_lock(>mmu_lock);
-   }
}
}
 
-   kvm_flush_remote_tlbs(kvm);
spin_unlock(>mmu_lock);
+
+   /*
+* We can flush all the TLBs out of the mmu lock without TLB
+* corruption since we just change the spte from writable to
+* readonly so that we only need to care the case of changing
+* spte from present to present (changing the spte from present
+* to nonpresent will flush all the TLBs immediately), in other
+* words, the only case we care is mmu_spte_update() where we
+* haved checked SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE
+* instead of PT_WRITABLE_MASK, that means it does not depend
+* on PT_WRITABLE_MASK anymore.
+*/
+   kvm_flush_remote_tlbs(kvm);
 }
 
 #define BATCH_ZAP_PAGES10
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6ad0c07..72f1487 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3560,11 +3560,16 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct 
kvm_dirty_log *log)
offset = i * BITS_PER_LONG;
kvm_mmu_write_protect_pt_masked(kvm, memslot, offset, mask);
}
-   if (is_dirty)
-   kvm_flush_remote_tlbs(kvm);
 
spin_unlock(>mmu_lock);
 
+   /*
+* All the TLBs can be flushed out of mmu lock, see the comments in
+* kvm_mmu_slot_remove_write_access().
+*/
+   if (is_dirty)
+   kvm_flush_remote_tlbs(kvm);
+
r = -EFAULT;
if (copy_to_user(log->dirty_bitmap, dirty_bitmap_buffer, n))
goto out;
-- 
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 05/15] KVM: MMU: flush tlb out of mmu lock when write-protect the sptes

[Xiao Guangrong]

Now we can flush all the TLBs out of the mmu lock without TLB corruption when
write-proect the sptes, it is because:
- we have marked large sptes readonly instead of dropping them that means we
  just change the spte from writable to readonly so that we only need to care
  the case of changing spte from present to present (changing the spte from
  present to nonpresent will flush all the TLBs immediately), in other words,
  the only case we need to care is mmu_spte_update()

- in mmu_spte_update(), we have checked
  SPTE_HOST_WRITEABLE | PTE_MMU_WRITEABLE instead of PT_WRITABLE_MASK, that
  means it does not depend on PT_WRITABLE_MASK anymore

Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
 arch/x86/kvm/mmu.c | 18 ++
 arch/x86/kvm/x86.c |  9 +++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 7488229..a983570 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4320,15 +4320,25 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, 
int slot)
if (*rmapp)
__rmap_write_protect(kvm, rmapp, false);
 
-   if (need_resched() || spin_needbreak(kvm-mmu_lock)) {
-   kvm_flush_remote_tlbs(kvm);
+   if (need_resched() || spin_needbreak(kvm-mmu_lock))
cond_resched_lock(kvm-mmu_lock);
-   }
}
}
 
-   kvm_flush_remote_tlbs(kvm);
spin_unlock(kvm-mmu_lock);
+
+   /*
+* We can flush all the TLBs out of the mmu lock without TLB
+* corruption since we just change the spte from writable to
+* readonly so that we only need to care the case of changing
+* spte from present to present (changing the spte from present
+* to nonpresent will flush all the TLBs immediately), in other
+* words, the only case we care is mmu_spte_update() where we
+* haved checked SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE
+* instead of PT_WRITABLE_MASK, that means it does not depend
+* on PT_WRITABLE_MASK anymore.
+*/
+   kvm_flush_remote_tlbs(kvm);
 }
 
 #define BATCH_ZAP_PAGES10
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6ad0c07..72f1487 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3560,11 +3560,16 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct 
kvm_dirty_log *log)
offset = i * BITS_PER_LONG;
kvm_mmu_write_protect_pt_masked(kvm, memslot, offset, mask);
}
-   if (is_dirty)
-   kvm_flush_remote_tlbs(kvm);
 
spin_unlock(kvm-mmu_lock);
 
+   /*
+* All the TLBs can be flushed out of mmu lock, see the comments in
+* kvm_mmu_slot_remove_write_access().
+*/
+   if (is_dirty)
+   kvm_flush_remote_tlbs(kvm);
+
r = -EFAULT;
if (copy_to_user(log-dirty_bitmap, dirty_bitmap_buffer, n))
goto out;
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/