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@xxxxxxx>
> ---
> 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
--
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
