[RFC PATCH 0/3] arm64: KVM: host cache maintainance when guest caches are off

[RFC PATCH 0/3] arm64: KVM: host cache maintainance when guest caches are off

[Marc Zyngier]

When we run a guest with cache disabled, we don't flush the cache to
the Point of Coherency, hence possibly missing bits of data that have
been written in the cache, but have not yet reached memory.

We also have the opposite issue: when a guest enables its cache,
whatever sits in the cache is suddenly going to become visible,
shadowing whatever the guest has written into RAM.

There are several approaches to these issues:
- Using the DC bit when caches are off: this breaks guests assuming
  caches off while doing DMA operations. Bootloaders, for example.
  It also breaks the I-D coherency.
- Fetch the memory attributes on translation fault, and flush the
  cache while handling the fault. This relies on using the PAR_EL1
  register to obtain the Stage-1 memory attributes, and tends to be
  slow.
- Detecting the translation faults occuring with MMU off (and
  performing a cache clean), and trapping SCTLR_EL1 to detect the
  moment when the guest is turning its caches on (and performing a
  cache invalidation). Trapping of SCTLR_EL1 is then disabled to
  ensure the best performance.

This patch series implements the last solution, only on arm64 for the
time being (I'll add the necessary ARMv7 bits once we reach an
agreement on arm64).

Marc Zyngier (3):
  arm64: KVM: force cache clean on page fault when caches are off
  arm64: KVM: trap VM system registers until MMU and caches are ON
  arm64: KVM: flush VM pages before letting the guest enable caches

 arch/arm/include/asm/kvm_mmu.h   |  4 +--
 arch/arm/kvm/mmu.c               | 76 ++++++++++++++++++++++++++++++++++++++--
 arch/arm64/include/asm/kvm_arm.h |  3 +-
 arch/arm64/include/asm/kvm_mmu.h | 12 ++++---
 arch/arm64/kvm/sys_regs.c        | 61 ++++++++++++++++++++++++++------
 5 files changed, 136 insertions(+), 20 deletions(-)

-- 
1.8.3.4

[RFC PATCH 1/3] arm64: KVM: force cache clean on page fault when caches are off

[Marc Zyngier]

In order for the guest with caches off to observe data written
contained in a given page, we need to make sure that page is
committed to memory, and not just hanging in the cache (as
guest accesses are completely bypassing the cache until it
decides to enable it).

For this purpose, hook into the coherent_icache_guest_page
function and flush the region if the guest SCTLR_EL1
register doesn't show the MMU  and caches as being enabled.
The function also get renamed to coherent_cache_guest_page.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h   |  4 ++--
 arch/arm/kvm/mmu.c               |  4 ++--
 arch/arm64/include/asm/kvm_mmu.h | 11 +++++++----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 77de4a4..f997b9e 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -116,8 +116,8 @@ static inline void kvm_set_s2pmd_writable(pmd_t *pmd)
 
 struct kvm;
 
-static inline void coherent_icache_guest_page(struct kvm *kvm, hva_t hva,
-					      unsigned long size)
+static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
+					     unsigned long size)
 {
 	/*
 	 * If we are going to insert an instruction page and the icache is
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 5809069..415fd63 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -713,7 +713,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			kvm_set_s2pmd_writable(&new_pmd);
 			kvm_set_pfn_dirty(pfn);
 		}
-		coherent_icache_guest_page(kvm, hva & PMD_MASK, PMD_SIZE);
+		coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE);
 		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
 	} else {
 		pte_t new_pte = pfn_pte(pfn, PAGE_S2);
@@ -721,7 +721,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			kvm_set_s2pte_writable(&new_pte);
 			kvm_set_pfn_dirty(pfn);
 		}
-		coherent_icache_guest_page(kvm, hva, PAGE_SIZE);
+		coherent_cache_guest_page(vcpu, hva, PAGE_SIZE);
 		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
 	}
 
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 680f74e..2232dd0 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -106,7 +106,6 @@ static inline bool kvm_is_write_fault(unsigned long esr)
 	return true;
 }
 
-static inline void kvm_clean_dcache_area(void *addr, size_t size) {}
 static inline void kvm_clean_pgd(pgd_t *pgd) {}
 static inline void kvm_clean_pmd_entry(pmd_t *pmd) {}
 static inline void kvm_clean_pte(pte_t *pte) {}
@@ -124,9 +123,14 @@ static inline void kvm_set_s2pmd_writable(pmd_t *pmd)
 
 struct kvm;
 
-static inline void coherent_icache_guest_page(struct kvm *kvm, hva_t hva,
-					      unsigned long size)
+#define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
+
+static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
+					     unsigned long size)
 {
+	if ((vcpu_sys_reg(vcpu, SCTLR_EL1) & 0b101) != 0b101)
+		kvm_flush_dcache_to_poc((void *)hva, size);
+
 	if (!icache_is_aliasing()) {		/* PIPT */
 		flush_icache_range(hva, hva + size);
 	} else if (!icache_is_aivivt()) {	/* non ASID-tagged VIVT */
@@ -135,7 +139,6 @@ static inline void coherent_icache_guest_page(struct kvm *kvm, hva_t hva,
 	}
 }
 
