All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/7] ARM: KVM: various mmu related fixes for 3.10
@ 2013-05-14 11:11 ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall

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, based on 3.10-rc1.

* From v2:
  - [1/7] Drop the unnecessary "TLB invalidate all", as we already do
  it on a page-per-page level.
  - [3/7] Add a cache cleaning primitive, move the cleaning outside of
  kvm_set_pte(), and clean a range as large as possible when inserting
  PTEs.
  - [4,5,7/7] New patches

Marc Zyngier (7):
  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: use phys_addr_t instead of unsigned long long for HYP PGDs
  ARM: KVM: don't special case PC when doing an MMIO
  ARM: KVM: get rid of S2_PGD_SIZE
  ARM: KVM: drop use of PAGE_S2_DEVICE

 arch/arm/include/asm/kvm_arm.h     |  1 -
 arch/arm/include/asm/kvm_asm.h     |  2 --
 arch/arm/include/asm/kvm_emulate.h |  5 ----
 arch/arm/include/asm/kvm_host.h    |  4 +--
 arch/arm/include/asm/kvm_mmu.h     | 17 +++++++-----
 arch/arm/include/asm/pgtable.h     |  2 --
 arch/arm/kvm/arm.c                 |  8 +++---
 arch/arm/kvm/mmio.c                |  6 -----
 arch/arm/kvm/mmu.c                 | 53 ++++++++++++++++++++++----------------
 arch/arm/mm/mmu.c                  |  6 ++---
 10 files changed, 50 insertions(+), 54 deletions(-)

-- 
1.8.2.3



^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 0/7] ARM: KVM: various mmu related fixes for 3.10
@ 2013-05-14 11:11 ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

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, based on 3.10-rc1.

* From v2:
  - [1/7] Drop the unnecessary "TLB invalidate all", as we already do
  it on a page-per-page level.
  - [3/7] Add a cache cleaning primitive, move the cleaning outside of
  kvm_set_pte(), and clean a range as large as possible when inserting
  PTEs.
  - [4,5,7/7] New patches

Marc Zyngier (7):
  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: use phys_addr_t instead of unsigned long long for HYP PGDs
  ARM: KVM: don't special case PC when doing an MMIO
  ARM: KVM: get rid of S2_PGD_SIZE
  ARM: KVM: drop use of PAGE_S2_DEVICE

 arch/arm/include/asm/kvm_arm.h     |  1 -
 arch/arm/include/asm/kvm_asm.h     |  2 --
 arch/arm/include/asm/kvm_emulate.h |  5 ----
 arch/arm/include/asm/kvm_host.h    |  4 +--
 arch/arm/include/asm/kvm_mmu.h     | 17 +++++++-----
 arch/arm/include/asm/pgtable.h     |  2 --
 arch/arm/kvm/arm.c                 |  8 +++---
 arch/arm/kvm/mmio.c                |  6 -----
 arch/arm/kvm/mmu.c                 | 53 ++++++++++++++++++++++----------------
 arch/arm/mm/mmu.c                  |  6 ++---
 10 files changed, 50 insertions(+), 54 deletions(-)

-- 
1.8.2.3

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 1/7] ARM: KVM: be more thorough when invalidating TLBs
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall, Catalin Marinas

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/mmu.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 9657065..84ba67b 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);
 }
 
 /**
@@ -675,7 +687,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.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 1/7] ARM: KVM: be more thorough when invalidating TLBs
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

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/mmu.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 9657065..84ba67b 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);
 }
 
 /**
@@ -675,7 +687,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.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 2/7] ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall

__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.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 2/7] ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

__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.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall, Will Deacon, Catalin Marinas

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/include/asm/kvm_mmu.h | 17 +++++++++++------
 arch/arm/kvm/mmu.c             |  7 ++++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 472ac70..8c03c96 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -65,11 +65,6 @@ void kvm_clear_hyp_idmap(void);
 static inline void kvm_set_pte(pte_t *pte, pte_t new_pte)
 {
 	pte_val(*pte) = new_pte;
-	/*
-	 * flush_pmd_entry just takes a void pointer and cleans the necessary
-	 * cache entries, so we can reuse the function for ptes.
-	 */
-	flush_pmd_entry(pte);
 }
 
 static inline bool kvm_is_write_fault(unsigned long hsr)
@@ -83,9 +78,19 @@ static inline bool kvm_is_write_fault(unsigned long hsr)
 		return true;
 }
 
+static inline void kvm_clean_dcache_area(void *addr, size_t size)
+{
+	clean_dcache_area(addr, size);
+}
+
+static inline void kvm_clean_pte_entry(pte_t *pte)
+{
+	kvm_clean_dcache_area(pte, sizeof(*pte));
+}
+
 static inline void kvm_clean_pgd(pgd_t *pgd)
 {
-	clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
+	kvm_clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
 }
 
 static inline void kvm_clean_pmd_entry(pmd_t *pmd)
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 84ba67b..451bad3 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -113,6 +113,7 @@ 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));
+		kvm_clean_pte_entry(pte);
 		put_page(virt_to_page(pte));
 		kvm_tlb_flush_vmid_ipa(kvm, addr);
 	}
@@ -234,9 +235,10 @@ 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);
+
+	kvm_clean_dcache_area((void *)start, end - start);
 }
 
 static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
@@ -261,7 +263,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 +300,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);
@@ -469,6 +469,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
 	/* Create 2nd stage page table mapping - Level 3 */
 	old_pte = *pte;
 	kvm_set_pte(pte, *new_pte);
+	kvm_clean_pte_entry(pte);
 	if (pte_present(old_pte))
 		kvm_tlb_flush_vmid_ipa(kvm, addr);
 	else
-- 
1.8.2.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

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/include/asm/kvm_mmu.h | 17 +++++++++++------
 arch/arm/kvm/mmu.c             |  7 ++++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 472ac70..8c03c96 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -65,11 +65,6 @@ void kvm_clear_hyp_idmap(void);
 static inline void kvm_set_pte(pte_t *pte, pte_t new_pte)
 {
 	pte_val(*pte) = new_pte;
-	/*
-	 * flush_pmd_entry just takes a void pointer and cleans the necessary
-	 * cache entries, so we can reuse the function for ptes.
-	 */
-	flush_pmd_entry(pte);
 }
 
 static inline bool kvm_is_write_fault(unsigned long hsr)
@@ -83,9 +78,19 @@ static inline bool kvm_is_write_fault(unsigned long hsr)
 		return true;
 }
 
+static inline void kvm_clean_dcache_area(void *addr, size_t size)
+{
+	clean_dcache_area(addr, size);
+}
+
+static inline void kvm_clean_pte_entry(pte_t *pte)
+{
+	kvm_clean_dcache_area(pte, sizeof(*pte));
+}
+
 static inline void kvm_clean_pgd(pgd_t *pgd)
 {
-	clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
+	kvm_clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
 }
 
 static inline void kvm_clean_pmd_entry(pmd_t *pmd)
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 84ba67b..451bad3 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -113,6 +113,7 @@ 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));
+		kvm_clean_pte_entry(pte);
 		put_page(virt_to_page(pte));
 		kvm_tlb_flush_vmid_ipa(kvm, addr);
 	}
@@ -234,9 +235,10 @@ 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);
+
+	kvm_clean_dcache_area((void *)start, end - start);
 }
 
 static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
@@ -261,7 +263,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 +300,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);
@@ -469,6 +469,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
 	/* Create 2nd stage page table mapping - Level 3 */
 	old_pte = *pte;
 	kvm_set_pte(pte, *new_pte);
