Re: [PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2013-01-08 Thread Gleb Natapov 
On Mon, Jan 07, 2013 at 06:11:46PM -0200, Marcelo Tosatti wrote:
 On Tue, Dec 18, 2012 at 04:26:47PM +0900, Takuya Yoshikawa wrote:
  This is needed to make kvm_mmu_slot_remove_write_access() rmap based:
  otherwise we may end up using invalid rmap's.
  
  Signed-off-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp
 
 Why? memslot-arch.rmap[] has been properly allocated at this point.
 
FWIW a long time ago in a galaxy far, far away there was a check for
KVM_MEM_LOG_DIRTY_PAGES before call to kvm_mmu_slot_remove_write_access(),
but it was removed by 90cb0529dd230548a7, as far as I can tell, accidentally.

  ---
   arch/x86/kvm/x86.c  |9 -
   virt/kvm/kvm_main.c |1 -
   2 files changed, 8 insertions(+), 2 deletions(-)
  
  diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
  index 1c9c834..9451efa 100644
  --- a/arch/x86/kvm/x86.c
  +++ b/arch/x86/kvm/x86.c
  @@ -6897,7 +6897,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
  spin_lock(kvm-mmu_lock);
  if (nr_mmu_pages)
  kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
  -   kvm_mmu_slot_remove_write_access(kvm, mem-slot);
  +   /*
  +* Write protect all pages for dirty logging.
  +* Existing largepage mappings are destroyed here and new ones will
  +* not be created until the end of the logging.
  +*/
  +   if ((mem-flags  KVM_MEM_LOG_DIRTY_PAGES) 
  +   !(old.flags  KVM_MEM_LOG_DIRTY_PAGES))
  +   kvm_mmu_slot_remove_write_access(kvm, mem-slot);
  spin_unlock(kvm-mmu_lock);
  /*
   * If memory slot is created, or moved, we need to clear all
  diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
  index bd31096..0ef5daa 100644
  --- a/virt/kvm/kvm_main.c
  +++ b/virt/kvm/kvm_main.c
  @@ -805,7 +805,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
  if ((new.flags  KVM_MEM_LOG_DIRTY_PAGES)  !new.dirty_bitmap) {
  if (kvm_create_dirty_bitmap(new)  0)
  goto out_free;
  -   /* destroy any largepage mappings for dirty tracking */
  }
   
  if (!npages || base_gfn != old.base_gfn) {
  -- 
  1.7.5.4
  
  --
  To unsubscribe from this list: send the line unsubscribe kvm in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2013-01-07 Thread Marcelo Tosatti 
On Tue, Dec 18, 2012 at 04:26:47PM +0900, Takuya Yoshikawa wrote:
 This is needed to make kvm_mmu_slot_remove_write_access() rmap based:
 otherwise we may end up using invalid rmap's.
 
 Signed-off-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp

Why? memslot-arch.rmap[] has been properly allocated at this point.

 ---
  arch/x86/kvm/x86.c  |9 -
  virt/kvm/kvm_main.c |1 -
  2 files changed, 8 insertions(+), 2 deletions(-)
 
 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
 index 1c9c834..9451efa 100644
 --- a/arch/x86/kvm/x86.c
 +++ b/arch/x86/kvm/x86.c
 @@ -6897,7 +6897,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
   spin_lock(kvm-mmu_lock);
   if (nr_mmu_pages)
   kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
 - kvm_mmu_slot_remove_write_access(kvm, mem-slot);
 + /*
 +  * Write protect all pages for dirty logging.
 +  * Existing largepage mappings are destroyed here and new ones will
 +  * not be created until the end of the logging.
 +  */
 + if ((mem-flags  KVM_MEM_LOG_DIRTY_PAGES) 
 + !(old.flags  KVM_MEM_LOG_DIRTY_PAGES))
 + kvm_mmu_slot_remove_write_access(kvm, mem-slot);
   spin_unlock(kvm-mmu_lock);
   /*
* If memory slot is created, or moved, we need to clear all
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index bd31096..0ef5daa 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -805,7 +805,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
   if ((new.flags  KVM_MEM_LOG_DIRTY_PAGES)  !new.dirty_bitmap) {
   if (kvm_create_dirty_bitmap(new)  0)
   goto out_free;
 - /* destroy any largepage mappings for dirty tracking */
   }
  
   if (!npages || base_gfn != old.base_gfn) {
 -- 
 1.7.5.4
 
 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2012-12-24 Thread Gleb Natapov 
On Tue, Dec 18, 2012 at 04:26:47PM +0900, Takuya Yoshikawa wrote:
 This is needed to make kvm_mmu_slot_remove_write_access() rmap based:
 otherwise we may end up using invalid rmap's.
 
 Signed-off-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp
 ---
  arch/x86/kvm/x86.c  |9 -
  virt/kvm/kvm_main.c |1 -
  2 files changed, 8 insertions(+), 2 deletions(-)
 
 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
 index 1c9c834..9451efa 100644
 --- a/arch/x86/kvm/x86.c
 +++ b/arch/x86/kvm/x86.c
 @@ -6897,7 +6897,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
   spin_lock(kvm-mmu_lock);
   if (nr_mmu_pages)
   kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
 - kvm_mmu_slot_remove_write_access(kvm, mem-slot);
 + /*
 +  * Write protect all pages for dirty logging.
 +  * Existing largepage mappings are destroyed here and new ones will
 +  * not be created until the end of the logging.
 +  */
 + if ((mem-flags  KVM_MEM_LOG_DIRTY_PAGES) 
 + !(old.flags  KVM_MEM_LOG_DIRTY_PAGES))
 + kvm_mmu_slot_remove_write_access(kvm, mem-slot);
We should not check old slot flags here or at least check that
old.npages is not zero. Userspace may delete a slot using old flags,
then, if new memslot is created with dirty log enabled, it will not be
protected.

   spin_unlock(kvm-mmu_lock);
   /*
* If memory slot is created, or moved, we need to clear all
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index bd31096..0ef5daa 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -805,7 +805,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
   if ((new.flags  KVM_MEM_LOG_DIRTY_PAGES)  !new.dirty_bitmap) {
   if (kvm_create_dirty_bitmap(new)  0)
   goto out_free;
 - /* destroy any largepage mappings for dirty tracking */
   }
  
   if (!npages || base_gfn != old.base_gfn) {
 -- 
 1.7.5.4

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2012-12-24 Thread Takuya Yoshikawa 
On Mon, 24 Dec 2012 15:27:17 +0200
Gleb Natapov g...@redhat.com wrote:

  @@ -6897,7 +6897,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
  spin_lock(kvm-mmu_lock);
  if (nr_mmu_pages)
  kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
  -   kvm_mmu_slot_remove_write_access(kvm, mem-slot);
  +   /*
  +* Write protect all pages for dirty logging.
  +* Existing largepage mappings are destroyed here and new ones will
  +* not be created until the end of the logging.
  +*/
  +   if ((mem-flags  KVM_MEM_LOG_DIRTY_PAGES) 
  +   !(old.flags  KVM_MEM_LOG_DIRTY_PAGES))
  +   kvm_mmu_slot_remove_write_access(kvm, mem-slot);
 We should not check old slot flags here or at least check that
 old.npages is not zero. Userspace may delete a slot using old flags,
 then, if new memslot is created with dirty log enabled, it will not be
 protected.

The flag, KVM_MEM_LOG_DIRTY_PAGES, is explicitely cleared when we
delete a slot to free dirty_bitmap:

if (!npages)
mem-flags = ~KVM_MEM_LOG_DIRTY_PAGES;

So when old.npages is not zero, the second condition must be true.

Other parts are doing if (!slot-dirty_bitmap) to see if the slot
is in dirty logging mode.  If you prefer, we can do the same here.

Thanks,
Takuya
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2012-12-24 Thread Gleb Natapov 
On Tue, Dec 25, 2012 at 01:08:40PM +0900, Takuya Yoshikawa wrote:
 On Mon, 24 Dec 2012 15:27:17 +0200
 Gleb Natapov g...@redhat.com wrote:
 
   @@ -6897,7 +6897,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 spin_lock(kvm-mmu_lock);
 if (nr_mmu_pages)
 kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
   - kvm_mmu_slot_remove_write_access(kvm, mem-slot);
   + /*
   +  * Write protect all pages for dirty logging.
   +  * Existing largepage mappings are destroyed here and new ones will
   +  * not be created until the end of the logging.
   +  */
   + if ((mem-flags  KVM_MEM_LOG_DIRTY_PAGES) 
   + !(old.flags  KVM_MEM_LOG_DIRTY_PAGES))
   + kvm_mmu_slot_remove_write_access(kvm, mem-slot);
  We should not check old slot flags here or at least check that
  old.npages is not zero. Userspace may delete a slot using old flags,
  then, if new memslot is created with dirty log enabled, it will not be
  protected.
 
 The flag, KVM_MEM_LOG_DIRTY_PAGES, is explicitely cleared when we
 delete a slot to free dirty_bitmap:
 
   if (!npages)
   mem-flags = ~KVM_MEM_LOG_DIRTY_PAGES;
 
 So when old.npages is not zero, the second condition must be true.
 
Right. I was looking for where it is cleared and missed that.

 Other parts are doing if (!slot-dirty_bitmap) to see if the slot
 is in dirty logging mode.  If you prefer, we can do the same here.
 
Does not matter to me. If you think it will be more consistent do it.

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2012-12-24 Thread Takuya Yoshikawa 
On Tue, 25 Dec 2012 07:05:55 +0200
Gleb Natapov g...@redhat.com wrote:

  Other parts are doing if (!slot-dirty_bitmap) to see if the slot
  is in dirty logging mode.  If you prefer, we can do the same here.
  
 Does not matter to me. If you think it will be more consistent do it.

No preference at the moment.  But since .dirty_bitmap and .rmap may be
changed to control free_physmem(new, old) behaviour, I'll keep the code
as is.

I'll send some patches to clean up the current mem, new, old, slot
games in __kvm_set_memory_region() later.  I was confused many times
by these.

In addition, passing old slot by value to kvm_arch_commit_memory_region()
should be fixed as well.  The structure is too big to copy.

Thanks,
Takuya
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] KVM: Write protect the updated slot only when we start dirty logging

