[PATCH v3 0/6] KVM/arm64: Cache maintenance relaxations

[PATCH v3 0/6] KVM/arm64: Cache maintenance relaxations

[Marc Zyngier]

This small series makes use of features recently introduced in the
ARMv8 architecture to relax the cache maintenance operations on CPUs
that implement these features.

FWB is the most important one. It allows stage-2 to enforce the
cacheability of memory, no matter what the guest says. It also
mandates that the whole machine is cache coherent (no non-coherent
I/O), meaning we can drop a whole class of cache maintenance
operations.

FWB also has the same effect as CTR_EL0.IDC being set, and when
combined with CTR_EL0.DIC allows the removal of all cache maintenance
and tracking of pages being executed.

We also take the opportunity to drop a few useless CMOs that were
applied to the HYP page tables, but that were never necessary. This
ended up requiring quite a few changes in out page table accessors so
that they get consistent barriers. These barriers are a bit on the
heavy side, and could be further optimized, although that's probably
for a different patch series

* From v1:
  - Reordered the series in a slightly more consistent order
  - Added patches refactoring the PT accessors before dropping
    the CMOs
  - Removed final dsb(ishst) from the last patch, as all PT accessors
    now have their own barriers
  - Collected Acks from Catalin

* From v2:
  - Dropped the clean to PoU when FWB, as it always implies IDC
  - Added comments about the pertinence of Set/Way CMOs in guests
  - Collected Acks and RBs

Marc Zyngier (6):
  arm64: KVM: Add support for Stage-2 control of memory types and
    cacheability
  arm64: KVM: Handle Set/Way CMOs as NOPs if FWB is present
  arm64: KVM: Avoid marking pages as XN in Stage-2 if CTR_EL0.DIC is set
  KVM: arm/arm64: Consolidate page-table accessors
  KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate
  KVM: arm/arm64: Remove unnecessary CMOs when creating HYP page tables

 arch/arm/include/asm/kvm_mmu.h        | 14 ++-------
 arch/arm64/include/asm/cpucaps.h      |  3 +-
 arch/arm64/include/asm/kvm_arm.h      |  1 +
 arch/arm64/include/asm/kvm_emulate.h  |  2 ++
 arch/arm64/include/asm/kvm_mmu.h      | 34 +++++++++++++++-----
 arch/arm64/include/asm/memory.h       |  7 +++++
 arch/arm64/include/asm/pgtable-prot.h | 24 ++++++++++++--
 arch/arm64/include/asm/sysreg.h       |  1 +
 arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++
 arch/arm64/kvm/sys_regs.c             | 11 ++++++-
 virt/kvm/arm/mmu.c                    | 45 ++++++++++++++++++++++-----
 11 files changed, 131 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH v3 0/6] KVM/arm64: Cache maintenance relaxations

[Marc Zyngier]

This small series makes use of features recently introduced in the
ARMv8 architecture to relax the cache maintenance operations on CPUs
that implement these features.

FWB is the most important one. It allows stage-2 to enforce the
cacheability of memory, no matter what the guest says. It also
mandates that the whole machine is cache coherent (no non-coherent
I/O), meaning we can drop a whole class of cache maintenance
operations.

FWB also has the same effect as CTR_EL0.IDC being set, and when
combined with CTR_EL0.DIC allows the removal of all cache maintenance
and tracking of pages being executed.

We also take the opportunity to drop a few useless CMOs that were
applied to the HYP page tables, but that were never necessary. This
ended up requiring quite a few changes in out page table accessors so
that they get consistent barriers. These barriers are a bit on the
heavy side, and could be further optimized, although that's probably
for a different patch series

* From v1:
  - Reordered the series in a slightly more consistent order
  - Added patches refactoring the PT accessors before dropping
    the CMOs
  - Removed final dsb(ishst) from the last patch, as all PT accessors
    now have their own barriers
  - Collected Acks from Catalin

* From v2:
  - Dropped the clean to PoU when FWB, as it always implies IDC
  - Added comments about the pertinence of Set/Way CMOs in guests
  - Collected Acks and RBs

Marc Zyngier (6):
  arm64: KVM: Add support for Stage-2 control of memory types and
    cacheability
  arm64: KVM: Handle Set/Way CMOs as NOPs if FWB is present
  arm64: KVM: Avoid marking pages as XN in Stage-2 if CTR_EL0.DIC is set
  KVM: arm/arm64: Consolidate page-table accessors
  KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate
  KVM: arm/arm64: Remove unnecessary CMOs when creating HYP page tables

 arch/arm/include/asm/kvm_mmu.h        | 14 ++-------
 arch/arm64/include/asm/cpucaps.h      |  3 +-
 arch/arm64/include/asm/kvm_arm.h      |  1 +
 arch/arm64/include/asm/kvm_emulate.h  |  2 ++
 arch/arm64/include/asm/kvm_mmu.h      | 34 +++++++++++++++-----
 arch/arm64/include/asm/memory.h       |  7 +++++
 arch/arm64/include/asm/pgtable-prot.h | 24 ++++++++++++--
 arch/arm64/include/asm/sysreg.h       |  1 +
 arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++
 arch/arm64/kvm/sys_regs.c             | 11 ++++++-
 virt/kvm/arm/mmu.c                    | 45 ++++++++++++++++++++++-----
 11 files changed, 131 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
results in the strongest attribute of the two stages.  This means
that the hypervisor has to perform quite a lot of cache maintenance
just in case the guest has some non-cacheable mappings around.

ARMv8.4 solves this problem by offering a different mode (FWB) where
Stage-2 has total control over the memory attribute (this is limited
to systems where both I/O and instruction fetches are coherent with
the dcache). This is achieved by having a different set of memory
attributes in the page tables, and a new bit set in HCR_EL2.

On such a system, we can then safely sidestep any form of dcache
management.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/cpucaps.h      |  3 ++-
 arch/arm64/include/asm/kvm_arm.h      |  1 +
 arch/arm64/include/asm/kvm_emulate.h  |  2 ++
 arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
 arch/arm64/include/asm/memory.h       |  7 +++++++
 arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
 arch/arm64/include/asm/sysreg.h       |  1 +
 arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
 virt/kvm/arm/mmu.c                    |  4 ++++
 9 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 8a699c708fc9..ed84d6536830 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -49,7 +49,8 @@
 #define ARM64_HAS_CACHE_DIC			28
 #define ARM64_HW_DBM				29
 #define ARM64_SSBD				30
+#define ARM64_HAS_STAGE2_FWB			31
 
-#define ARM64_NCAPS				31
+#define ARM64_NCAPS				32
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 6dd285e979c9..aa45df752a16 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -23,6 +23,7 @@
 #include <asm/types.h>
 
 /* Hyp Configuration Register (HCR) bits */
+#define HCR_FWB		(UL(1) << 46)
 #define HCR_TEA		(UL(1) << 37)
 #define HCR_TERR	(UL(1) << 36)
 #define HCR_TLOR	(UL(1) << 35)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 1dab3a984608..dd98fdf33d99 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 		/* trap error record accesses */
 		vcpu->arch.hcr_el2 |= HCR_TERR;
 	}
+	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		vcpu->arch.hcr_el2 |= HCR_FWB;
 
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index fb9a7127bb75..620eb9e06bd8 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
 struct kvm;
 
 #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
+#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
 
 static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 {
@@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
 {
 	void *va = page_address(pfn_to_page(pfn));
 
+	/*
+	 * FWB implies IDC, so cache clean to PoC is not required in
+	 * this case.
+	 */
+	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		return;
+
 	kvm_flush_dcache_to_poc(va, size);
 }
 
@@ -287,20 +295,26 @@ static inline void __invalidate_icache_guest_page(kvm_pfn_t pfn,
 
 static inline void __kvm_flush_dcache_pte(pte_t pte)
 {
-	struct page *page = pte_page(pte);
-	kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
+		struct page *page = pte_page(pte);
+		kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
+	}
 }
 
 static inline void __kvm_flush_dcache_pmd(pmd_t pmd)
 {
-	struct page *page = pmd_page(pmd);
-	kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
+		struct page *page = pmd_page(pmd);
+		kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
+	}
 }
 
 static inline void __kvm_flush_dcache_pud(pud_t pud)
 {
-	struct page *page = pud_page(pud);
-	kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
+		struct page *page = pud_page(pud);
+		kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
+	}
 }
 
 #define kvm_virt_to_phys(x)		__pa_symbol(x)
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 49d99214f43c..b96442960aea 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -155,6 +155,13 @@
 #define MT_S2_NORMAL		0xf
 #define MT_S2_DEVICE_nGnRE	0x1
 
+/*
+ * Memory types for Stage-2 translation when ID_AA64MMFR2_EL1.FWB is 0001
+ * Stage-2 enforces Normal-WB and Device-nGnRE
+ */
+#define MT_S2_FWB_NORMAL	6
+#define MT_S2_FWB_DEVICE_nGnRE	1
+
 #ifdef CONFIG_ARM64_4K_PAGES
 #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
 #else
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 108ecad7acc5..c66c3047400e 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -67,8 +67,18 @@
 #define PAGE_HYP_RO		__pgprot(_HYP_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
 #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
 
-#define PAGE_S2			__pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
-#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
+#define PAGE_S2_MEMATTR(attr)						\
+	({								\
+		u64 __val;						\
+		if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))		\
+			__val = PTE_S2_MEMATTR(MT_S2_FWB_ ## attr);	\
+		else							\
+			__val = PTE_S2_MEMATTR(MT_S2_ ## attr);		\
+		__val;							\
+	 })
+
+#define PAGE_S2			__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
+#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
 
 #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
 #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index a8f84812c6e8..98af0b37fb31 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -576,6 +576,7 @@
 #define ID_AA64MMFR1_VMIDBITS_16	2
 
 /* id_aa64mmfr2 */
+#define ID_AA64MMFR2_FWB_SHIFT		40
 #define ID_AA64MMFR2_AT_SHIFT		32
 #define ID_AA64MMFR2_LVA_SHIFT		16
 #define ID_AA64MMFR2_IESB_SHIFT		12
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index f24892a40d2c..d58d1f0abe16 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -192,6 +192,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
 };
 
 static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
@@ -1026,6 +1027,14 @@ static void cpu_copy_el2regs(const struct arm64_cpu_capabilities *__unused)
 }
 #endif
 
+static void cpu_has_fwb(const struct arm64_cpu_capabilities *__unused)
+{
+	u64 val = read_sysreg_s(SYS_CLIDR_EL1);
+
+	/* Check that CLIDR_EL1.LOU{U,IS} are both 0 */
+	WARN_ON(val & (7 << 27 | 7 << 21));
+}
+
 static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		.desc = "GIC system register CPU interface",
@@ -1182,6 +1191,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
 		.matches = has_cache_dic,
 	},
+	{
+		.desc = "Stage-2 Force Write-Back",
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.capability = ARM64_HAS_STAGE2_FWB,
+		.sys_reg = SYS_ID_AA64MMFR2_EL1,
+		.sign = FTR_UNSIGNED,
+		.field_pos = ID_AA64MMFR2_FWB_SHIFT,
+		.min_field_value = 1,
+		.matches = has_cpuid_feature,
+		.cpu_enable = cpu_has_fwb,
+	},
 #ifdef CONFIG_ARM64_HW_AFDBM
 	{
 		/*
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 1d90d79706bd..ea7314296ad1 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -196,6 +196,10 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
  * This is why right after unmapping a page/section and invalidating
  * the corresponding TLBs, we call kvm_flush_dcache_p*() to make sure
  * the IO subsystem will never hit in the cache.
+ *
+ * This is all avoided on systems that have ARM64_HAS_STAGE2_FWB, as
+ * we then fully enforce cacheability of RAM, no matter what the guest
+ * does.
  */
 static void unmap_stage2_ptes(struct kvm *kvm, pmd_t *pmd,
 		       phys_addr_t addr, phys_addr_t end)
-- 
2.17.1

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
results in the strongest attribute of the two stages.  This means
that the hypervisor has to perform quite a lot of cache maintenance
just in case the guest has some non-cacheable mappings around.

ARMv8.4 solves this problem by offering a different mode (FWB) where
Stage-2 has total control over the memory attribute (this is limited
to systems where both I/O and instruction fetches are coherent with
the dcache). This is achieved by having a different set of memory
attributes in the page tables, and a new bit set in HCR_EL2.

On such a system, we can then safely sidestep any form of dcache
management.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/cpucaps.h      |  3 ++-
 arch/arm64/include/asm/kvm_arm.h      |  1 +
 arch/arm64/include/asm/kvm_emulate.h  |  2 ++
 arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
 arch/arm64/include/asm/memory.h       |  7 +++++++
 arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
 arch/arm64/include/asm/sysreg.h       |  1 +
 arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
 virt/kvm/arm/mmu.c                    |  4 ++++
 9 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 8a699c708fc9..ed84d6536830 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -49,7 +49,8 @@
 #define ARM64_HAS_CACHE_DIC			28
 #define ARM64_HW_DBM				29
 #define ARM64_SSBD				30
+#define ARM64_HAS_STAGE2_FWB			31
 
-#define ARM64_NCAPS				31
+#define ARM64_NCAPS				32
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 6dd285e979c9..aa45df752a16 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -23,6 +23,7 @@
 #include <asm/types.h>
 
 /* Hyp Configuration Register (HCR) bits */
+#define HCR_FWB		(UL(1) << 46)
 #define HCR_TEA		(UL(1) << 37)
 #define HCR_TERR	(UL(1) << 36)
 #define HCR_TLOR	(UL(1) << 35)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 1dab3a984608..dd98fdf33d99 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 		/* trap error record accesses */
 		vcpu->arch.hcr_el2 |= HCR_TERR;
 	}
+	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		vcpu->arch.hcr_el2 |= HCR_FWB;
 
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index fb9a7127bb75..620eb9e06bd8 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
 struct kvm;
 
 #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
