Re: KVM swapping with mmu notifiers

[Marcelo Tosatti <marcelo-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org>]

Hi Andrea,

> path for kvm so I guess not worth optimizing in the short/mid term,
> but by optimizing the invalidate_page case we may halve the number of
> tlb flushes for some common case. I leave it for later, the swapping
> is heavily I/O bound anyway so a some more ipi in smp host shouldn't
> be very measurable (on UP host it makes no difference to flush
> multiple times in practice).
> 
> Signed-off-by: Andrea Arcangeli <andrea-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index 4086080..c527d7d 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -18,6 +18,7 @@ config KVM
>  	tristate "Kernel-based Virtual Machine (KVM) support"
>  	depends on ARCH_SUPPORTS_KVM && EXPERIMENTAL
>  	select PREEMPT_NOTIFIERS
> +	select MMU_NOTIFIER
>  	select ANON_INODES
>  	---help---
>  	  Support hosting fully virtualized guest machines using hardware
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 324ff9a..103c270 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -532,6 +532,36 @@ static void rmap_write_protect(struct kvm *kvm, u64 gfn)
>  		kvm_flush_remote_tlbs(kvm);
>  }
>  
> +static void unmap_spte(struct kvm *kvm, u64 *spte)
> +{
> +	struct page *page = pfn_to_page((*spte & PT64_BASE_ADDR_MASK) >> PAGE_SHIFT);
> +	get_page(page);
> +	rmap_remove(kvm, spte);
> +	set_shadow_pte(spte, shadow_trap_nonpresent_pte);
> +	kvm_flush_remote_tlbs(kvm);
> +	__free_page(page);
> +}
> +
> +void kvm_rmap_unmap_gfn(struct kvm *kvm, gfn_t gfn)
> +{
> +	unsigned long *rmapp;
> +	u64 *spte, *curr_spte;
> +
> +	spin_lock(&kvm->mmu_lock);
> +	gfn = unalias_gfn(kvm, gfn);
> +	rmapp = gfn_to_rmap(kvm, gfn);

The alias and memslot maps are protected only by mmap_sem, so you
should make kvm_set_memory_region/set_memory_alias grab the mmu spinlock
in addition to mmap_sem in write mode.

kvm_mmu_zap_all() grabs the mmu lock.. that should probably move up into
the caller.

> +
> +	spte = rmap_next(kvm, rmapp, NULL);
> +	while (spte) {
> +		BUG_ON(!(*spte & PT_PRESENT_MASK));
> +		rmap_printk("rmap_swap_page: spte %p %llx\n", spte, *spte);
> +		curr_spte = spte;
> +		spte = rmap_next(kvm, rmapp, spte);
> +		unmap_spte(kvm, curr_spte);
> +	}
> +	spin_unlock(&kvm->mmu_lock);
> +}
> +
>  #ifdef MMU_DEBUG
>  static int is_empty_shadow_page(u64 *spt)
>  {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8a90403..e9a3f6e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3159,6 +3159,36 @@ void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
>  	free_page((unsigned long)vcpu->arch.pio_data);
>  }
>  
> +static inline struct kvm *mmu_notifier_to_kvm(struct mmu_notifier *mn)
> +{
> +	return container_of(mn, struct kvm, mmu_notifier);
> +}
> +
> +void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
> +				      struct mm_struct *mm,
> +				      unsigned long address)
> +{
> +	struct kvm *kvm = mmu_notifier_to_kvm(mn);
> +	gfn_t gfn = hva_to_gfn(kvm, address);
> +	BUG_ON(mm != kvm->mm);
> +	if (gfn == -1UL)
> +		return;
> +	kvm_rmap_unmap_gfn(kvm, gfn);

And then you also need to cover "hva_to_gfn()" to happen under the lock.

> +}
> +
> +void kvm_mmu_notifier_invalidate_range(struct mmu_notifier *mn,
> +				       struct mm_struct *mm,
> +				       unsigned long start, unsigned long end)
> +{
> +	for (; start < end; start += PAGE_SIZE)
> +		kvm_mmu_notifier_invalidate_page(mn, mm, start);
> +}
> +
> +static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {
> +	.invalidate_range	= kvm_mmu_notifier_invalidate_range,
> +	.invalidate_page	= kvm_mmu_notifier_invalidate_page,
> +};
> +
>  struct  kvm *kvm_arch_create_vm(void)
>  {
>  	struct kvm *kvm = kzalloc(sizeof(struct kvm), GFP_KERNEL);
> @@ -3167,6 +3197,7 @@ struct  kvm *kvm_arch_create_vm(void)
>  		return ERR_PTR(-ENOMEM);
>  
>  	INIT_LIST_HEAD(&kvm->arch.active_mmu_pages);
> +	kvm->mmu_notifier.ops = &kvm_mmu_notifier_ops;
>  
>  	return kvm;
>  }
> diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
> index d6db0de..feacd77 100644
> --- a/include/asm-x86/kvm_host.h
> +++ b/include/asm-x86/kvm_host.h
> @@ -404,6 +404,7 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu);
>  int kvm_mmu_setup(struct kvm_vcpu *vcpu);
>  void kvm_mmu_set_nonpresent_ptes(u64 trap_pte, u64 notrap_pte);
>  
> +void kvm_rmap_unmap_gfn(struct kvm *kvm, gfn_t gfn);
>  int kvm_mmu_reset_context(struct kvm_vcpu *vcpu);
>  void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
>  void kvm_mmu_zap_all(struct kvm *kvm);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2714068..85da7fa 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -117,6 +117,7 @@ struct kvm {
>  	struct kvm_io_bus pio_bus;
>  	struct kvm_vm_stat stat;
>  	struct kvm_arch arch;
> +	struct mmu_notifier mmu_notifier;
>  };
>  
>  /* The guest did something we don't support. */
> @@ -163,6 +164,7 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot old,
>  				int user_alloc);
>  gfn_t unalias_gfn(struct kvm *kvm, gfn_t gfn);
> +gfn_t hva_to_gfn(struct kvm *kvm, unsigned long addr);
>  struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn);
>  void kvm_release_page_clean(struct page *page);
>  void kvm_release_page_dirty(struct page *page);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4295623..8f1dd86 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -165,6 +165,7 @@ static struct kvm *kvm_create_vm(void)
>  
>  	kvm->mm = current->mm;
>  	atomic_inc(&kvm->mm->mm_count);
> +	mmu_notifier_register(&kvm->mmu_notifier, kvm->mm);
>  	spin_lock_init(&kvm->mmu_lock);
>  	kvm_io_bus_init(&kvm->pio_bus);
>  	mutex_init(&kvm->lock);
> @@ -454,6 +455,23 @@ static unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
>  	return (slot->userspace_addr + (gfn - slot->base_gfn) * PAGE_SIZE);
>  }
>  
> +gfn_t hva_to_gfn(struct kvm *kvm, unsigned long addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < kvm->nmemslots; i++) {
> +		struct kvm_memory_slot *memslot = &kvm->memslots[i];
> +		unsigned long start = memslot->userspace_addr;
> +		unsigned long end = start + (memslot->npages << PAGE_SHIFT);
> +
> +		if (addr >= start && addr < end) {
> +			gfn_t gfn_offset = (addr - start) >> PAGE_SHIFT;
> +			return memslot->base_gfn + gfn_offset;
> +		}
> +	}
> +	return -1UL;
> +}
> +
>  /*
>   * Requires current->mm->mmap_sem to be held
>   */
> 
> 
> 
> And here a compatibility patch to kvm-userland so the external module
> still compile and runs with older kernels w/o MMU_NOTIFIER patch applied.
> 
> Signed-off-by: Andrea Arcangeli <andrea-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> 
> diff --git a/kernel/external-module-compat.h b/kernel/external-module-compat.h
> index 67b9cc4..34ef0a5 100644
> --- a/kernel/external-module-compat.h
> +++ b/kernel/external-module-compat.h
> @@ -17,6 +17,28 @@
>  #include <linux/hrtimer.h>
>  #include <asm/bitops.h>
>  
> +#ifndef CONFIG_MMU_NOTIFIER
> +struct mmu_notifier;
> +
> +struct mmu_notifier_ops {
> +	void (*release)(struct mmu_notifier * mn,
> +			struct mm_struct *mm);
> +	void (*invalidate_page)(struct mmu_notifier * mn,
> +				struct mm_struct *mm,
> +				unsigned long address);
> +	void (*invalidate_range)(struct mmu_notifier * mn,
> +				 struct mm_struct *mm,
> +				 unsigned long start, unsigned long end);
> +};
> +
> +struct mmu_notifier {
> +	const struct mmu_notifier_ops *ops;
> +};
> +#define mmu_notifier_register(mn, mm) do {} while(0)
> +#define mmu_notifier_unregister(mn) do {} while (0)
> +#define mmu_notifier_release(mm) do {} while (0)
> +#endif
> +
>  /*
>   * 2.6.16 does not have GFP_NOWAIT
>   */
> 
> 
> Here another patch for kvm-userland where I can't see symmetry between
> the lack of atomic_inc despite mmdrop is still run. I can't possibly
> see how this is supposedly not required when compiled into the kernel
> vs external module. Either atomic_inc is needed in both or none. Even
> if I'm right still this bug wasn't destabilizing because
> atomic_inc_and_dec only fires once in the overflow check, so it
> shouldn't matter to run one mmdrop more than needed, but it's good for
> correctness.
> 
> Signed-off-by: Andrea Arcangeli <andrea-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> 
> diff --git a/kernel/hack-module.awk b/kernel/hack-module.awk
> index 5187c96..884bc50 100644
> --- a/kernel/hack-module.awk
> +++ b/kernel/hack-module.awk
> @@ -33,8 +33,6 @@
>      vmx_load_host_state = 0
>  }
>  
> -/atomic_inc\(&kvm->mm->mm_count\);/ { $0 = "//" $0 }
> -
>  /^\t\.fault = / {
>      fcn = gensub(/,/, "", "g", $3)
>      $0 = "\t.VMA_OPS_FAULT(fault) = VMA_OPS_FAULT_FUNC(" fcn "),"
> 
> 
> I'll post the mmu-notifiers patch (required in the host kernel to run
> the above) separately in CC with more mailing lists because that's not
> KVM code at all and we hope to get it merged in the mainline kernel
> soon after getting feedback on the interface from the other users of
> the mmu notifiers.
> 
> Thanks!
> Andrea
> 
> -------------------------------------------------------------------------
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace