[PATCH v2 0/5] ARM: KVM: various mmu related fixes for 3.10

[PATCH v2 0/5] ARM: KVM: various 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 (5):
  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 tables
  ARM: KVM: get rid of S2_PGD_SIZE
  ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs

 arch/arm/include/asm/kvm_arm.h  |  1 -
 arch/arm/include/asm/kvm_asm.h  |  2 --
 arch/arm/include/asm/kvm_host.h |  4 ++--
 arch/arm/kvm/arm.c              |  8 +++----
 arch/arm/kvm/interrupts.S       |  2 ++
 arch/arm/kvm/mmu.c              | 48 +++++++++++++++++++++++------------------
 6 files changed, 35 insertions(+), 30 deletions(-)

-- 
1.8.2.1

[PATCH v2 1/5] 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        | 42 +++++++++++++++++++++++++++---------------
 2 files changed, 29 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..4a838db 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -43,7 +43,14 @@ 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);
+	/*
+	 * This function also gets called when dealing with HYP page
+	 * tables. As HYP doesn't have an associated struct kvm (and
+	 * the HYP page tables are fairly static), we don't do
+	 * anything there.
+	 */
+	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 +85,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 +109,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 +124,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 +149,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 +176,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 +211,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 +405,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 +425,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 +688,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 v2 2/5] 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 v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[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 already done),
and actually not required if running on a SMP capable platform
(the HW PTW can snoop other cpus' L1).

Remove this code and let ae8a8b9553bd (ARM: 7691/1: mm: kill unused
TLB_CAN_READ_FROM_L1_CACHE and use ALT_SMP instead) turn it into
a no-op for 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/kvm/mmu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 4a838db..d79b594 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -234,7 +234,6 @@ 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));
 		pfn++;
 	} while (addr += PAGE_SIZE, addr != end);
 }
@@ -261,7 +260,6 @@ 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));
 		}
 
 		next = pmd_addr_end(addr, end);
@@ -299,7 +297,6 @@ 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));
 		}
 
 		next = pgd_addr_end(addr, end);
-- 
1.8.2.1

[PATCH v2 4/5] ARM: KVM: get rid of S2_PGD_SIZE

[Marc Zyngier]

S2_PGD_SIZE defines the number of pages used by a stage-2 PGD
and is unused, except for a VM_BUG_ON check that missuses the
define.

As the check is very unlikely to ever triggered except in
circumstances where KVM is the least of our worries, just kill
both the define and the VM_BUG_ON check.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_arm.h | 1 -
 arch/arm/kvm/mmu.c             | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
index 124623e..64e9696 100644
--- a/arch/arm/include/asm/kvm_arm.h
+++ b/arch/arm/include/asm/kvm_arm.h
@@ -135,7 +135,6 @@
 #define KVM_PHYS_MASK	(KVM_PHYS_SIZE - 1ULL)
 #define PTRS_PER_S2_PGD	(1ULL << (KVM_PHYS_SHIFT - 30))
 #define S2_PGD_ORDER	get_order(PTRS_PER_S2_PGD * sizeof(pgd_t))
-#define S2_PGD_SIZE	(1 << S2_PGD_ORDER)
 
 /* Virtualization Translation Control Register (VTCR) bits */
 #define VTCR_SH0	(3 << 12)
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index d79b594..3c506c7 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -379,9 +379,6 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
 	if (!pgd)
 		return -ENOMEM;
 
-	/* stage-2 pgd must be aligned to its size */
-	VM_BUG_ON((unsigned long)pgd & (S2_PGD_SIZE - 1));
-
 	memset(pgd, 0, PTRS_PER_S2_PGD * sizeof(pgd_t));
 	kvm_clean_pgd(pgd);
 	kvm->arch.pgd = pgd;
-- 
1.8.2.1

[PATCH v2 5/5] ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs

[Marc Zyngier]

HYP PGDs are passed around as phys_addr_t, except just before calling
into the hypervisor init code, where they are cast to a rather weird
unsigned long long.

Just keep them around as phys_addr_t, which is what makes the most
sense.

Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h | 4 ++--
 arch/arm/kvm/arm.c              | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 57cb786..ff49193 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -190,8 +190,8 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
 int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		int exception_index);
 
