Re: [PATCH] RFC: alias rework

[Izik Eidus <ieidus@redhat.com>]

On Mon, 25 Jan 2010 17:45:53 -0200
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> Izik,
> 
> On Mon, Jan 25, 2010 at 03:53:44PM +0200, Izik Eidus wrote:
> > >From f94dcd1ccabbcdb51ed7c37c5f58f00a5c1b7eec Mon Sep 17 00:00:00 2001
> > From: Izik Eidus <ieidus@redhat.com>
> > 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 <ieidus@redhat.com>
> > ---
> >  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(-)
> > 
> 
> > @@ -2661,7 +2611,18 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> >  		struct kvm_memslots *slots, *old_slots;
> >  
> >  		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.

> 
> > +#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;
> > +			}
> > +		}
> > +	}
> > +}
> > +
> > +#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.

Thanks.