+#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
 
 static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 {
@@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
 {
 	void *va = page_address(pfn_to_page(pfn));
 
+	/*
+	 * FWB implies IDC, so cache clean to PoC is not required in
+	 * this case.
+	 */
+	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		return;
+
 	kvm_flush_dcache_to_poc(va, size);
 }
 
@@ -287,20 +295,26 @@ static inline void __invalidate_icache_guest_page(kvm_pfn_t pfn,
 
 static inline void __kvm_flush_dcache_pte(pte_t pte)
 {
-	struct page *page = pte_page(pte);
-	kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
+		struct page *page = pte_page(pte);
+		kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
+	}
 }
 
 static inline void __kvm_flush_dcache_pmd(pmd_t pmd)
 {
-	struct page *page = pmd_page(pmd);
-	kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
+		struct page *page = pmd_page(pmd);
+		kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
+	}
 }
 
 static inline void __kvm_flush_dcache_pud(pud_t pud)
 {
-	struct page *page = pud_page(pud);
-	kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
+		struct page *page = pud_page(pud);
+		kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
+	}
 }
 
 #define kvm_virt_to_phys(x)		__pa_symbol(x)
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 49d99214f43c..b96442960aea 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -155,6 +155,13 @@
 #define MT_S2_NORMAL		0xf
 #define MT_S2_DEVICE_nGnRE	0x1
 
+/*
+ * Memory types for Stage-2 translation when ID_AA64MMFR2_EL1.FWB is 0001
+ * Stage-2 enforces Normal-WB and Device-nGnRE
+ */
+#define MT_S2_FWB_NORMAL	6
+#define MT_S2_FWB_DEVICE_nGnRE	1
+
 #ifdef CONFIG_ARM64_4K_PAGES
 #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
 #else
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 108ecad7acc5..c66c3047400e 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -67,8 +67,18 @@
 #define PAGE_HYP_RO		__pgprot(_HYP_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
 #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
 
-#define PAGE_S2			__pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
-#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
+#define PAGE_S2_MEMATTR(attr)						\
+	({								\
+		u64 __val;						\
+		if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))		\
+			__val = PTE_S2_MEMATTR(MT_S2_FWB_ ## attr);	\
+		else							\
+			__val = PTE_S2_MEMATTR(MT_S2_ ## attr);		\
+		__val;							\
+	 })
+
+#define PAGE_S2			__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
+#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
 
 #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
 #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index a8f84812c6e8..98af0b37fb31 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -576,6 +576,7 @@
 #define ID_AA64MMFR1_VMIDBITS_16	2
 
 /* id_aa64mmfr2 */
+#define ID_AA64MMFR2_FWB_SHIFT		40
 #define ID_AA64MMFR2_AT_SHIFT		32
 #define ID_AA64MMFR2_LVA_SHIFT		16
 #define ID_AA64MMFR2_IESB_SHIFT		12
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index f24892a40d2c..d58d1f0abe16 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -192,6 +192,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
 };
 
 static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
@@ -1026,6 +1027,14 @@ static void cpu_copy_el2regs(const struct arm64_cpu_capabilities *__unused)
 }
 #endif
 
+static void cpu_has_fwb(const struct arm64_cpu_capabilities *__unused)
+{
+	u64 val = read_sysreg_s(SYS_CLIDR_EL1);
+
+	/* Check that CLIDR_EL1.LOU{U,IS} are both 0 */
+	WARN_ON(val & (7 << 27 | 7 << 21));
+}
+
 static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		.desc = "GIC system register CPU interface",
@@ -1182,6 +1191,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
 		.matches = has_cache_dic,
 	},
+	{
+		.desc = "Stage-2 Force Write-Back",
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.capability = ARM64_HAS_STAGE2_FWB,
+		.sys_reg = SYS_ID_AA64MMFR2_EL1,
+		.sign = FTR_UNSIGNED,
+		.field_pos = ID_AA64MMFR2_FWB_SHIFT,
+		.min_field_value = 1,
+		.matches = has_cpuid_feature,
+		.cpu_enable = cpu_has_fwb,
+	},
 #ifdef CONFIG_ARM64_HW_AFDBM
 	{
 		/*
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 1d90d79706bd..ea7314296ad1 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -196,6 +196,10 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
  * This is why right after unmapping a page/section and invalidating
  * the corresponding TLBs, we call kvm_flush_dcache_p*() to make sure
  * the IO subsystem will never hit in the cache.
+ *
+ * This is all avoided on systems that have ARM64_HAS_STAGE2_FWB, as
+ * we then fully enforce cacheability of RAM, no matter what the guest
+ * does.
  */
 static void unmap_stage2_ptes(struct kvm *kvm, pmd_t *pmd,
 		       phys_addr_t addr, phys_addr_t end)
-- 
2.17.1

[PATCH v3 2/6] arm64: KVM: Handle Set/Way CMOs as NOPs if FWB is present

[Marc Zyngier]

Set/Way handling is one of the ugliest corners of KVM. We shouldn't
have to handle that, but better safe than sorry.

Thankfully, FWB fixes this for us by not requiering any maintenance
(the guest is forced to use cacheable memory, no matter what it says,
and the whole system is garanteed to be cache coherent), which means
we don't have to emulate S/W CMOs, and don't have to track VM ops either.

We still have to trap S/W though, if only to prevent the guest from
doing something bad.

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a4363735d3f8..774d72155904 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -194,7 +194,16 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	kvm_set_way_flush(vcpu);
+	/*
+	 * Only track S/W ops if we don't have FWB. It still indicates
+	 * that the guest is a bit broken (S/W operations should only
+	 * be done by firmware, knowing that there is only a single
+	 * CPU left in the system, and certainly not from non-secure
+	 * software).
+	 */
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		kvm_set_way_flush(vcpu);
+
 	return true;
 }
 
-- 
2.17.1

[PATCH v3 2/6] arm64: KVM: Handle Set/Way CMOs as NOPs if FWB is present

[Marc Zyngier]

Set/Way handling is one of the ugliest corners of KVM. We shouldn't
have to handle that, but better safe than sorry.

Thankfully, FWB fixes this for us by not requiering any maintenance
(the guest is forced to use cacheable memory, no matter what it says,
and the whole system is garanteed to be cache coherent), which means
we don't have to emulate S/W CMOs, and don't have to track VM ops either.

We still have to trap S/W though, if only to prevent the guest from
doing something bad.

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a4363735d3f8..774d72155904 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -194,7 +194,16 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	kvm_set_way_flush(vcpu);
+	/*
+	 * Only track S/W ops if we don't have FWB. It still indicates
+	 * that the guest is a bit broken (S/W operations should only
+	 * be done by firmware, knowing that there is only a single
+	 * CPU left in the system, and certainly not from non-secure
+	 * software).
+	 */
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		kvm_set_way_flush(vcpu);
+
 	return true;
 }
 
-- 
2.17.1

[PATCH v3 3/6] arm64: KVM: Avoid marking pages as XN in Stage-2 if CTR_EL0.DIC is set

[Marc Zyngier]

On systems where CTR_EL0.DIC is set, we don't need to perform
icache invalidation to guarantee that we'll fetch the right
instruction stream.

This also means that taking a permission fault to invalidate the
icache is an unnecessary overhead.

On such systems, we can safely leave the page as being executable.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index c66c3047400e..78b942c1bea4 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -77,8 +77,18 @@
 		__val;							\
 	 })
 
-#define PAGE_S2			__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
-#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
+#define PAGE_S2_XN							\
+	({								\
+		u64 __val;						\
+		if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC))		\
+			__val = 0;					\
+		else							\
+			__val = PTE_S2_XN;				\
+		__val;							\
+	})
+
+#define PAGE_S2			__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PAGE_S2_XN)
+#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PAGE_S2_XN)
 
 #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
 #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
-- 
2.17.1

[PATCH v3 3/6] arm64: KVM: Avoid marking pages as XN in Stage-2 if CTR_EL0.DIC is set

[Marc Zyngier]

On systems where CTR_EL0.DIC is set, we don't need to perform
icache invalidation to guarantee that we'll fetch the right
instruction stream.

This also means that taking a permission fault to invalidate the
icache is an unnecessary overhead.

On such systems, we can safely leave the page as being executable.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index c66c3047400e..78b942c1bea4 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -77,8 +77,18 @@
 		__val;							\
 	 })
 
-#define PAGE_S2			__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
-#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
+#define PAGE_S2_XN							\
+	({								\
+		u64 __val;						\
+		if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC))		\
+			__val = 0;					\
+		else							\
+			__val = PTE_S2_XN;				\
+		__val;							\
+	})
+
+#define PAGE_S2			__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PAGE_S2_XN)
+#define PAGE_S2_DEVICE		__pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PAGE_S2_XN)
 
 #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
 #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
-- 
2.17.1

[PATCH v3 4/6] KVM: arm/arm64: Consolidate page-table accessors

[Marc Zyngier]

The arm and arm64 KVM page tables accessors are pointlessly different
between the two architectures, and likely both wrong one way or another:
arm64 lacks a dsb(), and arm doesn't use WRITE_ONCE.

Let's unify them.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h   | 12 -----------
 arch/arm64/include/asm/kvm_mmu.h |  3 ---
 virt/kvm/arm/mmu.c               | 35 ++++++++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 8553d68b7c8a..b2feaea1434c 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -75,18 +75,6 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
-static inline void kvm_set_pmd(pmd_t *pmd, pmd_t new_pmd)
-{
-	*pmd = new_pmd;
-	dsb(ishst);
-}
-
-static inline void kvm_set_pte(pte_t *pte, pte_t new_pte)
-{
-	*pte = new_pte;
-	dsb(ishst);
-}
-
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= L_PTE_S2_RDWR;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 620eb9e06bd8..25c9a91f6a87 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -169,9 +169,6 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
-#define	kvm_set_pte(ptep, pte)		set_pte(ptep, pte)
-#define	kvm_set_pmd(pmdp, pmd)		set_pmd(pmdp, pmd)
-
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= PTE_S2_RDWR;
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index ea7314296ad1..87d5c91b9b6a 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -177,6 +177,33 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
 	put_page(virt_to_page(pmd));
 }
 
+static inline void kvm_set_pte(pte_t *ptep, pte_t new_pte)
+{
+	WRITE_ONCE(*ptep, new_pte);
+	dsb(ishst);
+}
+
+static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd)
+{
+	WRITE_ONCE(*pmdp, new_pmd);
+	dsb(ishst);
+}
+
+static inline void kvm_pmd_populate(pmd_t *pmdp, pte_t *ptep)
+{
+	pmd_populate_kernel(NULL, pmdp, ptep);
+}
+
+static inline void kvm_pud_populate(pud_t *pudp, pmd_t *pmdp)
+{
+	pud_populate(NULL, pudp, pmdp);
+}
+
+static inline void kvm_pgd_populate(pgd_t *pgdp, pud_t *pudp)
+{
+	pgd_populate(NULL, pgdp, pudp);
+}
+
 /*
  * Unmapping vs dcache management:
  *
@@ -605,7 +632,7 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
 				kvm_err("Cannot allocate Hyp pte\n");
 				return -ENOMEM;
 			}
-			pmd_populate_kernel(NULL, pmd, pte);
+			kvm_pmd_populate(pmd, pte);
 			get_page(virt_to_page(pmd));
 			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
 		}
@@ -638,7 +665,7 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
 				kvm_err("Cannot allocate Hyp pmd\n");
 				return -ENOMEM;
 			}
-			pud_populate(NULL, pud, pmd);
+			kvm_pud_populate(pud, pmd);
 			get_page(virt_to_page(pud));
 			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
 		}
@@ -675,7 +702,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
 				err = -ENOMEM;
 				goto out;
 			}
-			pgd_populate(NULL, pgd, pud);
+			kvm_pgd_populate(pgd, pud);
 			get_page(virt_to_page(pgd));
 			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
 		}
@@ -1094,7 +1121,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
 		if (!cache)
 			return 0; /* ignore calls from kvm_set_spte_hva */
 		pte = mmu_memory_cache_alloc(cache);
-		pmd_populate_kernel(NULL, pmd, pte);
+		kvm_pmd_populate(pmd, pte);
 		get_page(virt_to_page(pmd));
 	}
 
-- 
2.17.1

[PATCH v3 4/6] KVM: arm/arm64: Consolidate page-table accessors

[Marc Zyngier]

The arm and arm64 KVM page tables accessors are pointlessly different
between the two architectures, and likely both wrong one way or another:
arm64 lacks a dsb(), and arm doesn't use WRITE_ONCE.

Let's unify them.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h   | 12 -----------
 arch/arm64/include/asm/kvm_mmu.h |  3 ---
 virt/kvm/arm/mmu.c               | 35 ++++++++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 8553d68b7c8a..b2feaea1434c 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -75,18 +75,6 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
-static inline void kvm_set_pmd(pmd_t *pmd, pmd_t new_pmd)
-{
-	*pmd = new_pmd;
-	dsb(ishst);
-}
-
-static inline void kvm_set_pte(pte_t *pte, pte_t new_pte)
-{
-	*pte = new_pte;
-	dsb(ishst);
-}
-
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= L_PTE_S2_RDWR;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 620eb9e06bd8..25c9a91f6a87 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -169,9 +169,6 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
-#define	kvm_set_pte(ptep, pte)		set_pte(ptep, pte)
-#define	kvm_set_pmd(pmdp, pmd)		set_pmd(pmdp, pmd)
-
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= PTE_S2_RDWR;
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index ea7314296ad1..87d5c91b9b6a 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -177,6 +177,33 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
 	put_page(virt_to_page(pmd));
 }
 
+static inline void kvm_set_pte(pte_t *ptep, pte_t new_pte)
+{
+	WRITE_ONCE(*ptep, new_pte);
+	dsb(ishst);
+}
+
+static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd)
+{
+	WRITE_ONCE(*pmdp, new_pmd);
+	dsb(ishst);
+}
+
+static inline void kvm_pmd_populate(pmd_t *pmdp, pte_t *ptep)
+{
+	pmd_populate_kernel(NULL, pmdp, ptep);
+}
+
+static inline void kvm_pud_populate(pud_t *pudp, pmd_t *pmdp)
+{
+	pud_populate(NULL, pudp, pmdp);
+}
+
+static inline void kvm_pgd_populate(pgd_t *pgdp, pud_t *pudp)
+{
+	pgd_populate(NULL, pgdp, pudp);
+}
+
 /*
  * Unmapping vs dcache management:
  *
@@ -605,7 +632,7 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
 				kvm_err("Cannot allocate Hyp pte\n");
 				return -ENOMEM;
 			}
-			pmd_populate_kernel(NULL, pmd, pte);
+			kvm_pmd_populate(pmd, pte);
 			get_page(virt_to_page(pmd));
 			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
 		}
@@ -638,7 +665,7 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
 				kvm_err("Cannot allocate Hyp pmd\n");
 				return -ENOMEM;
 			}
-			pud_populate(NULL, pud, pmd);
+			kvm_pud_populate(pud, pmd);
 			get_page(virt_to_page(pud));
 			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
 		}
@@ -675,7 +702,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
 				err = -ENOMEM;
 				goto out;
 			}
-			pgd_populate(NULL, pgd, pud);
+			kvm_pgd_populate(pgd, pud);
 			get_page(virt_to_page(pgd));
 			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
 		}
@@ -1094,7 +1121,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
 		if (!cache)
 			return 0; /* ignore calls from kvm_set_spte_hva */
 		pte = mmu_memory_cache_alloc(cache);
