[PATCH 0/4] ARM: KVM: random mmu related fixes for 3.10

[PATCH 0/4] ARM: KVM: random mmu related fixes for 3.10

[Marc Zyngier]

This patch series fixes a number of of KVM/ARM issues that have either
been spotted during the review of the arm64 code, or while reworking
related code.

Only the first patch fixes a potential (if unlikely) problem, the
others are either cosmetic or performance related.

Tested on TC-2.

Marc Zyngier (4):
  ARM: KVM: be more thorough when invalidating TLBs
  ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid
  ARM: KVM: relax cache maintainance when building page pables
  ARM: KVM: fix use of S2_PGD_SIZE

 arch/arm/include/asm/kvm_asm.h |  2 --
 arch/arm/include/asm/kvm_mmu.h |  1 +
 arch/arm/kvm/interrupts.S      |  2 ++
 arch/arm/kvm/mmu.c             | 44 ++++++++++++++++++++++++------------------
 4 files changed, 28 insertions(+), 21 deletions(-)

-- 
1.8.2.1

[PATCH 1/4] ARM: KVM: be more thorough when invalidating TLBs

[Marc Zyngier]

The KVM/ARM MMU code doesn't take care of invalidating TLBs before
freeing a {pte,pmd} table. This could cause problems if the page
is reallocated and then speculated into by another CPU.

Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/interrupts.S |  2 ++
 arch/arm/kvm/mmu.c        | 36 +++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index f7793df..9e2d906c 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -37,6 +37,8 @@ __kvm_hyp_code_start:
  *
  * void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
  *
+ * Invalidate TLB for the given IPA, or invalidate all if ipa is zero.
+ *
  * We rely on the hardware to broadcast the TLB invalidation to all CPUs
  * inside the inner-shareable domain (which is the case for all v7
  * implementations).  If we come across a non-IS SMP implementation, we'll
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 9657065..71f2a57 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -43,7 +43,8 @@ static phys_addr_t hyp_idmap_vector;
 
 static void kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
 {
-	kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);
+	if (kvm)
+		kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);
 }
 
 static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
@@ -78,18 +79,20 @@ static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc)
 	return p;
 }
 
-static void clear_pud_entry(pud_t *pud)
+static void clear_pud_entry(struct kvm *kvm, pud_t *pud, phys_addr_t addr)
 {
 	pmd_t *pmd_table = pmd_offset(pud, 0);
 	pud_clear(pud);
+	kvm_tlb_flush_vmid_ipa(kvm, addr);
 	pmd_free(NULL, pmd_table);
 	put_page(virt_to_page(pud));
 }
 
-static void clear_pmd_entry(pmd_t *pmd)
+static void clear_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr)
 {
 	pte_t *pte_table = pte_offset_kernel(pmd, 0);
 	pmd_clear(pmd);
+	kvm_tlb_flush_vmid_ipa(kvm, addr);
 	pte_free_kernel(NULL, pte_table);
 	put_page(virt_to_page(pmd));
 }
@@ -100,11 +103,12 @@ static bool pmd_empty(pmd_t *pmd)
 	return page_count(pmd_page) == 1;
 }
 
-static void clear_pte_entry(pte_t *pte)
+static void clear_pte_entry(struct kvm *kvm, pte_t *pte, phys_addr_t addr)
 {
 	if (pte_present(*pte)) {
 		kvm_set_pte(pte, __pte(0));
 		put_page(virt_to_page(pte));
+		kvm_tlb_flush_vmid_ipa(kvm, addr);
 	}
 }
 
@@ -114,7 +118,8 @@ static bool pte_empty(pte_t *pte)
 	return page_count(pte_page) == 1;
 }
 
-static void unmap_range(pgd_t *pgdp, unsigned long long start, u64 size)
+static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
+			unsigned long long start, u64 size)
 {
 	pgd_t *pgd;
 	pud_t *pud;
@@ -138,15 +143,15 @@ static void unmap_range(pgd_t *pgdp, unsigned long long start, u64 size)
 		}
 
 		pte = pte_offset_kernel(pmd, addr);
-		clear_pte_entry(pte);
+		clear_pte_entry(kvm, pte, addr);
 		range = PAGE_SIZE;
 
 		/* If we emptied the pte, walk back up the ladder */
 		if (pte_empty(pte)) {
-			clear_pmd_entry(pmd);
+			clear_pmd_entry(kvm, pmd, addr);
 			range = PMD_SIZE;
 			if (pmd_empty(pmd)) {
-				clear_pud_entry(pud);
+				clear_pud_entry(kvm, pud, addr);
 				range = PUD_SIZE;
 			}
 		}