-#define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
 
 #endif /* __ASSEMBLY__ */
 #endif /* __ARM64_KVM_MMU_H__ */
-- 
1.8.3.4

[RFC PATCH 2/3] arm64: KVM: trap VM system registers until MMU and caches are ON

[Marc Zyngier]

In order to be able to detect the point where the guest enables
its MMU and caches, trap all the VM related system registers.

Once we see the guest enabling both the MMU and the caches, we
can go back to a saner mode of operation, which is to leave these
registers in complete control of the guest.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/kvm_arm.h |  3 ++-
 arch/arm64/kvm/sys_regs.c        | 58 ++++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index c98ef47..fd0a651 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -62,6 +62,7 @@
  * RW:		64bit by default, can be overriden for 32bit VMs
  * TAC:		Trap ACTLR
  * TSC:		Trap SMC
+ * TVM:		Trap VM ops (until M+C set in SCTLR_EL1)
  * TSW:		Trap cache operations by set/way
  * TWE:		Trap WFE
  * TWI:		Trap WFI
@@ -74,7 +75,7 @@
  * SWIO:	Turn set/way invalidates into set/way clean+invalidate
  */
 #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
-			 HCR_BSU_IS | HCR_FB | HCR_TAC | \
+			 HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \
 			 HCR_AMO | HCR_IMO | HCR_FMO | \
 			 HCR_SWIO | HCR_TIDCP | HCR_RW)
 #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 02e9d09..5e92b9e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -121,6 +121,42 @@ done:
 }
 
 /*
+ * Generic accessor for VM registers. Only called as long as HCR_TVM
+ * is set.
+ */
+static bool access_vm_reg(struct kvm_vcpu *vcpu,
+			  const struct sys_reg_params *p,
+			  const struct sys_reg_desc *r)
+{
+	BUG_ON(!p->is_write);
+
+	vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
+	return true;
+}
+
+/*
+ * SCTLR_EL1 accessor. Only called as long as HCR_TVM is set.  If the
+ * guest enables the MMU, we stop trapping the VM sys_regs and leave
+ * it in complete control of the caches.
+ */
+static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
+			     const struct sys_reg_params *p,
+			     const struct sys_reg_desc *r)
+{
+	unsigned long val;
+
+	BUG_ON(!p->is_write);
+
+	val = *vcpu_reg(vcpu, p->Rt);
+	vcpu_sys_reg(vcpu, r->reg) = val;
+
+	if ((val & (0b101)) == 0b101)	/* MMU+Caches enabled? */
+		vcpu->arch.hcr_el2 &= ~HCR_TVM;
+
+	return true;
+}
+
+/*
  * We could trap ID_DFR0 and tell the guest we don't support performance
  * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
  * NAKed, so it will read the PMCR anyway.
@@ -185,32 +221,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  NULL, reset_mpidr, MPIDR_EL1 },
 	/* SCTLR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
-	  NULL, reset_val, SCTLR_EL1, 0x00C50078 },
+	  access_sctlr_el1, reset_val, SCTLR_EL1, 0x00C50078 },
 	/* CPACR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b010),
 	  NULL, reset_val, CPACR_EL1, 0 },
 	/* TTBR0_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b000),
-	  NULL, reset_unknown, TTBR0_EL1 },
+	  access_vm_reg, reset_unknown, TTBR0_EL1 },
 	/* TTBR1_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b001),
-	  NULL, reset_unknown, TTBR1_EL1 },
+	  access_vm_reg, reset_unknown, TTBR1_EL1 },
 	/* TCR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b010),
-	  NULL, reset_val, TCR_EL1, 0 },
+	  access_vm_reg, reset_val, TCR_EL1, 0 },
 
 	/* AFSR0_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b000),
-	  NULL, reset_unknown, AFSR0_EL1 },
+	  access_vm_reg, reset_unknown, AFSR0_EL1 },
 	/* AFSR1_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b001),
-	  NULL, reset_unknown, AFSR1_EL1 },
+	  access_vm_reg, reset_unknown, AFSR1_EL1 },
 	/* ESR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0010), Op2(0b000),
-	  NULL, reset_unknown, ESR_EL1 },
+	  access_vm_reg, reset_unknown, ESR_EL1 },
 	/* FAR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0110), CRm(0b0000), Op2(0b000),
-	  NULL, reset_unknown, FAR_EL1 },
+	  access_vm_reg, reset_unknown, FAR_EL1 },
 	/* PAR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0111), CRm(0b0100), Op2(0b000),
 	  NULL, reset_unknown, PAR_EL1 },
@@ -224,17 +260,17 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	/* MAIR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1010), CRm(0b0010), Op2(0b000),
-	  NULL, reset_unknown, MAIR_EL1 },
+	  access_vm_reg, reset_unknown, MAIR_EL1 },
 	/* AMAIR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1010), CRm(0b0011), Op2(0b000),
-	  NULL, reset_amair_el1, AMAIR_EL1 },
+	  access_vm_reg, reset_amair_el1, AMAIR_EL1 },
 
 	/* VBAR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b0000), Op2(0b000),
 	  NULL, reset_val, VBAR_EL1, 0 },
 	/* CONTEXTIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
-	  NULL, reset_val, CONTEXTIDR_EL1, 0 },
+	  access_vm_reg, reset_val, CONTEXTIDR_EL1, 0 },
 	/* TPIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b100),
 	  NULL, reset_unknown, TPIDR_EL1 },
-- 
1.8.3.4

[RFC PATCH 3/3] arm64: KVM: flush VM pages before letting the guest enable caches

[Marc Zyngier]

When the guest runs with caches disabled (like in an early boot
sequence, for example), all the writes are diectly going to RAM,
bypassing the caches altogether.

Once the MMU and caches are enabled, whatever sits in the cache
becomes suddently visible, which isn't what the guest expects.

A way to avoid this potential disaster is to invalidate the cache
when the MMU is being turned on. For this, we hook into the SCTLR_EL1
trapping code, and scan the stage-2 page tables, invalidating the
pages/sections that have already been mapped in.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/kvm/mmu.c               | 72 ++++++++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/kvm_mmu.h |  1 +
 arch/arm64/kvm/sys_regs.c        |  5 ++-
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 415fd63..704c939 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -187,6 +187,78 @@ static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
 	}
 }
 
+void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
+		       unsigned long addr, unsigned long end)
+{
+	pte_t *pte;
+
+	pte = pte_offset_kernel(pmd, addr);
+	do {
+		if (!pte_none(*pte)) {
+			hva_t hva = gfn_to_hva(kvm, addr >> PAGE_SHIFT);
+			kvm_flush_dcache_to_poc((void*)hva, PAGE_SIZE);
+		}
+	} while(pte++, addr += PAGE_SIZE, addr != end);
+}
+
+void stage2_flush_pmds(struct kvm *kvm, pud_t *pud,
+		       unsigned long addr, unsigned long end)
+{
+	pmd_t *pmd;
+	unsigned long next;
+
+	pmd = pmd_offset(pud, addr);
+	do {
+		next = pmd_addr_end(addr, end);
+		if (!pmd_none(*pmd)) {
+			if (kvm_pmd_huge(*pmd)) {
+				hva_t hva = gfn_to_hva(kvm, addr >> PAGE_SHIFT);
+				kvm_flush_dcache_to_poc((void*)hva, PMD_SIZE);
+			} else {
+				stage2_flush_ptes(kvm, pmd, addr, next);
+			}
+		}
+	} while(pmd++, addr = next, addr != end);
+}
+
+void stage2_flush_puds(struct kvm *kvm, pgd_t *pgd,
+		       unsigned long addr, unsigned long end)
+{
+	pud_t *pud;
+	unsigned long next;
+
+	pud = pud_offset(pgd, addr);
+	do {
+		next = pud_addr_end(addr, end);
+		if (!pud_none(*pud)) {
+			if (pud_huge(*pud)) {
+				hva_t hva = gfn_to_hva(kvm, addr >> PAGE_SHIFT);
+				kvm_flush_dcache_to_poc((void*)hva, PUD_SIZE);
+			} else {
+				stage2_flush_pmds(kvm, pud, addr, next);
+			}
+		}
+	} while(pud++, addr = next, addr != end);
+}
+
+void stage2_flush_vm(struct kvm *kvm)
+{
+	unsigned long long addr = 0;
+	unsigned long end = KVM_PHYS_SIZE;
+	unsigned long next;
+	pgd_t *pgd;
+
+	spin_lock(&kvm->mmu_lock);
+
+	pgd = kvm->arch.pgd + pgd_index(addr);
+	do {
+		next = pgd_addr_end(addr, end);
+		stage2_flush_puds(kvm, pgd, addr, next);
+	} while(pgd++, addr = next, addr != end);
+
+	spin_unlock(&kvm->mmu_lock);
+}
+
 /**
  * free_boot_hyp_pgd - free HYP boot page tables
  *
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 2232dd0..b7b2ca3 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -139,6 +139,7 @@ static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
 	}
 }
 
+void stage2_flush_vm(struct kvm *kvm);
 
 #endif /* __ASSEMBLY__ */
 #endif /* __ARM64_KVM_MMU_H__ */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5e92b9e..32e440f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -27,6 +27,7 @@
 #include <asm/kvm_host.h>
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_coproc.h>
+#include <asm/kvm_mmu.h>
 #include <asm/cacheflush.h>
 #include <asm/cputype.h>
 #include <trace/events/kvm.h>