-		pmd_populate_kernel(NULL, pmd, pte);
+		kvm_pmd_populate(pmd, pte);
 		get_page(virt_to_page(pmd));
 	}
 
-- 
2.17.1

[PATCH v3 5/6] KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate

[Marc Zyngier]

The {pmd,pud,pgd}_populate accessors usage in the kernel have always
been a bit weird in KVM. We don't have a struct mm to pass (and
neither does the kernel most of the time, but still...), and
the 32bit code has all kind of cache maintenance that doesn't make
sense on ARMv7+ when MP extensions are mandatory (which is the
case when the VEs are present).

Let's bite the bullet and provide our own implementations. The
only bit of architectural code left has to do with building the table
entry itself (arm64 having up to 52bit PA, arm lacking PUD level).

Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h   | 4 ++++
 arch/arm64/include/asm/kvm_mmu.h | 7 +++++++
 virt/kvm/arm/mmu.c               | 8 +++++---
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index b2feaea1434c..265ea9cf7df7 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -75,6 +75,10 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
+#define kvm_mk_pmd(ptep)	__pmd(__pa(ptep) | PMD_TYPE_TABLE)
+#define kvm_mk_pud(pmdp)	__pud(__pa(pmdp) | PMD_TYPE_TABLE)
+#define kvm_mk_pgd(pudp)	({ BUILD_BUG(); 0; })
+
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= L_PTE_S2_RDWR;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 25c9a91f6a87..2f05be2bed63 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -169,6 +169,13 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
+#define kvm_mk_pmd(ptep)					\
+	__pmd(__phys_to_pmd_val(__pa(ptep) | PMD_TYPE_TABLE))
+#define kvm_mk_pud(pmdp)					\
+	__pud(__phys_to_pud_val(__pa(pmdp) | PMD_TYPE_TABLE))
+#define kvm_mk_pgd(pudp)					\
+	__pgd(__phys_to_pgd_val(__pa(pudp) | PUD_TYPE_TABLE))
+
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= PTE_S2_RDWR;
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 87d5c91b9b6a..eade30caaa3c 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -191,17 +191,19 @@ static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd)
 
 static inline void kvm_pmd_populate(pmd_t *pmdp, pte_t *ptep)
 {
-	pmd_populate_kernel(NULL, pmdp, ptep);
+	kvm_set_pmd(pmdp, kvm_mk_pmd(ptep));
 }
 
 static inline void kvm_pud_populate(pud_t *pudp, pmd_t *pmdp)
 {
-	pud_populate(NULL, pudp, pmdp);
+	WRITE_ONCE(*pudp, kvm_mk_pud(pmdp));
+	dsb(ishst);
 }
 
 static inline void kvm_pgd_populate(pgd_t *pgdp, pud_t *pudp)
 {
-	pgd_populate(NULL, pgdp, pudp);
+	WRITE_ONCE(*pgdp, kvm_mk_pgd(pudp));
+	dsb(ishst);
 }
 
 /*
-- 
2.17.1

[PATCH v3 5/6] KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate

[Marc Zyngier]

The {pmd,pud,pgd}_populate accessors usage in the kernel have always
been a bit weird in KVM. We don't have a struct mm to pass (and
neither does the kernel most of the time, but still...), and
the 32bit code has all kind of cache maintenance that doesn't make
sense on ARMv7+ when MP extensions are mandatory (which is the
case when the VEs are present).

Let's bite the bullet and provide our own implementations. The
only bit of architectural code left has to do with building the table
entry itself (arm64 having up to 52bit PA, arm lacking PUD level).

Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_mmu.h   | 4 ++++
 arch/arm64/include/asm/kvm_mmu.h | 7 +++++++
 virt/kvm/arm/mmu.c               | 8 +++++---
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index b2feaea1434c..265ea9cf7df7 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -75,6 +75,10 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
+#define kvm_mk_pmd(ptep)	__pmd(__pa(ptep) | PMD_TYPE_TABLE)
+#define kvm_mk_pud(pmdp)	__pud(__pa(pmdp) | PMD_TYPE_TABLE)
+#define kvm_mk_pgd(pudp)	({ BUILD_BUG(); 0; })
+
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= L_PTE_S2_RDWR;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 25c9a91f6a87..2f05be2bed63 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -169,6 +169,13 @@ phys_addr_t kvm_get_idmap_vector(void);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 
+#define kvm_mk_pmd(ptep)					\
+	__pmd(__phys_to_pmd_val(__pa(ptep) | PMD_TYPE_TABLE))
+#define kvm_mk_pud(pmdp)					\
+	__pud(__phys_to_pud_val(__pa(pmdp) | PMD_TYPE_TABLE))
+#define kvm_mk_pgd(pudp)					\
+	__pgd(__phys_to_pgd_val(__pa(pudp) | PUD_TYPE_TABLE))
+
 static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
 {
 	pte_val(pte) |= PTE_S2_RDWR;
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 87d5c91b9b6a..eade30caaa3c 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -191,17 +191,19 @@ static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd)
 
 static inline void kvm_pmd_populate(pmd_t *pmdp, pte_t *ptep)
 {
-	pmd_populate_kernel(NULL, pmdp, ptep);
+	kvm_set_pmd(pmdp, kvm_mk_pmd(ptep));
 }
 
 static inline void kvm_pud_populate(pud_t *pudp, pmd_t *pmdp)
 {
-	pud_populate(NULL, pudp, pmdp);
+	WRITE_ONCE(*pudp, kvm_mk_pud(pmdp));
+	dsb(ishst);
 }
 
 static inline void kvm_pgd_populate(pgd_t *pgdp, pud_t *pudp)
 {
-	pgd_populate(NULL, pgdp, pudp);
+	WRITE_ONCE(*pgdp, kvm_mk_pgd(pudp));
+	dsb(ishst);
 }
 
 /*
-- 
2.17.1

[PATCH v3 6/6] KVM: arm/arm64: Remove unnecessary CMOs when creating HYP page tables

[Marc Zyngier]

There is no need to perform cache maintenance operations when
creating the HYP page tables if we have the multiprocessing
extensions. ARMv7 mandates them with the virtualization support,
and ARMv8 just mandates them unconditionally.

Let's remove these operations.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/mmu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index eade30caaa3c..97d27cd9c654 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -609,7 +609,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start,
 		pte = pte_offset_kernel(pmd, addr);
 		kvm_set_pte(pte, pfn_pte(pfn, prot));
 		get_page(virt_to_page(pte));
-		kvm_flush_dcache_to_poc(pte, sizeof(*pte));
 		pfn++;
 	} while (addr += PAGE_SIZE, addr != end);
 }
@@ -636,7 +635,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
 			}
 			kvm_pmd_populate(pmd, pte);
 			get_page(virt_to_page(pmd));
-			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
 		}
 
 		next = pmd_addr_end(addr, end);
@@ -669,7 +667,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
 			}
 			kvm_pud_populate(pud, pmd);
 			get_page(virt_to_page(pud));
-			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
 		}
 
 		next = pud_addr_end(addr, end);
@@ -706,7 +703,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
 			}
 			kvm_pgd_populate(pgd, pud);
 			get_page(virt_to_page(pgd));
-			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
 		}
 
 		next = pgd_addr_end(addr, end);
-- 
2.17.1

[PATCH v3 6/6] KVM: arm/arm64: Remove unnecessary CMOs when creating HYP page tables

[Marc Zyngier]

There is no need to perform cache maintenance operations when
creating the HYP page tables if we have the multiprocessing
extensions. ARMv7 mandates them with the virtualization support,
and ARMv8 just mandates them unconditionally.

Let's remove these operations.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/mmu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index eade30caaa3c..97d27cd9c654 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -609,7 +609,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start,
 		pte = pte_offset_kernel(pmd, addr);
 		kvm_set_pte(pte, pfn_pte(pfn, prot));
 		get_page(virt_to_page(pte));
-		kvm_flush_dcache_to_poc(pte, sizeof(*pte));
 		pfn++;
 	} while (addr += PAGE_SIZE, addr != end);
 }
@@ -636,7 +635,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
 			}
 			kvm_pmd_populate(pmd, pte);
 			get_page(virt_to_page(pmd));
-			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
 		}
 
 		next = pmd_addr_end(addr, end);
@@ -669,7 +667,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
 			}
 			kvm_pud_populate(pud, pmd);
 			get_page(virt_to_page(pud));
-			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
 		}
 
 		next = pud_addr_end(addr, end);
@@ -706,7 +703,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
 			}
 			kvm_pgd_populate(pgd, pud);
 			get_page(virt_to_page(pgd));
-			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
 		}
 
 		next = pgd_addr_end(addr, end);
-- 
2.17.1

Re: [PATCH v3 5/6] KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate

[Suzuki K Poulose]

Hi Marc,

On 27/06/18 13:20, Marc Zyngier wrote:
> The {pmd,pud,pgd}_populate accessors usage in the kernel have always
> been a bit weird in KVM. We don't have a struct mm to pass (and
> neither does the kernel most of the time, but still...), and
> the 32bit code has all kind of cache maintenance that doesn't make
> sense on ARMv7+ when MP extensions are mandatory (which is the
> case when the VEs are present).
> 
> Let's bite the bullet and provide our own implementations. The
> only bit of architectural code left has to do with building the table
> entry itself (arm64 having up to 52bit PA, arm lacking PUD level).
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm/include/asm/kvm_mmu.h   | 4 ++++
>   arch/arm64/include/asm/kvm_mmu.h | 7 +++++++
>   virt/kvm/arm/mmu.c               | 8 +++++---
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index b2feaea1434c..265ea9cf7df7 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -75,6 +75,10 @@ phys_addr_t kvm_get_idmap_vector(void);
>   int kvm_mmu_init(void);
>   void kvm_clear_hyp_idmap(void);
>   
> +#define kvm_mk_pmd(ptep)	__pmd(__pa(ptep) | PMD_TYPE_TABLE)
> +#define kvm_mk_pud(pmdp)	__pud(__pa(pmdp) | PMD_TYPE_TABLE)
> +#define kvm_mk_pgd(pudp)	({ BUILD_BUG(); 0; })
> +
>   static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
>   {
>   	pte_val(pte) |= L_PTE_S2_RDWR;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 25c9a91f6a87..2f05be2bed63 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -169,6 +169,13 @@ phys_addr_t kvm_get_idmap_vector(void);
>   int kvm_mmu_init(void);
>   void kvm_clear_hyp_idmap(void);
>   
> +#define kvm_mk_pmd(ptep)					\
> +	__pmd(__phys_to_pmd_val(__pa(ptep) | PMD_TYPE_TABLE))
> +#define kvm_mk_pud(pmdp)					\
> +	__pud(__phys_to_pud_val(__pa(pmdp) | PMD_TYPE_TABLE))
> +#define kvm_mk_pgd(pudp)					\
> +	__pgd(__phys_to_pgd_val(__pa(pudp) | PUD_TYPE_TABLE))
> +

I believe this is wrong, as the __phys_to_p.d_val could strip of the
TABLE bit. The correct usage is :

__pXd(__phys_to_pXd_val(__pa(ptr)) | PxD_TYPE_TABLE)



Thanks
Suzuki

[PATCH v3 5/6] KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate

[Suzuki K Poulose]

Hi Marc,

On 27/06/18 13:20, Marc Zyngier wrote:
> The {pmd,pud,pgd}_populate accessors usage in the kernel have always
> been a bit weird in KVM. We don't have a struct mm to pass (and
> neither does the kernel most of the time, but still...), and
> the 32bit code has all kind of cache maintenance that doesn't make
> sense on ARMv7+ when MP extensions are mandatory (which is the
> case when the VEs are present).
> 
> Let's bite the bullet and provide our own implementations. The
> only bit of architectural code left has to do with building the table
> entry itself (arm64 having up to 52bit PA, arm lacking PUD level).
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm/include/asm/kvm_mmu.h   | 4 ++++
>   arch/arm64/include/asm/kvm_mmu.h | 7 +++++++
>   virt/kvm/arm/mmu.c               | 8 +++++---
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index b2feaea1434c..265ea9cf7df7 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -75,6 +75,10 @@ phys_addr_t kvm_get_idmap_vector(void);
>   int kvm_mmu_init(void);
>   void kvm_clear_hyp_idmap(void);
>   
> +#define kvm_mk_pmd(ptep)	__pmd(__pa(ptep) | PMD_TYPE_TABLE)
> +#define kvm_mk_pud(pmdp)	__pud(__pa(pmdp) | PMD_TYPE_TABLE)
> +#define kvm_mk_pgd(pudp)	({ BUILD_BUG(); 0; })
> +
>   static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
>   {
>   	pte_val(pte) |= L_PTE_S2_RDWR;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 25c9a91f6a87..2f05be2bed63 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -169,6 +169,13 @@ phys_addr_t kvm_get_idmap_vector(void);
>   int kvm_mmu_init(void);
>   void kvm_clear_hyp_idmap(void);
>   
> +#define kvm_mk_pmd(ptep)					\
> +	__pmd(__phys_to_pmd_val(__pa(ptep) | PMD_TYPE_TABLE))
> +#define kvm_mk_pud(pmdp)					\
> +	__pud(__phys_to_pud_val(__pa(pmdp) | PMD_TYPE_TABLE))
> +#define kvm_mk_pgd(pudp)					\
> +	__pgd(__phys_to_pgd_val(__pa(pudp) | PUD_TYPE_TABLE))
> +

I believe this is wrong, as the __phys_to_p.d_val could strip of the
TABLE bit. The correct usage is :

__pXd(__phys_to_pXd_val(__pa(ptr)) | PxD_TYPE_TABLE)



Thanks
Suzuki

Re: [PATCH v3 4/6] KVM: arm/arm64: Consolidate page-table accessors

[Suzuki K Poulose]

Marc,

On 27/06/18 13:20, Marc Zyngier wrote:
> The arm and arm64 KVM page tables accessors are pointlessly different
> between the two architectures, and likely both wrong one way or another:
> arm64 lacks a dsb(), and arm doesn't use WRITE_ONCE.
> 
> Let's unify them.

nit: Feel free to ignore. The changes introducing kvm_p.d_populate
is not mentioned in the commit description and they do look a bit
disconnected from the change described above.

> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>


> +static inline void kvm_pmd_populate(pmd_t *pmdp, pte_t *ptep)
> +{
> +	pmd_populate_kernel(NULL, pmdp, ptep);
> +}
> +
> +static inline void kvm_pud_populate(pud_t *pudp, pmd_t *pmdp)
> +{
> +	pud_populate(NULL, pudp, pmdp);
> +}
> +
> +static inline void kvm_pgd_populate(pgd_t *pgdp, pud_t *pudp)
> +{
> +	pgd_populate(NULL, pgdp, pudp);
> +}
> +

>   /*
>    * Unmapping vs dcache management:
>    *
> @@ -605,7 +632,7 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
>   				kvm_err("Cannot allocate Hyp pte\n");
>   				return -ENOMEM;
>   			}
> -			pmd_populate_kernel(NULL, pmd, pte);
> +			kvm_pmd_populate(pmd, pte);
>   			get_page(virt_to_page(pmd));
>   			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
>   		}
> @@ -638,7 +665,7 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
>   				kvm_err("Cannot allocate Hyp pmd\n");
>   				return -ENOMEM;
>   			}
> -			pud_populate(NULL, pud, pmd);
> +			kvm_pud_populate(pud, pmd);
>   			get_page(virt_to_page(pud));
>   			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
>   		}
> @@ -675,7 +702,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
>   				err = -ENOMEM;
>   				goto out;
>   			}
> -			pgd_populate(NULL, pgd, pud);
> +			kvm_pgd_populate(pgd, pud);
>   			get_page(virt_to_page(pgd));
>   			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
>   		}
> @@ -1094,7 +1121,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>   		if (!cache)
>   			return 0; /* ignore calls from kvm_set_spte_hva */
>   		pte = mmu_memory_cache_alloc(cache);
> -		pmd_populate_kernel(NULL, pmd, pte);
> +		kvm_pmd_populate(pmd, pte);
>   		get_page(virt_to_page(pmd));
>   	}
>   
> 


