RFC: alias rework

From f94dcd1ccabbcdb51ed7c37c5f58f00a5c1b7eec Mon Sep 17 00:00:00 2001

Quoted 1 line

From f94dcd1ccabbcdb51ed7c37c5f58f00a5c1b7eec Mon Sep 17 00:00:00 2001

From: Izik Eidus <[email protected]>
Date: Mon, 25 Jan 2010 15:49:41 +0200
Subject: [PATCH] RFC: alias rework

This patch remove the old way of aliasing inside kvm and move into using aliasing with the same virtual addresses

This patch is really just early RFC just to know if you guys like this direction, and I need to clean some parts of it and test it more before I feel it ready to be merged...

Comments are more than welcome.

Thanks.

Signed-off-by Izik Eidus <[email protected]>

---
 arch/ia64/include/asm/kvm_host.h |    1 +
 arch/ia64/kvm/kvm-ia64.c         |    5 --
 arch/powerpc/kvm/powerpc.c       |    5 --
 arch/s390/include/asm/kvm_host.h |    1 +
 arch/s390/kvm/kvm-s390.c         |    5 --
 arch/x86/include/asm/kvm_host.h  |   19 ------
 arch/x86/include/asm/vmx.h       |    6 +-
 arch/x86/kvm/mmu.c               |   19 ++-----
 arch/x86/kvm/x86.c               |  114 +++++++++++--------------------------
 include/linux/kvm_host.h         |   11 +--
 virt/kvm/kvm_main.c              |   80 +++++++++++++++++++-------
 11 files changed, 107 insertions(+), 159 deletions(-)

virt/kvm/kvm_main.c +29 -0

