[PATCH RFC v2 1/3] KVM: Guard mmu_notifier specific code with CONFIG_MMU_NOTIFIER

[PATCH RFC v2 1/3] KVM: Guard mmu_notifier specific code with CONFIG_MMU_NOTIFIER

[Marc Zyngier]

In order to avoid compilation failure when KVM is not compiled in,
guard the mmu_notifier specific sections with both CONFIG_MMU_NOTIFIER
and KVM_ARCH_WANT_MMU_NOTIFIER, like it is being done in the rest of
the KVM code.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/kvm_host.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 900c763..a596b47 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -287,7 +287,7 @@ struct kvm {
 	struct hlist_head irq_ack_notifier_list;
 #endif
 
-#ifdef KVM_ARCH_WANT_MMU_NOTIFIER
+#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
 	struct mmu_notifier mmu_notifier;
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;
@@ -695,7 +695,7 @@ struct kvm_stats_debugfs_item {
 extern struct kvm_stats_debugfs_item debugfs_entries[];
 extern struct dentry *kvm_debugfs_dir;
 
-#ifdef KVM_ARCH_WANT_MMU_NOTIFIER
+#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
 static inline int mmu_notifier_retry(struct kvm_vcpu *vcpu, unsigned long mmu_seq)
 {
 	if (unlikely(vcpu->kvm->mmu_notifier_count))
-- 
1.7.3.4

[PATCH RFC v2 2/3] ARM: KVM: mark the end of the HYP mode code with __kvm_hyp_code_end

[Marc Zyngier]

Use __kvm_hyp_code_end to mark the end of the main HYP code instead of
__kvm_vcpu_run_end. It's a bit cleaner as we're about to add more code
to that section.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_asm.h |    3 ++-
 arch/arm/kvm/arm.c             |    4 ++--
 arch/arm/kvm/interrupts.S      |    8 +++++---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 89c318ea..5ee7bd3 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -45,7 +45,8 @@ extern char __kvm_hyp_vector[];
 extern char __kvm_hyp_vector_end[];
 
 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
-extern char __kvm_vcpu_run_end[];
+
+extern char __kvm_hyp_code_end[];
 #endif
 
 #endif /* __ARM_KVM_ASM_H__ */
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 14ccc4d..602e087 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -636,9 +636,9 @@ static int init_hyp_mode(void)
 	 * Map the world-switch code
 	 */
 	err = create_hyp_mappings(kvm_hyp_pgd,
-				  __kvm_vcpu_run, __kvm_vcpu_run_end);
+				  __kvm_vcpu_run, __kvm_hyp_code_end);
 	if (err) {
-		kvm_err(err, "Cannot map world-switch code");
+		kvm_err(err, "Cannot map hyp mode code");
 		goto out_free_mappings;
 	}
 
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index fbc26ca..8b7e5e9 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -351,11 +351,13 @@ return_to_ioctl:
 THUMB(	orr	lr, lr, #1)
 	mov	pc, lr
 
-	.ltorg
 
-__kvm_vcpu_run_end:
-	.globl __kvm_vcpu_run_end
+	
+	.ltorg
 
+__kvm_hyp_code_end:
+	.globl	__kvm_hyp_code_end
+	
 
 @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
 @  Hypervisor exception vector and handlers
-- 
1.7.3.4

[PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Marc Zyngier]

Add the necessary infrastructure to handle MMU notifiers on KVM/ARM.
As we don't have shadow page tables, the implementation is actually very
simple. The only supported operation is kvm_unmap_hva(), where we remove
the HVA from the 2nd stage translation. All other hooks are NOPs.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
The aging ops are left unused for the moment, until I actually understand what
they are used for and whether they apply to the ARM architecture.

>From v1:
- Fixed the brown paper bug of invalidating the hva instead of the ipa

 arch/arm/include/asm/kvm_asm.h  |    2 +
 arch/arm/include/asm/kvm_host.h |   19 ++++++++++++++++
 arch/arm/kvm/Kconfig            |    1 +
 arch/arm/kvm/interrupts.S       |   18 ++++++++++++++++
 arch/arm/kvm/mmu.c              |   44 ++++++++++++++++++++++++++++++++++++--
 5 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 5ee7bd3..18be9bb 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -36,6 +36,7 @@ asm(".equ SMCHYP_HVBAR_W, 0xfffffff0");
 #endif /* __ASSEMBLY__ */
 
 #ifndef __ASSEMBLY__
+struct kvm;
 struct kvm_vcpu;
 
 extern char __kvm_hyp_init[];
@@ -46,6 +47,7 @@ extern char __kvm_hyp_vector_end[];
 
 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
+extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
 extern char __kvm_hyp_code_end[];
 #endif
 
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 555a6f1..1c0c68b 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -109,4 +109,23 @@ struct kvm_vm_stat {
 struct kvm_vcpu_stat {
 };
 
+#define KVM_ARCH_WANT_MMU_NOTIFIER
+struct kvm;
+int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
+
+/* We do not have shadow page tables, hence the empty hooks */
+static inline int kvm_age_hva(struct kvm *kvm, unsigned long hva)
+{
+	return 0;
+}
+
+static inline int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
+{
+	return 0;
+}
+
+static inline void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
+{
+}
+
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index ccabbb3..7ce9173 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -36,6 +36,7 @@ config KVM_ARM_HOST
 	depends on KVM
 	depends on MMU
 	depends on CPU_V7 || ARM_VIRT_EXT
+	select	MMU_NOTIFIER
 	---help---
 	  Provides host support for ARM processors.
 
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 8b7e5e9..8822fb3 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -351,7 +351,25 @@ return_to_ioctl:
 THUMB(	orr	lr, lr, #1)
 	mov	pc, lr
 
+ENTRY(__kvm_tlb_flush_vmid)
+	hvc	#0			@ Switch to Hyp mode
+	push	{r2, r3}
 
+	ldrd	r2, r3, [r0, #KVM_VTTBR]
+	mcrr	p15, 6, r2, r3, c2	@ Write VTTBR
+	isb
+	mcr     p15, 0, r0, c8, c7, 0	@ TBLIALL
+	dsb
+	isb
+	mov	r2, #0
+	mov	r3, #0
+	mcrr	p15, 6, r2, r3, c2	@ Back to VMID #0
+	isb
+
+	pop	{r2, r3}
+	hvc	#0			@ Back to SVC
+	mov	pc, lr
+ENDPROC(__kvm_tlb_flush_vmid)
 	
 	.ltorg
 
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index baeb8a1..3f8d83b 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -245,12 +245,12 @@ void kvm_free_stage2_pgd(struct kvm *kvm)
 	kvm->arch.pgd = NULL;
 }
 
-static int __user_mem_abort(struct kvm *kvm, phys_addr_t addr, pfn_t pfn)
+static int stage2_set_pte(struct kvm *kvm, phys_addr_t addr, pte_t new_pte)
 {
 	pgd_t *pgd;
 	pud_t *pud;
 	pmd_t *pmd;
-	pte_t *pte, new_pte;
+	pte_t *pte;
 
 	/* Create 2nd stage page table mapping - Level 1 */
 	pgd = kvm->arch.pgd + pgd_index(addr);
@@ -279,12 +279,18 @@ static int __user_mem_abort(struct kvm *kvm, phys_addr_t addr, pfn_t pfn)
 		pte = pte_offset_kernel(pmd, addr);
 
 	/* Create 2nd stage page table mapping - Level 3 */
-	new_pte = pfn_pte(pfn, PAGE_KVM_GUEST);
 	set_pte_ext(pte, new_pte, 0);
 
 	return 0;
 }
 
+static int __user_mem_abort(struct kvm *kvm, phys_addr_t addr, pfn_t pfn)
+{
+	pte_t new_pte = pfn_pte(pfn, PAGE_KVM_GUEST);
+
+	return stage2_set_pte(kvm, addr, new_pte);
+}
+
 static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			  gfn_t gfn, struct kvm_memory_slot *memslot)
 {
@@ -510,3 +516,35 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 	return user_mem_abort(vcpu, fault_ipa, gfn, memslot);
 }
+
+int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
+{
+	static const pte_t null_pte;
+	struct kvm_memslots *slots;
+	struct kvm_memory_slot *memslot;
+	int needs_stage2_flush = 0;
+
+	slots = kvm_memslots(kvm);
+
+	/* we only care about the pages that the guest sees */
+	kvm_for_each_memslot(memslot, slots) {
+		unsigned long start = memslot->userspace_addr;
+		unsigned long end;
+
+		end = start + (memslot->npages << PAGE_SHIFT);
+		if (hva >= start && hva < end) {
+			gpa_t gpa_offset = hva - start;
+			gpa_t gpa = (memslot->base_gfn << PAGE_SHIFT) + gpa_offset;
+
+			if (stage2_set_pte(kvm, gpa, null_pte))
+				continue; /* Something bad happened, try to carry on */
+
+			needs_stage2_flush = 1;
+		}
+	}
+
+	if (needs_stage2_flush)
+		__kvm_tlb_flush_vmid(kvm);
+
+	return 0;
+}
-- 
1.7.3.4

Re: [PATCH RFC v2 2/3] ARM: KVM: mark the end of the HYP mode code with __kvm_hyp_code_end

[Christoffer Dall]

On Fri, Feb 10, 2012 at 2:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Use __kvm_hyp_code_end to mark the end of the main HYP code instead of
> __kvm_vcpu_run_end. It's a bit cleaner as we're about to add more code
> to that section.

this is good, but should we not rename the beginning of the section as
well (something like __kvm_hyp_code_start)???

Then we can include your new snippet and also the
__kvm_flush_vm_context in that section and perform a single mapping in
the init code - much nicer.

>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_asm.h |    3 ++-
>  arch/arm/kvm/arm.c             |    4 ++--
>  arch/arm/kvm/interrupts.S      |    8 +++++---
>  3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 89c318ea..5ee7bd3 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -45,7 +45,8 @@ extern char __kvm_hyp_vector[];
>  extern char __kvm_hyp_vector_end[];
>
>  extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
> -extern char __kvm_vcpu_run_end[];
> +
> +extern char __kvm_hyp_code_end[];
>  #endif
>
>  #endif /* __ARM_KVM_ASM_H__ */
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 14ccc4d..602e087 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -636,9 +636,9 @@ static int init_hyp_mode(void)
>         * Map the world-switch code
>         */
>        err = create_hyp_mappings(kvm_hyp_pgd,
> -                                 __kvm_vcpu_run, __kvm_vcpu_run_end);
> +                                 __kvm_vcpu_run, __kvm_hyp_code_end);
>        if (err) {
> -               kvm_err(err, "Cannot map world-switch code");
> +               kvm_err(err, "Cannot map hyp mode code");
>                goto out_free_mappings;
>        }
>
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index fbc26ca..8b7e5e9 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -351,11 +351,13 @@ return_to_ioctl:
>  THUMB( orr     lr, lr, #1)
>        mov     pc, lr
>
> -       .ltorg
>
> -__kvm_vcpu_run_end:
> -       .globl __kvm_vcpu_run_end
> +
> +       .ltorg
>
> +__kvm_hyp_code_end:
> +       .globl  __kvm_hyp_code_end
> +
>
>  @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>  @  Hypervisor exception vector and handlers
> --
> 1.7.3.4
>

Re: [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Christoffer Dall]

On Fri, Feb 10, 2012 at 2:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Add the necessary infrastructure to handle MMU notifiers on KVM/ARM.
> As we don't have shadow page tables, the implementation is actually very
> simple. The only supported operation is kvm_unmap_hva(), where we remove
> the HVA from the 2nd stage translation. All other hooks are NOPs.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> The aging ops are left unused for the moment, until I actually understand what
> they are used for and whether they apply to the ARM architecture.
>
> From v1:
> - Fixed the brown paper bug of invalidating the hva instead of the ipa
>
>  arch/arm/include/asm/kvm_asm.h  |    2 +
>  arch/arm/include/asm/kvm_host.h |   19 ++++++++++++++++
>  arch/arm/kvm/Kconfig            |    1 +
>  arch/arm/kvm/interrupts.S       |   18 ++++++++++++++++
>  arch/arm/kvm/mmu.c              |   44 ++++++++++++++++++++++++++++++++++++--
>  5 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 5ee7bd3..18be9bb 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -36,6 +36,7 @@ asm(".equ SMCHYP_HVBAR_W, 0xfffffff0");
>  #endif /* __ASSEMBLY__ */
>
>  #ifndef __ASSEMBLY__
> +struct kvm;
>  struct kvm_vcpu;
>
>  extern char __kvm_hyp_init[];
> @@ -46,6 +47,7 @@ extern char __kvm_hyp_vector_end[];
>
>  extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>
> +extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
>  extern char __kvm_hyp_code_end[];
>  #endif
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 555a6f1..1c0c68b 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -109,4 +109,23 @@ struct kvm_vm_stat {
>  struct kvm_vcpu_stat {
>  };
>
> +#define KVM_ARCH_WANT_MMU_NOTIFIER
> +struct kvm;
> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
> +
> +/* We do not have shadow page tables, hence the empty hooks */
> +static inline int kvm_age_hva(struct kvm *kvm, unsigned long hva)
> +{
> +       return 0;
> +}
> +
> +static inline int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
> +{
> +       return 0;
> +}
> +
> +static inline void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
> +{
> +}
> +
>  #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
> index ccabbb3..7ce9173 100644
> --- a/arch/arm/kvm/Kconfig
> +++ b/arch/arm/kvm/Kconfig
> @@ -36,6 +36,7 @@ config KVM_ARM_HOST
>        depends on KVM
>        depends on MMU
>        depends on CPU_V7 || ARM_VIRT_EXT
> +       select  MMU_NOTIFIER
>        ---help---
>          Provides host support for ARM processors.
>
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 8b7e5e9..8822fb3 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -351,7 +351,25 @@ return_to_ioctl:
>  THUMB( orr     lr, lr, #1)
>        mov     pc, lr
>

I would prefer moving this to the top of the file before all the
macros so the coherency between the world-switch to the guest and the
return path is clearer (see the v6 staging branch where I already
added a function).

What I want to avoid is this

__switch:
   do some switch stuff
__return:
   do some return stuff

cache_fun_1:
   foo

cache_fun_2:
   foo

cache_fun_3:
   foo

cache_fun_4:
   foo

vector:
   b __return


> +ENTRY(__kvm_tlb_flush_vmid)
> +       hvc     #0                      @ Switch to Hyp mode
> +       push    {r2, r3}
>
> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
> +       isb
> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
> +       dsb
> +       isb
> +       mov     r2, #0
> +       mov     r3, #0
> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
> +       isb
> +
> +       pop     {r2, r3}
> +       hvc     #0                      @ Back to SVC
> +       mov     pc, lr
> +ENDPROC(__kvm_tlb_flush_vmid)
>
>        .ltorg
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index baeb8a1..3f8d83b 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -245,12 +245,12 @@ void kvm_free_stage2_pgd(struct kvm *kvm)
>        kvm->arch.pgd = NULL;
>  }
>
> -static int __user_mem_abort(struct kvm *kvm, phys_addr_t addr, pfn_t pfn)
> +static int stage2_set_pte(struct kvm *kvm, phys_addr_t addr, pte_t new_pte)
>  {
>        pgd_t *pgd;
>        pud_t *pud;
>        pmd_t *pmd;
> -       pte_t *pte, new_pte;
> +       pte_t *pte;
>
>        /* Create 2nd stage page table mapping - Level 1 */
>        pgd = kvm->arch.pgd + pgd_index(addr);
> @@ -279,12 +279,18 @@ static int __user_mem_abort(struct kvm *kvm, phys_addr_t addr, pfn_t pfn)
>                pte = pte_offset_kernel(pmd, addr);
>
>        /* Create 2nd stage page table mapping - Level 3 */
> -       new_pte = pfn_pte(pfn, PAGE_KVM_GUEST);
>        set_pte_ext(pte, new_pte, 0);
>
>        return 0;
>  }
>
> +static int __user_mem_abort(struct kvm *kvm, phys_addr_t addr, pfn_t pfn)
> +{
> +       pte_t new_pte = pfn_pte(pfn, PAGE_KVM_GUEST);
> +
> +       return stage2_set_pte(kvm, addr, new_pte);
> +}
> +
>  static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>                          gfn_t gfn, struct kvm_memory_slot *memslot)
>  {
> @@ -510,3 +516,35 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>
>        return user_mem_abort(vcpu, fault_ipa, gfn, memslot);
>  }
> +
> +int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
> +{
> +       static const pte_t null_pte;
> +       struct kvm_memslots *slots;
> +       struct kvm_memory_slot *memslot;
> +       int needs_stage2_flush = 0;
> +
> +       slots = kvm_memslots(kvm);
> +
> +       /* we only care about the pages that the guest sees */
> +       kvm_for_each_memslot(memslot, slots) {

what a pain that you have to do this.

should we not create an arch-independent hva_to_gfn function, that
might return a bad_gfn if that hva is not in a memslot?

that would simplify this code quite a bit...

> +               unsigned long start = memslot->userspace_addr;
> +               unsigned long end;
> +
> +               end = start + (memslot->npages << PAGE_SHIFT);
> +               if (hva >= start && hva < end) {
> +                       gpa_t gpa_offset = hva - start;
> +                       gpa_t gpa = (memslot->base_gfn << PAGE_SHIFT) + gpa_offset;
> +
> +                       if (stage2_set_pte(kvm, gpa, null_pte))
> +                               continue; /* Something bad happened, try to carry on */

the something bad that happened here was that we're out of memory to
create page tables, which really shouldn't happen, because even if
QEMU accessed some of these pages without regard for KVM and these
pages are being evicted, we should certainly not start allocing page
tables to put a blank pte in there...

And, since we're here because of memory pressure in the first place,
continuing to hammer on the system is probably a bad idea...

consider checking for null_pte in stage_set_pte and an error should
not happen here, and if it does, it's more like a BUG();

> +
> +                       needs_stage2_flush = 1;
> +               }
> +       }
> +
> +       if (needs_stage2_flush)
> +               __kvm_tlb_flush_vmid(kvm);
> +
> +       return 0;
> +}
> --
> 1.7.3.4
>

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Antonios Motakis]

On 02/10/2012 11:22 PM, Marc Zyngier wrote:
> +ENTRY(__kvm_tlb_flush_vmid)
> +	hvc	#0			@ Switch to Hyp mode
> +	push	{r2, r3}
>
> +	ldrd	r2, r3, [r0, #KVM_VTTBR]
> +	mcrr	p15, 6, r2, r3, c2	@ Write VTTBR
> +	isb
> +	mcr     p15, 0, r0, c8, c7, 0	@ TBLIALL
> +	dsb
> +	isb
> +	mov	r2, #0
> +	mov	r3, #0
> +	mcrr	p15, 6, r2, r3, c2	@ Back to VMID #0
> +	isb
> +
> +	pop	{r2, r3}
> +	hvc	#0			@ Back to SVC
> +	mov	pc, lr
> +ENDPROC(__kvm_tlb_flush_vmid)

With the last VMID implementation, you could get the equivalent effect 
of a per-VMID flush, by just getting a new VMID for the current VM. So 
you could do a (kvm->arch.vmid = 0) to force a new VMID when the guest 
reruns, and save the overhead of that flush (you will do a complete 
flush every 255 times instead of a small one every single time).

Best regards,
Antonios

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Christoffer Dall]

On Sat, Feb 11, 2012 at 7:00 AM, Antonios Motakis
<a.motakis@virtualopensystems.com> wrote:
> On 02/10/2012 11:22 PM, Marc Zyngier wrote:
>>
>> +ENTRY(__kvm_tlb_flush_vmid)
>> +       hvc     #0                      @ Switch to Hyp mode
>> +       push    {r2, r3}
>>
>> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
>> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
>> +       isb
>> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
>> +       dsb
>> +       isb
>> +       mov     r2, #0
>> +       mov     r3, #0
>> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
>> +       isb
>> +
>> +       pop     {r2, r3}
>> +       hvc     #0                      @ Back to SVC
>> +       mov     pc, lr
>> +ENDPROC(__kvm_tlb_flush_vmid)
>
>
> With the last VMID implementation, you could get the equivalent effect of a
> per-VMID flush, by just getting a new VMID for the current VM. So you could
> do a (kvm->arch.vmid = 0) to force a new VMID when the guest reruns, and
> save the overhead of that flush (you will do a complete flush every 255
> times instead of a small one every single time).
>

to do this you would need to send an IPI if the guest is currently
executing on another CPU and make it exit the guest, so that the VMID
assignment will run before the guest potentially accesses that TLB
entry that points to the page that was just reclaimed - which I am not
sure will be better than this solution.

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Antonios Motakis]

On 02/11/2012 06:35 PM, Christoffer Dall wrote:
> On Sat, Feb 11, 2012 at 7:00 AM, Antonios Motakis
> <a.motakis@virtualopensystems.com>  wrote:
>> On 02/10/2012 11:22 PM, Marc Zyngier wrote:
>>> +ENTRY(__kvm_tlb_flush_vmid)
>>> +       hvc     #0                      @ Switch to Hyp mode
>>> +       push    {r2, r3}
>>>
>>> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
>>> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
>>> +       isb
>>> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
>>> +       dsb
>>> +       isb
>>> +       mov     r2, #0
>>> +       mov     r3, #0
>>> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
>>> +       isb
>>> +
>>> +       pop     {r2, r3}
>>> +       hvc     #0                      @ Back to SVC
>>> +       mov     pc, lr
>>> +ENDPROC(__kvm_tlb_flush_vmid)
>>
>> With the last VMID implementation, you could get the equivalent effect of a
>> per-VMID flush, by just getting a new VMID for the current VM. So you could
>> do a (kvm->arch.vmid = 0) to force a new VMID when the guest reruns, and
>> save the overhead of that flush (you will do a complete flush every 255
>> times instead of a small one every single time).
>>
> to do this you would need to send an IPI if the guest is currently
> executing on another CPU and make it exit the guest, so that the VMID
> assignment will run before the guest potentially accesses that TLB
> entry that points to the page that was just reclaimed - which I am not
> sure will be better than this solution.
Don't you have to do this anyway? You'd want the flush to be effective 
on all CPUs before proceeding.

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Christoffer Dall]

On Sat, Feb 11, 2012 at 10:33 AM, Antonios Motakis
<a.motakis@virtualopensystems.com> wrote:
> On 02/11/2012 06:35 PM, Christoffer Dall wrote:
>>
>> On Sat, Feb 11, 2012 at 7:00 AM, Antonios Motakis
>> <a.motakis@virtualopensystems.com>  wrote:
>>>
>>> On 02/10/2012 11:22 PM, Marc Zyngier wrote:
>>>>
>>>> +ENTRY(__kvm_tlb_flush_vmid)
>>>> +       hvc     #0                      @ Switch to Hyp mode
>>>> +       push    {r2, r3}
>>>>
>>>> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
>>>> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
>>>> +       isb
>>>> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
>>>> +       dsb
>>>> +       isb
>>>> +       mov     r2, #0
>>>> +       mov     r3, #0
>>>> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
>>>> +       isb
>>>> +
>>>> +       pop     {r2, r3}
>>>> +       hvc     #0                      @ Back to SVC
>>>> +       mov     pc, lr
>>>> +ENDPROC(__kvm_tlb_flush_vmid)
>>>
>>>
>>> With the last VMID implementation, you could get the equivalent effect of
>>> a
>>> per-VMID flush, by just getting a new VMID for the current VM. So you
>>> could
>>> do a (kvm->arch.vmid = 0) to force a new VMID when the guest reruns, and
>>> save the overhead of that flush (you will do a complete flush every 255
>>> times instead of a small one every single time).
>>>
>> to do this you would need to send an IPI if the guest is currently
>> executing on another CPU and make it exit the guest, so that the VMID
>> assignment will run before the guest potentially accesses that TLB
>> entry that points to the page that was just reclaimed - which I am not
>> sure will be better than this solution.
>
> Don't you have to do this anyway? You'd want the flush to be effective on
> all CPUs before proceeding.

hmm yeah, actually you do need this. Unless the -IS version of the
flush instruction covers all relevant cores in this case. Marc, I
don't think that the processor clearing out the page table entry will
necessarily belong to the same inner-shareable domain as the processor
potentially executing the VM, so therefore the -IS flushing version
would not be sufficient and we actually have to go and send an IPI.

So, it sounds to me like:
 1) we have to signal all vcpus using the VMID for which we are