Suzuki

[PATCH v3 4/6] KVM: arm/arm64: Consolidate page-table accessors

[Suzuki K Poulose]

Marc,

On 27/06/18 13:20, Marc Zyngier wrote:
> The arm and arm64 KVM page tables accessors are pointlessly different
> between the two architectures, and likely both wrong one way or another:
> arm64 lacks a dsb(), and arm doesn't use WRITE_ONCE.
> 
> Let's unify them.

nit: Feel free to ignore. The changes introducing kvm_p.d_populate
is not mentioned in the commit description and they do look a bit
disconnected from the change described above.

> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>


> +static inline void kvm_pmd_populate(pmd_t *pmdp, pte_t *ptep)
> +{
> +	pmd_populate_kernel(NULL, pmdp, ptep);
> +}
> +
> +static inline void kvm_pud_populate(pud_t *pudp, pmd_t *pmdp)
> +{
> +	pud_populate(NULL, pudp, pmdp);
> +}
> +
> +static inline void kvm_pgd_populate(pgd_t *pgdp, pud_t *pudp)
> +{
> +	pgd_populate(NULL, pgdp, pudp);
> +}
> +

>   /*
>    * Unmapping vs dcache management:
>    *
> @@ -605,7 +632,7 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
>   				kvm_err("Cannot allocate Hyp pte\n");
>   				return -ENOMEM;
>   			}
> -			pmd_populate_kernel(NULL, pmd, pte);
> +			kvm_pmd_populate(pmd, pte);
>   			get_page(virt_to_page(pmd));
>   			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
>   		}
> @@ -638,7 +665,7 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
>   				kvm_err("Cannot allocate Hyp pmd\n");
>   				return -ENOMEM;
>   			}
> -			pud_populate(NULL, pud, pmd);
> +			kvm_pud_populate(pud, pmd);
>   			get_page(virt_to_page(pud));
>   			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
>   		}
> @@ -675,7 +702,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
>   				err = -ENOMEM;
>   				goto out;
>   			}
> -			pgd_populate(NULL, pgd, pud);
> +			kvm_pgd_populate(pgd, pud);
>   			get_page(virt_to_page(pgd));
>   			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
>   		}
> @@ -1094,7 +1121,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>   		if (!cache)
>   			return 0; /* ignore calls from kvm_set_spte_hva */
>   		pte = mmu_memory_cache_alloc(cache);
> -		pmd_populate_kernel(NULL, pmd, pte);
> +		kvm_pmd_populate(pmd, pte);
>   		get_page(virt_to_page(pmd));
>   	}
>   
> 


Suzuki

Re: [PATCH v3 5/6] KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate

[Marc Zyngier]

On 27/06/18 14:47, Suzuki K Poulose wrote:
> Hi Marc,
> 
> On 27/06/18 13:20, Marc Zyngier wrote:
>> The {pmd,pud,pgd}_populate accessors usage in the kernel have always
>> been a bit weird in KVM. We don't have a struct mm to pass (and
>> neither does the kernel most of the time, but still...), and
>> the 32bit code has all kind of cache maintenance that doesn't make
>> sense on ARMv7+ when MP extensions are mandatory (which is the
>> case when the VEs are present).
>>
>> Let's bite the bullet and provide our own implementations. The
>> only bit of architectural code left has to do with building the table
>> entry itself (arm64 having up to 52bit PA, arm lacking PUD level).
>>
>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   arch/arm/include/asm/kvm_mmu.h   | 4 ++++
>>   arch/arm64/include/asm/kvm_mmu.h | 7 +++++++
>>   virt/kvm/arm/mmu.c               | 8 +++++---
>>   3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index b2feaea1434c..265ea9cf7df7 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -75,6 +75,10 @@ phys_addr_t kvm_get_idmap_vector(void);
>>   int kvm_mmu_init(void);
>>   void kvm_clear_hyp_idmap(void);
>>   
>> +#define kvm_mk_pmd(ptep)	__pmd(__pa(ptep) | PMD_TYPE_TABLE)
>> +#define kvm_mk_pud(pmdp)	__pud(__pa(pmdp) | PMD_TYPE_TABLE)
>> +#define kvm_mk_pgd(pudp)	({ BUILD_BUG(); 0; })
>> +
>>   static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
>>   {
>>   	pte_val(pte) |= L_PTE_S2_RDWR;
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index 25c9a91f6a87..2f05be2bed63 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -169,6 +169,13 @@ phys_addr_t kvm_get_idmap_vector(void);
>>   int kvm_mmu_init(void);
>>   void kvm_clear_hyp_idmap(void);
>>   
>> +#define kvm_mk_pmd(ptep)					\
>> +	__pmd(__phys_to_pmd_val(__pa(ptep) | PMD_TYPE_TABLE))
>> +#define kvm_mk_pud(pmdp)					\
>> +	__pud(__phys_to_pud_val(__pa(pmdp) | PMD_TYPE_TABLE))
>> +#define kvm_mk_pgd(pudp)					\
>> +	__pgd(__phys_to_pgd_val(__pa(pudp) | PUD_TYPE_TABLE))
>> +
> 
> I believe this is wrong, as the __phys_to_p.d_val could strip of the
> TABLE bit. The correct usage is :
> 
> __pXd(__phys_to_pXd_val(__pa(ptr)) | PxD_TYPE_TABLE)

Ah, you're absolutely correct! Fixed now.

Thanks,

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

[PATCH v3 5/6] KVM: arm/arm64: Stop using {pmd,pud,pgd}_populate

[Marc Zyngier]

On 27/06/18 14:47, Suzuki K Poulose wrote:
> Hi Marc,
> 
> On 27/06/18 13:20, Marc Zyngier wrote:
>> The {pmd,pud,pgd}_populate accessors usage in the kernel have always
>> been a bit weird in KVM. We don't have a struct mm to pass (and
>> neither does the kernel most of the time, but still...), and
>> the 32bit code has all kind of cache maintenance that doesn't make
>> sense on ARMv7+ when MP extensions are mandatory (which is the
>> case when the VEs are present).
>>
>> Let's bite the bullet and provide our own implementations. The
>> only bit of architectural code left has to do with building the table
>> entry itself (arm64 having up to 52bit PA, arm lacking PUD level).
>>
>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   arch/arm/include/asm/kvm_mmu.h   | 4 ++++
>>   arch/arm64/include/asm/kvm_mmu.h | 7 +++++++
>>   virt/kvm/arm/mmu.c               | 8 +++++---
>>   3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index b2feaea1434c..265ea9cf7df7 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -75,6 +75,10 @@ phys_addr_t kvm_get_idmap_vector(void);
>>   int kvm_mmu_init(void);
>>   void kvm_clear_hyp_idmap(void);
>>   
>> +#define kvm_mk_pmd(ptep)	__pmd(__pa(ptep) | PMD_TYPE_TABLE)
>> +#define kvm_mk_pud(pmdp)	__pud(__pa(pmdp) | PMD_TYPE_TABLE)
>> +#define kvm_mk_pgd(pudp)	({ BUILD_BUG(); 0; })
>> +
>>   static inline pte_t kvm_s2pte_mkwrite(pte_t pte)
>>   {
>>   	pte_val(pte) |= L_PTE_S2_RDWR;
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index 25c9a91f6a87..2f05be2bed63 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -169,6 +169,13 @@ phys_addr_t kvm_get_idmap_vector(void);
>>   int kvm_mmu_init(void);
>>   void kvm_clear_hyp_idmap(void);
>>   
>> +#define kvm_mk_pmd(ptep)					\
>> +	__pmd(__phys_to_pmd_val(__pa(ptep) | PMD_TYPE_TABLE))
>> +#define kvm_mk_pud(pmdp)					\
>> +	__pud(__phys_to_pud_val(__pa(pmdp) | PMD_TYPE_TABLE))
>> +#define kvm_mk_pgd(pudp)					\
>> +	__pgd(__phys_to_pgd_val(__pa(pudp) | PUD_TYPE_TABLE))
>> +
> 
> I believe this is wrong, as the __phys_to_p.d_val could strip of the
> TABLE bit. The correct usage is :
> 
> __pXd(__phys_to_pXd_val(__pa(ptr)) | PxD_TYPE_TABLE)

Ah, you're absolutely correct! Fixed now.

Thanks,

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

Re: [PATCH v3 4/6] KVM: arm/arm64: Consolidate page-table accessors

[Marc Zyngier]

On 27/06/18 14:59, Suzuki K Poulose wrote:
> Marc,
> 
> On 27/06/18 13:20, Marc Zyngier wrote:
>> The arm and arm64 KVM page tables accessors are pointlessly different
>> between the two architectures, and likely both wrong one way or another:
>> arm64 lacks a dsb(), and arm doesn't use WRITE_ONCE.
>>
>> Let's unify them.
> 
> nit: Feel free to ignore. The changes introducing kvm_p.d_populate
> is not mentioned in the commit description and they do look a bit
> disconnected from the change described above.

That's a good point. I've moved those into patch #5, where they make
much more sense.

Thanks,

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

[PATCH v3 4/6] KVM: arm/arm64: Consolidate page-table accessors

[Marc Zyngier]

On 27/06/18 14:59, Suzuki K Poulose wrote:
> Marc,
> 
> On 27/06/18 13:20, Marc Zyngier wrote:
>> The arm and arm64 KVM page tables accessors are pointlessly different
>> between the two architectures, and likely both wrong one way or another:
>> arm64 lacks a dsb(), and arm doesn't use WRITE_ONCE.
>>
>> Let's unify them.
> 
> nit: Feel free to ignore. The changes introducing kvm_p.d_populate
> is not mentioned in the commit description and they do look a bit
> disconnected from the change described above.

That's a good point. I've moved those into patch #5, where they make
much more sense.

Thanks,

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

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Suzuki K Poulose]

Marc,

On 27/06/18 13:20, Marc Zyngier wrote:
> Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> results in the strongest attribute of the two stages.  This means
> that the hypervisor has to perform quite a lot of cache maintenance
> just in case the guest has some non-cacheable mappings around.
> 
> ARMv8.4 solves this problem by offering a different mode (FWB) where
> Stage-2 has total control over the memory attribute (this is limited
> to systems where both I/O and instruction fetches are coherent with
> the dcache). This is achieved by having a different set of memory
> attributes in the page tables, and a new bit set in HCR_EL2.
> 
> On such a system, we can then safely sidestep any form of dcache
> management.
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm64/include/asm/cpucaps.h      |  3 ++-
>   arch/arm64/include/asm/kvm_arm.h      |  1 +
>   arch/arm64/include/asm/kvm_emulate.h  |  2 ++
>   arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
>   arch/arm64/include/asm/memory.h       |  7 +++++++
>   arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
>   arch/arm64/include/asm/sysreg.h       |  1 +
>   arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
>   virt/kvm/arm/mmu.c                    |  4 ++++
>   9 files changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 8a699c708fc9..ed84d6536830 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -49,7 +49,8 @@
>   #define ARM64_HAS_CACHE_DIC			28
>   #define ARM64_HW_DBM				29
>   #define ARM64_SSBD				30
> +#define ARM64_HAS_STAGE2_FWB			31
>   
> -#define ARM64_NCAPS				31
> +#define ARM64_NCAPS				32
>   
>   #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6dd285e979c9..aa45df752a16 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -23,6 +23,7 @@
>   #include <asm/types.h>
>   
>   /* Hyp Configuration Register (HCR) bits */
> +#define HCR_FWB		(UL(1) << 46)
>   #define HCR_TEA		(UL(1) << 37)
>   #define HCR_TERR	(UL(1) << 36)
>   #define HCR_TLOR	(UL(1) << 35)
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 1dab3a984608..dd98fdf33d99 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>   		/* trap error record accesses */
>   		vcpu->arch.hcr_el2 |= HCR_TERR;
>   	}
> +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> +		vcpu->arch.hcr_el2 |= HCR_FWB;
>   
>   	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>   		vcpu->arch.hcr_el2 &= ~HCR_RW;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index fb9a7127bb75..620eb9e06bd8 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
>   struct kvm;
>   
>   #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))

We don't seem to use this new helper anywhere. Otherwise looks good to me.

Suzuki

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Suzuki K Poulose]

Marc,

