[PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Takuya Yoshikawa]

Patches 1 to 3 are trivial.

Patch 4 is the main cause of the increased lines, but I think the new
code makes it easier to understand why each condition in
__kvm_set_memory_region() is there.

If you don't agree with patch 4, please consider taking the rest of the
series at this time.

Takuya Yoshikawa (4):
  KVM: set_memory_region: Don't jump to out_free unnecessarily
  KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
  KVM: set_memory_region: Remove unnecessary variable memslot
  KVM: set_memory_region: Identify the requested change explicitly

 virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
 1 files changed, 59 insertions(+), 35 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] KVM: set_memory_region: Don't jump to out_free unnecessarily

[Takuya Yoshikawa]

This makes the separation between the sanity checks and the rest of the
code a bit clearer.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 virt/kvm/kvm_main.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e45c20c..d5e4bf9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -778,9 +778,9 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	 */
 	r = -EINVAL;
 	if (npages && old.npages && npages != old.npages)
-		goto out_free;
+		goto out;
 	if (!npages && !old.npages)
-		goto out_free;
+		goto out;
 
 	/* Check for overlaps */
 	r = -EEXIST;
@@ -789,7 +789,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 			continue;
 		if (!((base_gfn + npages <= slot->base_gfn) ||
 		      (base_gfn >= slot->base_gfn + slot->npages)))
-			goto out_free;
+			goto out;
 	}
 
 	/* Free page dirty bitmap if unneeded */
@@ -891,7 +891,6 @@ out_free:
 	kvm_free_physmem_slot(&new, &old);
 out:
 	return r;
-
 }
 EXPORT_SYMBOL_GPL(__kvm_set_memory_region);
 
-- 
1.7.5.4

[PATCH 2/4] KVM: set_memory_region: Don't check for overlaps unless we create or move a slot

[Takuya Yoshikawa]

Don't need the check for deleting an existing slot or just modifiying
the flags.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 virt/kvm/kvm_main.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d5e4bf9..f6c8cdc 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -782,14 +782,16 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	if (!npages && !old.npages)
 		goto out;
 