2012-12-17 Thread Takuya Yoshikawa 
This is needed to make kvm_mmu_slot_remove_write_access() rmap based:
otherwise we may end up using invalid rmap's.

Signed-off-by: Takuya Yoshikawa yoshikawa_takuya...@lab.ntt.co.jp
---
 arch/x86/kvm/x86.c  |9 -
 virt/kvm/kvm_main.c |1 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1c9c834..9451efa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6897,7 +6897,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
spin_lock(kvm-mmu_lock);
if (nr_mmu_pages)
kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
-   kvm_mmu_slot_remove_write_access(kvm, mem-slot);
+   /*
+* Write protect all pages for dirty logging.
+* Existing largepage mappings are destroyed here and new ones will
+* not be created until the end of the logging.
+*/
+   if ((mem-flags  KVM_MEM_LOG_DIRTY_PAGES) 
+   !(old.flags  KVM_MEM_LOG_DIRTY_PAGES))
+   kvm_mmu_slot_remove_write_access(kvm, mem-slot);
spin_unlock(kvm-mmu_lock);
/*
 * If memory slot is created, or moved, we need to clear all
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index bd31096..0ef5daa 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -805,7 +805,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
if ((new.flags  KVM_MEM_LOG_DIRTY_PAGES)  !new.dirty_bitmap) {
if (kvm_create_dirty_bitmap(new)  0)
goto out_free;
-   /* destroy any largepage mappings for dirty tracking */
}
 
if (!npages || base_gfn != old.base_gfn) {
-- 
1.7.5.4

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html