On 27/06/18 13:20, Marc Zyngier wrote:
> Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> results in the strongest attribute of the two stages.  This means
> that the hypervisor has to perform quite a lot of cache maintenance
> just in case the guest has some non-cacheable mappings around.
> 
> ARMv8.4 solves this problem by offering a different mode (FWB) where
> Stage-2 has total control over the memory attribute (this is limited
> to systems where both I/O and instruction fetches are coherent with
> the dcache). This is achieved by having a different set of memory
> attributes in the page tables, and a new bit set in HCR_EL2.
> 
> On such a system, we can then safely sidestep any form of dcache
> management.
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm64/include/asm/cpucaps.h      |  3 ++-
>   arch/arm64/include/asm/kvm_arm.h      |  1 +
>   arch/arm64/include/asm/kvm_emulate.h  |  2 ++
>   arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
>   arch/arm64/include/asm/memory.h       |  7 +++++++
>   arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
>   arch/arm64/include/asm/sysreg.h       |  1 +
>   arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
>   virt/kvm/arm/mmu.c                    |  4 ++++
>   9 files changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 8a699c708fc9..ed84d6536830 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -49,7 +49,8 @@
>   #define ARM64_HAS_CACHE_DIC			28
>   #define ARM64_HW_DBM				29
>   #define ARM64_SSBD				30
> +#define ARM64_HAS_STAGE2_FWB			31
>   
> -#define ARM64_NCAPS				31
> +#define ARM64_NCAPS				32
>   
>   #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6dd285e979c9..aa45df752a16 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -23,6 +23,7 @@
>   #include <asm/types.h>
>   
>   /* Hyp Configuration Register (HCR) bits */
> +#define HCR_FWB		(UL(1) << 46)
>   #define HCR_TEA		(UL(1) << 37)
>   #define HCR_TERR	(UL(1) << 36)
>   #define HCR_TLOR	(UL(1) << 35)
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 1dab3a984608..dd98fdf33d99 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>   		/* trap error record accesses */
>   		vcpu->arch.hcr_el2 |= HCR_TERR;
>   	}
> +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> +		vcpu->arch.hcr_el2 |= HCR_FWB;
>   
>   	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>   		vcpu->arch.hcr_el2 &= ~HCR_RW;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index fb9a7127bb75..620eb9e06bd8 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
>   struct kvm;
>   
>   #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))

We don't seem to use this new helper anywhere. Otherwise looks good to me.

Suzuki

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

On 27/06/18 17:20, Suzuki K Poulose wrote:
> Marc,
> 
> On 27/06/18 13:20, Marc Zyngier wrote:
>> Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
>> results in the strongest attribute of the two stages.  This means
>> that the hypervisor has to perform quite a lot of cache maintenance
>> just in case the guest has some non-cacheable mappings around.
>>
>> ARMv8.4 solves this problem by offering a different mode (FWB) where
>> Stage-2 has total control over the memory attribute (this is limited
>> to systems where both I/O and instruction fetches are coherent with
>> the dcache). This is achieved by having a different set of memory
>> attributes in the page tables, and a new bit set in HCR_EL2.
>>
>> On such a system, we can then safely sidestep any form of dcache
>> management.
>>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   arch/arm64/include/asm/cpucaps.h      |  3 ++-
>>   arch/arm64/include/asm/kvm_arm.h      |  1 +
>>   arch/arm64/include/asm/kvm_emulate.h  |  2 ++
>>   arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
>>   arch/arm64/include/asm/memory.h       |  7 +++++++
>>   arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
>>   arch/arm64/include/asm/sysreg.h       |  1 +
>>   arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
>>   virt/kvm/arm/mmu.c                    |  4 ++++
>>   9 files changed, 69 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
>> index 8a699c708fc9..ed84d6536830 100644
>> --- a/arch/arm64/include/asm/cpucaps.h
>> +++ b/arch/arm64/include/asm/cpucaps.h
>> @@ -49,7 +49,8 @@
>>   #define ARM64_HAS_CACHE_DIC			28
>>   #define ARM64_HW_DBM				29
>>   #define ARM64_SSBD				30
>> +#define ARM64_HAS_STAGE2_FWB			31
>>   
>> -#define ARM64_NCAPS				31
>> +#define ARM64_NCAPS				32
>>   
>>   #endif /* __ASM_CPUCAPS_H */
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index 6dd285e979c9..aa45df752a16 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -23,6 +23,7 @@
>>   #include <asm/types.h>
>>   
>>   /* Hyp Configuration Register (HCR) bits */
>> +#define HCR_FWB		(UL(1) << 46)
>>   #define HCR_TEA		(UL(1) << 37)
>>   #define HCR_TERR	(UL(1) << 36)
>>   #define HCR_TLOR	(UL(1) << 35)
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index 1dab3a984608..dd98fdf33d99 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>>   		/* trap error record accesses */
>>   		vcpu->arch.hcr_el2 |= HCR_TERR;
>>   	}
>> +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
>> +		vcpu->arch.hcr_el2 |= HCR_FWB;
>>   
>>   	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>>   		vcpu->arch.hcr_el2 &= ~HCR_RW;
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index fb9a7127bb75..620eb9e06bd8 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
>>   struct kvm;
>>   
>>   #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
>> +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> 
> We don't seem to use this new helper anywhere. Otherwise looks good to me.

Good point, I forgot to remove it when I dropped the flush to PoU...
I'll sort it.

Thanks,

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

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

On 27/06/18 17:20, Suzuki K Poulose wrote:
> Marc,
> 
> On 27/06/18 13:20, Marc Zyngier wrote:
>> Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
>> results in the strongest attribute of the two stages.  This means
>> that the hypervisor has to perform quite a lot of cache maintenance
>> just in case the guest has some non-cacheable mappings around.
>>
>> ARMv8.4 solves this problem by offering a different mode (FWB) where
>> Stage-2 has total control over the memory attribute (this is limited
>> to systems where both I/O and instruction fetches are coherent with
>> the dcache). This is achieved by having a different set of memory
>> attributes in the page tables, and a new bit set in HCR_EL2.
>>
>> On such a system, we can then safely sidestep any form of dcache
>> management.
>>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   arch/arm64/include/asm/cpucaps.h      |  3 ++-
>>   arch/arm64/include/asm/kvm_arm.h      |  1 +
>>   arch/arm64/include/asm/kvm_emulate.h  |  2 ++
>>   arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
>>   arch/arm64/include/asm/memory.h       |  7 +++++++
>>   arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
>>   arch/arm64/include/asm/sysreg.h       |  1 +
>>   arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
>>   virt/kvm/arm/mmu.c                    |  4 ++++
>>   9 files changed, 69 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
>> index 8a699c708fc9..ed84d6536830 100644
>> --- a/arch/arm64/include/asm/cpucaps.h
>> +++ b/arch/arm64/include/asm/cpucaps.h
>> @@ -49,7 +49,8 @@
>>   #define ARM64_HAS_CACHE_DIC			28
>>   #define ARM64_HW_DBM				29
>>   #define ARM64_SSBD				30
>> +#define ARM64_HAS_STAGE2_FWB			31
>>   
>> -#define ARM64_NCAPS				31
>> +#define ARM64_NCAPS				32
>>   
>>   #endif /* __ASM_CPUCAPS_H */
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index 6dd285e979c9..aa45df752a16 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -23,6 +23,7 @@
>>   #include <asm/types.h>
>>   
>>   /* Hyp Configuration Register (HCR) bits */
>> +#define HCR_FWB		(UL(1) << 46)
>>   #define HCR_TEA		(UL(1) << 37)
>>   #define HCR_TERR	(UL(1) << 36)
>>   #define HCR_TLOR	(UL(1) << 35)
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index 1dab3a984608..dd98fdf33d99 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>>   		/* trap error record accesses */
>>   		vcpu->arch.hcr_el2 |= HCR_TERR;
>>   	}
>> +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
>> +		vcpu->arch.hcr_el2 |= HCR_FWB;
>>   
>>   	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>>   		vcpu->arch.hcr_el2 &= ~HCR_RW;
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index fb9a7127bb75..620eb9e06bd8 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
>>   struct kvm;
>>   
>>   #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
>> +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> 
> We don't seem to use this new helper anywhere. Otherwise looks good to me.

Good point, I forgot to remove it when I dropped the flush to PoU...
I'll sort it.

Thanks,

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

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Christoffer Dall]