+	kvm_clean_pte_entry(pte);
 	if (pte_present(old_pte))
 		kvm_tlb_flush_vmid_ipa(kvm, addr);
 	else
-- 
1.8.2.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 4/7] ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall, Catalin Marinas

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 37d216d..327a1fb 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -789,8 +789,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;
@@ -798,8 +798,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.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 4/7] ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

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 37d216d..327a1fb 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -789,8 +789,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;
@@ -798,8 +798,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.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 5/7] ARM: KVM: don't special case PC when doing an MMIO
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall, Catalin Marinas

Admitedly, reading a MMIO register to load PC is very weird.
Writing PC to a MMIO register is probably even worse. But
the architecture doesn't forbid any of these, and injecting
a Prefetch Abort is the wrong thing to do anyway.

Remove this check altogether, and let the adventurous guest
wander into LaLaLand if they feel compelled to do so.

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

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 82b4bab..a464e8d 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -65,11 +65,6 @@ static inline bool vcpu_mode_priv(struct kvm_vcpu *vcpu)
 	return cpsr_mode > USR_MODE;;
 }
 
-static inline bool kvm_vcpu_reg_is_pc(struct kvm_vcpu *vcpu, int reg)
-{
-	return reg == 15;
-}
-
 static inline u32 kvm_vcpu_get_hsr(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.fault.hsr;
diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 72a12f2..b8e06b7 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -86,12 +86,6 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	sign_extend = kvm_vcpu_dabt_issext(vcpu);
 	rt = kvm_vcpu_dabt_get_rd(vcpu);
 
-	if (kvm_vcpu_reg_is_pc(vcpu, rt)) {
-		/* IO memory trying to read/write pc */
-		kvm_inject_pabt(vcpu, kvm_vcpu_get_hfar(vcpu));
-		return 1;
-	}
-
 	mmio->is_write = is_write;
 	mmio->phys_addr = fault_ipa;
 	mmio->len = len;
-- 
1.8.2.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 5/7] ARM: KVM: don't special case PC when doing an MMIO
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

Admitedly, reading a MMIO register to load PC is very weird.
Writing PC to a MMIO register is probably even worse. But
the architecture doesn't forbid any of these, and injecting
a Prefetch Abort is the wrong thing to do anyway.

Remove this check altogether, and let the adventurous guest
wander into LaLaLand if they feel compelled to do so.

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

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 82b4bab..a464e8d 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -65,11 +65,6 @@ static inline bool vcpu_mode_priv(struct kvm_vcpu *vcpu)
 	return cpsr_mode > USR_MODE;;
 }
 