@@ -150,8 +151,10 @@ static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
 	val = *vcpu_reg(vcpu, p->Rt);
 	vcpu_sys_reg(vcpu, r->reg) = val;
 
-	if ((val & (0b101)) == 0b101)	/* MMU+Caches enabled? */
+	if ((val & (0b101)) == 0b101) {	/* MMU+Caches enabled? */
 		vcpu->arch.hcr_el2 &= ~HCR_TVM;
+		stage2_flush_vm(vcpu->kvm);
+	}
 
 	return true;
 }
-- 
1.8.3.4

Re: [RFC PATCH 2/3] arm64: KVM: trap VM system registers until MMU and caches are ON

[Anup Patel]

On Fri, Jan 17, 2014 at 8:33 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> In order to be able to detect the point where the guest enables
> its MMU and caches, trap all the VM related system registers.
>
> Once we see the guest enabling both the MMU and the caches, we
> can go back to a saner mode of operation, which is to leave these
> registers in complete control of the guest.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/include/asm/kvm_arm.h |  3 ++-
>  arch/arm64/kvm/sys_regs.c        | 58 ++++++++++++++++++++++++++++++++--------
>  2 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index c98ef47..fd0a651 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -62,6 +62,7 @@
>   * RW:         64bit by default, can be overriden for 32bit VMs
>   * TAC:                Trap ACTLR
>   * TSC:                Trap SMC
> + * TVM:                Trap VM ops (until M+C set in SCTLR_EL1)
>   * TSW:                Trap cache operations by set/way
>   * TWE:                Trap WFE
>   * TWI:                Trap WFI
> @@ -74,7 +75,7 @@
>   * SWIO:       Turn set/way invalidates into set/way clean+invalidate
>   */
>  #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
> -                        HCR_BSU_IS | HCR_FB | HCR_TAC | \
> +                        HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \
>                          HCR_AMO | HCR_IMO | HCR_FMO | \
>                          HCR_SWIO | HCR_TIDCP | HCR_RW)
>  #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 02e9d09..5e92b9e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -121,6 +121,42 @@ done:
>  }
>
>  /*
> + * Generic accessor for VM registers. Only called as long as HCR_TVM
> + * is set.
> + */
> +static bool access_vm_reg(struct kvm_vcpu *vcpu,
> +                         const struct sys_reg_params *p,
> +                         const struct sys_reg_desc *r)
> +{
> +       BUG_ON(!p->is_write);
> +
> +       vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> +       return true;
> +}
> +
> +/*
> + * SCTLR_EL1 accessor. Only called as long as HCR_TVM is set.  If the
> + * guest enables the MMU, we stop trapping the VM sys_regs and leave
> + * it in complete control of the caches.
> + */
> +static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
> +                            const struct sys_reg_params *p,
> +                            const struct sys_reg_desc *r)
> +{
> +       unsigned long val;
> +
> +       BUG_ON(!p->is_write);
> +
> +       val = *vcpu_reg(vcpu, p->Rt);
> +       vcpu_sys_reg(vcpu, r->reg) = val;
> +
> +       if ((val & (0b101)) == 0b101)   /* MMU+Caches enabled? */
> +               vcpu->arch.hcr_el2 &= ~HCR_TVM;
> +
> +       return true;
> +}
> +
> +/*
>   * We could trap ID_DFR0 and tell the guest we don't support performance
>   * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
>   * NAKed, so it will read the PMCR anyway.
> @@ -185,32 +221,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>           NULL, reset_mpidr, MPIDR_EL1 },
>         /* SCTLR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
> -         NULL, reset_val, SCTLR_EL1, 0x00C50078 },
> +         access_sctlr_el1, reset_val, SCTLR_EL1, 0x00C50078 },

This patch in its current form breaks Aarch32 VMs on Foundation v8 Model
because encoding for Aarch64 VM registers we get Op0=0b11 and for Aarch32
VM registers we get Op0=0b00 when trapped.

Either its a Foundation v8 Model bug or we need to add more enteries in
sys_reg_desc[] for Aarch32 VM registers with Op0=0b00.

>         /* CPACR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b010),
>           NULL, reset_val, CPACR_EL1, 0 },
>         /* TTBR0_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b000),
> -         NULL, reset_unknown, TTBR0_EL1 },
> +         access_vm_reg, reset_unknown, TTBR0_EL1 },
>         /* TTBR1_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b001),
> -         NULL, reset_unknown, TTBR1_EL1 },
> +         access_vm_reg, reset_unknown, TTBR1_EL1 },
>         /* TCR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b010),
> -         NULL, reset_val, TCR_EL1, 0 },
> +         access_vm_reg, reset_val, TCR_EL1, 0 },
>
>         /* AFSR0_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b000),
> -         NULL, reset_unknown, AFSR0_EL1 },
> +         access_vm_reg, reset_unknown, AFSR0_EL1 },
>         /* AFSR1_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b001),
> -         NULL, reset_unknown, AFSR1_EL1 },
> +         access_vm_reg, reset_unknown, AFSR1_EL1 },
>         /* ESR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0010), Op2(0b000),
> -         NULL, reset_unknown, ESR_EL1 },
> +         access_vm_reg, reset_unknown, ESR_EL1 },
>         /* FAR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0110), CRm(0b0000), Op2(0b000),
> -         NULL, reset_unknown, FAR_EL1 },
> +         access_vm_reg, reset_unknown, FAR_EL1 },
>         /* PAR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b0111), CRm(0b0100), Op2(0b000),
>           NULL, reset_unknown, PAR_EL1 },
> @@ -224,17 +260,17 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>
>         /* MAIR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b1010), CRm(0b0010), Op2(0b000),
> -         NULL, reset_unknown, MAIR_EL1 },
> +         access_vm_reg, reset_unknown, MAIR_EL1 },
>         /* AMAIR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b1010), CRm(0b0011), Op2(0b000),
> -         NULL, reset_amair_el1, AMAIR_EL1 },
> +         access_vm_reg, reset_amair_el1, AMAIR_EL1 },
>
>         /* VBAR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b0000), Op2(0b000),
>           NULL, reset_val, VBAR_EL1, 0 },
>         /* CONTEXTIDR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
> -         NULL, reset_val, CONTEXTIDR_EL1, 0 },
> +         access_vm_reg, reset_val, CONTEXTIDR_EL1, 0 },
>         /* TPIDR_EL1 */
>         { Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b100),
>           NULL, reset_unknown, TPIDR_EL1 },
> --
> 1.8.3.4
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Regards,
Anup

