Re: [PATCH 18/38] KVM: PPC: E500: Implement MMU notifiers

From: Scott Wood <scottwood@freescale.com>
To: Alexander Graf <agraf@suse.de>
Cc: <kvm-ppc@vger.kernel.org>, KVM list <kvm@vger.kernel.org>
Subject: Re: [PATCH 18/38] KVM: PPC: E500: Implement MMU notifiers
Date: Tue, 14 Aug 2012 20:20:47 -0500	[thread overview]
Message-ID: <502AF96F.8090705@freescale.com> (raw)
In-Reply-To: <1344985483-7440-19-git-send-email-agraf@suse.de>

On 08/14/2012 06:04 PM, Alexander Graf wrote:
> The e500 target has lived without mmu notifiers ever since it got
> introduced, but fails for the user space check on them with hugetlbfs.
> 
> So in order to get that one working, implement mmu notifiers in a
> reasonably dumb fashion and be happy. On embedded hardware, we almost
> never end up with mmu notifier calls, since most people don't overcommit.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/include/asm/kvm_host.h |    3 +-
>  arch/powerpc/include/asm/kvm_ppc.h  |    1 +
>  arch/powerpc/kvm/Kconfig            |    2 +
>  arch/powerpc/kvm/booke.c            |    6 +++
>  arch/powerpc/kvm/e500_tlb.c         |   60 +++++++++++++++++++++++++++++++---
>  5 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index a29e091..ff8d51c 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -45,7 +45,8 @@
>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
>  #endif
>  
> -#ifdef CONFIG_KVM_BOOK3S_64_HV
> +#if defined(CONFIG_KVM_BOOK3S_64_HV) || defined(CONFIG_KVM_E500V2) || \
> +    defined(CONFIG_KVM_E500MC)
>  #include <linux/mmu_notifier.h>
>  
>  #define KVM_ARCH_WANT_MMU_NOTIFIER
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 0124937..c38e824 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -104,6 +104,7 @@ extern void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
>                                         struct kvm_interrupt *irq);
>  extern void kvmppc_core_dequeue_external(struct kvm_vcpu *vcpu,
>                                           struct kvm_interrupt *irq);
> +extern void kvmppc_core_flush_tlb(struct kvm_vcpu *vcpu);
>  
>  extern int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>                                    unsigned int op, int *advance);
> diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
> index f4dacb9..40cad8c 100644
> --- a/arch/powerpc/kvm/Kconfig
> +++ b/arch/powerpc/kvm/Kconfig
> @@ -123,6 +123,7 @@ config KVM_E500V2
>  	depends on EXPERIMENTAL && E500 && !PPC_E500MC
>  	select KVM
>  	select KVM_MMIO
> +	select MMU_NOTIFIER
>  	---help---
>  	  Support running unmodified E500 guest kernels in virtual machines on
>  	  E500v2 host processors.
> @@ -138,6 +139,7 @@ config KVM_E500MC
>  	select KVM
>  	select KVM_MMIO
>  	select KVM_BOOKE_HV
> +	select MMU_NOTIFIER
>  	---help---
>  	  Support running unmodified E500MC/E5500 (32-bit) guest kernels in
>  	  virtual machines on E500MC/E5500 host processors.
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 70a86c0..52f6cbb 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -459,6 +459,10 @@ static void kvmppc_check_requests(struct kvm_vcpu *vcpu)
>  	if (vcpu->requests) {
>  		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu))
>  			update_timer_ints(vcpu);
> +#if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
> +		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
> +			kvmppc_core_flush_tlb(vcpu);
> +#endif
>  	}
>  }

Can we define a new symbol that means "e500_tlb.c is used"?  Or just say
that all new TLB implementations shall support MMU notifiers, and change
this to ifndef 4xx.  Or make this a tlb flush callback with a no-op stub
in 4xx.

Of course this isn't critical and shouldn't hold up the pull request
since I got around to the review late -- just something to think about
for further refactoring.

> @@ -579,6 +583,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>  #endif
>  
>  	kvm_guest_exit();
> +	vcpu->mode = OUTSIDE_GUEST_MODE;
> +	smp_wmb();
>  
>  out:
>  	vcpu->mode = OUTSIDE_GUEST_MODE;

This looks wrong.

> diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c
> index 93f3b92..06273a7 100644
> --- a/arch/powerpc/kvm/e500_tlb.c
> +++ b/arch/powerpc/kvm/e500_tlb.c
> @@ -303,18 +303,15 @@ static inline void kvmppc_e500_ref_setup(struct tlbe_ref *ref,
>  	ref->pfn = pfn;
>  	ref->flags = E500_TLB_VALID;
>  
> -	if (tlbe_is_writable(gtlbe))
> +	if (tlbe_is_writable(gtlbe)) {
>  		ref->flags |= E500_TLB_DIRTY;
> +		kvm_set_pfn_dirty(pfn);
> +	}
>  }

Is there any reason to keep E500_TLB_DIRTY around?  You seem to be
removing the only code that checks it.

> @@ -357,6 +354,13 @@ static void clear_tlb_refs(struct kvmppc_vcpu_e500 *vcpu_e500)
>  	clear_tlb_privs(vcpu_e500);
>  }
>  
> +void kvmppc_core_flush_tlb(struct kvm_vcpu *vcpu)
> +{
> +	struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
> +	clear_tlb_refs(vcpu_e500);
> +	clear_tlb1_bitmap(vcpu_e500);
> +}

Should we move clear_tlb1_bitmap() into clear_tlb_refs()?  That is, is
it a bug that we don't do it in ioctl_dirty_tlb()?

> +/************* MMU Notifiers *************/
> +

Is this really necessary?

> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
> +{
> +	/*
> +	 * Flush all shadow tlb entries everywhere. This is slow, but
> +	 * we are 100% sure that we catch the to be unmapped page
> +	 */
> +	kvm_flush_remote_tlbs(kvm);
> +
> +	return 0;
> +}
> +
> +int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end)
> +{
> +	/* kvm_unmap_hva flushes everything anyways */
> +	kvm_unmap_hva(kvm, start);
> +
> +	return 0;
> +}

I'd feel better about this calling kvm_flush_remote_tlbs() directly
rather than hoping that someone who enhances kvm_unmap_hva() updates
this function as well.

-Scott