clearing page table entries
 2) make sure that they, either
    2a) flush their TLBs
    2b) get a new VMID

seems like 2b might be slightly faster, but leaves more entries in the
TLB that are then unused - not sure if that's a bad thing considering
the replacement policy. Perhaps 2a is cleaner...

Thoughts anyone?

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Alexander Graf]

On 12.02.2012, at 02:12, Christoffer Dall <c.dall@virtualopensystems.com> wrote:

> On Sat, Feb 11, 2012 at 10:33 AM, Antonios Motakis
> <a.motakis@virtualopensystems.com> wrote:
>> On 02/11/2012 06:35 PM, Christoffer Dall wrote:
>>> 
>>> On Sat, Feb 11, 2012 at 7:00 AM, Antonios Motakis
>>> <a.motakis@virtualopensystems.com>  wrote:
>>>> 
>>>> On 02/10/2012 11:22 PM, Marc Zyngier wrote:
>>>>> 
>>>>> +ENTRY(__kvm_tlb_flush_vmid)
>>>>> +       hvc     #0                      @ Switch to Hyp mode
>>>>> +       push    {r2, r3}
>>>>> 
>>>>> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
>>>>> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
>>>>> +       isb
>>>>> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
>>>>> +       dsb
>>>>> +       isb
>>>>> +       mov     r2, #0
>>>>> +       mov     r3, #0
>>>>> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
>>>>> +       isb
>>>>> +
>>>>> +       pop     {r2, r3}
>>>>> +       hvc     #0                      @ Back to SVC
>>>>> +       mov     pc, lr
>>>>> +ENDPROC(__kvm_tlb_flush_vmid)
>>>> 
>>>> 
>>>> With the last VMID implementation, you could get the equivalent effect of
>>>> a
>>>> per-VMID flush, by just getting a new VMID for the current VM. So you
>>>> could
>>>> do a (kvm->arch.vmid = 0) to force a new VMID when the guest reruns, and
>>>> save the overhead of that flush (you will do a complete flush every 255
>>>> times instead of a small one every single time).
>>>> 
>>> to do this you would need to send an IPI if the guest is currently
>>> executing on another CPU and make it exit the guest, so that the VMID
>>> assignment will run before the guest potentially accesses that TLB
>>> entry that points to the page that was just reclaimed - which I am not
>>> sure will be better than this solution.
>> 
>> Don't you have to do this anyway? You'd want the flush to be effective on
>> all CPUs before proceeding.
> 
> hmm yeah, actually you do need this. Unless the -IS version of the
> flush instruction covers all relevant cores in this case. Marc, I
> don't think that the processor clearing out the page table entry will
> necessarily belong to the same inner-shareable domain as the processor
> potentially executing the VM, so therefore the -IS flushing version
> would not be sufficient and we actually have to go and send an IPI.
> 
> So, it sounds to me like:
> 1) we have to signal all vcpus using the VMID for which we are
> clearing page table entries
> 2) make sure that they, either
>    2a) flush their TLBs
>    2b) get a new VMID
> 
> seems like 2b might be slightly faster, but leaves more entries in the
> TLB that are then unused - not sure if that's a bad thing considering
> the replacement policy. Perhaps 2a is cleaner...