-	/* Check for overlaps */
-	r = -EEXIST;
-	kvm_for_each_memslot(slot, kvm->memslots) {
-		if (slot->id >= KVM_USER_MEM_SLOTS || slot == memslot)
-			continue;
-		if (!((base_gfn + npages <= slot->base_gfn) ||
-		      (base_gfn >= slot->base_gfn + slot->npages)))
-			goto out;
+	if ((npages && !old.npages) || (base_gfn != old.base_gfn)) {
+		/* Check for overlaps */
+		r = -EEXIST;
+		kvm_for_each_memslot(slot, kvm->memslots) {
+			if (slot->id >= KVM_USER_MEM_SLOTS || slot == memslot)
+				continue;
+			if (!((base_gfn + npages <= slot->base_gfn) ||
+			      (base_gfn >= slot->base_gfn + slot->npages)))
+				goto out;
+		}
 	}
 
 	/* Free page dirty bitmap if unneeded */
-- 
1.7.5.4

[PATCH 3/4] KVM: set_memory_region: Remove unnecessary variable memslot

[Takuya Yoshikawa]

One such variable, slot, is enough for holding a pointer temporarily.
We also remove another local variable named slot, which is limited in
a block, since it is confusing to have the same name in this function.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 virt/kvm/kvm_main.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f6c8cdc..8cb4e71 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -728,7 +728,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	int r;
 	gfn_t base_gfn;
 	unsigned long npages;
-	struct kvm_memory_slot *memslot, *slot;
+	struct kvm_memory_slot *slot;
 	struct kvm_memory_slot old, new;
 	struct kvm_memslots *slots = NULL, *old_memslots;
 
@@ -754,7 +754,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
 		goto out;
 
-	memslot = id_to_memslot(kvm->memslots, mem->slot);
+	slot = id_to_memslot(kvm->memslots, mem->slot);
 	base_gfn = mem->guest_phys_addr >> PAGE_SHIFT;
 	npages = mem->memory_size >> PAGE_SHIFT;
 
@@ -765,7 +765,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	if (!npages)
 		mem->flags &= ~KVM_MEM_LOG_DIRTY_PAGES;
 
-	new = old = *memslot;
+	new = old = *slot;
 
 	new.id = mem->slot;
 	new.base_gfn = base_gfn;
@@ -786,7 +786,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		/* Check for overlaps */
 		r = -EEXIST;
 		kvm_for_each_memslot(slot, kvm->memslots) {
-			if (slot->id >= KVM_USER_MEM_SLOTS || slot == memslot)
+			if ((slot->id >= KVM_USER_MEM_SLOTS) ||
+			    (slot->id == mem->slot))
 				continue;
 			if (!((base_gfn + npages <= slot->base_gfn) ||
 			      (base_gfn >= slot->base_gfn + slot->npages)))
@@ -823,8 +824,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	}
 
 	if (!npages || base_gfn != old.base_gfn) {
-		struct kvm_memory_slot *slot;
-
 		r = -ENOMEM;
 		slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
 				GFP_KERNEL);
-- 
1.7.5.4

[PATCH 4/4] KVM: set_memory_region: Identify the requested change explicitly

[Takuya Yoshikawa]

KVM_SET_USER_MEMORY_REGION forces __kvm_set_memory_region() to identify
what kind of change is being requested by checking the parameters.  The
current code does this checking at various points in code and each
condition used there is not easy to understand at first glance.

This patch consolidates these and introduces an enum to name the
possible changes to clean up the code.

Although this does not introduce any user visible changes, there are two
changes which optimize the code a bit:

  1. If we have nothing to change, the new code returns immediately.
  2. If we just modify the flags, no changes about the mapping, the
     new code does not call kvm_iommu_map_pages().

One interesting thing we noticed was about the case 1.  Although this
case seemed unimportant, QEMU was relying on the current behaviour:
when we returned a value other than 0, e.g. -EINVAL, live migration
failed at the final stage with a section mismatch error.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 virt/kvm/kvm_main.c |   64 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8cb4e71..5426e96 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -714,6 +714,24 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
 }
 
 /*
+ * KVM_SET_USER_MEMORY_REGION ioctl allows the following operations:
+ * - create a new memory slot
+ * - delete an existing memory slot
+ * - modify an existing memory slot
+ *   -- move it in the guest physical memory space
+ *   -- just change its flags
+ *
+ * Since flags can be changed by some of these operations, the following
+ * differentiation is the best we can do for __kvm_set_memory_region():
+ */
+enum kvm_mr_change {
+	KVM_MR_CREATE,
+	KVM_MR_DELETE,
+	KVM_MR_MOVE,
+	KVM_MR_FLAGS_ONLY,
+};
+
+/*
  * Allocate some memory and give it an address in the guest physical address
  * space.
  *
@@ -731,6 +749,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	struct kvm_memory_slot *slot;
 	struct kvm_memory_slot old, new;
 	struct kvm_memslots *slots = NULL, *old_memslots;
+	enum kvm_mr_change change;
 
 	r = check_memory_region_flags(mem);
 	if (r)
@@ -772,17 +791,30 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	new.npages = npages;
 	new.flags = mem->flags;
 
-	/*
-	 * Disallow changing a memory slot's size or changing anything about
-	 * zero sized slots that doesn't involve making them non-zero.
-	 */
 	r = -EINVAL;
-	if (npages && old.npages && npages != old.npages)
-		goto out;
-	if (!npages && !old.npages)
+	if (npages) {
+		if (!old.npages)
+			change = KVM_MR_CREATE;
+		else { /* Modify an existing slot. */
+			if ((mem->userspace_addr != old.userspace_addr) ||
+			    (npages != old.npages))
+				goto out;
+
+			if (base_gfn != old.base_gfn)
+				change = KVM_MR_MOVE;
+			else if (new.flags != old.flags)
+				change = KVM_MR_FLAGS_ONLY;
+			else { /* Nothing to change. */
+				r = 0;
+				goto out;
+			}
+		}
+	} else if (old.npages) {
+		change = KVM_MR_DELETE;
+	} else /* Modify a non-existent slot: disallowed. */
 		goto out;
 