-static inline void __cpu_init_hyp_mode(unsigned long long boot_pgd_ptr,
-				       unsigned long long pgd_ptr,
+static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr,
+				       phys_addr_t pgd_ptr,
 				       unsigned long hyp_stack_ptr,
 				       unsigned long vector_ptr)
 {
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index fa177df..801ffce 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -797,8 +797,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 
 static void cpu_init_hyp_mode(void *dummy)
 {
-	unsigned long long boot_pgd_ptr;
-	unsigned long long pgd_ptr;
+	phys_addr_t boot_pgd_ptr;
+	phys_addr_t pgd_ptr;
 	unsigned long hyp_stack_ptr;
 	unsigned long stack_page;
 	unsigned long vector_ptr;
@@ -806,8 +806,8 @@ static void cpu_init_hyp_mode(void *dummy)
 	/* Switch from the HYP stub to our own HYP init vector */
 	__hyp_set_vectors(kvm_get_idmap_vector());
 
-	boot_pgd_ptr = (unsigned long long)kvm_mmu_get_boot_httbr();
-	pgd_ptr = (unsigned long long)kvm_mmu_get_httbr();
+	boot_pgd_ptr = kvm_mmu_get_boot_httbr();
+	pgd_ptr = kvm_mmu_get_httbr();
 	stack_page = __get_cpu_var(kvm_arm_hyp_stack_page);
 	hyp_stack_ptr = stack_page + PAGE_SIZE;
 	vector_ptr = (unsigned long)__kvm_hyp_vector;
-- 
1.8.2.1

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Catalin Marinas]

On Thu, May 02, 2013 at 03:39:00PM +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 already done),
> and actually not required if running on a SMP capable platform
> (the HW PTW can snoop other cpus' L1).
> 
> Remove this code and let ae8a8b9553bd (ARM: 7691/1: mm: kill unused
> TLB_CAN_READ_FROM_L1_CACHE and use ALT_SMP instead) turn it into
> a no-op for SMP ARMv7.

I think the comment is a bit misleading. You actually don't need
kvm_flush_dcache_to_poc() since you already do this in kvm_set_pte()
etc.

-- 
Catalin

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Will Deacon]

On Thu, May 02, 2013 at 03:39:00PM +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 already done),
> and actually not required if running on a SMP capable platform
> (the HW PTW can snoop other cpus' L1).
> 
> Remove this code and let ae8a8b9553bd (ARM: 7691/1: mm: kill unused
> TLB_CAN_READ_FROM_L1_CACHE and use ALT_SMP instead) turn it into
> a no-op for 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/kvm/mmu.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 4a838db..d79b594 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -234,7 +234,6 @@ 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));
>  		pfn++;
>  	} while (addr += PAGE_SIZE, addr != end);

Could you remove the flushing code out of kvm_set_pte, then flush the range
after the loop? Then you get one flush and one barrier for the whole range.

Will

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

[Catalin Marinas]

On Thu, May 02, 2013 at 03:38:58PM +0100, Marc Zyngier wrote:
> 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
...
> -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);
>  	}
>  }
...
>  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 +425,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 */

Do you still need this here if you invalidated each individual pte in
clear_pte_entry()? I think you can remove it from clear_pte_entry() and
just leave it here (more efficient probably) since you wouldn't free the
actual pages pointed at by the pte before unmapping.

>  	free_pages((unsigned long)kvm->arch.pgd, S2_PGD_ORDER);
>  	kvm->arch.pgd = NULL;
>  }
> @@ -675,7 +688,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);

Here you removed it relying on clear_pte_entry(), I think you could keep
it (see above).

-- 
Catalin

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Christoffer Dall]

On Thu, May 2, 2013 at 8:03 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, May 02, 2013 at 03:39:00PM +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 already done),
>> and actually not required if running on a SMP capable platform
>> (the HW PTW can snoop other cpus' L1).
>>
>> Remove this code and let ae8a8b9553bd (ARM: 7691/1: mm: kill unused
>> TLB_CAN_READ_FROM_L1_CACHE and use ALT_SMP instead) turn it into
>> a no-op for 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/kvm/mmu.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 4a838db..d79b594 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -234,7 +234,6 @@ 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));
>>               pfn++;
>>       } while (addr += PAGE_SIZE, addr != end);
>
> Could you remove the flushing code out of kvm_set_pte, then flush the range
> after the loop? Then you get one flush and one barrier for the whole range.
>
that would be different from how it's done in the rest of the kernel
where you're expecting set_pte to do the necessary flushing for you,
no?

instead, if this optimization is worth it, the code could just use
pte_val(pte) = XX;

-Christoffer

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Will Deacon]