-static inline bool kvm_vcpu_reg_is_pc(struct kvm_vcpu *vcpu, int reg)
-{
-	return reg == 15;
-}
-
 static inline u32 kvm_vcpu_get_hsr(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.fault.hsr;
diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 72a12f2..b8e06b7 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -86,12 +86,6 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	sign_extend = kvm_vcpu_dabt_issext(vcpu);
 	rt = kvm_vcpu_dabt_get_rd(vcpu);
 
-	if (kvm_vcpu_reg_is_pc(vcpu, rt)) {
-		/* IO memory trying to read/write pc */
-		kvm_inject_pabt(vcpu, kvm_vcpu_get_hfar(vcpu));
-		return 1;
-	}
-
 	mmio->is_write = is_write;
 	mmio->phys_addr = fault_ipa;
 	mmio->len = len;
-- 
1.8.2.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall

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 451bad3..693d16c 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -382,9 +382,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.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

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 451bad3..693d16c 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -382,9 +382,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.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-14 11:11   ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, kvm; +Cc: cdall

At the moment, when mapping a device into Stage-2 for a guest,
we override whatever the guest uses by forcing a device memory
type in Stage-2.

While this is not exactly wrong, this isn't really the "spirit" of
the architecture. The hardware shouldn't have to cope for a broken
guest mapping to a device as normal memory.

As such, let's get rid of the memory type overrides, and map
everything as PAGE_S2 in Stage-2. This simplifies the code and
let the guest do its own thing, whether it is stupid or not.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/pgtable.h | 2 --
 arch/arm/kvm/mmu.c             | 2 +-
 arch/arm/mm/mmu.c              | 6 ++----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 9bcd262..c3410a8 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -81,7 +81,6 @@ extern pgprot_t		pgprot_user;
 extern pgprot_t		pgprot_kernel;
 extern pgprot_t		pgprot_hyp_device;
 extern pgprot_t		pgprot_s2;
-extern pgprot_t		pgprot_s2_device;
 
 #define _MOD_PROT(p, b)	__pgprot(pgprot_val(p) | (b))
 
@@ -97,7 +96,6 @@ extern pgprot_t		pgprot_s2_device;
 #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
 #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
 #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
-#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_USER | L_PTE_S2_RDONLY)
 
 #define __PAGE_NONE		__pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
 #define __PAGE_SHARED		__pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 693d16c..f2a8416 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -495,7 +495,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
 	pfn = __phys_to_pfn(pa);
 
 	for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
-		pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
+		pte_t pte = pfn_pte(pfn, PAGE_S2);
 		kvm_set_s2pte_writable(&pte);
 
 		ret = mmu_topup_memory_cache(&cache, 2, 2);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e0d8565..bc001f5 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -60,7 +60,6 @@ pgprot_t pgprot_user;
 pgprot_t pgprot_kernel;
 pgprot_t pgprot_hyp_device;
 pgprot_t pgprot_s2;
-pgprot_t pgprot_s2_device;
 
 EXPORT_SYMBOL(pgprot_user);
 EXPORT_SYMBOL(pgprot_kernel);
@@ -343,7 +342,7 @@ static void __init build_mem_type_table(void)
 	struct cachepolicy *cp;
 	unsigned int cr = get_cr();
 	pteval_t user_pgprot, kern_pgprot, vecs_pgprot;
-	pteval_t hyp_device_pgprot, s2_pgprot, s2_device_pgprot;
+	pteval_t hyp_device_pgprot, s2_pgprot;
 	int cpu_arch = cpu_architecture();
 	int i;
 
@@ -456,7 +455,7 @@ static void __init build_mem_type_table(void)
 	cp = &cache_policies[cachepolicy];
 	vecs_pgprot = kern_pgprot = user_pgprot = cp->pte;
 	s2_pgprot = cp->pte_s2;
-	hyp_device_pgprot = s2_device_pgprot = mem_types[MT_DEVICE].prot_pte;
+	hyp_device_pgprot = mem_types[MT_DEVICE].prot_pte;
 
 	/*
 	 * ARMv6 and above have extended page tables.
@@ -536,7 +535,6 @@ static void __init build_mem_type_table(void)
 	pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG |
 				 L_PTE_DIRTY | kern_pgprot);
 	pgprot_s2  = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | s2_pgprot);
-	pgprot_s2_device  = __pgprot(s2_device_pgprot);
 	pgprot_hyp_device  = __pgprot(hyp_device_pgprot);
 
 	mem_types[MT_LOW_VECTORS].prot_l1 |= ecc_mask;
-- 
1.8.2.3



^ permalink raw reply related	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
@ 2013-05-14 11:11   ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

At the moment, when mapping a device into Stage-2 for a guest,
we override whatever the guest uses by forcing a device memory
type in Stage-2.

While this is not exactly wrong, this isn't really the "spirit" of
the architecture. The hardware shouldn't have to cope for a broken
guest mapping to a device as normal memory.

As such, let's get rid of the memory type overrides, and map
everything as PAGE_S2 in Stage-2. This simplifies the code and
let the guest do its own thing, whether it is stupid or not.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/pgtable.h | 2 --
 arch/arm/kvm/mmu.c             | 2 +-
 arch/arm/mm/mmu.c              | 6 ++----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 9bcd262..c3410a8 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -81,7 +81,6 @@ extern pgprot_t		pgprot_user;
 extern pgprot_t		pgprot_kernel;
 extern pgprot_t		pgprot_hyp_device;
 extern pgprot_t		pgprot_s2;
-extern pgprot_t		pgprot_s2_device;
 
 #define _MOD_PROT(p, b)	__pgprot(pgprot_val(p) | (b))
 
@@ -97,7 +96,6 @@ extern pgprot_t		pgprot_s2_device;
 #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
 #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
 #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
-#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_USER | L_PTE_S2_RDONLY)
 
 #define __PAGE_NONE		__pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
 #define __PAGE_SHARED		__pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 693d16c..f2a8416 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -495,7 +495,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
 	pfn = __phys_to_pfn(pa);
 
 	for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
-		pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
+		pte_t pte = pfn_pte(pfn, PAGE_S2);
 		kvm_set_s2pte_writable(&pte);
 
 		ret = mmu_topup_memory_cache(&cache, 2, 2);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e0d8565..bc001f5 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -60,7 +60,6 @@ pgprot_t pgprot_user;
 pgprot_t pgprot_kernel;
 pgprot_t pgprot_hyp_device;
 pgprot_t pgprot_s2;
-pgprot_t pgprot_s2_device;
 
 EXPORT_SYMBOL(pgprot_user);
 EXPORT_SYMBOL(pgprot_kernel);
@@ -343,7 +342,7 @@ static void __init build_mem_type_table(void)
 	struct cachepolicy *cp;
 	unsigned int cr = get_cr();
 	pteval_t user_pgprot, kern_pgprot, vecs_pgprot;
-	pteval_t hyp_device_pgprot, s2_pgprot, s2_device_pgprot;
+	pteval_t hyp_device_pgprot, s2_pgprot;
 	int cpu_arch = cpu_architecture();
 	int i;
 
@@ -456,7 +455,7 @@ static void __init build_mem_type_table(void)
 	cp = &cache_policies[cachepolicy];
 	vecs_pgprot = kern_pgprot = user_pgprot = cp->pte;
 	s2_pgprot = cp->pte_s2;
-	hyp_device_pgprot = s2_device_pgprot = mem_types[MT_DEVICE].prot_pte;
+	hyp_device_pgprot = mem_types[MT_DEVICE].prot_pte;
 
 	/*
 	 * ARMv6 and above have extended page tables.
@@ -536,7 +535,6 @@ static void __init build_mem_type_table(void)
 	pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG |
 				 L_PTE_DIRTY | kern_pgprot);
 	pgprot_s2  = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | s2_pgprot);
-	pgprot_s2_device  = __pgprot(s2_device_pgprot);
 	pgprot_hyp_device  = __pgprot(hyp_device_pgprot);
 
 	mem_types[MT_LOW_VECTORS].prot_l1 |= ecc_mask;
-- 
1.8.2.3

^ permalink raw reply related	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-14 13:05     ` Will Deacon
  -1 siblings, 0 replies; 44+ messages in thread
From: Will Deacon @ 2013-05-14 13:05 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm, cdall, Catalin Marinas

On Tue, May 14, 2013 at 12:11:36PM +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>

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

Will

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables
@ 2013-05-14 13:05     ` Will Deacon
  0 siblings, 0 replies; 44+ messages in thread
From: Will Deacon @ 2013-05-14 13:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:36PM +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>

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

Will

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/7] ARM: KVM: various mmu related fixes for 3.10
  2013-05-14 11:11 ` Marc Zyngier
@ 2013-05-21 16:07   ` Catalin Marinas
  -1 siblings, 0 replies; 44+ messages in thread
From: Catalin Marinas @ 2013-05-21 16:07 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm, cdall

On Tue, May 14, 2013 at 12:11:33PM +0100, Marc Zyngier wrote:
> 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, based on 3.10-rc1.
> 
> * From v2:
>   - [1/7] Drop the unnecessary "TLB invalidate all", as we already do
>   it on a page-per-page level.
>   - [3/7] Add a cache cleaning primitive, move the cleaning outside of
>   kvm_set_pte(), and clean a range as large as possible when inserting
>   PTEs.
>   - [4,5,7/7] New patches
> 
> Marc Zyngier (7):
>   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: use phys_addr_t instead of unsigned long long for HYP PGDs
>   ARM: KVM: don't special case PC when doing an MMIO
>   ARM: KVM: get rid of S2_PGD_SIZE
>   ARM: KVM: drop use of PAGE_S2_DEVICE

This series looks good to me:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 0/7] ARM: KVM: various mmu related fixes for 3.10
@ 2013-05-21 16:07   ` Catalin Marinas
  0 siblings, 0 replies; 44+ messages in thread
From: Catalin Marinas @ 2013-05-21 16:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:33PM +0100, Marc Zyngier wrote:
> 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, based on 3.10-rc1.
> 
> * From v2:
>   - [1/7] Drop the unnecessary "TLB invalidate all", as we already do
>   it on a page-per-page level.
>   - [3/7] Add a cache cleaning primitive, move the cleaning outside of
>   kvm_set_pte(), and clean a range as large as possible when inserting
>   PTEs.
>   - [4,5,7/7] New patches
> 
> Marc Zyngier (7):
>   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: use phys_addr_t instead of unsigned long long for HYP PGDs
>   ARM: KVM: don't special case PC when doing an MMIO
>   ARM: KVM: get rid of S2_PGD_SIZE
>   ARM: KVM: drop use of PAGE_S2_DEVICE

This series looks good to me:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-27 20:01     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-27 20:01 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, KVM General, Catalin Marinas

On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> At the moment, when mapping a device into Stage-2 for a guest,
> we override whatever the guest uses by forcing a device memory
> type in Stage-2.
>
> While this is not exactly wrong, this isn't really the "spirit" of
> the architecture. The hardware shouldn't have to cope for a broken
> guest mapping to a device as normal memory.
>

So I'm trying to think of a scenario where this feature in the
architecture would actually be useful, and it sounds like from you
guys that it's only useful to properly run a broken guest.

Are we 100% sure that a malicious guest can't leverage this to break
isolation? I'm thinking something along the lines of writing to a
device (for example the gic virtual cpu interface) with a cached
mapping. If such a write is in fact written back to cache, and not
evicted from the cache before a later time, where a different VM is
running, can't that adversely affect the other VM?

Probably this can never happen, but I wasn't able to convince myself
of this from going through the ARM ARM...?

> As such, let's get rid of the memory type overrides, and map
> everything as PAGE_S2 in Stage-2. This simplifies the code and
> let the guest do its own thing, whether it is stupid or not.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/pgtable.h | 2 --
>  arch/arm/kvm/mmu.c             | 2 +-
>  arch/arm/mm/mmu.c              | 6 ++----
>  3 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 9bcd262..c3410a8 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -81,7 +81,6 @@ extern pgprot_t               pgprot_user;
>  extern pgprot_t                pgprot_kernel;
>  extern pgprot_t                pgprot_hyp_device;
>  extern pgprot_t                pgprot_s2;
> -extern pgprot_t                pgprot_s2_device;
>
>  #define _MOD_PROT(p, b)        __pgprot(pgprot_val(p) | (b))
>
> @@ -97,7 +96,6 @@ extern pgprot_t               pgprot_s2_device;
>  #define PAGE_HYP               _MOD_PROT(pgprot_kernel, L_PTE_HYP)
>  #define PAGE_HYP_DEVICE                _MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
>  #define PAGE_S2                        _MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE         _MOD_PROT(pgprot_s2_device, L_PTE_USER | L_PTE_S2_RDONLY)
>
>  #define __PAGE_NONE            __pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
>  #define __PAGE_SHARED          __pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 693d16c..f2a8416 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -495,7 +495,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>         pfn = __phys_to_pfn(pa);
>
>         for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
> -               pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
> +               pte_t pte = pfn_pte(pfn, PAGE_S2);
>                 kvm_set_s2pte_writable(&pte);
>
>                 ret = mmu_topup_memory_cache(&cache, 2, 2);
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index e0d8565..bc001f5 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -60,7 +60,6 @@ pgprot_t pgprot_user;
>  pgprot_t pgprot_kernel;
>  pgprot_t pgprot_hyp_device;
>  pgprot_t pgprot_s2;
> -pgprot_t pgprot_s2_device;
>
>  EXPORT_SYMBOL(pgprot_user);
>  EXPORT_SYMBOL(pgprot_kernel);
> @@ -343,7 +342,7 @@ static void __init build_mem_type_table(void)
>         struct cachepolicy *cp;
>         unsigned int cr = get_cr();
>         pteval_t user_pgprot, kern_pgprot, vecs_pgprot;
> -       pteval_t hyp_device_pgprot, s2_pgprot, s2_device_pgprot;
> +       pteval_t hyp_device_pgprot, s2_pgprot;
>         int cpu_arch = cpu_architecture();
>         int i;
>
> @@ -456,7 +455,7 @@ static void __init build_mem_type_table(void)
>         cp = &cache_policies[cachepolicy];
>         vecs_pgprot = kern_pgprot = user_pgprot = cp->pte;
>         s2_pgprot = cp->pte_s2;
> -       hyp_device_pgprot = s2_device_pgprot = mem_types[MT_DEVICE].prot_pte;
> +       hyp_device_pgprot = mem_types[MT_DEVICE].prot_pte;
>
>         /*
>          * ARMv6 and above have extended page tables.
> @@ -536,7 +535,6 @@ static void __init build_mem_type_table(void)
>         pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG |
>                                  L_PTE_DIRTY | kern_pgprot);
>         pgprot_s2  = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | s2_pgprot);
> -       pgprot_s2_device  = __pgprot(s2_device_pgprot);
>         pgprot_hyp_device  = __pgprot(hyp_device_pgprot);
>
>         mem_types[MT_LOW_VECTORS].prot_l1 |= ecc_mask;
> --
> 1.8.2.3
>
>

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
@ 2013-05-27 20:01     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-27 20:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> At the moment, when mapping a device into Stage-2 for a guest,
> we override whatever the guest uses by forcing a device memory
> type in Stage-2.
>
> While this is not exactly wrong, this isn't really the "spirit" of
> the architecture. The hardware shouldn't have to cope for a broken
> guest mapping to a device as normal memory.
>

So I'm trying to think of a scenario where this feature in the
architecture would actually be useful, and it sounds like from you
guys that it's only useful to properly run a broken guest.

Are we 100% sure that a malicious guest can't leverage this to break
isolation? I'm thinking something along the lines of writing to a
device (for example the gic virtual cpu interface) with a cached
mapping. If such a write is in fact written back to cache, and not
evicted from the cache before a later time, where a different VM is
running, can't that adversely affect the other VM?

Probably this can never happen, but I wasn't able to convince myself
of this from going through the ARM ARM...?

> As such, let's get rid of the memory type overrides, and map
> everything as PAGE_S2 in Stage-2. This simplifies the code and
> let the guest do its own thing, whether it is stupid or not.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/pgtable.h | 2 --
>  arch/arm/kvm/mmu.c             | 2 +-
>  arch/arm/mm/mmu.c              | 6 ++----
>  3 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 9bcd262..c3410a8 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -81,7 +81,6 @@ extern pgprot_t               pgprot_user;
>  extern pgprot_t                pgprot_kernel;
>  extern pgprot_t                pgprot_hyp_device;
>  extern pgprot_t                pgprot_s2;
> -extern pgprot_t                pgprot_s2_device;
>
>  #define _MOD_PROT(p, b)        __pgprot(pgprot_val(p) | (b))
>
> @@ -97,7 +96,6 @@ extern pgprot_t               pgprot_s2_device;
>  #define PAGE_HYP               _MOD_PROT(pgprot_kernel, L_PTE_HYP)
>  #define PAGE_HYP_DEVICE                _MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
>  #define PAGE_S2                        _MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE         _MOD_PROT(pgprot_s2_device, L_PTE_USER | L_PTE_S2_RDONLY)
>
>  #define __PAGE_NONE            __pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
>  #define __PAGE_SHARED          __pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 693d16c..f2a8416 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -495,7 +495,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>         pfn = __phys_to_pfn(pa);
>
>         for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
> -               pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
> +               pte_t pte = pfn_pte(pfn, PAGE_S2);
>                 kvm_set_s2pte_writable(&pte);
>
>                 ret = mmu_topup_memory_cache(&cache, 2, 2);
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index e0d8565..bc001f5 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -60,7 +60,6 @@ pgprot_t pgprot_user;
>  pgprot_t pgprot_kernel;
>  pgprot_t pgprot_hyp_device;
>  pgprot_t pgprot_s2;
> -pgprot_t pgprot_s2_device;
>
>  EXPORT_SYMBOL(pgprot_user);
>  EXPORT_SYMBOL(pgprot_kernel);
> @@ -343,7 +342,7 @@ static void __init build_mem_type_table(void)
>         struct cachepolicy *cp;
>         unsigned int cr = get_cr();
>         pteval_t user_pgprot, kern_pgprot, vecs_pgprot;
> -       pteval_t hyp_device_pgprot, s2_pgprot, s2_device_pgprot;
> +       pteval_t hyp_device_pgprot, s2_pgprot;
>         int cpu_arch = cpu_architecture();
>         int i;
>
> @@ -456,7 +455,7 @@ static void __init build_mem_type_table(void)
>         cp = &cache_policies[cachepolicy];
>         vecs_pgprot = kern_pgprot = user_pgprot = cp->pte;
>         s2_pgprot = cp->pte_s2;
> -       hyp_device_pgprot = s2_device_pgprot = mem_types[MT_DEVICE].prot_pte;
> +       hyp_device_pgprot = mem_types[MT_DEVICE].prot_pte;
>
>         /*
>          * ARMv6 and above have extended page tables.
> @@ -536,7 +535,6 @@ static void __init build_mem_type_table(void)
>         pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG |
>                                  L_PTE_DIRTY | kern_pgprot);
>         pgprot_s2  = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | s2_pgprot);
> -       pgprot_s2_device  = __pgprot(s2_device_pgprot);
>         pgprot_hyp_device  = __pgprot(hyp_device_pgprot);
>
>         mem_types[MT_LOW_VECTORS].prot_l1 |= ecc_mask;
> --
> 1.8.2.3
>
>

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/7] ARM: KVM: be more thorough when invalidating TLBs
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  1:53     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  1:53 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm, Catalin Marinas

On Tue, May 14, 2013 at 12:11:34PM +0100, Marc Zyngier 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/mmu.c | 41 ++++++++++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 9657065..84ba67b 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);
>  }
>  
>  /**
> @@ -675,7 +687,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.3
> 
> 
I think this could optimized by rewriting the handle_hva_to_gpa function
to use unmap_stage2_range for an actual range, but that funciton should
be rewritten to be generic for KVM anyhow.  I'll add it to my todo list.

I'll apply this patch and send it further upstream for an -rc release.

Thanks,
-Chritoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 1/7] ARM: KVM: be more thorough when invalidating TLBs
@ 2013-05-28  1:53     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  1:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:34PM +0100, Marc Zyngier 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/mmu.c | 41 ++++++++++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 9657065..84ba67b 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);
>  }
>  
>  /**
> @@ -675,7 +687,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.3
> 
> 
I think this could optimized by rewriting the handle_hva_to_gpa function
to use unmap_stage2_range for an actual range, but that funciton should
be rewritten to be generic for KVM anyhow.  I'll add it to my todo list.

I'll apply this patch and send it further upstream for an -rc release.

Thanks,
-Chritoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/7] ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  1:54     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  1:54 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm

On Tue, May 14, 2013 at 12:11:35PM +0100, Marc Zyngier wrote:
> __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.3

Applied, thanks.
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 2/7] ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid
@ 2013-05-28  1:54     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  1:54 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:35PM +0100, Marc Zyngier wrote:
> __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.3

Applied, thanks.
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  2:10     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:10 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm, Will Deacon, Catalin Marinas

On Tue, May 14, 2013 at 12:11:36PM +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/include/asm/kvm_mmu.h | 17 +++++++++++------
>  arch/arm/kvm/mmu.c             |  7 ++++---
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 472ac70..8c03c96 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -65,11 +65,6 @@ void kvm_clear_hyp_idmap(void);
>  static inline void kvm_set_pte(pte_t *pte, pte_t new_pte)
>  {
>  	pte_val(*pte) = new_pte;
> -	/*
> -	 * flush_pmd_entry just takes a void pointer and cleans the necessary
> -	 * cache entries, so we can reuse the function for ptes.
> -	 */
> -	flush_pmd_entry(pte);
>  }
>  
>  static inline bool kvm_is_write_fault(unsigned long hsr)
> @@ -83,9 +78,19 @@ static inline bool kvm_is_write_fault(unsigned long hsr)
>  		return true;
>  }
>  
> +static inline void kvm_clean_dcache_area(void *addr, size_t size)
> +{
> +	clean_dcache_area(addr, size);
> +}
> +
> +static inline void kvm_clean_pte_entry(pte_t *pte)
> +{
> +	kvm_clean_dcache_area(pte, sizeof(*pte));
> +}
> +
>  static inline void kvm_clean_pgd(pgd_t *pgd)
>  {
> -	clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
> +	kvm_clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
>  }
>  
>  static inline void kvm_clean_pmd_entry(pmd_t *pmd)
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 84ba67b..451bad3 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -113,6 +113,7 @@ 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));
> +		kvm_clean_pte_entry(pte);
>  		put_page(virt_to_page(pte));
>  		kvm_tlb_flush_vmid_ipa(kvm, addr);
>  	}
> @@ -234,9 +235,10 @@ 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);
> +
> +	kvm_clean_dcache_area((void *)start, end - start);
>  }
>  
>  static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
> @@ -261,7 +263,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 +300,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);
> @@ -469,6 +469,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>  	/* Create 2nd stage page table mapping - Level 3 */
>  	old_pte = *pte;
>  	kvm_set_pte(pte, *new_pte);
> +	kvm_clean_pte_entry(pte);
>  	if (pte_present(old_pte))
>  		kvm_tlb_flush_vmid_ipa(kvm, addr);
>  	else
> -- 
> 1.8.2.3
> 
> 
I don't care for this change, you have three places where you call
kvm_set_pte and immediately follow with kvm_clean_pte_entry now, just to
optimize the uncommon case only relevant when creating new VMs, and at
the same time do things differently from the rest of the kernel with
set_pte functions (yes, I know it's a static in this file, but that
doesn't mean that readers of this file wouldn't make that association).
The next function that calls kvm_set_pte must now also remember to call
the clean functions, bah.