Re: [RFC PATCH 2/3] arm64: KVM: trap VM system registers until MMU and caches are ON

[Marc Zyngier]

Hi Anup,

On 20/01/14 12:00, Anup Patel wrote:
> On Fri, Jan 17, 2014 at 8:33 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> In order to be able to detect the point where the guest enables
>> its MMU and caches, trap all the VM related system registers.
>>
>> Once we see the guest enabling both the MMU and the caches, we
>> can go back to a saner mode of operation, which is to leave these
>> registers in complete control of the guest.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_arm.h |  3 ++-
>>  arch/arm64/kvm/sys_regs.c        | 58 ++++++++++++++++++++++++++++++++--------
>>  2 files changed, 49 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index c98ef47..fd0a651 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -62,6 +62,7 @@
>>   * RW:         64bit by default, can be overriden for 32bit VMs
>>   * TAC:                Trap ACTLR
>>   * TSC:                Trap SMC
>> + * TVM:                Trap VM ops (until M+C set in SCTLR_EL1)
>>   * TSW:                Trap cache operations by set/way
>>   * TWE:                Trap WFE
>>   * TWI:                Trap WFI
>> @@ -74,7 +75,7 @@
>>   * SWIO:       Turn set/way invalidates into set/way clean+invalidate
>>   */
>>  #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
>> -                        HCR_BSU_IS | HCR_FB | HCR_TAC | \
>> +                        HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>                          HCR_AMO | HCR_IMO | HCR_FMO | \
>>                          HCR_SWIO | HCR_TIDCP | HCR_RW)
>>  #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 02e9d09..5e92b9e 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -121,6 +121,42 @@ done:
>>  }
>>
>>  /*
>> + * Generic accessor for VM registers. Only called as long as HCR_TVM
>> + * is set.
>> + */
>> +static bool access_vm_reg(struct kvm_vcpu *vcpu,
>> +                         const struct sys_reg_params *p,
>> +                         const struct sys_reg_desc *r)
>> +{
>> +       BUG_ON(!p->is_write);
>> +
>> +       vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
>> +       return true;
>> +}
>> +
>> +/*
>> + * SCTLR_EL1 accessor. Only called as long as HCR_TVM is set.  If the
>> + * guest enables the MMU, we stop trapping the VM sys_regs and leave
>> + * it in complete control of the caches.
>> + */
>> +static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
>> +                            const struct sys_reg_params *p,
>> +                            const struct sys_reg_desc *r)
>> +{
>> +       unsigned long val;
>> +
>> +       BUG_ON(!p->is_write);
>> +
>> +       val = *vcpu_reg(vcpu, p->Rt);
>> +       vcpu_sys_reg(vcpu, r->reg) = val;
>> +
>> +       if ((val & (0b101)) == 0b101)   /* MMU+Caches enabled? */
>> +               vcpu->arch.hcr_el2 &= ~HCR_TVM;
>> +
>> +       return true;
>> +}
>> +
>> +/*
>>   * We could trap ID_DFR0 and tell the guest we don't support performance
>>   * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
>>   * NAKed, so it will read the PMCR anyway.
>> @@ -185,32 +221,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>           NULL, reset_mpidr, MPIDR_EL1 },
>>         /* SCTLR_EL1 */
>>         { Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
>> -         NULL, reset_val, SCTLR_EL1, 0x00C50078 },
>> +         access_sctlr_el1, reset_val, SCTLR_EL1, 0x00C50078 },
> 
> This patch in its current form breaks Aarch32 VMs on Foundation v8 Model
> because encoding for Aarch64 VM registers we get Op0=0b11 and for Aarch32
> VM registers we get Op0=0b00 when trapped.
> 
> Either its a Foundation v8 Model bug or we need to add more enteries in
> sys_reg_desc[] for Aarch32 VM registers with Op0=0b00.