+#ifdef CONFIG_X86
+
+static void update_alias_slots(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+	int i;
+
+	for (i = KVM_MEMORY_SLOTS; i < KVM_MEMORY_SLOTS + KVM_ALIAS_SLOTS;
+	     ++i) {
+		struct kvm_memory_slot *alias_memslot =
+						&kvm->memslots->memslots[i];
+		unsigned long size = slot->npages << PAGE_SHIFT;
+
+		if (alias_memslot->real_base_gfn >= slot->base_gfn &&
+		    alias_memslot->real_base_gfn < slot->base_gfn + size) {
+			if (slot->dirty_bitmap) {
+				unsigned long bitmap_addr;
+				unsigned long dirty_offset;
+				unsigned long offset_addr =
+						(alias_memslot->real_base_gfn -
+	    					slot->base_gfn) << PAGE_SHIFT;
+				alias_memslot->userspace_addr = 
+					slot->userspace_addr + offset_addr;
+
+				dirty_offset =
+					ALIGN(offset_addr, BITS_PER_LONG) / 8;
+				bitmap_addr = (unsigned long) slot->dirty_bitmap;
+				bitmap_addr += dirty_offset;
+				alias_memslot->dirty_bitmap = (unsigned long *)bitmap_addr;
+				alias_memslot->base_gfn = alias_memslot->real_base_gfn;

Izik,

Quoted 35 lines · Izik Eidus

Quoted 1 line

From f94dcd1ccabbcdb51ed7c37c5f58f00a5c1b7eec Mon Sep 17 00:00:00 2001

From: Izik Eidus <[email protected]>
Date: Mon, 25 Jan 2010 15:49:41 +0200
Subject: [PATCH] RFC: alias rework

This patch remove the old way of aliasing inside kvm and move into using aliasing with the same virtual addresses

This patch is really just early RFC just to know if you guys like this direction, and I need to clean some parts of it and test it more before I feel it ready to be merged...

Comments are more than welcome.

Thanks.

Signed-off-by Izik Eidus <[email protected]>

---
 arch/ia64/include/asm/kvm_host.h |    1 +
 arch/ia64/kvm/kvm-ia64.c         |    5 --
 arch/powerpc/kvm/powerpc.c       |    5 --
 arch/s390/include/asm/kvm_host.h |    1 +
 arch/s390/kvm/kvm-s390.c         |    5 --
 arch/x86/include/asm/kvm_host.h  |   19 ------
 arch/x86/include/asm/vmx.h       |    6 +-
 arch/x86/kvm/mmu.c               |   19 ++-----
 arch/x86/kvm/x86.c               |  114 +++++++++++--------------------------
 include/linux/kvm_host.h         |   11 +--
 virt/kvm/kvm_main.c              |   80 +++++++++++++++++++-------
 11 files changed, 107 insertions(+), 159 deletions(-)

arch/x86/kvm/x86.c +0 -0

@@ -2661,7 +2611,18 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		struct kvm_memslots *slots, *old_slots;
 

arch/x86/kvm/x86.c int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, · line 2615

Quoted 3 lines

arch/x86/kvm/x86.c +2 -0

 		spin_lock(&kvm->mmu_lock);
+		for (i = KVM_MEMORY_SLOTS; i < KVM_MEMORY_SLOTS +
+		      KVM_ALIAS_SLOTS; ++i) {

The plan is to kill KVM_ALIAS_SLOTS (aliases will share the 32 mem slots), right?

Quoted 35 lines

virt/kvm/kvm_main.c +34 -0

+#ifdef CONFIG_X86
+
+static void update_alias_slots(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+	int i;
+
+	for (i = KVM_MEMORY_SLOTS; i < KVM_MEMORY_SLOTS + KVM_ALIAS_SLOTS;
+	     ++i) {
+		struct kvm_memory_slot *alias_memslot =
+						&kvm->memslots->memslots[i];
+		unsigned long size = slot->npages << PAGE_SHIFT;
+
+		if (alias_memslot->real_base_gfn >= slot->base_gfn &&
+		    alias_memslot->real_base_gfn < slot->base_gfn + size) {
+			if (slot->dirty_bitmap) {
+				unsigned long bitmap_addr;
+				unsigned long dirty_offset;
+				unsigned long offset_addr =
+						(alias_memslot->real_base_gfn -
+	    					slot->base_gfn) << PAGE_SHIFT;
+				alias_memslot->userspace_addr = 
+					slot->userspace_addr + offset_addr;
+
+				dirty_offset =
+					ALIGN(offset_addr, BITS_PER_LONG) / 8;
+				bitmap_addr = (unsigned long) slot->dirty_bitmap;
+				bitmap_addr += dirty_offset;
+				alias_memslot->dirty_bitmap = (unsigned long *)bitmap_addr;
+				alias_memslot->base_gfn = alias_memslot->real_base_gfn;
+				alias_memslot->npages = alias_memslot->real_npages;
+			} else if (!slot->rmap) {
+				alias_memslot->base_gfn = 0;
+				alias_memslot->npages = 0;
+			}

virt/kvm/kvm_main.c out_err_nodisable: · line 491

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

+		}
+	}
+}
+
+#endif

Can't see why is this needed. What is the problem with nuking "child" aliases when deleting a real memslot?

Quoted 38 lines · On Mon, 25 Jan 2010 17:45:53 -0200 Marcelo Tosatti <[email protected]> wrote:

Izik,

Quoted 35 lines · Izik Eidus

Quoted 1 line

From f94dcd1ccabbcdb51ed7c37c5f58f00a5c1b7eec Mon Sep 17 00:00:00 2001

From: Izik Eidus <[email protected]>
Date: Mon, 25 Jan 2010 15:49:41 +0200
Subject: [PATCH] RFC: alias rework

This patch remove the old way of aliasing inside kvm and move into using aliasing with the same virtual addresses

This patch is really just early RFC just to know if you guys like this direction, and I need to clean some parts of it and test it more before I feel it ready to be merged...

Comments are more than welcome.

Thanks.

Signed-off-by Izik Eidus <[email protected]>

---
 arch/ia64/include/asm/kvm_host.h |    1 +
 arch/ia64/kvm/kvm-ia64.c         |    5 --
 arch/powerpc/kvm/powerpc.c       |    5 --
 arch/s390/include/asm/kvm_host.h |    1 +
 arch/s390/kvm/kvm-s390.c         |    5 --
 arch/x86/include/asm/kvm_host.h  |   19 ------
 arch/x86/include/asm/vmx.h       |    6 +-
 arch/x86/kvm/mmu.c               |   19 ++-----
 arch/x86/kvm/x86.c               |  114 +++++++++++--------------------------
 include/linux/kvm_host.h         |   11 +--
 virt/kvm/kvm_main.c              |   80 +++++++++++++++++++-------
 11 files changed, 107 insertions(+), 159 deletions(-)

arch/x86/kvm/x86.c +0 -0

@@ -2661,7 +2611,18 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		struct kvm_memslots *slots, *old_slots;
 

Quoted 6 lines

arch/x86/kvm/x86.c int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, · line 2615

Quoted 3 lines

arch/x86/kvm/x86.c +2 -0

 		spin_lock(&kvm->mmu_lock);
+		for (i = KVM_MEMORY_SLOTS; i < KVM_MEMORY_SLOTS +
+		      KVM_ALIAS_SLOTS; ++i) {

The plan is to kill KVM_ALIAS_SLOTS (aliases will share the 32 mem slots), right?

Hrmm I think we got to have this addition 4 KVM_MEMORY_SLOTS to keep the same beahivor with old userspaces
beacuse maybe some userspace apps use 32 slots already?

I dont mind remove it if you guys don`t think this is the case.

Quoted 34 lines

Quoted 34 lines

virt/kvm/kvm_main.c +34 -0

+#ifdef CONFIG_X86
+
+static void update_alias_slots(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+	int i;
+
+	for (i = KVM_MEMORY_SLOTS; i < KVM_MEMORY_SLOTS + KVM_ALIAS_SLOTS;
+	     ++i) {
+		struct kvm_memory_slot *alias_memslot =
+						&kvm->memslots->memslots[i];
+		unsigned long size = slot->npages << PAGE_SHIFT;
+
+		if (alias_memslot->real_base_gfn >= slot->base_gfn &&
+		    alias_memslot->real_base_gfn < slot->base_gfn + size) {
+			if (slot->dirty_bitmap) {
+				unsigned long bitmap_addr;
+				unsigned long dirty_offset;
+				unsigned long offset_addr =
+						(alias_memslot->real_base_gfn -
+	    					slot->base_gfn) << PAGE_SHIFT;
+				alias_memslot->userspace_addr = 
+					slot->userspace_addr + offset_addr;
+
+				dirty_offset =
+					ALIGN(offset_addr, BITS_PER_LONG) / 8;
+				bitmap_addr = (unsigned long) slot->dirty_bitmap;
+				bitmap_addr += dirty_offset;
+				alias_memslot->dirty_bitmap = (unsigned long *)bitmap_addr;
+				alias_memslot->base_gfn = alias_memslot->real_base_gfn;
+				alias_memslot->npages = alias_memslot->real_npages;
+			} else if (!slot->rmap) {
+				alias_memslot->base_gfn = 0;
+				alias_memslot->npages = 0;
+			}

Quoted 8 lines

virt/kvm/kvm_main.c out_err_nodisable: · line 491

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

+		}
+	}
+}
+
+#endif

Can't see why is this needed. What is the problem with nuking "child" aliases when deleting a real memslot?

The problem is that this memslot still point in the virtual address of the host, This mean that gfn_to_memslot/page will still work on gfns and will result in pages that are mapped into the virtual address that the userspace requested to remove from KVM.

With current code, if a memslot is deleted, access through any aliases that use it will fail (BTW it looks this is not properly handled, but thats a separate problem).

So AFAICS there is no requirement for an alias to continue "operable" if its parent memslot is deleted.

Or is this a feature you need?

Motivation is that nukeing aliases is simpler than adjusting them.

Quoted 3 lines · On Mon, 25 Jan 2010 18:20:39 -0200 Marcelo Tosatti <[email protected]> wrote:

With current code, if a memslot is deleted, access through any aliases that use it will fail (BTW it looks this is not properly handled, but thats a separate problem).

Yea I had some still open concerns about this code (this why I sent it on RFC)

Quoted 3 lines

So AFAICS there is no requirement for an alias to continue "operable" if its parent memslot is deleted.

With this patch alias will stop to opearte when the parent is deleted just like the behivor with the current code...

base_gfn will be set to 0 and npages will be set to 0 as well (the true values wil be hide in real_base_gfn...), so gfn_to_memslot and gfn_to_page will fail....

Quoted 2 lines

Or is this a feature you need?

I dont need it (I asked Avi to do something), So he said he want to nuke the aliasing from kvm and keep supporting the old userspace`s

Do you have any other way to achive this?

Btw I do realize it might be better not to push this patch and just keep the old way of treating aliasing as we have now, I really don`t mind.

Quoted 3 lines

Motivation is that nukeing aliases is simpler than adjusting them.

Agree.

Quoted 18 lines · Izik Eidus

Quoted 3 lines · On Mon, 25 Jan 2010 18:20:39 -0200 Marcelo Tosatti <[email protected]> wrote:

With current code, if a memslot is deleted, access through any aliases that use it will fail (BTW it looks this is not properly handled, but thats a separate problem).

Yea I had some still open concerns about this code (this why I sent it on RFC)

Quoted 3 lines

So AFAICS there is no requirement for an alias to continue "operable" if its parent memslot is deleted.

With this patch alias will stop to opearte when the parent is deleted just like the behivor with the current code...

Quoted 3 lines

base_gfn will be set to 0 and npages will be set to 0 as well (the true values wil be hide in real_base_gfn...), so gfn_to_memslot and gfn_to_page will fail....

But you adjust the alias (and keep it valid) if dirty logging is enabled?

Quoted 7 lines

Quoted 3 lines

Or is this a feature you need?

I dont need it (I asked Avi to do something), So he said he want to nuke the aliasing from kvm and keep supporting the old userspace`s

With feature i meant keeping the alias around when parent slot is deleted.

Quoted 2 lines

Do you have any other way to achive this?

No.

Quoted 8 lines

Btw I do realize it might be better not to push this patch and just keep the old way of treating aliasing as we have now, I really don`t mind.

Quoted 3 lines

Motivation is that nukeing aliases is simpler than adjusting them.

Agree.

Quoted 3 lines

Or is this a feature you need?

I dont need it (I asked Avi to do something), So he said he want to nuke the aliasing from kvm and keep supporting the old userspace`s

Do you have any other way to achive this?

Btw I do realize it might be better not to push this patch and just keep the old way of treating aliasing as we have now, I really don`t mind.

Quoted 9 lines · Izik Eidus

Quoted 3 lines

Or is this a feature you need?

I dont need it (I asked Avi to do something), So he said he want to nuke the aliasing from kvm and keep supporting the old userspace`s

Do you have any other way to achive this?

Quoted 3 lines

Btw I do realize it might be better not to push this patch and just keep the old way of treating aliasing as we have now, I really don`t mind.

How about implementing an alias pointing at a deleted slot as an invalid slot?

If the slot comes back later, we can revalidate it.