-	if ((npages && !old.npages) || (base_gfn != old.base_gfn)) {
+	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
 		/* Check for overlaps */
 		r = -EEXIST;
 		kvm_for_each_memslot(slot, kvm->memslots) {
@@ -800,20 +832,12 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		new.dirty_bitmap = NULL;
 
 	r = -ENOMEM;
-
-	/*
-	 * Allocate if a slot is being created.  If modifying a slot,
-	 * the userspace_addr cannot change.
-	 */
-	if (!old.npages) {
+	if (change == KVM_MR_CREATE) {
 		new.user_alloc = user_alloc;
 		new.userspace_addr = mem->userspace_addr;
 
 		if (kvm_arch_create_memslot(&new, npages))
 			goto out_free;
-	} else if (npages && mem->userspace_addr != old.userspace_addr) {
-		r = -EINVAL;
-		goto out_free;
 	}
 
 	/* Allocate page dirty bitmap if needed */
@@ -823,7 +847,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		/* destroy any largepage mappings for dirty tracking */
 	}
 
-	if (!npages || base_gfn != old.base_gfn) {
+	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) {
 		r = -ENOMEM;
 		slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
 				GFP_KERNEL);
@@ -865,14 +889,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	}
 
 	/* map new memory slot into the iommu */
-	if (npages) {
+	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
 		r = kvm_iommu_map_pages(kvm, &new);
 		if (r)
 			goto out_slots;
 	}
 
 	/* actual memory is freed via old in kvm_free_physmem_slot below */
-	if (!npages) {
+	if (change == KVM_MR_DELETE) {
 		new.dirty_bitmap = NULL;
 		memset(&new.arch, 0, sizeof(new.arch));
 	}
-- 
1.7.5.4

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Marcelo Tosatti]

On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> Patches 1 to 3 are trivial.
> 
> Patch 4 is the main cause of the increased lines, but I think the new
> code makes it easier to understand why each condition in
> __kvm_set_memory_region() is there.
> 
> If you don't agree with patch 4, please consider taking the rest of the
> series at this time.
> 
> Takuya Yoshikawa (4):
>   KVM: set_memory_region: Don't jump to out_free unnecessarily
>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
>   KVM: set_memory_region: Remove unnecessary variable memslot
>   KVM: set_memory_region: Identify the requested change explicitly
> 
>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 59 insertions(+), 35 deletions(-)
> 
> -- 
> 1.7.5.4

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

BTW, while at it, its probably worthwhile to restrict flags
modifications: change from flags = 0 to flags = read-only is 
incomplete. Xiao, should it be allowed only during creation?

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Xiao Guangrong]

On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
>> Patches 1 to 3 are trivial.
>>
>> Patch 4 is the main cause of the increased lines, but I think the new
>> code makes it easier to understand why each condition in
>> __kvm_set_memory_region() is there.
>>
>> If you don't agree with patch 4, please consider taking the rest of the
>> series at this time.
>>
>> Takuya Yoshikawa (4):
>>   KVM: set_memory_region: Don't jump to out_free unnecessarily
>>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
>>   KVM: set_memory_region: Remove unnecessary variable memslot
>>   KVM: set_memory_region: Identify the requested change explicitly
>>
>>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
>>  1 files changed, 59 insertions(+), 35 deletions(-)
>>
>> -- 
>> 1.7.5.4
> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> BTW, while at it, its probably worthwhile to restrict flags
> modifications: change from flags = 0 to flags = read-only is 
> incomplete. Xiao, should it be allowed only during creation?

Will Readonly memory be used for VM-mem-sharing in the future?
I remember you mentioned about mem-sharing things before. ;)

Actually, It is safe on KVM MMU because all the gfns in the slot
can become readonly after calling __kvm_set_memory_region. It is
unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
when the flag is changed.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Gleb Natapov]