X86 basically does 2b, but has per-cpu tlb tags.

On PPC, we statically map the guest id to a guest atm.


Alex

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Marc Zyngier]

On 12/02/12 01:12, Christoffer Dall wrote:
> On Sat, Feb 11, 2012 at 10:33 AM, Antonios Motakis
> <a.motakis@virtualopensystems.com> wrote:
>> On 02/11/2012 06:35 PM, Christoffer Dall wrote:
>>>
>>> On Sat, Feb 11, 2012 at 7:00 AM, Antonios Motakis
>>> <a.motakis@virtualopensystems.com>  wrote:
>>>>
>>>> On 02/10/2012 11:22 PM, Marc Zyngier wrote:
>>>>>
>>>>> +ENTRY(__kvm_tlb_flush_vmid)
>>>>> +       hvc     #0                      @ Switch to Hyp mode
>>>>> +       push    {r2, r3}
>>>>>
>>>>> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
>>>>> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
>>>>> +       isb
>>>>> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
>>>>> +       dsb
>>>>> +       isb
>>>>> +       mov     r2, #0
>>>>> +       mov     r3, #0
>>>>> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
>>>>> +       isb
>>>>> +
>>>>> +       pop     {r2, r3}
>>>>> +       hvc     #0                      @ Back to SVC
>>>>> +       mov     pc, lr
>>>>> +ENDPROC(__kvm_tlb_flush_vmid)
>>>>
>>>>
>>>> With the last VMID implementation, you could get the equivalent effect of
>>>> a
>>>> per-VMID flush, by just getting a new VMID for the current VM. So you
>>>> could
>>>> do a (kvm->arch.vmid = 0) to force a new VMID when the guest reruns, and
>>>> save the overhead of that flush (you will do a complete flush every 255
>>>> times instead of a small one every single time).
>>>>
>>> to do this you would need to send an IPI if the guest is currently
>>> executing on another CPU and make it exit the guest, so that the VMID
>>> assignment will run before the guest potentially accesses that TLB
>>> entry that points to the page that was just reclaimed - which I am not
>>> sure will be better than this solution.
>>
>> Don't you have to do this anyway? You'd want the flush to be effective on
>> all CPUs before proceeding.
> 
> hmm yeah, actually you do need this. Unless the -IS version of the
> flush instruction covers all relevant cores in this case. Marc, I
> don't think that the processor clearing out the page table entry will
> necessarily belong to the same inner-shareable domain as the processor
> potentially executing the VM, so therefore the -IS flushing version
> would not be sufficient and we actually have to go and send an IPI.