@@ -165,14 +170,14 @@ void free_boot_hyp_pgd(void)
 	mutex_lock(&kvm_hyp_pgd_mutex);
 
 	if (boot_hyp_pgd) {
-		unmap_range(boot_hyp_pgd, hyp_idmap_start, PAGE_SIZE);
-		unmap_range(boot_hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
+		unmap_range(NULL, boot_hyp_pgd, hyp_idmap_start, PAGE_SIZE);
+		unmap_range(NULL, boot_hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
 		kfree(boot_hyp_pgd);
 		boot_hyp_pgd = NULL;
 	}
 
 	if (hyp_pgd)
-		unmap_range(hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
+		unmap_range(NULL, hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
 
 	kfree(init_bounce_page);
 	init_bounce_page = NULL;
@@ -200,9 +205,10 @@ void free_hyp_pgds(void)
 
 	if (hyp_pgd) {
 		for (addr = PAGE_OFFSET; virt_addr_valid(addr); addr += PGDIR_SIZE)
-			unmap_range(hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
+			unmap_range(NULL, hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
 		for (addr = VMALLOC_START; is_vmalloc_addr((void*)addr); addr += PGDIR_SIZE)
-			unmap_range(hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
+			unmap_range(NULL, hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
+
 		kfree(hyp_pgd);
 		hyp_pgd = NULL;
 	}
@@ -393,7 +399,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
  */
 static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
 {
-	unmap_range(kvm->arch.pgd, start, size);
+	unmap_range(kvm, kvm->arch.pgd, start, size);
 }
 
 /**
@@ -413,6 +419,7 @@ void kvm_free_stage2_pgd(struct kvm *kvm)
 		return;
 
 	unmap_stage2_range(kvm, 0, KVM_PHYS_SIZE);
+	kvm_tlb_flush_vmid_ipa(kvm, 0);	/* Invalidate TLB ALL */
 	free_pages((unsigned long)kvm->arch.pgd, S2_PGD_ORDER);
 	kvm->arch.pgd = NULL;
 }
@@ -675,7 +682,6 @@ static void handle_hva_to_gpa(struct kvm *kvm,
 static void kvm_unmap_hva_handler(struct kvm *kvm, gpa_t gpa, void *data)
 {
 	unmap_stage2_range(kvm, gpa, PAGE_SIZE);
-	kvm_tlb_flush_vmid_ipa(kvm, gpa);
 }
 
 int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-- 
1.8.2.1

[PATCH 2/4] ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid

[Marc Zyngier]

__kvm_tlb_flush_vmid has been renamed to __kvm_tlb_flush_vmid_ipa,
and the old prototype should have been removed when the code was
modified.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_asm.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 18d5032..6ae3d2a 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -72,8 +72,6 @@ extern char __kvm_hyp_vector[];
 extern char __kvm_hyp_code_start[];
 extern char __kvm_hyp_code_end[];
 
-extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
-
 extern void __kvm_flush_vm_context(void);
 extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
 
-- 
1.8.2.1

[PATCH 3/4] ARM: KVM: relax cache maintainance when building page pables

[Marc Zyngier]

Patch 5a677ce044f1 (ARM: KVM: switch to a dual-step HYP init code)
introduced code that flushes page tables to the point of coherency.
This is overkill (point of unification is enough), and actually
not required if running on a SMP capable platform.

Change this code to use clean_dcache_area(), which is updated by
ae8a8b9553bd (ARM: 7691/1: mm: kill unused TLB_CAN_READ_FROM_L1_CACHE
and use ALT_SMP instead) to behave as a no-op on SMP ARMv7.

Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h | 1 +
 arch/arm/kvm/mmu.c             | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 472ac70..2459821 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -129,6 +129,7 @@ static inline void coherent_icache_guest_page(struct kvm *kvm, gfn_t gfn)
 }
 
 #define kvm_flush_dcache_to_poc(a,l)	__cpuc_flush_dcache_area((a), (l))
+#define kvm_clean_dcache_to_pou(a,l)	clean_dcache_area((a), (l))
 
 #endif	/* !__ASSEMBLY__ */
 
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 71f2a57..b978ebe 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -228,7 +228,7 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start,
 		pte = pte_offset_kernel(pmd, addr);
 		kvm_set_pte(pte, pfn_pte(pfn, prot));
 		get_page(virt_to_page(pte));
-		kvm_flush_dcache_to_poc(pte, sizeof(*pte));
+		kvm_clean_dcache_to_pou(pte, sizeof(*pte));
 		pfn++;
 	} while (addr += PAGE_SIZE, addr != end);
 }
@@ -255,7 +255,7 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
 			}
 			pmd_populate_kernel(NULL, pmd, pte);
 			get_page(virt_to_page(pmd));
-			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
+			kvm_clean_dcache_to_pou(pmd, sizeof(*pmd));
 		}
 
 		next = pmd_addr_end(addr, end);
@@ -293,7 +293,7 @@ static int __create_hyp_mappings(pgd_t *pgdp,
 			}
 			pud_populate(NULL, pud, pmd);
 			get_page(virt_to_page(pud));
-			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
+			kvm_clean_dcache_to_pou(pud, sizeof(*pud));
 		}
 
 		next = pgd_addr_end(addr, end);
-- 
1.8.2.1

[PATCH 4/4] ARM: KVM: fix use of S2_PGD_SIZE

[Marc Zyngier]

S2_PGD_SIZE describe the number of pages a used by a stage-2 PGD,
not the size of the PGD.

Fix the VM_BUG_ON() call that doesn't check much when S2_PGD_SIZE
is equal to 1.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index b978ebe..09ece5c 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -377,7 +377,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
 		return -ENOMEM;
 
 	/* stage-2 pgd must be aligned to its size */
-	VM_BUG_ON((unsigned long)pgd & (S2_PGD_SIZE - 1));
+	VM_BUG_ON((unsigned long)pgd & (PAGE_SIZE * S2_PGD_SIZE - 1));
 
 	memset(pgd, 0, PTRS_PER_S2_PGD * sizeof(pgd_t));
 	kvm_clean_pgd(pgd);
-- 
1.8.2.1

[PATCH 3/4] ARM: KVM: relax cache maintainance when building page pables

[Will Deacon]

On Tue, Apr 30, 2013 at 03:17:25PM +0100, Marc Zyngier wrote:
> Patch 5a677ce044f1 (ARM: KVM: switch to a dual-step HYP init code)
> introduced code that flushes page tables to the point of coherency.
> This is overkill (point of unification is enough), and actually
> not required if running on a SMP capable platform.
> 
> Change this code to use clean_dcache_area(), which is updated by
> ae8a8b9553bd (ARM: 7691/1: mm: kill unused TLB_CAN_READ_FROM_L1_CACHE
> and use ALT_SMP instead) to behave as a no-op on SMP ARMv7.
> 
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_mmu.h | 1 +
>  arch/arm/kvm/mmu.c             | 6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 472ac70..2459821 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -129,6 +129,7 @@ static inline void coherent_icache_guest_page(struct kvm *kvm, gfn_t gfn)
>  }
>  
>  #define kvm_flush_dcache_to_poc(a,l)	__cpuc_flush_dcache_area((a), (l))
> +#define kvm_clean_dcache_to_pou(a,l)	clean_dcache_area((a), (l))

I don't think the name helps, since this sounds like it could be used for
things other than flushing page tables (for example, writing instructions)
but apart from that:

  Reviewed-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 1/4] ARM: KVM: be more thorough when invalidating TLBs