On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> results in the strongest attribute of the two stages.  This means
> that the hypervisor has to perform quite a lot of cache maintenance
> just in case the guest has some non-cacheable mappings around.
>
> ARMv8.4 solves this problem by offering a different mode (FWB) where
> Stage-2 has total control over the memory attribute (this is limited
> to systems where both I/O and instruction fetches are coherent with
> the dcache). This is achieved by having a different set of memory
> attributes in the page tables, and a new bit set in HCR_EL2.
>
> On such a system, we can then safely sidestep any form of dcache
> management.
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/cpucaps.h      |  3 ++-
>  arch/arm64/include/asm/kvm_arm.h      |  1 +
>  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
>  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
>  arch/arm64/include/asm/memory.h       |  7 +++++++
>  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
>  arch/arm64/include/asm/sysreg.h       |  1 +
>  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
>  virt/kvm/arm/mmu.c                    |  4 ++++
>  9 files changed, 69 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 8a699c708fc9..ed84d6536830 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -49,7 +49,8 @@
>  #define ARM64_HAS_CACHE_DIC                  28
>  #define ARM64_HW_DBM                         29
>  #define ARM64_SSBD                           30
> +#define ARM64_HAS_STAGE2_FWB                 31
>
> -#define ARM64_NCAPS                          31
> +#define ARM64_NCAPS                          32
>
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6dd285e979c9..aa45df752a16 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -23,6 +23,7 @@
>  #include <asm/types.h>
>
>  /* Hyp Configuration Register (HCR) bits */
> +#define HCR_FWB              (UL(1) << 46)
>  #define HCR_TEA              (UL(1) << 37)
>  #define HCR_TERR     (UL(1) << 36)
>  #define HCR_TLOR     (UL(1) << 35)
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 1dab3a984608..dd98fdf33d99 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>               /* trap error record accesses */
>               vcpu->arch.hcr_el2 |= HCR_TERR;
>       }
> +     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> +             vcpu->arch.hcr_el2 |= HCR_FWB;
>
>       if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>               vcpu->arch.hcr_el2 &= ~HCR_RW;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index fb9a7127bb75..620eb9e06bd8 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
>  struct kvm;
>
>  #define kvm_flush_dcache_to_poc(a,l) __flush_dcache_area((a), (l))
> +#define kvm_flush_dcache_to_pou(a,l) __clean_dcache_area_pou((a), (l))
>
>  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
>  {
> @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
>  {
>       void *va = page_address(pfn_to_page(pfn));
>
> +     /*
> +      * FWB implies IDC, so cache clean to PoC is not required in
> +      * this case.
> +      */

I don't understand this comment.  __clean_dcache_guest_page is called
when faulting in pages for translation faults (fault_status != FSC_PERM)
and PoC is about coherency between ram and caches, not instruction and
data caches, so how is this related to IDC?

> +     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> +             return;
> +
>       kvm_flush_dcache_to_poc(va, size);
>  }
>
> @@ -287,20 +295,26 @@ static inline void __invalidate_icache_guest_page(kvm_pfn_t pfn,
>
>  static inline void __kvm_flush_dcache_pte(pte_t pte)
>  {
> -     struct page *page = pte_page(pte);
> -     kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
> +     if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> +             struct page *page = pte_page(pte);
> +             kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
> +     }
>  }
>
>  static inline void __kvm_flush_dcache_pmd(pmd_t pmd)
>  {
> -     struct page *page = pmd_page(pmd);
> -     kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
> +     if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> +             struct page *page = pmd_page(pmd);
> +             kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
> +     }
>  }
>
>  static inline void __kvm_flush_dcache_pud(pud_t pud)
>  {
> -     struct page *page = pud_page(pud);
> -     kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
> +     if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> +             struct page *page = pud_page(pud);
> +             kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
> +     }
>  }
>
>  #define kvm_virt_to_phys(x)          __pa_symbol(x)
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 49d99214f43c..b96442960aea 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -155,6 +155,13 @@
>  #define MT_S2_NORMAL         0xf
>  #define MT_S2_DEVICE_nGnRE   0x1
>
> +/*
> + * Memory types for Stage-2 translation when ID_AA64MMFR2_EL1.FWB is 0001
> + * Stage-2 enforces Normal-WB and Device-nGnRE
> + */
> +#define MT_S2_FWB_NORMAL     6
> +#define MT_S2_FWB_DEVICE_nGnRE       1
> +
>  #ifdef CONFIG_ARM64_4K_PAGES
>  #define IOREMAP_MAX_ORDER    (PUD_SHIFT)
>  #else
> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> index 108ecad7acc5..c66c3047400e 100644
> --- a/arch/arm64/include/asm/pgtable-prot.h
> +++ b/arch/arm64/include/asm/pgtable-prot.h
> @@ -67,8 +67,18 @@
>  #define PAGE_HYP_RO          __pgprot(_HYP_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
>  #define PAGE_HYP_DEVICE              __pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
>
> -#define PAGE_S2                      __pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
> -#define PAGE_S2_DEVICE               __pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
> +#define PAGE_S2_MEMATTR(attr)                                                \
> +     ({                                                              \
> +             u64 __val;                                              \
> +             if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))          \
> +                     __val = PTE_S2_MEMATTR(MT_S2_FWB_ ## attr);     \
> +             else                                                    \
> +                     __val = PTE_S2_MEMATTR(MT_S2_ ## attr);         \
> +             __val;                                                  \
> +      })
> +
> +#define PAGE_S2                      __pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
> +#define PAGE_S2_DEVICE               __pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
>
>  #define PAGE_NONE            __pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
>  #define PAGE_SHARED          __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index a8f84812c6e8..98af0b37fb31 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -576,6 +576,7 @@
>  #define ID_AA64MMFR1_VMIDBITS_16     2
>
>  /* id_aa64mmfr2 */
> +#define ID_AA64MMFR2_FWB_SHIFT               40
>  #define ID_AA64MMFR2_AT_SHIFT                32
>  #define ID_AA64MMFR2_LVA_SHIFT               16
>  #define ID_AA64MMFR2_IESB_SHIFT              12
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index f24892a40d2c..d58d1f0abe16 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -192,6 +192,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
>  };
>
>  static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
> +     ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0),
>       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0),
>       ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0),
>       ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
> @@ -1026,6 +1027,14 @@ static void cpu_copy_el2regs(const struct arm64_cpu_capabilities *__unused)
>  }
>  #endif
>
> +static void cpu_has_fwb(const struct arm64_cpu_capabilities *__unused)
> +{
> +     u64 val = read_sysreg_s(SYS_CLIDR_EL1);
> +
> +     /* Check that CLIDR_EL1.LOU{U,IS} are both 0 */
> +     WARN_ON(val & (7 << 27 | 7 << 21));
> +}
> +
>  static const struct arm64_cpu_capabilities arm64_features[] = {
>       {
>               .desc = "GIC system register CPU interface",
> @@ -1182,6 +1191,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>               .type = ARM64_CPUCAP_SYSTEM_FEATURE,
>               .matches = has_cache_dic,
>       },
> +     {
> +             .desc = "Stage-2 Force Write-Back",
> +             .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> +             .capability = ARM64_HAS_STAGE2_FWB,
> +             .sys_reg = SYS_ID_AA64MMFR2_EL1,
> +             .sign = FTR_UNSIGNED,
> +             .field_pos = ID_AA64MMFR2_FWB_SHIFT,
> +             .min_field_value = 1,
> +             .matches = has_cpuid_feature,
> +             .cpu_enable = cpu_has_fwb,
> +     },
>  #ifdef CONFIG_ARM64_HW_AFDBM
>       {
>               /*
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index 1d90d79706bd..ea7314296ad1 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -196,6 +196,10 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
>   * This is why right after unmapping a page/section and invalidating
>   * the corresponding TLBs, we call kvm_flush_dcache_p*() to make sure
>   * the IO subsystem will never hit in the cache.
> + *
> + * This is all avoided on systems that have ARM64_HAS_STAGE2_FWB, as
> + * we then fully enforce cacheability of RAM, no matter what the guest
> + * does.
>   */
>  static void unmap_stage2_ptes(struct kvm *kvm, pmd_t *pmd,
>                      phys_addr_t addr, phys_addr_t end)
> --
> 2.17.1
>

Otherwise looks good.

Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Christoffer Dall]

On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> results in the strongest attribute of the two stages.  This means
> that the hypervisor has to perform quite a lot of cache maintenance
> just in case the guest has some non-cacheable mappings around.
>
> ARMv8.4 solves this problem by offering a different mode (FWB) where
> Stage-2 has total control over the memory attribute (this is limited
> to systems where both I/O and instruction fetches are coherent with
> the dcache). This is achieved by having a different set of memory
> attributes in the page tables, and a new bit set in HCR_EL2.
>
> On such a system, we can then safely sidestep any form of dcache
> management.
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/cpucaps.h      |  3 ++-
>  arch/arm64/include/asm/kvm_arm.h      |  1 +
>  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
>  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
>  arch/arm64/include/asm/memory.h       |  7 +++++++
>  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
>  arch/arm64/include/asm/sysreg.h       |  1 +
>  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
>  virt/kvm/arm/mmu.c                    |  4 ++++
>  9 files changed, 69 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 8a699c708fc9..ed84d6536830 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -49,7 +49,8 @@
>  #define ARM64_HAS_CACHE_DIC                  28
>  #define ARM64_HW_DBM                         29
>  #define ARM64_SSBD                           30
> +#define ARM64_HAS_STAGE2_FWB                 31
>
> -#define ARM64_NCAPS                          31
> +#define ARM64_NCAPS                          32
>
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6dd285e979c9..aa45df752a16 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -23,6 +23,7 @@
>  #include <asm/types.h>
>
>  /* Hyp Configuration Register (HCR) bits */
> +#define HCR_FWB              (UL(1) << 46)
>  #define HCR_TEA              (UL(1) << 37)
>  #define HCR_TERR     (UL(1) << 36)
>  #define HCR_TLOR     (UL(1) << 35)
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 1dab3a984608..dd98fdf33d99 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>               /* trap error record accesses */
>               vcpu->arch.hcr_el2 |= HCR_TERR;
>       }
> +     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> +             vcpu->arch.hcr_el2 |= HCR_FWB;
>
>       if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>               vcpu->arch.hcr_el2 &= ~HCR_RW;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index fb9a7127bb75..620eb9e06bd8 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
>  struct kvm;
>
>  #define kvm_flush_dcache_to_poc(a,l) __flush_dcache_area((a), (l))
> +#define kvm_flush_dcache_to_pou(a,l) __clean_dcache_area_pou((a), (l))
>
>  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
>  {
> @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
>  {
>       void *va = page_address(pfn_to_page(pfn));
>
> +     /*
> +      * FWB implies IDC, so cache clean to PoC is not required in
> +      * this case.
> +      */

I don't understand this comment.  __clean_dcache_guest_page is called
when faulting in pages for translation faults (fault_status != FSC_PERM)
and PoC is about coherency between ram and caches, not instruction and
data caches, so how is this related to IDC?

> +     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> +             return;
> +
>       kvm_flush_dcache_to_poc(va, size);
>  }
>
> @@ -287,20 +295,26 @@ static inline void __invalidate_icache_guest_page(kvm_pfn_t pfn,
>
>  static inline void __kvm_flush_dcache_pte(pte_t pte)
>  {
> -     struct page *page = pte_page(pte);
> -     kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
> +     if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> +             struct page *page = pte_page(pte);
> +             kvm_flush_dcache_to_poc(page_address(page), PAGE_SIZE);
> +     }
>  }
>
>  static inline void __kvm_flush_dcache_pmd(pmd_t pmd)
>  {
> -     struct page *page = pmd_page(pmd);
> -     kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
> +     if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> +             struct page *page = pmd_page(pmd);
> +             kvm_flush_dcache_to_poc(page_address(page), PMD_SIZE);
> +     }
>  }
>
>  static inline void __kvm_flush_dcache_pud(pud_t pud)
>  {
> -     struct page *page = pud_page(pud);
> -     kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
> +     if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> +             struct page *page = pud_page(pud);
> +             kvm_flush_dcache_to_poc(page_address(page), PUD_SIZE);
> +     }
>  }
>
>  #define kvm_virt_to_phys(x)          __pa_symbol(x)
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 49d99214f43c..b96442960aea 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -155,6 +155,13 @@
>  #define MT_S2_NORMAL         0xf
>  #define MT_S2_DEVICE_nGnRE   0x1
>
> +/*
> + * Memory types for Stage-2 translation when ID_AA64MMFR2_EL1.FWB is 0001
> + * Stage-2 enforces Normal-WB and Device-nGnRE
> + */
> +#define MT_S2_FWB_NORMAL     6
> +#define MT_S2_FWB_DEVICE_nGnRE       1
> +
>  #ifdef CONFIG_ARM64_4K_PAGES
>  #define IOREMAP_MAX_ORDER    (PUD_SHIFT)
>  #else
> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> index 108ecad7acc5..c66c3047400e 100644
> --- a/arch/arm64/include/asm/pgtable-prot.h
> +++ b/arch/arm64/include/asm/pgtable-prot.h
> @@ -67,8 +67,18 @@
>  #define PAGE_HYP_RO          __pgprot(_HYP_PAGE_DEFAULT | PTE_HYP | PTE_RDONLY | PTE_HYP_XN)
>  #define PAGE_HYP_DEVICE              __pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
>
> -#define PAGE_S2                      __pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
> -#define PAGE_S2_DEVICE               __pgprot(_PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
> +#define PAGE_S2_MEMATTR(attr)                                                \
> +     ({                                                              \
> +             u64 __val;                                              \
> +             if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))          \
> +                     __val = PTE_S2_MEMATTR(MT_S2_FWB_ ## attr);     \
> +             else                                                    \
> +                     __val = PTE_S2_MEMATTR(MT_S2_ ## attr);         \
> +             __val;                                                  \
> +      })
> +
> +#define PAGE_S2                      __pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(NORMAL) | PTE_S2_RDONLY | PTE_S2_XN)
> +#define PAGE_S2_DEVICE               __pgprot(_PROT_DEFAULT | PAGE_S2_MEMATTR(DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_S2_XN)
>
>  #define PAGE_NONE            __pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
>  #define PAGE_SHARED          __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index a8f84812c6e8..98af0b37fb31 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -576,6 +576,7 @@
>  #define ID_AA64MMFR1_VMIDBITS_16     2
>
>  /* id_aa64mmfr2 */
> +#define ID_AA64MMFR2_FWB_SHIFT               40
>  #define ID_AA64MMFR2_AT_SHIFT                32
>  #define ID_AA64MMFR2_LVA_SHIFT               16
>  #define ID_AA64MMFR2_IESB_SHIFT              12
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index f24892a40d2c..d58d1f0abe16 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -192,6 +192,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
>  };
>
>  static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
> +     ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0),
>       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0),
>       ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0),
>       ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
> @@ -1026,6 +1027,14 @@ static void cpu_copy_el2regs(const struct arm64_cpu_capabilities *__unused)
>  }
>  #endif
>
> +static void cpu_has_fwb(const struct arm64_cpu_capabilities *__unused)
> +{
> +     u64 val = read_sysreg_s(SYS_CLIDR_EL1);
> +
> +     /* Check that CLIDR_EL1.LOU{U,IS} are both 0 */
> +     WARN_ON(val & (7 << 27 | 7 << 21));
> +}
> +
>  static const struct arm64_cpu_capabilities arm64_features[] = {
>       {
>               .desc = "GIC system register CPU interface",
> @@ -1182,6 +1191,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>               .type = ARM64_CPUCAP_SYSTEM_FEATURE,
>               .matches = has_cache_dic,
>       },
> +     {
> +             .desc = "Stage-2 Force Write-Back",
> +             .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> +             .capability = ARM64_HAS_STAGE2_FWB,
> +             .sys_reg = SYS_ID_AA64MMFR2_EL1,
> +             .sign = FTR_UNSIGNED,
> +             .field_pos = ID_AA64MMFR2_FWB_SHIFT,
> +             .min_field_value = 1,
> +             .matches = has_cpuid_feature,
> +             .cpu_enable = cpu_has_fwb,
> +     },
>  #ifdef CONFIG_ARM64_HW_AFDBM
>       {
>               /*
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index 1d90d79706bd..ea7314296ad1 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -196,6 +196,10 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
>   * This is why right after unmapping a page/section and invalidating
>   * the corresponding TLBs, we call kvm_flush_dcache_p*() to make sure
>   * the IO subsystem will never hit in the cache.
> + *
> + * This is all avoided on systems that have ARM64_HAS_STAGE2_FWB, as
> + * we then fully enforce cacheability of RAM, no matter what the guest
> + * does.
>   */
>  static void unmap_stage2_ptes(struct kvm *kvm, pmd_t *pmd,
>                      phys_addr_t addr, phys_addr_t end)
> --
> 2.17.1
>

Otherwise looks good.

Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

On Thu, 28 Jun 2018 21:56:38 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > results in the strongest attribute of the two stages.  This means
> > that the hypervisor has to perform quite a lot of cache maintenance
> > just in case the guest has some non-cacheable mappings around.
> > 
> > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > Stage-2 has total control over the memory attribute (this is limited
> > to systems where both I/O and instruction fetches are coherent with
> > the dcache). This is achieved by having a different set of memory
> > attributes in the page tables, and a new bit set in HCR_EL2.
> > 
> > On such a system, we can then safely sidestep any form of dcache
> > management.
> > 
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> >  arch/arm64/include/asm/memory.h       |  7 +++++++
> >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> >  arch/arm64/include/asm/sysreg.h       |  1 +
> >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> >  virt/kvm/arm/mmu.c                    |  4 ++++
> >  9 files changed, 69 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > index 8a699c708fc9..ed84d6536830 100644
> > --- a/arch/arm64/include/asm/cpucaps.h
> > +++ b/arch/arm64/include/asm/cpucaps.h
> > @@ -49,7 +49,8 @@
> >  #define ARM64_HAS_CACHE_DIC			28
> >  #define ARM64_HW_DBM				29
> >  #define ARM64_SSBD				30
> > +#define ARM64_HAS_STAGE2_FWB			31
> >  
> > -#define ARM64_NCAPS				31
> > +#define ARM64_NCAPS				32
> >  
> >  #endif /* __ASM_CPUCAPS_H */
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index 6dd285e979c9..aa45df752a16 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -23,6 +23,7 @@
> >  #include <asm/types.h>
> >  
> >  /* Hyp Configuration Register (HCR) bits */
> > +#define HCR_FWB		(UL(1) << 46)
> >  #define HCR_TEA		(UL(1) << 37)
> >  #define HCR_TERR	(UL(1) << 36)
> >  #define HCR_TLOR	(UL(1) << 35)
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index 1dab3a984608..dd98fdf33d99 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> >  		/* trap error record accesses */
> >  		vcpu->arch.hcr_el2 |= HCR_TERR;
> >  	}
> > +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > +		vcpu->arch.hcr_el2 |= HCR_FWB;
> >  
> >  	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> >  		vcpu->arch.hcr_el2 &= ~HCR_RW;
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index fb9a7127bb75..620eb9e06bd8 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> >  struct kvm;
> >  
> >  #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> > +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> >  
> >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> >  {
> > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> >  {
> >  	void *va = page_address(pfn_to_page(pfn));
> >  
> > +	/*
> > +	 * FWB implies IDC, so cache clean to PoC is not required in
> > +	 * this case.
> > +	 */
> 
> I don't understand this comment.  __clean_dcache_guest_page is called
> when faulting in pages for translation faults (fault_status != FSC_PERM)
> and PoC is about coherency between ram and caches, not instruction and
> data caches, so how is this related to IDC?

There's a shortcut in this comment, so let me develop a tiny bit:

- FWB makes memory the memory cacheable from the PoV of the guest, so
  we don't need to clean to the PoC for the guest to access it even
  when running with the stage-1 MMU off or non-cacheable mappings.

- FWB implies IDC so there is no need to clean to the PoU for
  instruction fetches to get the right thing either.

The combination of these two statements implies that we don't need to
clean anything at all.

Does it make more sense?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

On Thu, 28 Jun 2018 21:56:38 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > results in the strongest attribute of the two stages.  This means
> > that the hypervisor has to perform quite a lot of cache maintenance
> > just in case the guest has some non-cacheable mappings around.
> > 
> > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > Stage-2 has total control over the memory attribute (this is limited
> > to systems where both I/O and instruction fetches are coherent with
> > the dcache). This is achieved by having a different set of memory
> > attributes in the page tables, and a new bit set in HCR_EL2.
> > 
> > On such a system, we can then safely sidestep any form of dcache
> > management.
> > 
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> >  arch/arm64/include/asm/memory.h       |  7 +++++++
> >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> >  arch/arm64/include/asm/sysreg.h       |  1 +
> >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> >  virt/kvm/arm/mmu.c                    |  4 ++++
> >  9 files changed, 69 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > index 8a699c708fc9..ed84d6536830 100644
> > --- a/arch/arm64/include/asm/cpucaps.h
> > +++ b/arch/arm64/include/asm/cpucaps.h
> > @@ -49,7 +49,8 @@
> >  #define ARM64_HAS_CACHE_DIC			28
> >  #define ARM64_HW_DBM				29
> >  #define ARM64_SSBD				30
> > +#define ARM64_HAS_STAGE2_FWB			31
> >  
> > -#define ARM64_NCAPS				31
> > +#define ARM64_NCAPS				32
> >  
> >  #endif /* __ASM_CPUCAPS_H */
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index 6dd285e979c9..aa45df752a16 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -23,6 +23,7 @@
> >  #include <asm/types.h>
> >  
> >  /* Hyp Configuration Register (HCR) bits */
> > +#define HCR_FWB		(UL(1) << 46)
> >  #define HCR_TEA		(UL(1) << 37)
> >  #define HCR_TERR	(UL(1) << 36)
> >  #define HCR_TLOR	(UL(1) << 35)
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index 1dab3a984608..dd98fdf33d99 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> >  		/* trap error record accesses */
> >  		vcpu->arch.hcr_el2 |= HCR_TERR;
> >  	}
> > +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > +		vcpu->arch.hcr_el2 |= HCR_FWB;
> >  
> >  	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> >  		vcpu->arch.hcr_el2 &= ~HCR_RW;
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index fb9a7127bb75..620eb9e06bd8 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> >  struct kvm;
> >  
> >  #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> > +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> >  
> >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> >  {
> > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> >  {
> >  	void *va = page_address(pfn_to_page(pfn));
> >  
> > +	/*
> > +	 * FWB implies IDC, so cache clean to PoC is not required in
> > +	 * this case.
> > +	 */
> 
> I don't understand this comment.  __clean_dcache_guest_page is called
> when faulting in pages for translation faults (fault_status != FSC_PERM)
> and PoC is about coherency between ram and caches, not instruction and
> data caches, so how is this related to IDC?

There's a shortcut in this comment, so let me develop a tiny bit:

- FWB makes memory the memory cacheable from the PoV of the guest, so
  we don't need to clean to the PoC for the guest to access it even
  when running with the stage-1 MMU off or non-cacheable mappings.

- FWB implies IDC so there is no need to clean to the PoU for
  instruction fetches to get the right thing either.

The combination of these two statements implies that we don't need to
clean anything at all.

Does it make more sense?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Christoffer Dall]

On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> On Thu, 28 Jun 2018 21:56:38 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > > results in the strongest attribute of the two stages.  This means
> > > that the hypervisor has to perform quite a lot of cache maintenance
> > > just in case the guest has some non-cacheable mappings around.
> > >
> > > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > > Stage-2 has total control over the memory attribute (this is limited
> > > to systems where both I/O and instruction fetches are coherent with
> > > the dcache). This is achieved by having a different set of memory
> > > attributes in the page tables, and a new bit set in HCR_EL2.
> > >
> > > On such a system, we can then safely sidestep any form of dcache
> > > management.
> > >
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> > >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> > >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> > >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> > >  arch/arm64/include/asm/memory.h       |  7 +++++++
> > >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> > >  arch/arm64/include/asm/sysreg.h       |  1 +
> > >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> > >  virt/kvm/arm/mmu.c                    |  4 ++++
> > >  9 files changed, 69 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > > index 8a699c708fc9..ed84d6536830 100644
> > > --- a/arch/arm64/include/asm/cpucaps.h
> > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > @@ -49,7 +49,8 @@
> > >  #define ARM64_HAS_CACHE_DIC                      28
> > >  #define ARM64_HW_DBM                             29
> > >  #define ARM64_SSBD                               30
> > > +#define ARM64_HAS_STAGE2_FWB                     31
> > >
> > > -#define ARM64_NCAPS                              31
> > > +#define ARM64_NCAPS                              32
> > >
> > >  #endif /* __ASM_CPUCAPS_H */
> > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > index 6dd285e979c9..aa45df752a16 100644
> > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > @@ -23,6 +23,7 @@
> > >  #include <asm/types.h>
> > >
> > >  /* Hyp Configuration Register (HCR) bits */
> > > +#define HCR_FWB          (UL(1) << 46)
> > >  #define HCR_TEA          (UL(1) << 37)
> > >  #define HCR_TERR (UL(1) << 36)
> > >  #define HCR_TLOR (UL(1) << 35)
> > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > index 1dab3a984608..dd98fdf33d99 100644
> > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > >           /* trap error record accesses */
> > >           vcpu->arch.hcr_el2 |= HCR_TERR;
> > >   }
> > > + if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > > +         vcpu->arch.hcr_el2 |= HCR_FWB;
> > >
> > >   if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > >           vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > index fb9a7127bb75..620eb9e06bd8 100644
> > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> > >  struct kvm;
> > >
> > >  #define kvm_flush_dcache_to_poc(a,l)     __flush_dcache_area((a), (l))
> > > +#define kvm_flush_dcache_to_pou(a,l)     __clean_dcache_area_pou((a), (l))
> > >
> > >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > >  {
> > > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > >  {
> > >   void *va = page_address(pfn_to_page(pfn));
> > >
> > > + /*
> > > +  * FWB implies IDC, so cache clean to PoC is not required in
> > > +  * this case.
> > > +  */
> >
> > I don't understand this comment.  __clean_dcache_guest_page is called
> > when faulting in pages for translation faults (fault_status != FSC_PERM)
> > and PoC is about coherency between ram and caches, not instruction and
> > data caches, so how is this related to IDC?
>
> There's a shortcut in this comment, so let me develop a tiny bit:
>
> - FWB makes memory the memory cacheable from the PoV of the guest, so
>   we don't need to clean to the PoC for the guest to access it even
>   when running with the stage-1 MMU off or non-cacheable mappings.
>
> - FWB implies IDC so there is no need to clean to the PoU for
>   instruction fetches to get the right thing either.
>
> The combination of these two statements implies that we don't need to
> clean anything at all.
>
> Does it make more sense?

Yes, this makes sense, but the comment is weird, because it's not
because FWB implies IDC that you don't have to clean to PoC, so I would
suggest either removing the comment, or rewording it to something like:

/*
 * With FWB we ensure that the guest always accesses memory cacheable
 * and we don't have to clean to PoC when faulting in pages.
 * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
 * in this case.
 */

I think the comment as it stands now, only makes sense in the context of
the conversation we've had on this patch on the list.

Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Christoffer Dall]

On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> On Thu, 28 Jun 2018 21:56:38 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > > results in the strongest attribute of the two stages.  This means
> > > that the hypervisor has to perform quite a lot of cache maintenance
> > > just in case the guest has some non-cacheable mappings around.
> > >
> > > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > > Stage-2 has total control over the memory attribute (this is limited
> > > to systems where both I/O and instruction fetches are coherent with
> > > the dcache). This is achieved by having a different set of memory
> > > attributes in the page tables, and a new bit set in HCR_EL2.
> > >
> > > On such a system, we can then safely sidestep any form of dcache
> > > management.
> > >
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> > >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> > >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> > >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> > >  arch/arm64/include/asm/memory.h       |  7 +++++++
> > >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> > >  arch/arm64/include/asm/sysreg.h       |  1 +
> > >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> > >  virt/kvm/arm/mmu.c                    |  4 ++++
> > >  9 files changed, 69 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > > index 8a699c708fc9..ed84d6536830 100644
> > > --- a/arch/arm64/include/asm/cpucaps.h
> > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > @@ -49,7 +49,8 @@
> > >  #define ARM64_HAS_CACHE_DIC                      28
> > >  #define ARM64_HW_DBM                             29
> > >  #define ARM64_SSBD                               30
> > > +#define ARM64_HAS_STAGE2_FWB                     31
> > >
> > > -#define ARM64_NCAPS                              31
> > > +#define ARM64_NCAPS                              32
> > >
> > >  #endif /* __ASM_CPUCAPS_H */
> > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > index 6dd285e979c9..aa45df752a16 100644
> > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > @@ -23,6 +23,7 @@
> > >  #include <asm/types.h>
> > >
> > >  /* Hyp Configuration Register (HCR) bits */
> > > +#define HCR_FWB          (UL(1) << 46)
> > >  #define HCR_TEA          (UL(1) << 37)
> > >  #define HCR_TERR (UL(1) << 36)
> > >  #define HCR_TLOR (UL(1) << 35)
> > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > index 1dab3a984608..dd98fdf33d99 100644
> > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > >           /* trap error record accesses */
> > >           vcpu->arch.hcr_el2 |= HCR_TERR;
> > >   }
> > > + if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > > +         vcpu->arch.hcr_el2 |= HCR_FWB;
> > >
> > >   if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > >           vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > index fb9a7127bb75..620eb9e06bd8 100644
> > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> > >  struct kvm;
> > >
> > >  #define kvm_flush_dcache_to_poc(a,l)     __flush_dcache_area((a), (l))
> > > +#define kvm_flush_dcache_to_pou(a,l)     __clean_dcache_area_pou((a), (l))
> > >
> > >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > >  {
> > > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > >  {
> > >   void *va = page_address(pfn_to_page(pfn));
> > >
> > > + /*
> > > +  * FWB implies IDC, so cache clean to PoC is not required in
> > > +  * this case.
> > > +  */
> >
> > I don't understand this comment.  __clean_dcache_guest_page is called
> > when faulting in pages for translation faults (fault_status != FSC_PERM)
> > and PoC is about coherency between ram and caches, not instruction and
> > data caches, so how is this related to IDC?
>
> There's a shortcut in this comment, so let me develop a tiny bit:
>
> - FWB makes memory the memory cacheable from the PoV of the guest, so
>   we don't need to clean to the PoC for the guest to access it even
>   when running with the stage-1 MMU off or non-cacheable mappings.
>
> - FWB implies IDC so there is no need to clean to the PoU for
>   instruction fetches to get the right thing either.
>
> The combination of these two statements implies that we don't need to
> clean anything at all.
>
> Does it make more sense?

Yes, this makes sense, but the comment is weird, because it's not
because FWB implies IDC that you don't have to clean to PoC, so I would
suggest either removing the comment, or rewording it to something like:

/*
 * With FWB we ensure that the guest always accesses memory cacheable
 * and we don't have to clean to PoC when faulting in pages.
 * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
 * in this case.
 */

I think the comment as it stands now, only makes sense in the context of
the conversation we've had on this patch on the list.

Thanks,
-Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

On Fri, 29 Jun 2018 10:07:50 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> > On Thu, 28 Jun 2018 21:56:38 +0100,
> > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > 
> > > On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > > > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > > > results in the strongest attribute of the two stages.  This means
> > > > that the hypervisor has to perform quite a lot of cache maintenance
> > > > just in case the guest has some non-cacheable mappings around.
> > > > 
> > > > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > > > Stage-2 has total control over the memory attribute (this is limited
> > > > to systems where both I/O and instruction fetches are coherent with
> > > > the dcache). This is achieved by having a different set of memory
> > > > attributes in the page tables, and a new bit set in HCR_EL2.
> > > > 
> > > > On such a system, we can then safely sidestep any form of dcache
> > > > management.
> > > > 
> > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > ---
> > > >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> > > >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> > > >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> > > >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> > > >  arch/arm64/include/asm/memory.h       |  7 +++++++
> > > >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> > > >  arch/arm64/include/asm/sysreg.h       |  1 +
> > > >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> > > >  virt/kvm/arm/mmu.c                    |  4 ++++
> > > >  9 files changed, 69 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > > > index 8a699c708fc9..ed84d6536830 100644
> > > > --- a/arch/arm64/include/asm/cpucaps.h
> > > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > > @@ -49,7 +49,8 @@
> > > >  #define ARM64_HAS_CACHE_DIC			28
> > > >  #define ARM64_HW_DBM				29
> > > >  #define ARM64_SSBD				30
> > > > +#define ARM64_HAS_STAGE2_FWB			31
> > > >  
> > > > -#define ARM64_NCAPS				31
> > > > +#define ARM64_NCAPS				32
> > > >  
> > > >  #endif /* __ASM_CPUCAPS_H */
> > > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > > index 6dd285e979c9..aa45df752a16 100644
> > > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > > @@ -23,6 +23,7 @@
> > > >  #include <asm/types.h>
> > > >  
> > > >  /* Hyp Configuration Register (HCR) bits */
> > > > +#define HCR_FWB		(UL(1) << 46)
> > > >  #define HCR_TEA		(UL(1) << 37)
> > > >  #define HCR_TERR	(UL(1) << 36)
> > > >  #define HCR_TLOR	(UL(1) << 35)
> > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > > index 1dab3a984608..dd98fdf33d99 100644
> > > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > >  		/* trap error record accesses */
> > > >  		vcpu->arch.hcr_el2 |= HCR_TERR;
> > > >  	}
> > > > +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > > > +		vcpu->arch.hcr_el2 |= HCR_FWB;
> > > >  
> > > >  	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > > >  		vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > > index fb9a7127bb75..620eb9e06bd8 100644
> > > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> > > >  struct kvm;
> > > >  
> > > >  #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> > > > +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> > > >  
> > > >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > > >  {
> > > > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > > >  {
> > > >  	void *va = page_address(pfn_to_page(pfn));
> > > >  
> > > > +	/*
> > > > +	 * FWB implies IDC, so cache clean to PoC is not required in
> > > > +	 * this case.
> > > > +	 */
> > > 
> > > I don't understand this comment.  __clean_dcache_guest_page is called
> > > when faulting in pages for translation faults (fault_status != FSC_PERM)
> > > and PoC is about coherency between ram and caches, not instruction and
> > > data caches, so how is this related to IDC?
> > 
> > There's a shortcut in this comment, so let me develop a tiny bit:
> > 
> > - FWB makes memory the memory cacheable from the PoV of the guest, so
> >   we don't need to clean to the PoC for the guest to access it even
> >   when running with the stage-1 MMU off or non-cacheable mappings.
> > 
> > - FWB implies IDC so there is no need to clean to the PoU for
> >   instruction fetches to get the right thing either.
> > 
> > The combination of these two statements implies that we don't need to
> > clean anything at all.
> > 
> > Does it make more sense?
> 
> Yes, this makes sense, but the comment is weird, because it's not
> because FWB implies IDC that you don't have to clean to PoC, so I would
> suggest either removing the comment, or rewording it to something like:
> 
> /*
>  * With FWB we ensure that the guest always accesses memory cacheable
>  * and we don't have to clean to PoC when faulting in pages.
>  * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
>  * in this case.
>  */
> 
> I think the comment as it stands now, only makes sense in the context of
> the conversation we've had on this patch on the list.

Fair enough. I'll update the comment to reflect the above.

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier]

On Fri, 29 Jun 2018 10:07:50 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> > On Thu, 28 Jun 2018 21:56:38 +0100,
> > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > 
> > > On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > > > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > > > results in the strongest attribute of the two stages.  This means
> > > > that the hypervisor has to perform quite a lot of cache maintenance
> > > > just in case the guest has some non-cacheable mappings around.
> > > > 
> > > > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > > > Stage-2 has total control over the memory attribute (this is limited
> > > > to systems where both I/O and instruction fetches are coherent with
> > > > the dcache). This is achieved by having a different set of memory
> > > > attributes in the page tables, and a new bit set in HCR_EL2.
> > > > 
> > > > On such a system, we can then safely sidestep any form of dcache
> > > > management.
> > > > 
> > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > ---
> > > >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> > > >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> > > >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> > > >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> > > >  arch/arm64/include/asm/memory.h       |  7 +++++++
> > > >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> > > >  arch/arm64/include/asm/sysreg.h       |  1 +
> > > >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> > > >  virt/kvm/arm/mmu.c                    |  4 ++++
> > > >  9 files changed, 69 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > > > index 8a699c708fc9..ed84d6536830 100644
> > > > --- a/arch/arm64/include/asm/cpucaps.h
> > > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > > @@ -49,7 +49,8 @@
> > > >  #define ARM64_HAS_CACHE_DIC			28
> > > >  #define ARM64_HW_DBM				29
> > > >  #define ARM64_SSBD				30
> > > > +#define ARM64_HAS_STAGE2_FWB			31
> > > >  
> > > > -#define ARM64_NCAPS				31
> > > > +#define ARM64_NCAPS				32
> > > >  
> > > >  #endif /* __ASM_CPUCAPS_H */
> > > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > > index 6dd285e979c9..aa45df752a16 100644
> > > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > > @@ -23,6 +23,7 @@
> > > >  #include <asm/types.h>
> > > >  
> > > >  /* Hyp Configuration Register (HCR) bits */
> > > > +#define HCR_FWB		(UL(1) << 46)
> > > >  #define HCR_TEA		(UL(1) << 37)
> > > >  #define HCR_TERR	(UL(1) << 36)
> > > >  #define HCR_TLOR	(UL(1) << 35)
> > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > > index 1dab3a984608..dd98fdf33d99 100644
> > > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > >  		/* trap error record accesses */
> > > >  		vcpu->arch.hcr_el2 |= HCR_TERR;
> > > >  	}
> > > > +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > > > +		vcpu->arch.hcr_el2 |= HCR_FWB;
> > > >  
> > > >  	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > > >  		vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > > index fb9a7127bb75..620eb9e06bd8 100644
> > > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> > > >  struct kvm;
> > > >  
> > > >  #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> > > > +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> > > >  
> > > >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > > >  {
> > > > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > > >  {
> > > >  	void *va = page_address(pfn_to_page(pfn));
> > > >  
> > > > +	/*
> > > > +	 * FWB implies IDC, so cache clean to PoC is not required in
> > > > +	 * this case.
> > > > +	 */
> > > 
> > > I don't understand this comment.  __clean_dcache_guest_page is called
> > > when faulting in pages for translation faults (fault_status != FSC_PERM)
> > > and PoC is about coherency between ram and caches, not instruction and
> > > data caches, so how is this related to IDC?
> > 
> > There's a shortcut in this comment, so let me develop a tiny bit:
> > 
> > - FWB makes memory the memory cacheable from the PoV of the guest, so
> >   we don't need to clean to the PoC for the guest to access it even
> >   when running with the stage-1 MMU off or non-cacheable mappings.
> > 
> > - FWB implies IDC so there is no need to clean to the PoU for
> >   instruction fetches to get the right thing either.
> > 
> > The combination of these two statements implies that we don't need to
> > clean anything at all.
> > 
> > Does it make more sense?
> 
> Yes, this makes sense, but the comment is weird, because it's not
> because FWB implies IDC that you don't have to clean to PoC, so I would
> suggest either removing the comment, or rewording it to something like:
> 
> /*
>  * With FWB we ensure that the guest always accesses memory cacheable
>  * and we don't have to clean to PoC when faulting in pages.
>  * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
>  * in this case.
>  */
> 
> I think the comment as it stands now, only makes sense in the context of
> the conversation we've had on this patch on the list.

Fair enough. I'll update the comment to reflect the above.

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Christoffer Dall]

On Fri, Jun 29, 2018 at 10:21:27AM +0100, Marc Zyngier wrote:
> On Fri, 29 Jun 2018 10:07:50 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> > > On Thu, 28 Jun 2018 21:56:38 +0100,
> > > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > >
> > > > On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > > > > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > > > > results in the strongest attribute of the two stages.  This means
> > > > > that the hypervisor has to perform quite a lot of cache maintenance
> > > > > just in case the guest has some non-cacheable mappings around.
> > > > >
> > > > > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > > > > Stage-2 has total control over the memory attribute (this is limited
> > > > > to systems where both I/O and instruction fetches are coherent with
> > > > > the dcache). This is achieved by having a different set of memory
> > > > > attributes in the page tables, and a new bit set in HCR_EL2.
> > > > >
> > > > > On such a system, we can then safely sidestep any form of dcache
> > > > > management.
> > > > >
> > > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > ---
> > > > >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> > > > >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> > > > >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> > > > >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> > > > >  arch/arm64/include/asm/memory.h       |  7 +++++++
> > > > >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> > > > >  arch/arm64/include/asm/sysreg.h       |  1 +
> > > > >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> > > > >  virt/kvm/arm/mmu.c                    |  4 ++++
> > > > >  9 files changed, 69 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > > > > index 8a699c708fc9..ed84d6536830 100644
> > > > > --- a/arch/arm64/include/asm/cpucaps.h
> > > > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > > > @@ -49,7 +49,8 @@
> > > > >  #define ARM64_HAS_CACHE_DIC                  28
> > > > >  #define ARM64_HW_DBM                         29
> > > > >  #define ARM64_SSBD                           30
> > > > > +#define ARM64_HAS_STAGE2_FWB                 31
> > > > >
> > > > > -#define ARM64_NCAPS                          31
> > > > > +#define ARM64_NCAPS                          32
> > > > >
> > > > >  #endif /* __ASM_CPUCAPS_H */
> > > > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > > > index 6dd285e979c9..aa45df752a16 100644
> > > > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > > > @@ -23,6 +23,7 @@
> > > > >  #include <asm/types.h>
> > > > >
> > > > >  /* Hyp Configuration Register (HCR) bits */
> > > > > +#define HCR_FWB              (UL(1) << 46)
> > > > >  #define HCR_TEA              (UL(1) << 37)
> > > > >  #define HCR_TERR     (UL(1) << 36)
> > > > >  #define HCR_TLOR     (UL(1) << 35)
> > > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > > > index 1dab3a984608..dd98fdf33d99 100644
> > > > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > > > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > > >               /* trap error record accesses */
> > > > >               vcpu->arch.hcr_el2 |= HCR_TERR;
> > > > >       }
> > > > > +     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > > > > +             vcpu->arch.hcr_el2 |= HCR_FWB;
> > > > >
> > > > >       if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > > > >               vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > > > index fb9a7127bb75..620eb9e06bd8 100644
> > > > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > > > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> > > > >  struct kvm;
> > > > >
> > > > >  #define kvm_flush_dcache_to_poc(a,l) __flush_dcache_area((a), (l))
> > > > > +#define kvm_flush_dcache_to_pou(a,l) __clean_dcache_area_pou((a), (l))
> > > > >
> > > > >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > > > >  {
> > > > > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > > > >  {
> > > > >       void *va = page_address(pfn_to_page(pfn));
> > > > >
> > > > > +     /*
> > > > > +      * FWB implies IDC, so cache clean to PoC is not required in
> > > > > +      * this case.
> > > > > +      */
> > > >
> > > > I don't understand this comment.  __clean_dcache_guest_page is called
> > > > when faulting in pages for translation faults (fault_status != FSC_PERM)
> > > > and PoC is about coherency between ram and caches, not instruction and
> > > > data caches, so how is this related to IDC?
> > >
> > > There's a shortcut in this comment, so let me develop a tiny bit:
> > >
> > > - FWB makes memory the memory cacheable from the PoV of the guest, so
> > >   we don't need to clean to the PoC for the guest to access it even
> > >   when running with the stage-1 MMU off or non-cacheable mappings.
> > >
> > > - FWB implies IDC so there is no need to clean to the PoU for
> > >   instruction fetches to get the right thing either.
> > >
> > > The combination of these two statements implies that we don't need to
> > > clean anything at all.
> > >
> > > Does it make more sense?
> >
> > Yes, this makes sense, but the comment is weird, because it's not
> > because FWB implies IDC that you don't have to clean to PoC, so I would
> > suggest either removing the comment, or rewording it to something like:
> >
> > /*
> >  * With FWB we ensure that the guest always accesses memory cacheable
> >  * and we don't have to clean to PoC when faulting in pages.
> >  * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
> >  * in this case.
> >  */
> >
> > I think the comment as it stands now, only makes sense in the context of
> > the conversation we've had on this patch on the list.
>
> Fair enough. I'll update the comment to reflect the above.
>

Thanks, with that:

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Christoffer Dall]

On Fri, Jun 29, 2018 at 10:21:27AM +0100, Marc Zyngier wrote:
> On Fri, 29 Jun 2018 10:07:50 +0100,
> Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> > > On Thu, 28 Jun 2018 21:56:38 +0100,
> > > Christoffer Dall <christoffer.dall@arm.com> wrote:
> > > >
> > > > On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > > > > Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
> > > > > results in the strongest attribute of the two stages.  This means
> > > > > that the hypervisor has to perform quite a lot of cache maintenance
> > > > > just in case the guest has some non-cacheable mappings around.
> > > > >
> > > > > ARMv8.4 solves this problem by offering a different mode (FWB) where
> > > > > Stage-2 has total control over the memory attribute (this is limited
> > > > > to systems where both I/O and instruction fetches are coherent with
> > > > > the dcache). This is achieved by having a different set of memory
> > > > > attributes in the page tables, and a new bit set in HCR_EL2.
> > > > >
> > > > > On such a system, we can then safely sidestep any form of dcache
> > > > > management.
> > > > >
> > > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > ---
> > > > >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> > > > >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> > > > >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> > > > >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> > > > >  arch/arm64/include/asm/memory.h       |  7 +++++++
> > > > >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> > > > >  arch/arm64/include/asm/sysreg.h       |  1 +
> > > > >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> > > > >  virt/kvm/arm/mmu.c                    |  4 ++++
> > > > >  9 files changed, 69 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > > > > index 8a699c708fc9..ed84d6536830 100644
> > > > > --- a/arch/arm64/include/asm/cpucaps.h
> > > > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > > > @@ -49,7 +49,8 @@
> > > > >  #define ARM64_HAS_CACHE_DIC                  28
> > > > >  #define ARM64_HW_DBM                         29
> > > > >  #define ARM64_SSBD                           30
> > > > > +#define ARM64_HAS_STAGE2_FWB                 31
> > > > >
> > > > > -#define ARM64_NCAPS                          31
> > > > > +#define ARM64_NCAPS                          32
> > > > >
> > > > >  #endif /* __ASM_CPUCAPS_H */
> > > > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > > > > index 6dd285e979c9..aa45df752a16 100644
> > > > > --- a/arch/arm64/include/asm/kvm_arm.h
> > > > > +++ b/arch/arm64/include/asm/kvm_arm.h
> > > > > @@ -23,6 +23,7 @@
> > > > >  #include <asm/types.h>
> > > > >
> > > > >  /* Hyp Configuration Register (HCR) bits */
> > > > > +#define HCR_FWB              (UL(1) << 46)
> > > > >  #define HCR_TEA              (UL(1) << 37)
> > > > >  #define HCR_TERR     (UL(1) << 36)
> > > > >  #define HCR_TLOR     (UL(1) << 35)
> > > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > > > index 1dab3a984608..dd98fdf33d99 100644
> > > > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > > > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > > >               /* trap error record accesses */
> > > > >               vcpu->arch.hcr_el2 |= HCR_TERR;
> > > > >       }
> > > > > +     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > > > > +             vcpu->arch.hcr_el2 |= HCR_FWB;
> > > > >
> > > > >       if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > > > >               vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > > > index fb9a7127bb75..620eb9e06bd8 100644
> > > > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > > > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> > > > >  struct kvm;
> > > > >
> > > > >  #define kvm_flush_dcache_to_poc(a,l) __flush_dcache_area((a), (l))
> > > > > +#define kvm_flush_dcache_to_pou(a,l) __clean_dcache_area_pou((a), (l))
> > > > >
> > > > >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > > > >  {
> > > > > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > > > >  {
> > > > >       void *va = page_address(pfn_to_page(pfn));
> > > > >
> > > > > +     /*
> > > > > +      * FWB implies IDC, so cache clean to PoC is not required in
> > > > > +      * this case.
> > > > > +      */
> > > >
> > > > I don't understand this comment.  __clean_dcache_guest_page is called
> > > > when faulting in pages for translation faults (fault_status != FSC_PERM)
> > > > and PoC is about coherency between ram and caches, not instruction and
> > > > data caches, so how is this related to IDC?
> > >
> > > There's a shortcut in this comment, so let me develop a tiny bit:
> > >
> > > - FWB makes memory the memory cacheable from the PoV of the guest, so
> > >   we don't need to clean to the PoC for the guest to access it even
> > >   when running with the stage-1 MMU off or non-cacheable mappings.
> > >
> > > - FWB implies IDC so there is no need to clean to the PoU for
> > >   instruction fetches to get the right thing either.
> > >
> > > The combination of these two statements implies that we don't need to
> > > clean anything at all.
> > >
> > > Does it make more sense?
> >
> > Yes, this makes sense, but the comment is weird, because it's not
> > because FWB implies IDC that you don't have to clean to PoC, so I would
> > suggest either removing the comment, or rewording it to something like:
> >
> > /*
> >  * With FWB we ensure that the guest always accesses memory cacheable
> >  * and we don't have to clean to PoC when faulting in pages.
> >  * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
> >  * in this case.
> >  */
> >
> > I think the comment as it stands now, only makes sense in the context of
> > the conversation we've had on this patch on the list.
>
> Fair enough. I'll update the comment to reflect the above.
>

Thanks, with that:

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.