On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> >> Patches 1 to 3 are trivial.
> >>
> >> Patch 4 is the main cause of the increased lines, but I think the new
> >> code makes it easier to understand why each condition in
> >> __kvm_set_memory_region() is there.
> >>
> >> If you don't agree with patch 4, please consider taking the rest of the
> >> series at this time.
> >>
> >> Takuya Yoshikawa (4):
> >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> >>   KVM: set_memory_region: Remove unnecessary variable memslot
> >>   KVM: set_memory_region: Identify the requested change explicitly
> >>
> >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> >>  1 files changed, 59 insertions(+), 35 deletions(-)
> >>
> >> -- 
> >> 1.7.5.4
> > 
> > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > BTW, while at it, its probably worthwhile to restrict flags
> > modifications: change from flags = 0 to flags = read-only is 
> > incomplete. Xiao, should it be allowed only during creation?
> 
> Will Readonly memory be used for VM-mem-sharing in the future?
> I remember you mentioned about mem-sharing things before. ;)
> 
> Actually, It is safe on KVM MMU because all the gfns in the slot
> can become readonly after calling __kvm_set_memory_region. It is
> unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> when the flag is changed.

Which means if we allow changing flags from 0 to read_only or back we
need to call kvm_iommu_map_pages() when flags change. BTW as far as I
see currently IOMMU maps everything as read/write not matter what flags
say. Looks like a bug. Alex?

--
			Gleb.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Marcelo Tosatti]

On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> >> Patches 1 to 3 are trivial.
> >>
> >> Patch 4 is the main cause of the increased lines, but I think the new
> >> code makes it easier to understand why each condition in
> >> __kvm_set_memory_region() is there.
> >>
> >> If you don't agree with patch 4, please consider taking the rest of the
> >> series at this time.
> >>
> >> Takuya Yoshikawa (4):
> >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> >>   KVM: set_memory_region: Remove unnecessary variable memslot
> >>   KVM: set_memory_region: Identify the requested change explicitly
> >>
> >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> >>  1 files changed, 59 insertions(+), 35 deletions(-)
> >>
> >> -- 
> >> 1.7.5.4
> > 
> > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > BTW, while at it, its probably worthwhile to restrict flags
> > modifications: change from flags = 0 to flags = read-only is 
> > incomplete. Xiao, should it be allowed only during creation?
> 
> Will Readonly memory be used for VM-mem-sharing in the future?
> I remember you mentioned about mem-sharing things before. ;)

It can.

> Actually, It is safe on KVM MMU because all the gfns in the slot
> can become readonly after calling __kvm_set_memory_region. It is
> unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> when the flag is changed.

Kvm.git next does not clear spte W bit on 0 -> readonly flags change.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Gleb Natapov]

On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> Patches 1 to 3 are trivial.
> 
> Patch 4 is the main cause of the increased lines, but I think the new
> code makes it easier to understand why each condition in
> __kvm_set_memory_region() is there.
> 
> If you don't agree with patch 4, please consider taking the rest of the
> series at this time.
> 
> Takuya Yoshikawa (4):
>   KVM: set_memory_region: Don't jump to out_free unnecessarily
>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
>   KVM: set_memory_region: Remove unnecessary variable memslot
>   KVM: set_memory_region: Identify the requested change explicitly
> 
>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 59 insertions(+), 35 deletions(-)
> 
Applied 1-3. It is not clear whether kvm_iommu_map_pages() should be called
when flags change, so not applying 4 for now.

--
			Gleb.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Takuya Yoshikawa]

On Thu, 17 Jan 2013 14:42:02 +0200
Gleb Natapov <gleb@redhat.com> wrote:

> Applied 1-3. It is not clear whether kvm_iommu_map_pages() should be called
> when flags change, so not applying 4 for now.

Thank you.

Although I confirmed that kvm_iommu_map_pages() does nothing with
the flags, it needs to be confirmed by the author.

Another thing I'm not 100% sure about the API is whether we can
change the flags when we move the base: flags + move.  The current
implementation allows it, but not sure if it's a good thing.

	Takuya

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Gleb Natapov]

On Thu, Jan 17, 2013 at 11:26:53PM +0900, Takuya Yoshikawa wrote:
> On Thu, 17 Jan 2013 14:42:02 +0200
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > Applied 1-3. It is not clear whether kvm_iommu_map_pages() should be called
> > when flags change, so not applying 4 for now.
> 
> Thank you.
> 
> Although I confirmed that kvm_iommu_map_pages() does nothing with
> the flags, it needs to be confirmed by the author.
> 
Yes, it may be a bug. I asked Alex in other email.