I think the kvm_clean_pte_entry should just be called from within
kvm_set_pte.

-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables
@ 2013-05-28  2:10     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:36PM +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/include/asm/kvm_mmu.h | 17 +++++++++++------
>  arch/arm/kvm/mmu.c             |  7 ++++---
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 472ac70..8c03c96 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -65,11 +65,6 @@ void kvm_clear_hyp_idmap(void);
>  static inline void kvm_set_pte(pte_t *pte, pte_t new_pte)
>  {
>  	pte_val(*pte) = new_pte;
> -	/*
> -	 * flush_pmd_entry just takes a void pointer and cleans the necessary
> -	 * cache entries, so we can reuse the function for ptes.
> -	 */
> -	flush_pmd_entry(pte);
>  }
>  
>  static inline bool kvm_is_write_fault(unsigned long hsr)
> @@ -83,9 +78,19 @@ static inline bool kvm_is_write_fault(unsigned long hsr)
>  		return true;
>  }
>  
> +static inline void kvm_clean_dcache_area(void *addr, size_t size)
> +{
> +	clean_dcache_area(addr, size);
> +}
> +
> +static inline void kvm_clean_pte_entry(pte_t *pte)
> +{
> +	kvm_clean_dcache_area(pte, sizeof(*pte));
> +}
> +
>  static inline void kvm_clean_pgd(pgd_t *pgd)
>  {
> -	clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
> +	kvm_clean_dcache_area(pgd, PTRS_PER_S2_PGD * sizeof(pgd_t));
>  }
>  
>  static inline void kvm_clean_pmd_entry(pmd_t *pmd)
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 84ba67b..451bad3 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -113,6 +113,7 @@ 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));
> +		kvm_clean_pte_entry(pte);
>  		put_page(virt_to_page(pte));
>  		kvm_tlb_flush_vmid_ipa(kvm, addr);
>  	}
> @@ -234,9 +235,10 @@ 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);
> +
> +	kvm_clean_dcache_area((void *)start, end - start);
>  }
>  
>  static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
> @@ -261,7 +263,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 +300,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);
> @@ -469,6 +469,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>  	/* Create 2nd stage page table mapping - Level 3 */
>  	old_pte = *pte;
>  	kvm_set_pte(pte, *new_pte);
> +	kvm_clean_pte_entry(pte);
>  	if (pte_present(old_pte))
>  		kvm_tlb_flush_vmid_ipa(kvm, addr);
>  	else
> -- 
> 1.8.2.3
> 
> 
I don't care for this change, you have three places where you call
kvm_set_pte and immediately follow with kvm_clean_pte_entry now, just to
optimize the uncommon case only relevant when creating new VMs, and at
the same time do things differently from the rest of the kernel with
set_pte functions (yes, I know it's a static in this file, but that
doesn't mean that readers of this file wouldn't make that association).
The next function that calls kvm_set_pte must now also remember to call
the clean functions, bah.