That's a good point. But Op0 isn't defined for AArch32, the value is
simply hardcoded in kvm_handle_cp15_32/kvm_handle_cp15_64, which is
obviously horribly broken.

I'll work on a fix for that, thanks noticing it.

Does this series otherwise fix your L3 cache issue (assuming you stick
to 64bit guests)?

Cheers,

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

Re: [RFC PATCH 2/3] arm64: KVM: trap VM system registers until MMU and caches are ON

[Anup Patel]

Hi Marc,

On Mon, Jan 20, 2014 at 7:11 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Anup,
>
> On 20/01/14 12:00, Anup Patel wrote:
>> On Fri, Jan 17, 2014 at 8:33 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> In order to be able to detect the point where the guest enables
>>> its MMU and caches, trap all the VM related system registers.
>>>
>>> Once we see the guest enabling both the MMU and the caches, we
>>> can go back to a saner mode of operation, which is to leave these
>>> registers in complete control of the guest.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_arm.h |  3 ++-
>>>  arch/arm64/kvm/sys_regs.c        | 58 ++++++++++++++++++++++++++++++++--------
>>>  2 files changed, 49 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>> index c98ef47..fd0a651 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -62,6 +62,7 @@
>>>   * RW:         64bit by default, can be overriden for 32bit VMs
>>>   * TAC:                Trap ACTLR
>>>   * TSC:                Trap SMC
>>> + * TVM:                Trap VM ops (until M+C set in SCTLR_EL1)
>>>   * TSW:                Trap cache operations by set/way
>>>   * TWE:                Trap WFE
>>>   * TWI:                Trap WFI
>>> @@ -74,7 +75,7 @@
>>>   * SWIO:       Turn set/way invalidates into set/way clean+invalidate
>>>   */
>>>  #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
>>> -                        HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>> +                        HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>>                          HCR_AMO | HCR_IMO | HCR_FMO | \
>>>                          HCR_SWIO | HCR_TIDCP | HCR_RW)
>>>  #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 02e9d09..5e92b9e 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -121,6 +121,42 @@ done:
>>>  }
>>>
>>>  /*
>>> + * Generic accessor for VM registers. Only called as long as HCR_TVM
>>> + * is set.
>>> + */
>>> +static bool access_vm_reg(struct kvm_vcpu *vcpu,
>>> +                         const struct sys_reg_params *p,
>>> +                         const struct sys_reg_desc *r)
>>> +{
>>> +       BUG_ON(!p->is_write);
>>> +
>>> +       vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
>>> +       return true;
>>> +}
>>> +
>>> +/*
>>> + * SCTLR_EL1 accessor. Only called as long as HCR_TVM is set.  If the
>>> + * guest enables the MMU, we stop trapping the VM sys_regs and leave
>>> + * it in complete control of the caches.
>>> + */
>>> +static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
>>> +                            const struct sys_reg_params *p,
>>> +                            const struct sys_reg_desc *r)
>>> +{
>>> +       unsigned long val;
>>> +
>>> +       BUG_ON(!p->is_write);
>>> +
>>> +       val = *vcpu_reg(vcpu, p->Rt);
>>> +       vcpu_sys_reg(vcpu, r->reg) = val;
>>> +
>>> +       if ((val & (0b101)) == 0b101)   /* MMU+Caches enabled? */
>>> +               vcpu->arch.hcr_el2 &= ~HCR_TVM;
>>> +
>>> +       return true;
>>> +}
>>> +
>>> +/*
>>>   * We could trap ID_DFR0 and tell the guest we don't support performance
>>>   * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
>>>   * NAKed, so it will read the PMCR anyway.
>>> @@ -185,32 +221,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>           NULL, reset_mpidr, MPIDR_EL1 },
>>>         /* SCTLR_EL1 */
>>>         { Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
>>> -         NULL, reset_val, SCTLR_EL1, 0x00C50078 },
>>> +         access_sctlr_el1, reset_val, SCTLR_EL1, 0x00C50078 },
>>
>> This patch in its current form breaks Aarch32 VMs on Foundation v8 Model
>> because encoding for Aarch64 VM registers we get Op0=0b11 and for Aarch32
>> VM registers we get Op0=0b00 when trapped.
>>
>> Either its a Foundation v8 Model bug or we need to add more enteries in
>> sys_reg_desc[] for Aarch32 VM registers with Op0=0b00.
>
> That's a good point. But Op0 isn't defined for AArch32, the value is
> simply hardcoded in kvm_handle_cp15_32/kvm_handle_cp15_64, which is
> obviously horribly broken.
>
> I'll work on a fix for that, thanks noticing it.
>
> Does this series otherwise fix your L3 cache issue (assuming you stick
> to 64bit guests)?