> Another thing I'm not 100% sure about the API is whether we can
> change the flags when we move the base: flags + move.  The current
> implementation allows it, but not sure if it's a good thing.
> 
> 	Takuya

--
			Gleb.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Alex Williamson]

On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote:
> On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> > >> Patches 1 to 3 are trivial.
> > >>
> > >> Patch 4 is the main cause of the increased lines, but I think the new
> > >> code makes it easier to understand why each condition in
> > >> __kvm_set_memory_region() is there.
> > >>
> > >> If you don't agree with patch 4, please consider taking the rest of the
> > >> series at this time.
> > >>
> > >> Takuya Yoshikawa (4):
> > >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> > >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> > >>   KVM: set_memory_region: Remove unnecessary variable memslot
> > >>   KVM: set_memory_region: Identify the requested change explicitly
> > >>
> > >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> > >>  1 files changed, 59 insertions(+), 35 deletions(-)
> > >>
> > >> -- 
> > >> 1.7.5.4
> > > 
> > > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > > BTW, while at it, its probably worthwhile to restrict flags
> > > modifications: change from flags = 0 to flags = read-only is 
> > > incomplete. Xiao, should it be allowed only during creation?
> > 
> > Will Readonly memory be used for VM-mem-sharing in the future?
> > I remember you mentioned about mem-sharing things before. ;)
> > 
> > Actually, It is safe on KVM MMU because all the gfns in the slot
> > can become readonly after calling __kvm_set_memory_region. It is
> > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> > when the flag is changed.
> 
> Which means if we allow changing flags from 0 to read_only or back we
> need to call kvm_iommu_map_pages() when flags change. BTW as far as I
> see currently IOMMU maps everything as read/write not matter what flags
> say. Looks like a bug. Alex?

That's correct, legacy device assignment is always r/w.  vfio handles
this correctly.  Thanks,

Alex

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Gleb Natapov]

On Thu, Jan 17, 2013 at 10:12:06AM -0700, Alex Williamson wrote:
> On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote:
> > On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> > > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> > > >> Patches 1 to 3 are trivial.
> > > >>
> > > >> Patch 4 is the main cause of the increased lines, but I think the new
> > > >> code makes it easier to understand why each condition in
> > > >> __kvm_set_memory_region() is there.
> > > >>
> > > >> If you don't agree with patch 4, please consider taking the rest of the
> > > >> series at this time.
> > > >>
> > > >> Takuya Yoshikawa (4):
> > > >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> > > >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> > > >>   KVM: set_memory_region: Remove unnecessary variable memslot
> > > >>   KVM: set_memory_region: Identify the requested change explicitly
> > > >>
> > > >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> > > >>  1 files changed, 59 insertions(+), 35 deletions(-)
> > > >>
> > > >> -- 
> > > >> 1.7.5.4
> > > > 
> > > > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > > 
> > > > BTW, while at it, its probably worthwhile to restrict flags
> > > > modifications: change from flags = 0 to flags = read-only is 
> > > > incomplete. Xiao, should it be allowed only during creation?
> > > 
> > > Will Readonly memory be used for VM-mem-sharing in the future?
> > > I remember you mentioned about mem-sharing things before. ;)
> > > 
> > > Actually, It is safe on KVM MMU because all the gfns in the slot
> > > can become readonly after calling __kvm_set_memory_region. It is
> > > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> > > when the flag is changed.
> > 
> > Which means if we allow changing flags from 0 to read_only or back we
> > need to call kvm_iommu_map_pages() when flags change. BTW as far as I
> > see currently IOMMU maps everything as read/write not matter what flags
> > say. Looks like a bug. Alex?
> 
> That's correct, legacy device assignment is always r/w.  vfio handles
> this correctly.  Thanks,
> 
Does this mean that device can write into read only memory?

--
			Gleb.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Alex Williamson]