[Christoffer Dall]

On Tue, Apr 30, 2013 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> The KVM/ARM MMU code doesn't take care of invalidating TLBs before
> freeing a {pte,pmd} table. This could cause problems if the page
> is reallocated and then speculated into by another CPU.
>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/interrupts.S |  2 ++
>  arch/arm/kvm/mmu.c        | 36 +++++++++++++++++++++---------------
>  2 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index f7793df..9e2d906c 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -37,6 +37,8 @@ __kvm_hyp_code_start:
>   *
>   * void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
>   *
> + * Invalidate TLB for the given IPA, or invalidate all if ipa is zero.
> + *
>   * We rely on the hardware to broadcast the TLB invalidation to all CPUs
>   * inside the inner-shareable domain (which is the case for all v7
>   * implementations).  If we come across a non-IS SMP implementation, we'll
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 9657065..71f2a57 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -43,7 +43,8 @@ static phys_addr_t hyp_idmap_vector;
>
>  static void kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
>  {
> -       kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);
> +       if (kvm)
> +               kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);

this feels slightly abusive? could we add a comment that hyp page
table freeing don't need tlb flushing from these functions and don't
have a struct kvm?

alternatively it may be more clear to have something like:

if (kvm) /* Check if Hyp or Stage-2 page table */
    kvm_tlb_flush_vmid_ipa(kvm, addr);