Just started trying your patches today.
First tried on Foundation v8 Model.
Next we will try on X-Gene.

Me or Pranav will soon provide more feedback in this regard.

>
> Cheers,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Thanks,
Anup

Re: [RFC PATCH 2/3] arm64: KVM: trap VM system registers until MMU and caches are ON

[Pranavkumar Sawargaonkar]

Hi Marc,

On 20 January 2014 22:00, Anup Patel <anup@brainfault.org> wrote:
> Hi Marc,
>
> On Mon, Jan 20, 2014 at 7:11 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> Hi Anup,
>>
>> On 20/01/14 12:00, Anup Patel wrote:
>>> On Fri, Jan 17, 2014 at 8:33 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> In order to be able to detect the point where the guest enables
>>>> its MMU and caches, trap all the VM related system registers.
>>>>
>>>> Once we see the guest enabling both the MMU and the caches, we
>>>> can go back to a saner mode of operation, which is to leave these
>>>> registers in complete control of the guest.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_arm.h |  3 ++-
>>>>  arch/arm64/kvm/sys_regs.c        | 58 ++++++++++++++++++++++++++++++++--------
>>>>  2 files changed, 49 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>>> index c98ef47..fd0a651 100644
>>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>>> @@ -62,6 +62,7 @@
>>>>   * RW:         64bit by default, can be overriden for 32bit VMs
>>>>   * TAC:                Trap ACTLR
>>>>   * TSC:                Trap SMC
>>>> + * TVM:                Trap VM ops (until M+C set in SCTLR_EL1)
>>>>   * TSW:                Trap cache operations by set/way
>>>>   * TWE:                Trap WFE
>>>>   * TWI:                Trap WFI
>>>> @@ -74,7 +75,7 @@
>>>>   * SWIO:       Turn set/way invalidates into set/way clean+invalidate
>>>>   */
>>>>  #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
>>>> -                        HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>>> +                        HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>>>                          HCR_AMO | HCR_IMO | HCR_FMO | \
>>>>                          HCR_SWIO | HCR_TIDCP | HCR_RW)
>>>>  #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>> index 02e9d09..5e92b9e 100644
>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>> @@ -121,6 +121,42 @@ done:
>>>>  }
>>>>
>>>>  /*
>>>> + * Generic accessor for VM registers. Only called as long as HCR_TVM
>>>> + * is set.
>>>> + */
>>>> +static bool access_vm_reg(struct kvm_vcpu *vcpu,
>>>> +                         const struct sys_reg_params *p,
>>>> +                         const struct sys_reg_desc *r)
>>>> +{
>>>> +       BUG_ON(!p->is_write);
>>>> +
>>>> +       vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
>>>> +       return true;
>>>> +}
>>>> +
>>>> +/*
>>>> + * SCTLR_EL1 accessor. Only called as long as HCR_TVM is set.  If the
>>>> + * guest enables the MMU, we stop trapping the VM sys_regs and leave
>>>> + * it in complete control of the caches.
>>>> + */
>>>> +static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
>>>> +                            const struct sys_reg_params *p,
>>>> +                            const struct sys_reg_desc *r)
>>>> +{
>>>> +       unsigned long val;
>>>> +
>>>> +       BUG_ON(!p->is_write);
>>>> +
>>>> +       val = *vcpu_reg(vcpu, p->Rt);
>>>> +       vcpu_sys_reg(vcpu, r->reg) = val;
>>>> +
>>>> +       if ((val & (0b101)) == 0b101)   /* MMU+Caches enabled? */
>>>> +               vcpu->arch.hcr_el2 &= ~HCR_TVM;
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>> +/*
>>>>   * We could trap ID_DFR0 and tell the guest we don't support performance
>>>>   * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
>>>>   * NAKed, so it will read the PMCR anyway.
>>>> @@ -185,32 +221,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>           NULL, reset_mpidr, MPIDR_EL1 },
>>>>         /* SCTLR_EL1 */
>>>>         { Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
>>>> -         NULL, reset_val, SCTLR_EL1, 0x00C50078 },
>>>> +         access_sctlr_el1, reset_val, SCTLR_EL1, 0x00C50078 },
>>>
>>> This patch in its current form breaks Aarch32 VMs on Foundation v8 Model
>>> because encoding for Aarch64 VM registers we get Op0=0b11 and for Aarch32
>>> VM registers we get Op0=0b00 when trapped.
>>>
>>> Either its a Foundation v8 Model bug or we need to add more enteries in
>>> sys_reg_desc[] for Aarch32 VM registers with Op0=0b00.
>>
>> That's a good point. But Op0 isn't defined for AArch32, the value is
>> simply hardcoded in kvm_handle_cp15_32/kvm_handle_cp15_64, which is
>> obviously horribly broken.
>>
>> I'll work on a fix for that, thanks noticing it.
>>
>> Does this series otherwise fix your L3 cache issue (assuming you stick
>> to 64bit guests)?
>
> Just started trying your patches today.
> First tried on Foundation v8 Model.
> Next we will try on X-Gene.
>
> Me or Pranav will soon provide more feedback in this regard.
>