On Thu, 2013-01-17 at 19:20 +0200, Gleb Natapov wrote:
> On Thu, Jan 17, 2013 at 10:12:06AM -0700, Alex Williamson wrote:
> > On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote:
> > > On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> > > > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > > > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> > > > >> Patches 1 to 3 are trivial.
> > > > >>
> > > > >> Patch 4 is the main cause of the increased lines, but I think the new
> > > > >> code makes it easier to understand why each condition in
> > > > >> __kvm_set_memory_region() is there.
> > > > >>
> > > > >> If you don't agree with patch 4, please consider taking the rest of the
> > > > >> series at this time.
> > > > >>
> > > > >> Takuya Yoshikawa (4):
> > > > >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> > > > >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> > > > >>   KVM: set_memory_region: Remove unnecessary variable memslot
> > > > >>   KVM: set_memory_region: Identify the requested change explicitly
> > > > >>
> > > > >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> > > > >>  1 files changed, 59 insertions(+), 35 deletions(-)
> > > > >>
> > > > >> -- 
> > > > >> 1.7.5.4
> > > > > 
> > > > > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > > > 
> > > > > BTW, while at it, its probably worthwhile to restrict flags
> > > > > modifications: change from flags = 0 to flags = read-only is 
> > > > > incomplete. Xiao, should it be allowed only during creation?
> > > > 
> > > > Will Readonly memory be used for VM-mem-sharing in the future?
> > > > I remember you mentioned about mem-sharing things before. ;)
> > > > 
> > > > Actually, It is safe on KVM MMU because all the gfns in the slot
> > > > can become readonly after calling __kvm_set_memory_region. It is
> > > > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> > > > when the flag is changed.
> > > 
> > > Which means if we allow changing flags from 0 to read_only or back we
> > > need to call kvm_iommu_map_pages() when flags change. BTW as far as I
> > > see currently IOMMU maps everything as read/write not matter what flags
> > > say. Looks like a bug. Alex?
> > 
> > That's correct, legacy device assignment is always r/w.  vfio handles
> > this correctly.  Thanks,
> > 
> Does this mean that device can write into read only memory?

For legacy assignment, I would expect yes.  On bare metal, DMA from an
I/O device can obviously circumvent processor based memory attributes,
but here that might also include things like ROMs too.  Thanks,

Alex

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Gleb Natapov]

On Thu, Jan 17, 2013 at 10:30:05AM -0700, Alex Williamson wrote:
> On Thu, 2013-01-17 at 19:20 +0200, Gleb Natapov wrote:
> > On Thu, Jan 17, 2013 at 10:12:06AM -0700, Alex Williamson wrote:
> > > On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote:
> > > > On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> > > > > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > > > > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> > > > > >> Patches 1 to 3 are trivial.
> > > > > >>
> > > > > >> Patch 4 is the main cause of the increased lines, but I think the new
> > > > > >> code makes it easier to understand why each condition in
> > > > > >> __kvm_set_memory_region() is there.
> > > > > >>
> > > > > >> If you don't agree with patch 4, please consider taking the rest of the
> > > > > >> series at this time.
> > > > > >>
> > > > > >> Takuya Yoshikawa (4):
> > > > > >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> > > > > >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> > > > > >>   KVM: set_memory_region: Remove unnecessary variable memslot
> > > > > >>   KVM: set_memory_region: Identify the requested change explicitly
> > > > > >>
> > > > > >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> > > > > >>  1 files changed, 59 insertions(+), 35 deletions(-)
> > > > > >>
> > > > > >> -- 
> > > > > >> 1.7.5.4
> > > > > > 
> > > > > > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > > > > 
> > > > > > BTW, while at it, its probably worthwhile to restrict flags
> > > > > > modifications: change from flags = 0 to flags = read-only is 
> > > > > > incomplete. Xiao, should it be allowed only during creation?
> > > > > 
> > > > > Will Readonly memory be used for VM-mem-sharing in the future?
> > > > > I remember you mentioned about mem-sharing things before. ;)
> > > > > 
> > > > > Actually, It is safe on KVM MMU because all the gfns in the slot
> > > > > can become readonly after calling __kvm_set_memory_region. It is
> > > > > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> > > > > when the flag is changed.
> > > > 
> > > > Which means if we allow changing flags from 0 to read_only or back we
> > > > need to call kvm_iommu_map_pages() when flags change. BTW as far as I
> > > > see currently IOMMU maps everything as read/write not matter what flags
> > > > say. Looks like a bug. Alex?
> > > 
> > > That's correct, legacy device assignment is always r/w.  vfio handles
> > > this correctly.  Thanks,
> > > 
> > Does this mean that device can write into read only memory?
> 
> For legacy assignment, I would expect yes.  On bare metal, DMA from an
> I/O device can obviously circumvent processor based memory attributes,
> but here that might also include things like ROMs too.  Thanks,
> 
Why dropping IOMMU_WRITE from flags if slot is read only in
kvm_iommu_map_pages() will not work?