If we forget about the 11MPCore (which doesn't broadcast the TLB
invalidation in hardware), the TLBIALLIS operation makes sure all cores
belonging to the same inner shareable domain will see the TLB
invalidation at the same time. If they don't, this is a hardware bug.

Now, I do not have an example of a system where two CPUs are not part of
the same IS domain. Even big.LITTLE has all of the potential 8 cores in
an IS domain. If such a system exists one of these days, then it will be
worth considering having a separate method to cope with the case. Until
then, my opinion is to keep it as simple as possible.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [Android-virt] [PATCH RFC v2 3/3] ARM: KVM: Add support for MMU notifiers

[Christoffer Dall]

On Mon, Feb 13, 2012 at 5:13 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 12/02/12 01:12, Christoffer Dall wrote:
>> On Sat, Feb 11, 2012 at 10:33 AM, Antonios Motakis
>> <a.motakis@virtualopensystems.com> wrote:
>>> On 02/11/2012 06:35 PM, Christoffer Dall wrote:
>>>>
>>>> On Sat, Feb 11, 2012 at 7:00 AM, Antonios Motakis
>>>> <a.motakis@virtualopensystems.com>  wrote:
>>>>>
>>>>> On 02/10/2012 11:22 PM, Marc Zyngier wrote:
>>>>>>
>>>>>> +ENTRY(__kvm_tlb_flush_vmid)
>>>>>> +       hvc     #0                      @ Switch to Hyp mode
>>>>>> +       push    {r2, r3}
>>>>>>
>>>>>> +       ldrd    r2, r3, [r0, #KVM_VTTBR]
>>>>>> +       mcrr    p15, 6, r2, r3, c2      @ Write VTTBR
>>>>>> +       isb
>>>>>> +       mcr     p15, 0, r0, c8, c7, 0   @ TBLIALL
>>>>>> +       dsb
>>>>>> +       isb
>>>>>> +       mov     r2, #0
>>>>>> +       mov     r3, #0
>>>>>> +       mcrr    p15, 6, r2, r3, c2      @ Back to VMID #0
>>>>>> +       isb
>>>>>> +
>>>>>> +       pop     {r2, r3}
>>>>>> +       hvc     #0                      @ Back to SVC
>>>>>> +       mov     pc, lr
>>>>>> +ENDPROC(__kvm_tlb_flush_vmid)
>>>>>
>>>>>
>>>>> With the last VMID implementation, you could get the equivalent effect of
>>>>> a
>>>>> per-VMID flush, by just getting a new VMID for the current VM. So you
>>>>> could
>>>>> do a (kvm->arch.vmid = 0) to force a new VMID when the guest reruns, and
>>>>> save the overhead of that flush (you will do a complete flush every 255
>>>>> times instead of a small one every single time).
>>>>>
>>>> to do this you would need to send an IPI if the guest is currently
>>>> executing on another CPU and make it exit the guest, so that the VMID
>>>> assignment will run before the guest potentially accesses that TLB
>>>> entry that points to the page that was just reclaimed - which I am not
>>>> sure will be better than this solution.
>>>
>>> Don't you have to do this anyway? You'd want the flush to be effective on
>>> all CPUs before proceeding.
>>
>> hmm yeah, actually you do need this. Unless the -IS version of the
>> flush instruction covers all relevant cores in this case. Marc, I
>> don't think that the processor clearing out the page table entry will
>> necessarily belong to the same inner-shareable domain as the processor
>> potentially executing the VM, so therefore the -IS flushing version
>> would not be sufficient and we actually have to go and send an IPI.
>
> If we forget about the 11MPCore (which doesn't broadcast the TLB
> invalidation in hardware), the TLBIALLIS operation makes sure all cores
> belonging to the same inner shareable domain will see the TLB
> invalidation at the same time. If they don't, this is a hardware bug.
>
> Now, I do not have an example of a system where two CPUs are not part of
> the same IS domain. Even big.LITTLE has all of the potential 8 cores in
> an IS domain. If such a system exists one of these days, then it will be
> worth considering having a separate method to cope with the case. Until
> then, my opinion is to keep it as simple as possible.
>

ok, sounds good to me. Although, perhaps keep this as a comment somewhere...