Tested this patch with kvmtool on xgene, works fine !!

>>
>> Cheers,
>>
>>         M.
>> --
>> Jazz is not dead. It just smells funny...
Thanks,
Pranav

>
> Thanks,
> Anup
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Re: [RFC PATCH 2/3] arm64: KVM: trap VM system registers until MMU and caches are ON

[Marc Zyngier]

Hi Pranav,

On 21/01/14 12:17, Pranavkumar Sawargaonkar wrote:
> Hi Marc,
> 
> On 20 January 2014 22:00, Anup Patel <anup@brainfault.org> wrote:
>> Hi Marc,
>>
>> On Mon, Jan 20, 2014 at 7:11 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> Hi Anup,
>>>
>>> On 20/01/14 12:00, Anup Patel wrote:
>>>> On Fri, Jan 17, 2014 at 8:33 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> In order to be able to detect the point where the guest enables
>>>>> its MMU and caches, trap all the VM related system registers.
>>>>>
>>>>> Once we see the guest enabling both the MMU and the caches, we
>>>>> can go back to a saner mode of operation, which is to leave these
>>>>> registers in complete control of the guest.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_arm.h |  3 ++-
>>>>>  arch/arm64/kvm/sys_regs.c        | 58 ++++++++++++++++++++++++++++++++--------
>>>>>  2 files changed, 49 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>>>> index c98ef47..fd0a651 100644
>>>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>>>> @@ -62,6 +62,7 @@
>>>>>   * RW:         64bit by default, can be overriden for 32bit VMs
>>>>>   * TAC:                Trap ACTLR
>>>>>   * TSC:                Trap SMC
>>>>> + * TVM:                Trap VM ops (until M+C set in SCTLR_EL1)
>>>>>   * TSW:                Trap cache operations by set/way
>>>>>   * TWE:                Trap WFE
>>>>>   * TWI:                Trap WFI
>>>>> @@ -74,7 +75,7 @@
>>>>>   * SWIO:       Turn set/way invalidates into set/way clean+invalidate
>>>>>   */
>>>>>  #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
>>>>> -                        HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>>>> +                        HCR_TVM | HCR_BSU_IS | HCR_FB | HCR_TAC | \
>>>>>                          HCR_AMO | HCR_IMO | HCR_FMO | \
>>>>>                          HCR_SWIO | HCR_TIDCP | HCR_RW)
>>>>>  #define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
>>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>>> index 02e9d09..5e92b9e 100644
>>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>>> @@ -121,6 +121,42 @@ done:
>>>>>  }
>>>>>
>>>>>  /*
>>>>> + * Generic accessor for VM registers. Only called as long as HCR_TVM
>>>>> + * is set.
>>>>> + */
>>>>> +static bool access_vm_reg(struct kvm_vcpu *vcpu,
>>>>> +                         const struct sys_reg_params *p,
>>>>> +                         const struct sys_reg_desc *r)
>>>>> +{
>>>>> +       BUG_ON(!p->is_write);
>>>>> +
>>>>> +       vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
>>>>> +       return true;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * SCTLR_EL1 accessor. Only called as long as HCR_TVM is set.  If the
>>>>> + * guest enables the MMU, we stop trapping the VM sys_regs and leave
>>>>> + * it in complete control of the caches.
>>>>> + */
>>>>> +static bool access_sctlr_el1(struct kvm_vcpu *vcpu,
>>>>> +                            const struct sys_reg_params *p,
>>>>> +                            const struct sys_reg_desc *r)
>>>>> +{
>>>>> +       unsigned long val;
>>>>> +
>>>>> +       BUG_ON(!p->is_write);
>>>>> +
>>>>> +       val = *vcpu_reg(vcpu, p->Rt);
>>>>> +       vcpu_sys_reg(vcpu, r->reg) = val;
>>>>> +
>>>>> +       if ((val & (0b101)) == 0b101)   /* MMU+Caches enabled? */
>>>>> +               vcpu->arch.hcr_el2 &= ~HCR_TVM;
>>>>> +
>>>>> +       return true;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>>   * We could trap ID_DFR0 and tell the guest we don't support performance
>>>>>   * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
>>>>>   * NAKed, so it will read the PMCR anyway.
>>>>> @@ -185,32 +221,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>>           NULL, reset_mpidr, MPIDR_EL1 },
>>>>>         /* SCTLR_EL1 */
>>>>>         { Op0(0b11), Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b000),
>>>>> -         NULL, reset_val, SCTLR_EL1, 0x00C50078 },
>>>>> +         access_sctlr_el1, reset_val, SCTLR_EL1, 0x00C50078 },
>>>>
>>>> This patch in its current form breaks Aarch32 VMs on Foundation v8 Model
>>>> because encoding for Aarch64 VM registers we get Op0=0b11 and for Aarch32
>>>> VM registers we get Op0=0b00 when trapped.
>>>>
>>>> Either its a Foundation v8 Model bug or we need to add more enteries in
>>>> sys_reg_desc[] for Aarch32 VM registers with Op0=0b00.
>>>
>>> That's a good point. But Op0 isn't defined for AArch32, the value is
>>> simply hardcoded in kvm_handle_cp15_32/kvm_handle_cp15_64, which is
>>> obviously horribly broken.
>>>
>>> I'll work on a fix for that, thanks noticing it.
>>>
>>> Does this series otherwise fix your L3 cache issue (assuming you stick
>>> to 64bit guests)?
>>
>> Just started trying your patches today.
>> First tried on Foundation v8 Model.
>> Next we will try on X-Gene.
>>
>> Me or Pranav will soon provide more feedback in this regard.
>>
> 
> Tested this patch with kvmtool on xgene, works fine !!

Excellent, thanks for testing. I'll repost a new patch series with the
32bit stuff later today or tomorrow.

Cheers,

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