--
			Gleb.

Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1

[Alex Williamson]

On Thu, 2013-01-17 at 19:58 +0200, Gleb Natapov wrote:
> On Thu, Jan 17, 2013 at 10:30:05AM -0700, Alex Williamson wrote:
> > On Thu, 2013-01-17 at 19:20 +0200, Gleb Natapov wrote:
> > > On Thu, Jan 17, 2013 at 10:12:06AM -0700, Alex Williamson wrote:
> > > > On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote:
> > > > > On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> > > > > > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > > > > > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> > > > > > >> Patches 1 to 3 are trivial.
> > > > > > >>
> > > > > > >> Patch 4 is the main cause of the increased lines, but I think the new
> > > > > > >> code makes it easier to understand why each condition in
> > > > > > >> __kvm_set_memory_region() is there.
> > > > > > >>
> > > > > > >> If you don't agree with patch 4, please consider taking the rest of the
> > > > > > >> series at this time.
> > > > > > >>
> > > > > > >> Takuya Yoshikawa (4):
> > > > > > >>   KVM: set_memory_region: Don't jump to out_free unnecessarily
> > > > > > >>   KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> > > > > > >>   KVM: set_memory_region: Remove unnecessary variable memslot
> > > > > > >>   KVM: set_memory_region: Identify the requested change explicitly
> > > > > > >>
> > > > > > >>  virt/kvm/kvm_main.c |   94 ++++++++++++++++++++++++++++++++-------------------
> > > > > > >>  1 files changed, 59 insertions(+), 35 deletions(-)
> > > > > > >>
> > > > > > >> -- 
> > > > > > >> 1.7.5.4
> > > > > > > 
> > > > > > > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > > > > > 
> > > > > > > BTW, while at it, its probably worthwhile to restrict flags
> > > > > > > modifications: change from flags = 0 to flags = read-only is 
> > > > > > > incomplete. Xiao, should it be allowed only during creation?
> > > > > > 
> > > > > > Will Readonly memory be used for VM-mem-sharing in the future?
> > > > > > I remember you mentioned about mem-sharing things before. ;)
> > > > > > 
> > > > > > Actually, It is safe on KVM MMU because all the gfns in the slot
> > > > > > can become readonly after calling __kvm_set_memory_region. It is
> > > > > > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> > > > > > when the flag is changed.
> > > > > 
> > > > > Which means if we allow changing flags from 0 to read_only or back we
> > > > > need to call kvm_iommu_map_pages() when flags change. BTW as far as I
> > > > > see currently IOMMU maps everything as read/write not matter what flags
> > > > > say. Looks like a bug. Alex?
> > > > 
> > > > That's correct, legacy device assignment is always r/w.  vfio handles
> > > > this correctly.  Thanks,
> > > > 
> > > Does this mean that device can write into read only memory?
> > 
> > For legacy assignment, I would expect yes.  On bare metal, DMA from an
> > I/O device can obviously circumvent processor based memory attributes,
> > but here that might also include things like ROMs too.  Thanks,
> > 
> Why dropping IOMMU_WRITE from flags if slot is read only in
> kvm_iommu_map_pages() will not work?

Something like:

--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -76,7 +76,9 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slo
        gfn     = slot->base_gfn;
        end_gfn = gfn + slot->npages;
 
-       flags = IOMMU_READ | IOMMU_WRITE;
+       flags = IOMMU_READ;
+       if (slot->flags & KVM_MEM_READONLY)
+               flags |= IOMMU_WRITE;
        if (kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY)
                flags |= IOMMU_CACHE;
 
We'd still need the unmap/remap when flags change.  I'll do some testing
on the above and send as a proper patch.  Thanks,

Alex