On Mon, Jan 25, 2010 at 09:57:43PM +0200, Izik Eidus wrote:
> On Mon, 25 Jan 2010 17:45:53 -0200
> Marcelo Tosatti <mtosatti@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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.
I'm fine with sharing (it also makes the code simpler).
Don't know of any apps/configs using more than (or close to) 32 slots.
> > > +#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.
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