I think the kvm_clean_pte_entry should just be called from within
kvm_set_pte.

-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 4/7] ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  2:11     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:11 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm, Catalin Marinas

On Tue, May 14, 2013 at 12:11:37PM +0100, Marc Zyngier wrote:
> 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 37d216d..327a1fb 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -789,8 +789,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;
> @@ -798,8 +798,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.3
> 
> 
Applied, thanks.

-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 4/7] ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs
@ 2013-05-28  2:11     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:37PM +0100, Marc Zyngier wrote:
> 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 37d216d..327a1fb 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -789,8 +789,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;
> @@ -798,8 +798,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.3
> 
> 
Applied, thanks.

-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/7] ARM: KVM: don't special case PC when doing an MMIO
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  2:11     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:11 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm, Catalin Marinas

On Tue, May 14, 2013 at 12:11:38PM +0100, Marc Zyngier wrote:
> Admitedly, reading a MMIO register to load PC is very weird.
> Writing PC to a MMIO register is probably even worse. But
> the architecture doesn't forbid any of these, and injecting
> a Prefetch Abort is the wrong thing to do anyway.
> 
> Remove this check altogether, and let the adventurous guest
> wander into LaLaLand if they feel compelled to do so.
> 
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h | 5 -----
>  arch/arm/kvm/mmio.c                | 6 ------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 82b4bab..a464e8d 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -65,11 +65,6 @@ static inline bool vcpu_mode_priv(struct kvm_vcpu *vcpu)
>  	return cpsr_mode > USR_MODE;;
>  }
>  
> -static inline bool kvm_vcpu_reg_is_pc(struct kvm_vcpu *vcpu, int reg)
> -{
> -	return reg == 15;
> -}
> -
>  static inline u32 kvm_vcpu_get_hsr(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->arch.fault.hsr;
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 72a12f2..b8e06b7 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -86,12 +86,6 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	sign_extend = kvm_vcpu_dabt_issext(vcpu);
>  	rt = kvm_vcpu_dabt_get_rd(vcpu);
>  
> -	if (kvm_vcpu_reg_is_pc(vcpu, rt)) {
> -		/* IO memory trying to read/write pc */
> -		kvm_inject_pabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> -		return 1;
> -	}
> -
>  	mmio->is_write = is_write;
>  	mmio->phys_addr = fault_ipa;
>  	mmio->len = len;
> -- 
> 1.8.2.3
> 
> 
Applied, thanks.
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 5/7] ARM: KVM: don't special case PC when doing an MMIO
@ 2013-05-28  2:11     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:38PM +0100, Marc Zyngier wrote:
> Admitedly, reading a MMIO register to load PC is very weird.
> Writing PC to a MMIO register is probably even worse. But
> the architecture doesn't forbid any of these, and injecting
> a Prefetch Abort is the wrong thing to do anyway.
> 
> Remove this check altogether, and let the adventurous guest
> wander into LaLaLand if they feel compelled to do so.
> 
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h | 5 -----
>  arch/arm/kvm/mmio.c                | 6 ------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 82b4bab..a464e8d 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -65,11 +65,6 @@ static inline bool vcpu_mode_priv(struct kvm_vcpu *vcpu)
>  	return cpsr_mode > USR_MODE;;
>  }
>  
> -static inline bool kvm_vcpu_reg_is_pc(struct kvm_vcpu *vcpu, int reg)
> -{
> -	return reg == 15;
> -}
> -
>  static inline u32 kvm_vcpu_get_hsr(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->arch.fault.hsr;
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 72a12f2..b8e06b7 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -86,12 +86,6 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	sign_extend = kvm_vcpu_dabt_issext(vcpu);
>  	rt = kvm_vcpu_dabt_get_rd(vcpu);
>  
> -	if (kvm_vcpu_reg_is_pc(vcpu, rt)) {
> -		/* IO memory trying to read/write pc */
> -		kvm_inject_pabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> -		return 1;
> -	}
> -
>  	mmio->is_write = is_write;
>  	mmio->phys_addr = fault_ipa;
>  	mmio->len = len;
> -- 
> 1.8.2.3
> 
> 
Applied, thanks.
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  2:12     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:12 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm

On Tue, May 14, 2013 at 12:11:39PM +0100, Marc Zyngier wrote:
> 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 451bad3..693d16c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -382,9 +382,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.3
> 
> 
Applied, thanks.
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE
@ 2013-05-28  2:12     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:39PM +0100, Marc Zyngier wrote:
> 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 451bad3..693d16c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -382,9 +382,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.3
> 
> 
Applied, thanks.
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE
  2013-05-14 11:11   ` Marc Zyngier
@ 2013-05-28  2:15     ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:15 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvmarm, kvm

On Tue, May 14, 2013 at 12:11:39PM +0100, Marc Zyngier wrote:
> 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 451bad3..693d16c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -382,9 +382,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.3
> 
> 
Thanks for spotting this, applied.

-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE
@ 2013-05-28  2:15     ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28  2:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 14, 2013 at 12:11:39PM +0100, Marc Zyngier wrote:
> 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 451bad3..693d16c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -382,9 +382,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.3
> 
> 
Thanks for spotting this, applied.

-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
  2013-05-27 20:01     ` Christoffer Dall
@ 2013-05-28 10:11       ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-28 10:11 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: linux-arm-kernel, kvmarm, KVM General, Catalin Marinas

On 27/05/13 21:01, Christoffer Dall wrote:
> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> At the moment, when mapping a device into Stage-2 for a guest,
>> we override whatever the guest uses by forcing a device memory
>> type in Stage-2.
>>
>> While this is not exactly wrong, this isn't really the "spirit" of
>> the architecture. The hardware shouldn't have to cope for a broken
>> guest mapping to a device as normal memory.
>>
> 
> So I'm trying to think of a scenario where this feature in the
> architecture would actually be useful, and it sounds like from you
> guys that it's only useful to properly run a broken guest.
> 
> Are we 100% sure that a malicious guest can't leverage this to break
> isolation? I'm thinking something along the lines of writing to a
> device (for example the gic virtual cpu interface) with a cached
> mapping. If such a write is in fact written back to cache, and not
> evicted from the cache before a later time, where a different VM is
> running, can't that adversely affect the other VM?
> 
> Probably this can never happen, but I wasn't able to convince myself
> of this from going through the ARM ARM...?

I think you definitely have a point here, and I completely missed that
case. A shared device (like the GIC virtual CPU interface) must be
forced to a device memory type, otherwise we cannot ensure strict
isolation of guests.

I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
arm64 port.

Thanks,

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


^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
@ 2013-05-28 10:11       ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-28 10:11 UTC (permalink / raw)
  To: linux-arm-kernel

On 27/05/13 21:01, Christoffer Dall wrote:
> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> At the moment, when mapping a device into Stage-2 for a guest,
>> we override whatever the guest uses by forcing a device memory
>> type in Stage-2.
>>
>> While this is not exactly wrong, this isn't really the "spirit" of
>> the architecture. The hardware shouldn't have to cope for a broken
>> guest mapping to a device as normal memory.
>>
> 
> So I'm trying to think of a scenario where this feature in the
> architecture would actually be useful, and it sounds like from you
> guys that it's only useful to properly run a broken guest.
> 
> Are we 100% sure that a malicious guest can't leverage this to break
> isolation? I'm thinking something along the lines of writing to a
> device (for example the gic virtual cpu interface) with a cached
> mapping. If such a write is in fact written back to cache, and not
> evicted from the cache before a later time, where a different VM is
> running, can't that adversely affect the other VM?
> 
> Probably this can never happen, but I wasn't able to convince myself
> of this from going through the ARM ARM...?

I think you definitely have a point here, and I completely missed that
case. A shared device (like the GIC virtual CPU interface) must be
forced to a device memory type, otherwise we cannot ensure strict
isolation of guests.

I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
arm64 port.

Thanks,

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

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
  2013-05-28 10:11       ` Marc Zyngier
@ 2013-05-28 14:16         ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28 14:16 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: Catalin Marinas, kvmarm, linux-arm-kernel, KVM General

On Tue, May 28, 2013 at 3:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 27/05/13 21:01, Christoffer Dall wrote:
>> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> At the moment, when mapping a device into Stage-2 for a guest,
>>> we override whatever the guest uses by forcing a device memory
>>> type in Stage-2.
>>>
>>> While this is not exactly wrong, this isn't really the "spirit" of
>>> the architecture. The hardware shouldn't have to cope for a broken
>>> guest mapping to a device as normal memory.
>>>
>>
>> So I'm trying to think of a scenario where this feature in the
>> architecture would actually be useful, and it sounds like from you
>> guys that it's only useful to properly run a broken guest.
>>
>> Are we 100% sure that a malicious guest can't leverage this to break
>> isolation? I'm thinking something along the lines of writing to a
>> device (for example the gic virtual cpu interface) with a cached
>> mapping. If such a write is in fact written back to cache, and not
>> evicted from the cache before a later time, where a different VM is
>> running, can't that adversely affect the other VM?
>>
>> Probably this can never happen, but I wasn't able to convince myself
>> of this from going through the ARM ARM...?
>
> I think you definitely have a point here, and I completely missed that
> case. A shared device (like the GIC virtual CPU interface) must be
> forced to a device memory type, otherwise we cannot ensure strict
> isolation of guests.
>
> I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
> arm64 port.
>
We still need to get rid of the USER bit in the definition, and since
that's a purely arch/arm/* patch I assume it should go through RMK's
tree. Will you ack the other patch?

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
@ 2013-05-28 14:16         ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28 14:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 28, 2013 at 3:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 27/05/13 21:01, Christoffer Dall wrote:
>> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> At the moment, when mapping a device into Stage-2 for a guest,
>>> we override whatever the guest uses by forcing a device memory
>>> type in Stage-2.
>>>
>>> While this is not exactly wrong, this isn't really the "spirit" of
>>> the architecture. The hardware shouldn't have to cope for a broken
>>> guest mapping to a device as normal memory.
>>>
>>
>> So I'm trying to think of a scenario where this feature in the
>> architecture would actually be useful, and it sounds like from you
>> guys that it's only useful to properly run a broken guest.
>>
>> Are we 100% sure that a malicious guest can't leverage this to break
>> isolation? I'm thinking something along the lines of writing to a
>> device (for example the gic virtual cpu interface) with a cached
>> mapping. If such a write is in fact written back to cache, and not
>> evicted from the cache before a later time, where a different VM is
>> running, can't that adversely affect the other VM?
>>
>> Probably this can never happen, but I wasn't able to convince myself
>> of this from going through the ARM ARM...?
>
> I think you definitely have a point here, and I completely missed that
> case. A shared device (like the GIC virtual CPU interface) must be
> forced to a device memory type, otherwise we cannot ensure strict
> isolation of guests.
>
> I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
> arm64 port.
>
We still need to get rid of the USER bit in the definition, and since
that's a purely arch/arm/* patch I assume it should go through RMK's
tree. Will you ack the other patch?

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
  2013-05-28 14:16         ` Christoffer Dall
@ 2013-05-28 14:25           ` Marc Zyngier
  -1 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-28 14:25 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: Catalin Marinas, kvmarm, linux-arm-kernel, KVM General

On 28/05/13 15:16, Christoffer Dall wrote:
> On Tue, May 28, 2013 at 3:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 27/05/13 21:01, Christoffer Dall wrote:
>>> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> At the moment, when mapping a device into Stage-2 for a guest,
>>>> we override whatever the guest uses by forcing a device memory
>>>> type in Stage-2.
>>>>
>>>> While this is not exactly wrong, this isn't really the "spirit" of
>>>> the architecture. The hardware shouldn't have to cope for a broken
>>>> guest mapping to a device as normal memory.
>>>>
>>>
>>> So I'm trying to think of a scenario where this feature in the
>>> architecture would actually be useful, and it sounds like from you
>>> guys that it's only useful to properly run a broken guest.
>>>
>>> Are we 100% sure that a malicious guest can't leverage this to break
>>> isolation? I'm thinking something along the lines of writing to a
>>> device (for example the gic virtual cpu interface) with a cached
>>> mapping. If such a write is in fact written back to cache, and not
>>> evicted from the cache before a later time, where a different VM is
>>> running, can't that adversely affect the other VM?
>>>
>>> Probably this can never happen, but I wasn't able to convince myself
>>> of this from going through the ARM ARM...?
>>
>> I think you definitely have a point here, and I completely missed that
>> case. A shared device (like the GIC virtual CPU interface) must be
>> forced to a device memory type, otherwise we cannot ensure strict
>> isolation of guests.
>>
>> I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
>> arm64 port.
>>
> We still need to get rid of the USER bit in the definition, and since
> that's a purely arch/arm/* patch I assume it should go through RMK's
> tree. Will you ack the other patch?

Sure. Just also drop the call to kvm_set_s2pte_writable in
kvm_phys_addr_ioremap, which is not required now that PAGE_S2_DEVICE
implies RW. With that, you can add my Ack.


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


^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
@ 2013-05-28 14:25           ` Marc Zyngier
  0 siblings, 0 replies; 44+ messages in thread
From: Marc Zyngier @ 2013-05-28 14:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 28/05/13 15:16, Christoffer Dall wrote:
> On Tue, May 28, 2013 at 3:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 27/05/13 21:01, Christoffer Dall wrote:
>>> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> At the moment, when mapping a device into Stage-2 for a guest,
>>>> we override whatever the guest uses by forcing a device memory
>>>> type in Stage-2.
>>>>
>>>> While this is not exactly wrong, this isn't really the "spirit" of
>>>> the architecture. The hardware shouldn't have to cope for a broken
>>>> guest mapping to a device as normal memory.
>>>>
>>>
>>> So I'm trying to think of a scenario where this feature in the
>>> architecture would actually be useful, and it sounds like from you
>>> guys that it's only useful to properly run a broken guest.
>>>
>>> Are we 100% sure that a malicious guest can't leverage this to break
>>> isolation? I'm thinking something along the lines of writing to a
>>> device (for example the gic virtual cpu interface) with a cached
>>> mapping. If such a write is in fact written back to cache, and not
>>> evicted from the cache before a later time, where a different VM is
>>> running, can't that adversely affect the other VM?
>>>
>>> Probably this can never happen, but I wasn't able to convince myself
>>> of this from going through the ARM ARM...?
>>
>> I think you definitely have a point here, and I completely missed that
>> case. A shared device (like the GIC virtual CPU interface) must be
>> forced to a device memory type, otherwise we cannot ensure strict
>> isolation of guests.
>>
>> I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
>> arm64 port.
>>
> We still need to get rid of the USER bit in the definition, and since
> that's a purely arch/arm/* patch I assume it should go through RMK's
> tree. Will you ack the other patch?

Sure. Just also drop the call to kvm_set_s2pte_writable in
kvm_phys_addr_ioremap, which is not required now that PAGE_S2_DEVICE
implies RW. With that, you can add my Ack.


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

^ permalink raw reply	[flat|nested] 44+ messages in thread
* Re: [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
  2013-05-28 14:25           ` Marc Zyngier
@ 2013-05-28 14:29             ` Christoffer Dall
  -1 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28 14:29 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: Catalin Marinas, kvmarm, linux-arm-kernel, KVM General

cool, thanks.

On Tue, May 28, 2013 at 7:25 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 28/05/13 15:16, Christoffer Dall wrote:
>> On Tue, May 28, 2013 at 3:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 27/05/13 21:01, Christoffer Dall wrote:
>>>> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> At the moment, when mapping a device into Stage-2 for a guest,
>>>>> we override whatever the guest uses by forcing a device memory
>>>>> type in Stage-2.
>>>>>
>>>>> While this is not exactly wrong, this isn't really the "spirit" of
>>>>> the architecture. The hardware shouldn't have to cope for a broken
>>>>> guest mapping to a device as normal memory.
>>>>>
>>>>
>>>> So I'm trying to think of a scenario where this feature in the
>>>> architecture would actually be useful, and it sounds like from you
>>>> guys that it's only useful to properly run a broken guest.
>>>>
>>>> Are we 100% sure that a malicious guest can't leverage this to break
>>>> isolation? I'm thinking something along the lines of writing to a
>>>> device (for example the gic virtual cpu interface) with a cached
>>>> mapping. If such a write is in fact written back to cache, and not
>>>> evicted from the cache before a later time, where a different VM is
>>>> running, can't that adversely affect the other VM?
>>>>
>>>> Probably this can never happen, but I wasn't able to convince myself
>>>> of this from going through the ARM ARM...?
>>>
>>> I think you definitely have a point here, and I completely missed that
>>> case. A shared device (like the GIC virtual CPU interface) must be
>>> forced to a device memory type, otherwise we cannot ensure strict
>>> isolation of guests.
>>>
>>> I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
>>> arm64 port.
>>>
>> We still need to get rid of the USER bit in the definition, and since
>> that's a purely arch/arm/* patch I assume it should go through RMK's
>> tree. Will you ack the other patch?
>
> Sure. Just also drop the call to kvm_set_s2pte_writable in
> kvm_phys_addr_ioremap, which is not required now that PAGE_S2_DEVICE
> implies RW. With that, you can add my Ack.
>
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

^ permalink raw reply	[flat|nested] 44+ messages in thread
* [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE
@ 2013-05-28 14:29             ` Christoffer Dall
  0 siblings, 0 replies; 44+ messages in thread
From: Christoffer Dall @ 2013-05-28 14:29 UTC (permalink / raw)
  To: linux-arm-kernel

cool, thanks.

On Tue, May 28, 2013 at 7:25 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 28/05/13 15:16, Christoffer Dall wrote:
>> On Tue, May 28, 2013 at 3:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 27/05/13 21:01, Christoffer Dall wrote:
>>>> On Tue, May 14, 2013 at 4:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> At the moment, when mapping a device into Stage-2 for a guest,
>>>>> we override whatever the guest uses by forcing a device memory
>>>>> type in Stage-2.
>>>>>
>>>>> While this is not exactly wrong, this isn't really the "spirit" of
>>>>> the architecture. The hardware shouldn't have to cope for a broken
>>>>> guest mapping to a device as normal memory.
>>>>>
>>>>
>>>> So I'm trying to think of a scenario where this feature in the
>>>> architecture would actually be useful, and it sounds like from you
>>>> guys that it's only useful to properly run a broken guest.
>>>>
>>>> Are we 100% sure that a malicious guest can't leverage this to break
>>>> isolation? I'm thinking something along the lines of writing to a
>>>> device (for example the gic virtual cpu interface) with a cached
>>>> mapping. If such a write is in fact written back to cache, and not
>>>> evicted from the cache before a later time, where a different VM is
>>>> running, can't that adversely affect the other VM?
>>>>
>>>> Probably this can never happen, but I wasn't able to convince myself
>>>> of this from going through the ARM ARM...?
>>>
>>> I think you definitely have a point here, and I completely missed that
>>> case. A shared device (like the GIC virtual CPU interface) must be
>>> forced to a device memory type, otherwise we cannot ensure strict
>>> isolation of guests.
>>>
>>> I'll drop this patch from my series and add PAGE_S2_DEVICE back to the
>>> arm64 port.
>>>
>> We still need to get rid of the USER bit in the definition, and since
>> that's a purely arch/arm/* patch I assume it should go through RMK's
>> tree. Will you ack the other patch?
>
> Sure. Just also drop the call to kvm_set_s2pte_writable in
> kvm_phys_addr_ioremap, which is not required now that PAGE_S2_DEVICE
> implies RW. With that, you can add my Ack.
>
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

^ permalink raw reply	[flat|nested] 44+ messages in thread
end of thread, other threads:[~2013-05-28 14:29 UTC | newest]

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-14 11:11 [PATCH v3 0/7] ARM: KVM: various mmu related fixes for 3.10 Marc Zyngier
2013-05-14 11:11 ` Marc Zyngier
2013-05-14 11:11 ` [PATCH v3 1/7] ARM: KVM: be more thorough when invalidating TLBs Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-28  1:53   ` Christoffer Dall
2013-05-28  1:53     ` Christoffer Dall
2013-05-14 11:11 ` [PATCH v3 2/7] ARM: KVM: remove dead prototype for __kvm_tlb_flush_vmid Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-28  1:54   ` Christoffer Dall
2013-05-28  1:54     ` Christoffer Dall
2013-05-14 11:11 ` [PATCH v3 3/7] ARM: KVM: relax cache maintainance when building page tables Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-14 13:05   ` Will Deacon
2013-05-14 13:05     ` Will Deacon
2013-05-28  2:10   ` Christoffer Dall
2013-05-28  2:10     ` Christoffer Dall
2013-05-14 11:11 ` [PATCH v3 4/7] ARM: KVM: use phys_addr_t instead of unsigned long long for HYP PGDs Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-28  2:11   ` Christoffer Dall
2013-05-28  2:11     ` Christoffer Dall
2013-05-14 11:11 ` [PATCH v3 5/7] ARM: KVM: don't special case PC when doing an MMIO Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-28  2:11   ` Christoffer Dall
2013-05-28  2:11     ` Christoffer Dall
2013-05-14 11:11 ` [PATCH v3 6/7] ARM: KVM: get rid of S2_PGD_SIZE Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-28  2:12   ` Christoffer Dall
2013-05-28  2:12     ` Christoffer Dall
2013-05-28  2:15   ` Christoffer Dall
2013-05-28  2:15     ` Christoffer Dall
2013-05-14 11:11 ` [PATCH v3 7/7] ARM: KVM: drop use of PAGE_S2_DEVICE Marc Zyngier
2013-05-14 11:11   ` Marc Zyngier
2013-05-27 20:01   ` Christoffer Dall
2013-05-27 20:01     ` Christoffer Dall
2013-05-28 10:11     ` Marc Zyngier
2013-05-28 10:11       ` Marc Zyngier
2013-05-28 14:16       ` Christoffer Dall
2013-05-28 14:16         ` Christoffer Dall
2013-05-28 14:25         ` Marc Zyngier
2013-05-28 14:25           ` Marc Zyngier
2013-05-28 14:29           ` Christoffer Dall
2013-05-28 14:29             ` Christoffer Dall
2013-05-21 16:07 ` [PATCH v3 0/7] ARM: KVM: various mmu related fixes for 3.10 Catalin Marinas
2013-05-21 16:07   ` Catalin Marinas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.