in the callers, but, eh, maybe I'm over thinking this.

>  }
>
>  static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> @@ -78,18 +79,20 @@ static void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc)
>         return p;
>  }
>
> -static void clear_pud_entry(pud_t *pud)
> +static void clear_pud_entry(struct kvm *kvm, pud_t *pud, phys_addr_t addr)
>  {
>         pmd_t *pmd_table = pmd_offset(pud, 0);
>         pud_clear(pud);
> +       kvm_tlb_flush_vmid_ipa(kvm, addr);
>         pmd_free(NULL, pmd_table);
>         put_page(virt_to_page(pud));
>  }
>
> -static void clear_pmd_entry(pmd_t *pmd)
> +static void clear_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr)
>  {
>         pte_t *pte_table = pte_offset_kernel(pmd, 0);
>         pmd_clear(pmd);
> +       kvm_tlb_flush_vmid_ipa(kvm, addr);
>         pte_free_kernel(NULL, pte_table);
>         put_page(virt_to_page(pmd));
>  }
> @@ -100,11 +103,12 @@ static bool pmd_empty(pmd_t *pmd)
>         return page_count(pmd_page) == 1;
>  }
>
> -static void clear_pte_entry(pte_t *pte)
> +static void clear_pte_entry(struct kvm *kvm, pte_t *pte, phys_addr_t addr)
>  {
>         if (pte_present(*pte)) {
>                 kvm_set_pte(pte, __pte(0));
>                 put_page(virt_to_page(pte));
> +               kvm_tlb_flush_vmid_ipa(kvm, addr);

I enjoyed the fact that it's perfectly correct to have this flush
after the put_page.

>         }
>  }
>
> @@ -114,7 +118,8 @@ static bool pte_empty(pte_t *pte)
>         return page_count(pte_page) == 1;
>  }
>
> -static void unmap_range(pgd_t *pgdp, unsigned long long start, u64 size)
> +static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
> +                       unsigned long long start, u64 size)
>  {
>         pgd_t *pgd;
>         pud_t *pud;
> @@ -138,15 +143,15 @@ static void unmap_range(pgd_t *pgdp, unsigned long long start, u64 size)
>                 }
>
>                 pte = pte_offset_kernel(pmd, addr);
> -               clear_pte_entry(pte);
> +               clear_pte_entry(kvm, pte, addr);
>                 range = PAGE_SIZE;
>
>                 /* If we emptied the pte, walk back up the ladder */
>                 if (pte_empty(pte)) {
> -                       clear_pmd_entry(pmd);
> +                       clear_pmd_entry(kvm, pmd, addr);
>                         range = PMD_SIZE;
>                         if (pmd_empty(pmd)) {
> -                               clear_pud_entry(pud);
> +                               clear_pud_entry(kvm, pud, addr);
>                                 range = PUD_SIZE;
>                         }
>                 }
> @@ -165,14 +170,14 @@ void free_boot_hyp_pgd(void)
>         mutex_lock(&kvm_hyp_pgd_mutex);
>
>         if (boot_hyp_pgd) {
> -               unmap_range(boot_hyp_pgd, hyp_idmap_start, PAGE_SIZE);
> -               unmap_range(boot_hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
> +               unmap_range(NULL, boot_hyp_pgd, hyp_idmap_start, PAGE_SIZE);
> +               unmap_range(NULL, boot_hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
>                 kfree(boot_hyp_pgd);
>                 boot_hyp_pgd = NULL;
>         }
>
>         if (hyp_pgd)
> -               unmap_range(hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
> +               unmap_range(NULL, hyp_pgd, TRAMPOLINE_VA, PAGE_SIZE);
>
>         kfree(init_bounce_page);
>         init_bounce_page = NULL;
> @@ -200,9 +205,10 @@ void free_hyp_pgds(void)
>
>         if (hyp_pgd) {
>                 for (addr = PAGE_OFFSET; virt_addr_valid(addr); addr += PGDIR_SIZE)
> -                       unmap_range(hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
> +                       unmap_range(NULL, hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
>                 for (addr = VMALLOC_START; is_vmalloc_addr((void*)addr); addr += PGDIR_SIZE)
> -                       unmap_range(hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
> +                       unmap_range(NULL, hyp_pgd, KERN_TO_HYP(addr), PGDIR_SIZE);
> +
>                 kfree(hyp_pgd);
>                 hyp_pgd = NULL;
>         }
> @@ -393,7 +399,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>   */
>  static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
>  {
> -       unmap_range(kvm->arch.pgd, start, size);
> +       unmap_range(kvm, kvm->arch.pgd, start, size);
>  }
>
>  /**
> @@ -413,6 +419,7 @@ void kvm_free_stage2_pgd(struct kvm *kvm)
>                 return;
>
>         unmap_stage2_range(kvm, 0, KVM_PHYS_SIZE);
> +       kvm_tlb_flush_vmid_ipa(kvm, 0); /* Invalidate TLB ALL */
>         free_pages((unsigned long)kvm->arch.pgd, S2_PGD_ORDER);
>         kvm->arch.pgd = NULL;
>  }
> @@ -675,7 +682,6 @@ static void handle_hva_to_gpa(struct kvm *kvm,
>  static void kvm_unmap_hva_handler(struct kvm *kvm, gpa_t gpa, void *data)
>  {
>         unmap_stage2_range(kvm, gpa, PAGE_SIZE);
> -       kvm_tlb_flush_vmid_ipa(kvm, gpa);
>  }
>
>  int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
> --
> 1.8.2.1
>
>
looks good to me,

-Christoffer

[PATCH 4/4] ARM: KVM: fix use of S2_PGD_SIZE

[Christoffer Dall]

On Tue, Apr 30, 2013 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> S2_PGD_SIZE describe the number of pages a used by a stage-2 PGD,
> not the size of the PGD.
>
> Fix the VM_BUG_ON() call that doesn't check much when S2_PGD_SIZE
> is equal to 1.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index b978ebe..09ece5c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -377,7 +377,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>                 return -ENOMEM;
>
>         /* stage-2 pgd must be aligned to its size */
> -       VM_BUG_ON((unsigned long)pgd & (S2_PGD_SIZE - 1));
> +       VM_BUG_ON((unsigned long)pgd & (PAGE_SIZE * S2_PGD_SIZE - 1));

I think the define is broken and should be fixed and/or renamed instead.

>
>         memset(pgd, 0, PTRS_PER_S2_PGD * sizeof(pgd_t));
>         kvm_clean_pgd(pgd);
> --
> 1.8.2.1
>
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

[PATCH 4/4] ARM: KVM: fix use of S2_PGD_SIZE

[Marc Zyngier]

On 30/04/13 18:40, Christoffer Dall wrote:
> On Tue, Apr 30, 2013 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> S2_PGD_SIZE describe the number of pages a used by a stage-2 PGD,
>> not the size of the PGD.
>>
>> Fix the VM_BUG_ON() call that doesn't check much when S2_PGD_SIZE
>> is equal to 1.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/kvm/mmu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index b978ebe..09ece5c 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -377,7 +377,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>>                 return -ENOMEM;
>>
>>         /* stage-2 pgd must be aligned to its size */
>> -       VM_BUG_ON((unsigned long)pgd & (S2_PGD_SIZE - 1));
>> +       VM_BUG_ON((unsigned long)pgd & (PAGE_SIZE * S2_PGD_SIZE - 1));
> 
> I think the define is broken and should be fixed and/or renamed instead.

To be fair, I wouldn't mind the whole check to be dropped altogether. If
__get_free_pages doesn't give you an aligned allocation, the whole
kernel is going the way of the dodo, and we shouldn't really care...

Your call, really.

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

[PATCH 4/4] ARM: KVM: fix use of S2_PGD_SIZE

[Christoffer Dall]

On Tue, Apr 30, 2013 at 06:52:24PM +0100, Marc Zyngier wrote:
> On 30/04/13 18:40, Christoffer Dall wrote:
> > On Tue, Apr 30, 2013 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> S2_PGD_SIZE describe the number of pages a used by a stage-2 PGD,
> >> not the size of the PGD.
> >>
> >> Fix the VM_BUG_ON() call that doesn't check much when S2_PGD_SIZE
> >> is equal to 1.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm/kvm/mmu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index b978ebe..09ece5c 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -377,7 +377,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
> >>                 return -ENOMEM;
> >>
> >>         /* stage-2 pgd must be aligned to its size */
> >> -       VM_BUG_ON((unsigned long)pgd & (S2_PGD_SIZE - 1));
> >> +       VM_BUG_ON((unsigned long)pgd & (PAGE_SIZE * S2_PGD_SIZE - 1));
> > 
> > I think the define is broken and should be fixed and/or renamed instead.
> 
> To be fair, I wouldn't mind the whole check to be dropped altogether. If
> __get_free_pages doesn't give you an aligned allocation, the whole
> kernel is going the way of the dodo, and we shouldn't really care...
> 
> Your call, really.
> 
Yeah, this is from way back when, where the whole thing was different
and we were debugging all sorts of issues, so we can get rid of both the
check and the define in stead.  Want to take care of it as part of your
series?

Thanks,
-Christoffer

[PATCH 4/4] ARM: KVM: fix use of S2_PGD_SIZE

[Marc Zyngier]

On 30/04/13 18:56, Christoffer Dall wrote:
> On Tue, Apr 30, 2013 at 06:52:24PM +0100, Marc Zyngier wrote:
>> On 30/04/13 18:40, Christoffer Dall wrote:
>>> On Tue, Apr 30, 2013 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> S2_PGD_SIZE describe the number of pages a used by a stage-2 PGD,
>>>> not the size of the PGD.
>>>>
>>>> Fix the VM_BUG_ON() call that doesn't check much when S2_PGD_SIZE
>>>> is equal to 1.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm/kvm/mmu.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>>> index b978ebe..09ece5c 100644
>>>> --- a/arch/arm/kvm/mmu.c
>>>> +++ b/arch/arm/kvm/mmu.c
>>>> @@ -377,7 +377,7 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>>>>                 return -ENOMEM;
>>>>
>>>>         /* stage-2 pgd must be aligned to its size */
>>>> -       VM_BUG_ON((unsigned long)pgd & (S2_PGD_SIZE - 1));
>>>> +       VM_BUG_ON((unsigned long)pgd & (PAGE_SIZE * S2_PGD_SIZE - 1));
>>>
>>> I think the define is broken and should be fixed and/or renamed instead.
>>
>> To be fair, I wouldn't mind the whole check to be dropped altogether. If
>> __get_free_pages doesn't give you an aligned allocation, the whole
>> kernel is going the way of the dodo, and we shouldn't really care...
>>
>> Your call, really.
>>
> Yeah, this is from way back when, where the whole thing was different
> and we were debugging all sorts of issues, so we can get rid of both the
> check and the define in stead.  Want to take care of it as part of your
> series?

Sure. I'll kill it when I repost the series.

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

[PATCH 1/4] ARM: KVM: be more thorough when invalidating TLBs

[Marc Zyngier]

On 30/04/13 18:17, Christoffer Dall wrote:
> On Tue, Apr 30, 2013 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> The KVM/ARM MMU code doesn't take care of invalidating TLBs before
>> freeing a {pte,pmd} table. This could cause problems if the page
>> is reallocated and then speculated into by another CPU.
>>
>> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/kvm/interrupts.S |  2 ++
>>  arch/arm/kvm/mmu.c        | 36 +++++++++++++++++++++---------------
>>  2 files changed, 23 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
>> index f7793df..9e2d906c 100644
>> --- a/arch/arm/kvm/interrupts.S
>> +++ b/arch/arm/kvm/interrupts.S
>> @@ -37,6 +37,8 @@ __kvm_hyp_code_start:
>>   *
>>   * void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
>>   *
>> + * Invalidate TLB for the given IPA, or invalidate all if ipa is zero.
>> + *
>>   * We rely on the hardware to broadcast the TLB invalidation to all CPUs
>>   * inside the inner-shareable domain (which is the case for all v7
>>   * implementations).  If we come across a non-IS SMP implementation, we'll
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 9657065..71f2a57 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -43,7 +43,8 @@ static phys_addr_t hyp_idmap_vector;
>>
>>  static void kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
>>  {
>> -       kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);
>> +       if (kvm)
>> +               kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);
> 
> this feels slightly abusive? could we add a comment that hyp page
> table freeing don't need tlb flushing from these functions and don't
> have a struct kvm?
> 
> alternatively it may be more clear to have something like:
> 
> if (kvm) /* Check if Hyp or Stage-2 page table */
>     kvm_tlb_flush_vmid_ipa(kvm, addr);
> 
> in the callers, but, eh, maybe I'm over thinking this.

That code would be repeated three times, which feels a bit overkill.
I'll add a comment to the function so it is crystal clear.

Thanks,

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