On Thu, May 02, 2013 at 04:15:05PM +0100, Christoffer Dall wrote:
> On Thu, May 2, 2013 at 8:03 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, May 02, 2013 at 03:39:00PM +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 already done),
> >> and actually not required if running on a SMP capable platform
> >> (the HW PTW can snoop other cpus' L1).
> >>
> >> Remove this code and let ae8a8b9553bd (ARM: 7691/1: mm: kill unused
> >> TLB_CAN_READ_FROM_L1_CACHE and use ALT_SMP instead) turn it into
> >> a no-op for 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/kvm/mmu.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index 4a838db..d79b594 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -234,7 +234,6 @@ 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));
> >>               pfn++;
> >>       } while (addr += PAGE_SIZE, addr != end);
> >
> > Could you remove the flushing code out of kvm_set_pte, then flush the range
> > after the loop? Then you get one flush and one barrier for the whole range.
> >
> that would be different from how it's done in the rest of the kernel
> where you're expecting set_pte to do the necessary flushing for you,
> no?

Correct, but since kvm_set_pte seems only to be called from this file, I'm
not sure you need to match thes rest of the kernel.

> instead, if this optimization is worth it, the code could just use
> pte_val(pte) = XX;

or just *pte = XX;

Will

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

[Marc Zyngier]

On 02/05/13 16:13, Catalin Marinas wrote:
> On Thu, May 02, 2013 at 03:38:58PM +0100, Marc Zyngier wrote:
>> 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
> ...
>> -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);
>>  	}
>>  }
> ...
>>  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 +425,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 */
> 
> Do you still need this here if you invalidated each individual pte in
> clear_pte_entry()? I think you can remove it from clear_pte_entry() and
> just leave it here (more efficient probably) since you wouldn't free the
> actual pages pointed at by the pte before unmapping.

There is two cases we're trying to cater for:
- unmapping a single page from stage2 (page being swapped out, for example)
- unmapping the whole of stage2 (VM exiting)

We cannot loose the "local" operations, but the last one can indeed go.

Thanks,

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

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Christoffer Dall]

On Thu, May 02, 2013 at 04:17:53PM +0100, Will Deacon wrote:
> On Thu, May 02, 2013 at 04:15:05PM +0100, Christoffer Dall wrote:
> > On Thu, May 2, 2013 at 8:03 AM, Will Deacon <will.deacon@arm.com> wrote:
> > > On Thu, May 02, 2013 at 03:39:00PM +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 already done),
> > >> and actually not required if running on a SMP capable platform
> > >> (the HW PTW can snoop other cpus' L1).
> > >>
> > >> Remove this code and let ae8a8b9553bd (ARM: 7691/1: mm: kill unused
> > >> TLB_CAN_READ_FROM_L1_CACHE and use ALT_SMP instead) turn it into
> > >> a no-op for 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/kvm/mmu.c | 3 ---
> > >>  1 file changed, 3 deletions(-)
> > >>
> > >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > >> index 4a838db..d79b594 100644
> > >> --- a/arch/arm/kvm/mmu.c
> > >> +++ b/arch/arm/kvm/mmu.c
> > >> @@ -234,7 +234,6 @@ 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));
> > >>               pfn++;
> > >>       } while (addr += PAGE_SIZE, addr != end);
> > >
> > > Could you remove the flushing code out of kvm_set_pte, then flush the range
> > > after the loop? Then you get one flush and one barrier for the whole range.
> > >
> > that would be different from how it's done in the rest of the kernel
> > where you're expecting set_pte to do the necessary flushing for you,
> > no?
> 
> Correct, but since kvm_set_pte seems only to be called from this file, I'm
> not sure you need to match thes rest of the kernel.
> 
> > instead, if this optimization is worth it, the code could just use
> > pte_val(pte) = XX;
> 
> or just *pte = XX;
> 
That won't work with STRICT_M_TYPECHECKS though will it?

-Christoffer

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Will Deacon]

On Mon, May 13, 2013 at 06:17:37AM +0100, Christoffer Dall wrote:
> On Thu, May 02, 2013 at 04:17:53PM +0100, Will Deacon wrote:
> > or just *pte = XX;
> > 
> That won't work with STRICT_M_TYPECHECKS though will it?

Depends if XX is a pte_t or not :)

Will

[PATCH v2 3/5] ARM: KVM: relax cache maintainance when building page tables

[Christoffer Dall]

On Mon, May 13, 2013 at 1:58 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, May 13, 2013 at 06:17:37AM +0100, Christoffer Dall wrote:
>> On Thu, May 02, 2013 at 04:17:53PM +0100, Will Deacon wrote:
>> > or just *pte = XX;
>> >
>> That won't work with STRICT_M_TYPECHECKS though will it?
>
> Depends if XX is a pte_t or not :)
>
this is silly :)