[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

The open coded tests for checking whether a PTE maps a page as
uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
which is not guaranteed to work since the type of a mapping is an
index into the MAIR table, not a set of mutually exclusive bits.

Considering that, on arm64, the S2 type definitions use the following
MAIR indexes

    #define MT_S2_NORMAL            0xf
    #define MT_S2_DEVICE_nGnRE      0x1

we have been getting lucky merely because the S2 device mappings also
have the PTE_UXN bit set, which means that a device PTE still does not
equal a normal PTE after masking with the former type.

Instead, implement proper checking against the MAIR indexes that are
known to define uncached memory attributes.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
 arch/arm/kvm/mmu.c               |  5 ++---
 arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 405aa1883307..422973835d41 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
 				       pgd_t *merged_hyp_pgd,
 				       unsigned long hyp_idmap_start) { }
 
+static inline bool __kvm_pte_is_uncached(pte_t pte)
+{
+	switch (pte_val(pte) & L_PTE_MT_MASK) {
+	case L_PTE_MT_UNCACHED:
+	case L_PTE_MT_BUFFERABLE:
+	case L_PTE_MT_DEV_SHARED:
+		return true;
+	}
+	return false;
+}
+
 #endif	/* !__ASSEMBLY__ */
 
 #endif /* __ARM_KVM_MMU_H__ */
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 6984342da13d..eb9a06e3dbee 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
 			kvm_tlb_flush_vmid_ipa(kvm, addr);
 
 			/* No need to invalidate the cache for device mappings */
-			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
+			if (!__kvm_pte_is_uncached(old_pte))
 				kvm_flush_dcache_pte(old_pte);
 
 			put_page(virt_to_page(pte));
@@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
 
 	pte = pte_offset_kernel(pmd, addr);
 	do {
-		if (!pte_none(*pte) &&
-		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
+		if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
 			kvm_flush_dcache_pte(*pte);
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 }
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 61505676d085..5806f412a47a 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
 	merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
 }
 
+static inline bool __kvm_pte_is_uncached(pte_t pte)
+{
+	switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
+	case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
+	case PTE_ATTRINDX(MT_DEVICE_nGnRE):
+	case PTE_ATTRINDX(MT_DEVICE_GRE):
+	case PTE_ATTRINDX(MT_NORMAL_NC):
+		return true;
+	}
+	return false;
+}
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ARM64_KVM_MMU_H__ */
-- 
1.9.1

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

The open coded tests for checking whether a PTE maps a page as
uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
which is not guaranteed to work since the type of a mapping is an
index into the MAIR table, not a set of mutually exclusive bits.

Considering that, on arm64, the S2 type definitions use the following
MAIR indexes

    #define MT_S2_NORMAL            0xf
    #define MT_S2_DEVICE_nGnRE      0x1

we have been getting lucky merely because the S2 device mappings also
have the PTE_UXN bit set, which means that a device PTE still does not
equal a normal PTE after masking with the former type.

Instead, implement proper checking against the MAIR indexes that are
known to define uncached memory attributes.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
 arch/arm/kvm/mmu.c               |  5 ++---
 arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 405aa1883307..422973835d41 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
 				       pgd_t *merged_hyp_pgd,
 				       unsigned long hyp_idmap_start) { }
 
+static inline bool __kvm_pte_is_uncached(pte_t pte)
+{
+	switch (pte_val(pte) & L_PTE_MT_MASK) {
+	case L_PTE_MT_UNCACHED:
+	case L_PTE_MT_BUFFERABLE:
+	case L_PTE_MT_DEV_SHARED:
+		return true;
+	}
+	return false;
+}
+
 #endif	/* !__ASSEMBLY__ */
 
 #endif /* __ARM_KVM_MMU_H__ */
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 6984342da13d..eb9a06e3dbee 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
 			kvm_tlb_flush_vmid_ipa(kvm, addr);
 
 			/* No need to invalidate the cache for device mappings */
-			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
+			if (!__kvm_pte_is_uncached(old_pte))
 				kvm_flush_dcache_pte(old_pte);
 
 			put_page(virt_to_page(pte));
@@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
 
 	pte = pte_offset_kernel(pmd, addr);
 	do {
-		if (!pte_none(*pte) &&
-		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
+		if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
 			kvm_flush_dcache_pte(*pte);
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 }
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 61505676d085..5806f412a47a 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
 	merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
 }
 
+static inline bool __kvm_pte_is_uncached(pte_t pte)
+{
+	switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
+	case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
+	case PTE_ATTRINDX(MT_DEVICE_nGnRE):
+	case PTE_ATTRINDX(MT_DEVICE_GRE):
+	case PTE_ATTRINDX(MT_NORMAL_NC):
+		return true;
+	}
+	return false;
+}
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ARM64_KVM_MMU_H__ */
-- 
1.9.1

RE: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Pavel Fedin]

 Hello!

 I have tested this patch, it also fixes the crash on Exynos5410, and is indeed a better approach.

Tested-by: Pavel Fedin <p.fedin@samsung.com>

CC'ed general KVM mailing list too.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


> -----Original Message-----
> From: kvmarm-bounces@lists.cs.columbia.edu [mailto:kvmarm-bounces@lists.cs.columbia.edu] On
> Behalf Of Ard Biesheuvel
> Sent: Friday, November 06, 2015 2:43 PM
> To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; marc.zyngier@arm.com;
> christoffer.dall@linaro.org
> Cc: Ard Biesheuvel
> Subject: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness
> 
> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is an
> index into the MAIR table, not a set of mutually exclusive bits.
> 
> Considering that, on arm64, the S2 type definitions use the following
> MAIR indexes
> 
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
> 
> we have been getting lucky merely because the S2 device mappings also
> have the PTE_UXN bit set, which means that a device PTE still does not
> equal a normal PTE after masking with the former type.
> 
> Instead, implement proper checking against the MAIR indexes that are
> known to define uncached memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>  arch/arm/kvm/mmu.c               |  5 ++---
>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 405aa1883307..422973835d41 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  				       pgd_t *merged_hyp_pgd,
>  				       unsigned long hyp_idmap_start) { }
> 
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & L_PTE_MT_MASK) {
> +	case L_PTE_MT_UNCACHED:
> +	case L_PTE_MT_BUFFERABLE:
> +	case L_PTE_MT_DEV_SHARED:
> +		return true;
> +	}
> +	return false;
> +}
> +
>  #endif	/* !__ASSEMBLY__ */
> 
>  #endif /* __ARM_KVM_MMU_H__ */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6984342da13d..eb9a06e3dbee 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>  			kvm_tlb_flush_vmid_ipa(kvm, addr);
> 
>  			/* No need to invalidate the cache for device mappings */
> -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +			if (!__kvm_pte_is_uncached(old_pte))
>  				kvm_flush_dcache_pte(old_pte);
> 
>  			put_page(virt_to_page(pte));
> @@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
> 
>  	pte = pte_offset_kernel(pmd, addr);
>  	do {
> -		if (!pte_none(*pte) &&
> -		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +		if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
>  			kvm_flush_dcache_pte(*pte);
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
>  }
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 61505676d085..5806f412a47a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  	merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
>  }
> 
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRE):
> +	case PTE_ATTRINDX(MT_DEVICE_GRE):
> +	case PTE_ATTRINDX(MT_NORMAL_NC):
> +		return true;
> +	}
> +	return false;
> +}
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ARM64_KVM_MMU_H__ */
> --
> 1.9.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Pavel Fedin]

 Hello!

 I have tested this patch, it also fixes the crash on Exynos5410, and is indeed a better approach.

Tested-by: Pavel Fedin <p.fedin@samsung.com>

CC'ed general KVM mailing list too.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


> -----Original Message-----
> From: kvmarm-bounces at lists.cs.columbia.edu [mailto:kvmarm-bounces at lists.cs.columbia.edu] On
> Behalf Of Ard Biesheuvel
> Sent: Friday, November 06, 2015 2:43 PM
> To: linux-arm-kernel at lists.infradead.org; kvmarm at lists.cs.columbia.edu; marc.zyngier at arm.com;
> christoffer.dall at linaro.org
> Cc: Ard Biesheuvel
> Subject: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness
> 
> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is an
> index into the MAIR table, not a set of mutually exclusive bits.
> 
> Considering that, on arm64, the S2 type definitions use the following
> MAIR indexes
> 
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
> 
> we have been getting lucky merely because the S2 device mappings also
> have the PTE_UXN bit set, which means that a device PTE still does not
> equal a normal PTE after masking with the former type.
> 
> Instead, implement proper checking against the MAIR indexes that are
> known to define uncached memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>  arch/arm/kvm/mmu.c               |  5 ++---
>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 405aa1883307..422973835d41 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  				       pgd_t *merged_hyp_pgd,
>  				       unsigned long hyp_idmap_start) { }
> 
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & L_PTE_MT_MASK) {
> +	case L_PTE_MT_UNCACHED:
> +	case L_PTE_MT_BUFFERABLE:
> +	case L_PTE_MT_DEV_SHARED:
> +		return true;
> +	}
> +	return false;
> +}
> +
>  #endif	/* !__ASSEMBLY__ */
> 
>  #endif /* __ARM_KVM_MMU_H__ */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6984342da13d..eb9a06e3dbee 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>  			kvm_tlb_flush_vmid_ipa(kvm, addr);
> 
>  			/* No need to invalidate the cache for device mappings */
> -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +			if (!__kvm_pte_is_uncached(old_pte))
>  				kvm_flush_dcache_pte(old_pte);
> 
>  			put_page(virt_to_page(pte));
> @@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
> 
>  	pte = pte_offset_kernel(pmd, addr);
>  	do {
> -		if (!pte_none(*pte) &&
> -		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +		if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
>  			kvm_flush_dcache_pte(*pte);
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
>  }
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 61505676d085..5806f412a47a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  	merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
>  }
> 
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRE):
> +	case PTE_ATTRINDX(MT_DEVICE_GRE):
> +	case PTE_ATTRINDX(MT_NORMAL_NC):
> +		return true;
> +	}
> +	return false;
> +}
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ARM64_KVM_MMU_H__ */
> --
> 1.9.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Marc Zyngier]

On Fri, 6 Nov 2015 12:43:08 +0100
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is an
> index into the MAIR table, not a set of mutually exclusive bits.
> 
> Considering that, on arm64, the S2 type definitions use the following
> MAIR indexes
> 
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
> 
> we have been getting lucky merely because the S2 device mappings also
> have the PTE_UXN bit set, which means that a device PTE still does not
> equal a normal PTE after masking with the former type.
> 
> Instead, implement proper checking against the MAIR indexes that are
> known to define uncached memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Very well spotted, thanks Ard!

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Without deviation from the norm, progress is not possible.

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Marc Zyngier]

On Fri, 6 Nov 2015 12:43:08 +0100
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is an
> index into the MAIR table, not a set of mutually exclusive bits.
> 
> Considering that, on arm64, the S2 type definitions use the following
> MAIR indexes
> 
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
> 
> we have been getting lucky merely because the S2 device mappings also
> have the PTE_UXN bit set, which means that a device PTE still does not
> equal a normal PTE after masking with the former type.
> 
> Instead, implement proper checking against the MAIR indexes that are
> known to define uncached memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Very well spotted, thanks Ard!

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Christoffer Dall]

On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is an
> index into the MAIR table, not a set of mutually exclusive bits.
> 
> Considering that, on arm64, the S2 type definitions use the following
> MAIR indexes
> 
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
> 
> we have been getting lucky merely because the S2 device mappings also
> have the PTE_UXN bit set, which means that a device PTE still does not
> equal a normal PTE after masking with the former type.
> 
> Instead, implement proper checking against the MAIR indexes that are
> known to define uncached memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>  arch/arm/kvm/mmu.c               |  5 ++---
>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 405aa1883307..422973835d41 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  				       pgd_t *merged_hyp_pgd,
>  				       unsigned long hyp_idmap_start) { }
>  
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & L_PTE_MT_MASK) {
> +	case L_PTE_MT_UNCACHED:
> +	case L_PTE_MT_BUFFERABLE:
> +	case L_PTE_MT_DEV_SHARED:
> +		return true;
> +	}

so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
for stage-2 mappings and PAGE_HYP_DEVICE end up using
L_PTE_MT_DEV_SHARED.

Totally obvious.

> +	return false;
> +}
> +
>  #endif	/* !__ASSEMBLY__ */
>  
>  #endif /* __ARM_KVM_MMU_H__ */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6984342da13d..eb9a06e3dbee 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>  			kvm_tlb_flush_vmid_ipa(kvm, addr);
>  
>  			/* No need to invalidate the cache for device mappings */
> -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +			if (!__kvm_pte_is_uncached(old_pte))
>  				kvm_flush_dcache_pte(old_pte);
>  
>  			put_page(virt_to_page(pte));
> @@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>  
>  	pte = pte_offset_kernel(pmd, addr);
>  	do {
> -		if (!pte_none(*pte) &&
> -		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +		if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
>  			kvm_flush_dcache_pte(*pte);
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
>  }
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 61505676d085..5806f412a47a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  	merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
>  }
>  
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRE):
> +	case PTE_ATTRINDX(MT_DEVICE_GRE):
> +	case PTE_ATTRINDX(MT_NORMAL_NC):
> +		return true;
> +	}
> +	return false;
> +}
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ARM64_KVM_MMU_H__ */
> -- 
> 1.9.1
> 

Thanks for this patch, I'll queue it.

-Christoffer

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Christoffer Dall]

On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is an
> index into the MAIR table, not a set of mutually exclusive bits.
> 
> Considering that, on arm64, the S2 type definitions use the following
> MAIR indexes
> 
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
> 
> we have been getting lucky merely because the S2 device mappings also
> have the PTE_UXN bit set, which means that a device PTE still does not
> equal a normal PTE after masking with the former type.
> 
> Instead, implement proper checking against the MAIR indexes that are
> known to define uncached memory attributes.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>  arch/arm/kvm/mmu.c               |  5 ++---
>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 405aa1883307..422973835d41 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  				       pgd_t *merged_hyp_pgd,
>  				       unsigned long hyp_idmap_start) { }
>  
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & L_PTE_MT_MASK) {
> +	case L_PTE_MT_UNCACHED:
> +	case L_PTE_MT_BUFFERABLE:
> +	case L_PTE_MT_DEV_SHARED:
> +		return true;
> +	}

so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
for stage-2 mappings and PAGE_HYP_DEVICE end up using
L_PTE_MT_DEV_SHARED.

Totally obvious.

> +	return false;
> +}
> +
>  #endif	/* !__ASSEMBLY__ */
>  
>  #endif /* __ARM_KVM_MMU_H__ */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6984342da13d..eb9a06e3dbee 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>  			kvm_tlb_flush_vmid_ipa(kvm, addr);
>  
>  			/* No need to invalidate the cache for device mappings */
> -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +			if (!__kvm_pte_is_uncached(old_pte))
>  				kvm_flush_dcache_pte(old_pte);
>  
>  			put_page(virt_to_page(pte));
> @@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>  
>  	pte = pte_offset_kernel(pmd, addr);
>  	do {
> -		if (!pte_none(*pte) &&
> -		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +		if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
>  			kvm_flush_dcache_pte(*pte);
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
>  }
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 61505676d085..5806f412a47a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>  	merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
>  }
>  
> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> +{
> +	switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
> +	case PTE_ATTRINDX(MT_DEVICE_nGnRE):
> +	case PTE_ATTRINDX(MT_DEVICE_GRE):
> +	case PTE_ATTRINDX(MT_NORMAL_NC):
> +		return true;
> +	}
> +	return false;
> +}
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ARM64_KVM_MMU_H__ */
> -- 
> 1.9.1
> 

Thanks for this patch, I'll queue it.

-Christoffer

Re: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

On 9 November 2015 at 17:21, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
>> The open coded tests for checking whether a PTE maps a page as
>> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
>> which is not guaranteed to work since the type of a mapping is an
>> index into the MAIR table, not a set of mutually exclusive bits.
>>
>> Considering that, on arm64, the S2 type definitions use the following
>> MAIR indexes
>>
>>     #define MT_S2_NORMAL            0xf
>>     #define MT_S2_DEVICE_nGnRE      0x1
>>
>> we have been getting lucky merely because the S2 device mappings also
>> have the PTE_UXN bit set, which means that a device PTE still does not
>> equal a normal PTE after masking with the former type.
>>
>> Instead, implement proper checking against the MAIR indexes that are
>> known to define uncached memory attributes.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>>  arch/arm/kvm/mmu.c               |  5 ++---
>>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>>  3 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index 405aa1883307..422973835d41 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>>                                      pgd_t *merged_hyp_pgd,
>>                                      unsigned long hyp_idmap_start) { }
>>
>> +static inline bool __kvm_pte_is_uncached(pte_t pte)
>> +{
>> +     switch (pte_val(pte) & L_PTE_MT_MASK) {
>> +     case L_PTE_MT_UNCACHED:
>> +     case L_PTE_MT_BUFFERABLE:
>> +     case L_PTE_MT_DEV_SHARED:
>> +             return true;
>> +     }
>
> so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
> these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
> for stage-2 mappings and PAGE_HYP_DEVICE end up using
> L_PTE_MT_DEV_SHARED.
>
> Totally obvious.
>

Hmm, perhaps not. Would you prefer all aliases of the L_PTE_MT_xx
constants that map to device permissions to be listed here?

>> +     return false;
>> +}
>> +
>>  #endif       /* !__ASSEMBLY__ */
>>
>>  #endif /* __ARM_KVM_MMU_H__ */
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 6984342da13d..eb9a06e3dbee 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>>                       kvm_tlb_flush_vmid_ipa(kvm, addr);
>>
>>                       /* No need to invalidate the cache for device mappings */
>> -                     if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> +                     if (!__kvm_pte_is_uncached(old_pte))
>>                               kvm_flush_dcache_pte(old_pte);
>>
>>                       put_page(virt_to_page(pte));
>> @@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>>
>>       pte = pte_offset_kernel(pmd, addr);
>>       do {
>> -             if (!pte_none(*pte) &&
>> -                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> +             if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
>>                       kvm_flush_dcache_pte(*pte);
>>       } while (pte++, addr += PAGE_SIZE, addr != end);
>>  }
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index 61505676d085..5806f412a47a 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>>       merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
>>  }
>>
>> +static inline bool __kvm_pte_is_uncached(pte_t pte)
>> +{
>> +     switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
>> +     case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
>> +     case PTE_ATTRINDX(MT_DEVICE_nGnRE):
>> +     case PTE_ATTRINDX(MT_DEVICE_GRE):
>> +     case PTE_ATTRINDX(MT_NORMAL_NC):
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* __ARM64_KVM_MMU_H__ */
>> --
>> 1.9.1
>>
>
> Thanks for this patch, I'll queue it.
>
> -Christoffer

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

On 9 November 2015 at 17:21, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
>> The open coded tests for checking whether a PTE maps a page as
>> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
>> which is not guaranteed to work since the type of a mapping is an
>> index into the MAIR table, not a set of mutually exclusive bits.
>>
>> Considering that, on arm64, the S2 type definitions use the following
>> MAIR indexes
>>
>>     #define MT_S2_NORMAL            0xf
>>     #define MT_S2_DEVICE_nGnRE      0x1
>>
>> we have been getting lucky merely because the S2 device mappings also
>> have the PTE_UXN bit set, which means that a device PTE still does not
>> equal a normal PTE after masking with the former type.
>>
>> Instead, implement proper checking against the MAIR indexes that are
>> known to define uncached memory attributes.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>>  arch/arm/kvm/mmu.c               |  5 ++---
>>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>>  3 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index 405aa1883307..422973835d41 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>>                                      pgd_t *merged_hyp_pgd,
>>                                      unsigned long hyp_idmap_start) { }
>>
>> +static inline bool __kvm_pte_is_uncached(pte_t pte)
>> +{
>> +     switch (pte_val(pte) & L_PTE_MT_MASK) {
>> +     case L_PTE_MT_UNCACHED:
>> +     case L_PTE_MT_BUFFERABLE:
>> +     case L_PTE_MT_DEV_SHARED:
>> +             return true;
>> +     }
>
> so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
> these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
> for stage-2 mappings and PAGE_HYP_DEVICE end up using
> L_PTE_MT_DEV_SHARED.
>
> Totally obvious.
>

Hmm, perhaps not. Would you prefer all aliases of the L_PTE_MT_xx
constants that map to device permissions to be listed here?

>> +     return false;
>> +}
>> +
>>  #endif       /* !__ASSEMBLY__ */
>>
>>  #endif /* __ARM_KVM_MMU_H__ */
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 6984342da13d..eb9a06e3dbee 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -213,7 +213,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>>                       kvm_tlb_flush_vmid_ipa(kvm, addr);
>>
>>                       /* No need to invalidate the cache for device mappings */
>> -                     if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> +                     if (!__kvm_pte_is_uncached(old_pte))
>>                               kvm_flush_dcache_pte(old_pte);
>>
>>                       put_page(virt_to_page(pte));
>> @@ -305,8 +305,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>>
>>       pte = pte_offset_kernel(pmd, addr);
>>       do {
>> -             if (!pte_none(*pte) &&
>> -                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> +             if (!pte_none(*pte) && !__kvm_pte_is_uncached(*pte))
>>                       kvm_flush_dcache_pte(*pte);
>>       } while (pte++, addr += PAGE_SIZE, addr != end);
>>  }
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index 61505676d085..5806f412a47a 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -302,5 +302,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>>       merged_hyp_pgd[idmap_idx] = __pgd(__pa(boot_hyp_pgd) | PMD_TYPE_TABLE);
>>  }
>>
>> +static inline bool __kvm_pte_is_uncached(pte_t pte)
>> +{
>> +     switch (pte_val(pte) & PTE_ATTRINDX_MASK) {
>> +     case PTE_ATTRINDX(MT_DEVICE_nGnRnE):
>> +     case PTE_ATTRINDX(MT_DEVICE_nGnRE):
>> +     case PTE_ATTRINDX(MT_DEVICE_GRE):
>> +     case PTE_ATTRINDX(MT_NORMAL_NC):
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* __ARM64_KVM_MMU_H__ */
>> --
>> 1.9.1
>>
>
> Thanks for this patch, I'll queue it.
>
> -Christoffer

Re: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Christoffer Dall]

On Mon, Nov 09, 2015 at 05:27:40PM +0100, Ard Biesheuvel wrote:
> On 9 November 2015 at 17:21, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
> >> The open coded tests for checking whether a PTE maps a page as
> >> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> >> which is not guaranteed to work since the type of a mapping is an
> >> index into the MAIR table, not a set of mutually exclusive bits.
> >>
> >> Considering that, on arm64, the S2 type definitions use the following
> >> MAIR indexes
> >>
> >>     #define MT_S2_NORMAL            0xf
> >>     #define MT_S2_DEVICE_nGnRE      0x1
> >>
> >> we have been getting lucky merely because the S2 device mappings also
> >> have the PTE_UXN bit set, which means that a device PTE still does not
> >> equal a normal PTE after masking with the former type.
> >>
> >> Instead, implement proper checking against the MAIR indexes that are
> >> known to define uncached memory attributes.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
> >>  arch/arm/kvm/mmu.c               |  5 ++---
> >>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
> >>  3 files changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> >> index 405aa1883307..422973835d41 100644
> >> --- a/arch/arm/include/asm/kvm_mmu.h
> >> +++ b/arch/arm/include/asm/kvm_mmu.h
> >> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
> >>                                      pgd_t *merged_hyp_pgd,
> >>                                      unsigned long hyp_idmap_start) { }
> >>
> >> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> >> +{
> >> +     switch (pte_val(pte) & L_PTE_MT_MASK) {
> >> +     case L_PTE_MT_UNCACHED:
> >> +     case L_PTE_MT_BUFFERABLE:
> >> +     case L_PTE_MT_DEV_SHARED:
> >> +             return true;
> >> +     }
> >
> > so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
> > these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
> > for stage-2 mappings and PAGE_HYP_DEVICE end up using
> > L_PTE_MT_DEV_SHARED.
> >
> > Totally obvious.
> >
> 
> Hmm, perhaps not. Would you prefer all aliases of the L_PTE_MT_xx
> constants that map to device permissions to be listed here?
> 

Meh, there's no great solution and this code is all the kind of code
that you just need to take the time to understand.  We could add a
comment I suppose, if I got the above correct, I can throw something in?

-Christoffer

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Christoffer Dall]

On Mon, Nov 09, 2015 at 05:27:40PM +0100, Ard Biesheuvel wrote:
> On 9 November 2015 at 17:21, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
> >> The open coded tests for checking whether a PTE maps a page as
> >> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> >> which is not guaranteed to work since the type of a mapping is an
> >> index into the MAIR table, not a set of mutually exclusive bits.
> >>
> >> Considering that, on arm64, the S2 type definitions use the following
> >> MAIR indexes
> >>
> >>     #define MT_S2_NORMAL            0xf
> >>     #define MT_S2_DEVICE_nGnRE      0x1
> >>
> >> we have been getting lucky merely because the S2 device mappings also
> >> have the PTE_UXN bit set, which means that a device PTE still does not
> >> equal a normal PTE after masking with the former type.
> >>
> >> Instead, implement proper checking against the MAIR indexes that are
> >> known to define uncached memory attributes.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
> >>  arch/arm/kvm/mmu.c               |  5 ++---
> >>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
> >>  3 files changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> >> index 405aa1883307..422973835d41 100644
> >> --- a/arch/arm/include/asm/kvm_mmu.h
> >> +++ b/arch/arm/include/asm/kvm_mmu.h
> >> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
> >>                                      pgd_t *merged_hyp_pgd,
> >>                                      unsigned long hyp_idmap_start) { }
> >>
> >> +static inline bool __kvm_pte_is_uncached(pte_t pte)
> >> +{
> >> +     switch (pte_val(pte) & L_PTE_MT_MASK) {
> >> +     case L_PTE_MT_UNCACHED:
> >> +     case L_PTE_MT_BUFFERABLE:
> >> +     case L_PTE_MT_DEV_SHARED:
> >> +             return true;
> >> +     }
> >
> > so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
> > these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
> > for stage-2 mappings and PAGE_HYP_DEVICE end up using
> > L_PTE_MT_DEV_SHARED.
> >
> > Totally obvious.
> >
> 
> Hmm, perhaps not. Would you prefer all aliases of the L_PTE_MT_xx
> constants that map to device permissions to be listed here?
> 

Meh, there's no great solution and this code is all the kind of code
that you just need to take the time to understand.  We could add a
comment I suppose, if I got the above correct, I can throw something in?

-Christoffer

Re: [PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

On 9 November 2015 at 17:35, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Mon, Nov 09, 2015 at 05:27:40PM +0100, Ard Biesheuvel wrote:
>> On 9 November 2015 at 17:21, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> > On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
>> >> The open coded tests for checking whether a PTE maps a page as
>> >> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
>> >> which is not guaranteed to work since the type of a mapping is an
>> >> index into the MAIR table, not a set of mutually exclusive bits.
>> >>
>> >> Considering that, on arm64, the S2 type definitions use the following
>> >> MAIR indexes
>> >>
>> >>     #define MT_S2_NORMAL            0xf
>> >>     #define MT_S2_DEVICE_nGnRE      0x1
>> >>
>> >> we have been getting lucky merely because the S2 device mappings also
>> >> have the PTE_UXN bit set, which means that a device PTE still does not
>> >> equal a normal PTE after masking with the former type.
>> >>
>> >> Instead, implement proper checking against the MAIR indexes that are
>> >> known to define uncached memory attributes.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>> >>  arch/arm/kvm/mmu.c               |  5 ++---
>> >>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>> >>  3 files changed, 25 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> >> index 405aa1883307..422973835d41 100644
>> >> --- a/arch/arm/include/asm/kvm_mmu.h
>> >> +++ b/arch/arm/include/asm/kvm_mmu.h
>> >> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>> >>                                      pgd_t *merged_hyp_pgd,
>> >>                                      unsigned long hyp_idmap_start) { }
>> >>
>> >> +static inline bool __kvm_pte_is_uncached(pte_t pte)
>> >> +{
>> >> +     switch (pte_val(pte) & L_PTE_MT_MASK) {
>> >> +     case L_PTE_MT_UNCACHED:
>> >> +     case L_PTE_MT_BUFFERABLE:
>> >> +     case L_PTE_MT_DEV_SHARED:
>> >> +             return true;
>> >> +     }
>> >
>> > so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
>> > these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
>> > for stage-2 mappings and PAGE_HYP_DEVICE end up using
>> > L_PTE_MT_DEV_SHARED.
>> >
>> > Totally obvious.
>> >
>>
>> Hmm, perhaps not. Would you prefer all aliases of the L_PTE_MT_xx
>> constants that map to device permissions to be listed here?
>>
>
> Meh, there's no great solution and this code is all the kind of code
> that you just need to take the time to understand.  We could add a
> comment I suppose, if I got the above correct, I can throw something in?
>

Actually, I think the patch is wrong, and so is the commit message.

I got confused between HYP mappings and stage 2 mappings. HYP mappings
use an index into the MAIR (which HYP inherits from the kernel) but
the stage 2 mappings have a bit fiield describing the type.

So for one, I think that means that __kvm_pte_is_uncached() cannot be
used for both HYP and stage-2 PTE's, or we'd need to add a parameter
to distinguish between them.

For HYP mappings, we need to compare the MAIR index to values that are
known to refer to device or uncached mappings (as the patch does)
For S2 mappings, we need to mask the MemAttr[5:2] field, and interpret
it according to the description in the ARM ARM, i.e., MemAttr[3:2] ==
0b00 indicates device, MemAttr[3:0] == 0b0101 is uncached memory,
anything else requires cache maintenance.

-- 
Ard.

[PATCH] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

On 9 November 2015 at 17:35, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Mon, Nov 09, 2015 at 05:27:40PM +0100, Ard Biesheuvel wrote:
>> On 9 November 2015 at 17:21, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> > On Fri, Nov 06, 2015 at 12:43:08PM +0100, Ard Biesheuvel wrote:
>> >> The open coded tests for checking whether a PTE maps a page as
>> >> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
>> >> which is not guaranteed to work since the type of a mapping is an
>> >> index into the MAIR table, not a set of mutually exclusive bits.
>> >>
>> >> Considering that, on arm64, the S2 type definitions use the following
>> >> MAIR indexes
>> >>
>> >>     #define MT_S2_NORMAL            0xf
>> >>     #define MT_S2_DEVICE_nGnRE      0x1
>> >>
>> >> we have been getting lucky merely because the S2 device mappings also
>> >> have the PTE_UXN bit set, which means that a device PTE still does not
>> >> equal a normal PTE after masking with the former type.
>> >>
>> >> Instead, implement proper checking against the MAIR indexes that are
>> >> known to define uncached memory attributes.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm/include/asm/kvm_mmu.h   | 11 +++++++++++
>> >>  arch/arm/kvm/mmu.c               |  5 ++---
>> >>  arch/arm64/include/asm/kvm_mmu.h | 12 ++++++++++++
>> >>  3 files changed, 25 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> >> index 405aa1883307..422973835d41 100644
>> >> --- a/arch/arm/include/asm/kvm_mmu.h
>> >> +++ b/arch/arm/include/asm/kvm_mmu.h
>> >> @@ -279,6 +279,17 @@ static inline void __kvm_extend_hypmap(pgd_t *boot_hyp_pgd,
>> >>                                      pgd_t *merged_hyp_pgd,
>> >>                                      unsigned long hyp_idmap_start) { }
>> >>
>> >> +static inline bool __kvm_pte_is_uncached(pte_t pte)
>> >> +{
>> >> +     switch (pte_val(pte) & L_PTE_MT_MASK) {
>> >> +     case L_PTE_MT_UNCACHED:
>> >> +     case L_PTE_MT_BUFFERABLE:
>> >> +     case L_PTE_MT_DEV_SHARED:
>> >> +             return true;
>> >> +     }
>> >
>> > so PTEs created by setting PAGE_S2_DEVICE will end up hitting in one of
>> > these because L_PTE_S2_MT_DEV_SHARED is the same as L_PTE_MT_BUFFERABLE
>> > for stage-2 mappings and PAGE_HYP_DEVICE end up using
>> > L_PTE_MT_DEV_SHARED.
>> >
>> > Totally obvious.
>> >
>>
>> Hmm, perhaps not. Would you prefer all aliases of the L_PTE_MT_xx
>> constants that map to device permissions to be listed here?
>>
>
> Meh, there's no great solution and this code is all the kind of code
> that you just need to take the time to understand.  We could add a
> comment I suppose, if I got the above correct, I can throw something in?
>

Actually, I think the patch is wrong, and so is the commit message.

I got confused between HYP mappings and stage 2 mappings. HYP mappings
use an index into the MAIR (which HYP inherits from the kernel) but
the stage 2 mappings have a bit fiield describing the type.

So for one, I think that means that __kvm_pte_is_uncached() cannot be
used for both HYP and stage-2 PTE's, or we'd need to add a parameter
to distinguish between them.

For HYP mappings, we need to compare the MAIR index to values that are
known to refer to device or uncached mappings (as the patch does)
For S2 mappings, we need to mask the MemAttr[5:2] field, and interpret
it according to the description in the ARM ARM, i.e., MemAttr[3:2] ==
0b00 indicates device, MemAttr[3:0] == 0b0101 is uncached memory,
anything else requires cache maintenance.

-- 
Ard.

Re: [PATCH v2] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

(adding lists)

On 10 November 2015 at 10:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Hi all,
>
> I wonder if this is a better way to address the problem. It looks at
> the nature of the memory rather than the nature of the mapping, which
> is probably a more reliable indicator of whether cache maintenance is
> required when performing the unmap.
>
>
> -----------8<----------------
> The open coded tests for checking whether a PTE maps a page as
> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> which is not guaranteed to work since the type of a mapping is
> not a set of mutually exclusive bits
>
> For HYP mappings, the type is an index into the MAIR table (i.e, the
> index itself does not contain any information whatsoever about the
> type of the mapping), and for stage-2 mappings it is a bit field where
> normal memory and device types are defined as follows:
>
>     #define MT_S2_NORMAL            0xf
>     #define MT_S2_DEVICE_nGnRE      0x1
>
> I.e., masking *and* comparing with the latter matches on the former,
> and we have been getting lucky merely because the S2 device mappings
> also have the PTE_UXN bit set, or we would misidentify memory mappings
> as device mappings.
>
> Since the unmap_range() code path (which contains one instance of the
> flawed test) is used both for HYP mappings and stage-2 mappings, and
> considering the difference between the two, it is non-trivial to fix
> this by rewriting the tests in place, as it would involve passing
> down the type of mapping through all the functions.
>
> However, since HYP mappings and stage-2 mappings both deal with host
> physical addresses, we can simply check whether the mapping is backed
> by memory that is managed by the host kernel, and only perform the
> D-cache maintenance if this is the case.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6984342da13d..7dace909d5cf 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -98,6 +98,11 @@ static void kvm_flush_dcache_pud(pud_t pud)
>         __kvm_flush_dcache_pud(pud);
>  }
>
> +static bool kvm_is_device_pfn(unsigned long pfn)
> +{
> +       return !pfn_valid(pfn);
> +}
> +
>  /**
>   * stage2_dissolve_pmd() - clear and flush huge PMD entry
>   * @kvm:       pointer to kvm structure.
> @@ -213,7 +218,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>                         kvm_tlb_flush_vmid_ipa(kvm, addr);
>
>                         /* No need to invalidate the cache for device mappings */
> -                       if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +                       if (!kvm_is_device_pfn(__phys_to_pfn(addr)))
>                                 kvm_flush_dcache_pte(old_pte);
>
>                         put_page(virt_to_page(pte));
> @@ -305,8 +310,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>
>         pte = pte_offset_kernel(pmd, addr);
>         do {
> -               if (!pte_none(*pte) &&
> -                   (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +               if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
>                         kvm_flush_dcache_pte(*pte);
>         } while (pte++, addr += PAGE_SIZE, addr != end);
>  }
> @@ -1037,11 +1041,6 @@ static bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
>         return kvm_vcpu_dabt_iswrite(vcpu);
>  }
>
> -static bool kvm_is_device_pfn(unsigned long pfn)
> -{
> -       return !pfn_valid(pfn);
> -}
> -
>  /**
>   * stage2_wp_ptes - write protect PMD range
>   * @pmd:       pointer to pmd entry
> --
> 1.9.1
>

RE: [PATCH v2] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Pavel Fedin]

 Hello!

 Tested-by: Pavel Fedin <p.fedin@samsung.com>

 Personally i have a small concern about this way of testing. I know many ports of the kernel to proprietary systems, and they tend to have drivers which just deal with hardcoded physical memory regions on their own, without even registering them in the kernel.
 OTOH:
 1. KVM is not meant to be hacked this way as far as i can understand.
 2. Maintainers, i believe, would say: "Then all problems are problems of authors of those ports".
 3. Actually, this does not invent anything new, but reuses the approach being already used in other parts of the code. And this part is what personally i like.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, November 10, 2015 12:48 PM
> To: Christoffer Dall; Marc Zyngier; KVM devel mailing list; kvmarm@lists.cs.columbia.edu
> Cc: p.fedin@samsung.com; Ard Biesheuvel
> Subject: Re: [PATCH v2] ARM/arm64: KVM: test properly for a PTE's uncachedness
> 
> (adding lists)
> 
> On 10 November 2015 at 10:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > Hi all,
> >
> > I wonder if this is a better way to address the problem. It looks at
> > the nature of the memory rather than the nature of the mapping, which
> > is probably a more reliable indicator of whether cache maintenance is
> > required when performing the unmap.
> >
> >
> > -----------8<----------------
> > The open coded tests for checking whether a PTE maps a page as
> > uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> > which is not guaranteed to work since the type of a mapping is
> > not a set of mutually exclusive bits
> >
> > For HYP mappings, the type is an index into the MAIR table (i.e, the
> > index itself does not contain any information whatsoever about the
> > type of the mapping), and for stage-2 mappings it is a bit field where
> > normal memory and device types are defined as follows:
> >
> >     #define MT_S2_NORMAL            0xf
> >     #define MT_S2_DEVICE_nGnRE      0x1
> >
> > I.e., masking *and* comparing with the latter matches on the former,
> > and we have been getting lucky merely because the S2 device mappings
> > also have the PTE_UXN bit set, or we would misidentify memory mappings
> > as device mappings.
> >
> > Since the unmap_range() code path (which contains one instance of the
> > flawed test) is used both for HYP mappings and stage-2 mappings, and
> > considering the difference between the two, it is non-trivial to fix
> > this by rewriting the tests in place, as it would involve passing
> > down the type of mapping through all the functions.
> >
> > However, since HYP mappings and stage-2 mappings both deal with host
> > physical addresses, we can simply check whether the mapping is backed
> > by memory that is managed by the host kernel, and only perform the
> > D-cache maintenance if this is the case.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm/kvm/mmu.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 6984342da13d..7dace909d5cf 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -98,6 +98,11 @@ static void kvm_flush_dcache_pud(pud_t pud)
> >         __kvm_flush_dcache_pud(pud);
> >  }
> >
> > +static bool kvm_is_device_pfn(unsigned long pfn)
> > +{
> > +       return !pfn_valid(pfn);
> > +}
> > +
> >  /**
> >   * stage2_dissolve_pmd() - clear and flush huge PMD entry
> >   * @kvm:       pointer to kvm structure.
> > @@ -213,7 +218,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
> >                         kvm_tlb_flush_vmid_ipa(kvm, addr);
> >
> >                         /* No need to invalidate the cache for device mappings */
> > -                       if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> > +                       if (!kvm_is_device_pfn(__phys_to_pfn(addr)))
> >                                 kvm_flush_dcache_pte(old_pte);
> >
> >                         put_page(virt_to_page(pte));
> > @@ -305,8 +310,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
> >
> >         pte = pte_offset_kernel(pmd, addr);
> >         do {
> > -               if (!pte_none(*pte) &&
> > -                   (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> > +               if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
> >                         kvm_flush_dcache_pte(*pte);
> >         } while (pte++, addr += PAGE_SIZE, addr != end);
> >  }
> > @@ -1037,11 +1041,6 @@ static bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> >         return kvm_vcpu_dabt_iswrite(vcpu);
> >  }
> >
> > -static bool kvm_is_device_pfn(unsigned long pfn)
> > -{
> > -       return !pfn_valid(pfn);
> > -}
> > -
> >  /**
> >   * stage2_wp_ptes - write protect PMD range
> >   * @pmd:       pointer to pmd entry
> > --
> > 1.9.1
> >

Re: [PATCH v2] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

On 10 November 2015 at 13:22, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Tue, Nov 10, 2015 at 10:45:37AM +0100, Ard Biesheuvel wrote:
>> Hi all,
>>
>> I wonder if this is a better way to address the problem. It looks at
>> the nature of the memory rather than the nature of the mapping, which
>> is probably a more reliable indicator of whether cache maintenance is
>> required when performing the unmap.
>>
>>
>> -----------8<----------------
>> The open coded tests for checking whether a PTE maps a page as
>> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
>> which is not guaranteed to work since the type of a mapping is
>> not a set of mutually exclusive bits
>>
>> For HYP mappings, the type is an index into the MAIR table (i.e, the
>> index itself does not contain any information whatsoever about the
>> type of the mapping), and for stage-2 mappings it is a bit field where
>> normal memory and device types are defined as follows:
>>
>>     #define MT_S2_NORMAL            0xf
>>     #define MT_S2_DEVICE_nGnRE      0x1
>>
>> I.e., masking *and* comparing with the latter matches on the former,
>> and we have been getting lucky merely because the S2 device mappings
>> also have the PTE_UXN bit set, or we would misidentify memory mappings
>> as device mappings.
>>
>> Since the unmap_range() code path (which contains one instance of the
>> flawed test) is used both for HYP mappings and stage-2 mappings, and
>> considering the difference between the two, it is non-trivial to fix
>> this by rewriting the tests in place, as it would involve passing
>> down the type of mapping through all the functions.
>>
>> However, since HYP mappings and stage-2 mappings both deal with host
>> physical addresses, we can simply check whether the mapping is backed
>> by memory that is managed by the host kernel, and only perform the
>> D-cache maintenance if this is the case.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/kvm/mmu.c | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 6984342da13d..7dace909d5cf 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -98,6 +98,11 @@ static void kvm_flush_dcache_pud(pud_t pud)
>>       __kvm_flush_dcache_pud(pud);
>>  }
>>
>> +static bool kvm_is_device_pfn(unsigned long pfn)
>> +{
>> +     return !pfn_valid(pfn);
>> +}
>> +
>>  /**
>>   * stage2_dissolve_pmd() - clear and flush huge PMD entry
>>   * @kvm:     pointer to kvm structure.
>> @@ -213,7 +218,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>>                       kvm_tlb_flush_vmid_ipa(kvm, addr);
>>
>>                       /* No need to invalidate the cache for device mappings */
>> -                     if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> +                     if (!kvm_is_device_pfn(__phys_to_pfn(addr)))
>>                               kvm_flush_dcache_pte(old_pte);
>>
>>                       put_page(virt_to_page(pte));
>> @@ -305,8 +310,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>>
>>       pte = pte_offset_kernel(pmd, addr);
>>       do {
>> -             if (!pte_none(*pte) &&
>> -                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> +             if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
>>                       kvm_flush_dcache_pte(*pte);
>>       } while (pte++, addr += PAGE_SIZE, addr != end);
>>  }
>> @@ -1037,11 +1041,6 @@ static bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
>>       return kvm_vcpu_dabt_iswrite(vcpu);
>>  }
>>
>> -static bool kvm_is_device_pfn(unsigned long pfn)
>> -{
>> -     return !pfn_valid(pfn);
>> -}
>> -
>>  /**
>>   * stage2_wp_ptes - write protect PMD range
>>   * @pmd:     pointer to pmd entry
>> --
>> 1.9.1
>>
>
> So PAGE_HYP_DEVICE is used only to map the vgic-v2 regions and
> PAGE_S2_DEVICE is used to map the vgic VCPU interface and for all memory
> regions where the vma has (vm_flags & VM_PFNMAP).
>
> Will these, and only these, cases be covered by the pfn_valid check?
>

The pfn_valid() check will ensure that cache maintenance is only
performed on regions that are known to the host as memory, are managed
by the host (i.e., there is a struct page associated with them) and
will be accessed by the host via cacheable mappings (they are covered
by the linear mapping, or [on ARM] will be kmap'ed cacheable if they
are highmem). If you look at the commit that introduced these tests
(363ef89f8e9b arm/arm64: KVM: Invalidate data cache on unmap), the
concern it addresses is that the guest may perform uncached accesses
to regions that the host has mapped cacheable, meaning guest writes
may be shadowed by clean cachelines, making them invisble to cache
coherent I/O. So afaict, pfn_valid() is an appropriate check here.

pfn_valid() will not misidentify device regions as memory (unless the
host is really broken) so this should fix Pavel's case. The converse
case (a memory region misidentified as a device) could only happen for
a carve-out (i.e., via the /reserved-memory node) that is mapped
inside the guest via a pass-through (VM_PFNMAP) mapping. That case is
already dodgy, since the guest accesses would be forced
uncached/ordered due to the fact that those mappings are forced
PAGE_S2_DEVICE at stage 2 (as you mention), and would also be
misidentified by the current code (due to the PAGE_S2_DEVICE
attributes)

-- 
Ard.

Re: [PATCH v2] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Christoffer Dall]

On Tue, Nov 10, 2015 at 02:15:45PM +0100, Ard Biesheuvel wrote:
> On 10 November 2015 at 13:22, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Tue, Nov 10, 2015 at 10:45:37AM +0100, Ard Biesheuvel wrote:
> >> Hi all,
> >>
> >> I wonder if this is a better way to address the problem. It looks at
> >> the nature of the memory rather than the nature of the mapping, which
> >> is probably a more reliable indicator of whether cache maintenance is
> >> required when performing the unmap.
> >>
> >>
> >> -----------8<----------------
> >> The open coded tests for checking whether a PTE maps a page as
> >> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
> >> which is not guaranteed to work since the type of a mapping is
> >> not a set of mutually exclusive bits
> >>
> >> For HYP mappings, the type is an index into the MAIR table (i.e, the
> >> index itself does not contain any information whatsoever about the
> >> type of the mapping), and for stage-2 mappings it is a bit field where
> >> normal memory and device types are defined as follows:
> >>
> >>     #define MT_S2_NORMAL            0xf
> >>     #define MT_S2_DEVICE_nGnRE      0x1
> >>
> >> I.e., masking *and* comparing with the latter matches on the former,
> >> and we have been getting lucky merely because the S2 device mappings
> >> also have the PTE_UXN bit set, or we would misidentify memory mappings
> >> as device mappings.
> >>
> >> Since the unmap_range() code path (which contains one instance of the
> >> flawed test) is used both for HYP mappings and stage-2 mappings, and
> >> considering the difference between the two, it is non-trivial to fix
> >> this by rewriting the tests in place, as it would involve passing
> >> down the type of mapping through all the functions.
> >>
> >> However, since HYP mappings and stage-2 mappings both deal with host
> >> physical addresses, we can simply check whether the mapping is backed
> >> by memory that is managed by the host kernel, and only perform the
> >> D-cache maintenance if this is the case.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm/kvm/mmu.c | 15 +++++++--------
> >>  1 file changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index 6984342da13d..7dace909d5cf 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -98,6 +98,11 @@ static void kvm_flush_dcache_pud(pud_t pud)
> >>       __kvm_flush_dcache_pud(pud);
> >>  }
> >>
> >> +static bool kvm_is_device_pfn(unsigned long pfn)
> >> +{
> >> +     return !pfn_valid(pfn);
> >> +}
> >> +
> >>  /**
> >>   * stage2_dissolve_pmd() - clear and flush huge PMD entry
> >>   * @kvm:     pointer to kvm structure.
> >> @@ -213,7 +218,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
> >>                       kvm_tlb_flush_vmid_ipa(kvm, addr);
> >>
> >>                       /* No need to invalidate the cache for device mappings */
> >> -                     if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> >> +                     if (!kvm_is_device_pfn(__phys_to_pfn(addr)))
> >>                               kvm_flush_dcache_pte(old_pte);
> >>
> >>                       put_page(virt_to_page(pte));
> >> @@ -305,8 +310,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
> >>
> >>       pte = pte_offset_kernel(pmd, addr);
> >>       do {
> >> -             if (!pte_none(*pte) &&
> >> -                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> >> +             if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
> >>                       kvm_flush_dcache_pte(*pte);
> >>       } while (pte++, addr += PAGE_SIZE, addr != end);
> >>  }
> >> @@ -1037,11 +1041,6 @@ static bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> >>       return kvm_vcpu_dabt_iswrite(vcpu);
> >>  }
> >>
> >> -static bool kvm_is_device_pfn(unsigned long pfn)
> >> -{
> >> -     return !pfn_valid(pfn);
> >> -}
> >> -
> >>  /**
> >>   * stage2_wp_ptes - write protect PMD range
> >>   * @pmd:     pointer to pmd entry
> >> --
> >> 1.9.1
> >>
> >
> > So PAGE_HYP_DEVICE is used only to map the vgic-v2 regions and
> > PAGE_S2_DEVICE is used to map the vgic VCPU interface and for all memory
> > regions where the vma has (vm_flags & VM_PFNMAP).
> >
> > Will these, and only these, cases be covered by the pfn_valid check?
> >
> 
> The pfn_valid() check will ensure that cache maintenance is only
> performed on regions that are known to the host as memory, are managed
> by the host (i.e., there is a struct page associated with them) and
> will be accessed by the host via cacheable mappings (they are covered
> by the linear mapping, or [on ARM] will be kmap'ed cacheable if they
> are highmem). If you look at the commit that introduced these tests
> (363ef89f8e9b arm/arm64: KVM: Invalidate data cache on unmap), the
> concern it addresses is that the guest may perform uncached accesses
> to regions that the host has mapped cacheable, meaning guest writes
> may be shadowed by clean cachelines, making them invisble to cache
> coherent I/O. So afaict, pfn_valid() is an appropriate check here.

right, this I agree with.

> 
> pfn_valid() will not misidentify device regions as memory (unless the
> host is really broken) so this should fix Pavel's case. The converse
> case (a memory region misidentified as a device) could only happen for
> a carve-out (i.e., via the /reserved-memory node) that is mapped
> inside the guest via a pass-through (VM_PFNMAP) mapping. That case is
> already dodgy, since the guest accesses would be forced
> uncached/ordered due to the fact that those mappings are forced
> PAGE_S2_DEVICE at stage 2 (as you mention), and would also be
> misidentified by the current code (due to the PAGE_S2_DEVICE
> attributes)
> 

ok, but such pages should never be swapped out by the host, so I think
we're still ok here.

Will you send an updated proper patch to the list?  You can add my
RB if you want.

-Christoffer

Re: [PATCH v2] ARM/arm64: KVM: test properly for a PTE's uncachedness

[Ard Biesheuvel]

On 10 November 2015 at 14:40, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Tue, Nov 10, 2015 at 02:15:45PM +0100, Ard Biesheuvel wrote:
>> On 10 November 2015 at 13:22, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> > On Tue, Nov 10, 2015 at 10:45:37AM +0100, Ard Biesheuvel wrote:
>> >> Hi all,
>> >>
>> >> I wonder if this is a better way to address the problem. It looks at
>> >> the nature of the memory rather than the nature of the mapping, which
>> >> is probably a more reliable indicator of whether cache maintenance is
>> >> required when performing the unmap.
>> >>
>> >>
>> >> -----------8<----------------
>> >> The open coded tests for checking whether a PTE maps a page as
>> >> uncached use a flawed 'pte_val(xxx) & CONST != CONST' pattern,
>> >> which is not guaranteed to work since the type of a mapping is
>> >> not a set of mutually exclusive bits
>> >>
>> >> For HYP mappings, the type is an index into the MAIR table (i.e, the
>> >> index itself does not contain any information whatsoever about the
>> >> type of the mapping), and for stage-2 mappings it is a bit field where
>> >> normal memory and device types are defined as follows:
>> >>
>> >>     #define MT_S2_NORMAL            0xf
>> >>     #define MT_S2_DEVICE_nGnRE      0x1
>> >>
>> >> I.e., masking *and* comparing with the latter matches on the former,
>> >> and we have been getting lucky merely because the S2 device mappings
>> >> also have the PTE_UXN bit set, or we would misidentify memory mappings
>> >> as device mappings.
>> >>
>> >> Since the unmap_range() code path (which contains one instance of the
>> >> flawed test) is used both for HYP mappings and stage-2 mappings, and
>> >> considering the difference between the two, it is non-trivial to fix
>> >> this by rewriting the tests in place, as it would involve passing
>> >> down the type of mapping through all the functions.
>> >>
>> >> However, since HYP mappings and stage-2 mappings both deal with host
>> >> physical addresses, we can simply check whether the mapping is backed
>> >> by memory that is managed by the host kernel, and only perform the
>> >> D-cache maintenance if this is the case.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm/kvm/mmu.c | 15 +++++++--------
>> >>  1 file changed, 7 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> >> index 6984342da13d..7dace909d5cf 100644
>> >> --- a/arch/arm/kvm/mmu.c
>> >> +++ b/arch/arm/kvm/mmu.c
>> >> @@ -98,6 +98,11 @@ static void kvm_flush_dcache_pud(pud_t pud)
>> >>       __kvm_flush_dcache_pud(pud);
>> >>  }
>> >>
>> >> +static bool kvm_is_device_pfn(unsigned long pfn)
>> >> +{
>> >> +     return !pfn_valid(pfn);
>> >> +}
>> >> +
>> >>  /**
>> >>   * stage2_dissolve_pmd() - clear and flush huge PMD entry
>> >>   * @kvm:     pointer to kvm structure.
>> >> @@ -213,7 +218,7 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>> >>                       kvm_tlb_flush_vmid_ipa(kvm, addr);
>> >>
>> >>                       /* No need to invalidate the cache for device mappings */
>> >> -                     if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> >> +                     if (!kvm_is_device_pfn(__phys_to_pfn(addr)))
>> >>                               kvm_flush_dcache_pte(old_pte);
>> >>
>> >>                       put_page(virt_to_page(pte));
>> >> @@ -305,8 +310,7 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>> >>
>> >>       pte = pte_offset_kernel(pmd, addr);
>> >>       do {
>> >> -             if (!pte_none(*pte) &&
>> >> -                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>> >> +             if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
>> >>                       kvm_flush_dcache_pte(*pte);
>> >>       } while (pte++, addr += PAGE_SIZE, addr != end);
>> >>  }
>> >> @@ -1037,11 +1041,6 @@ static bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
>> >>       return kvm_vcpu_dabt_iswrite(vcpu);
>> >>  }
>> >>
>> >> -static bool kvm_is_device_pfn(unsigned long pfn)
>> >> -{
>> >> -     return !pfn_valid(pfn);
>> >> -}
>> >> -
>> >>  /**
>> >>   * stage2_wp_ptes - write protect PMD range
>> >>   * @pmd:     pointer to pmd entry
>> >> --
>> >> 1.9.1
>> >>
>> >
>> > So PAGE_HYP_DEVICE is used only to map the vgic-v2 regions and
>> > PAGE_S2_DEVICE is used to map the vgic VCPU interface and for all memory
>> > regions where the vma has (vm_flags & VM_PFNMAP).
>> >
>> > Will these, and only these, cases be covered by the pfn_valid check?
>> >
>>
>> The pfn_valid() check will ensure that cache maintenance is only
>> performed on regions that are known to the host as memory, are managed
>> by the host (i.e., there is a struct page associated with them) and
>> will be accessed by the host via cacheable mappings (they are covered
>> by the linear mapping, or [on ARM] will be kmap'ed cacheable if they
>> are highmem). If you look at the commit that introduced these tests
>> (363ef89f8e9b arm/arm64: KVM: Invalidate data cache on unmap), the
>> concern it addresses is that the guest may perform uncached accesses
>> to regions that the host has mapped cacheable, meaning guest writes
>> may be shadowed by clean cachelines, making them invisble to cache
>> coherent I/O. So afaict, pfn_valid() is an appropriate check here.
>
> right, this I agree with.
>
>>
>> pfn_valid() will not misidentify device regions as memory (unless the
>> host is really broken) so this should fix Pavel's case. The converse
>> case (a memory region misidentified as a device) could only happen for
>> a carve-out (i.e., via the /reserved-memory node) that is mapped
>> inside the guest via a pass-through (VM_PFNMAP) mapping. That case is
>> already dodgy, since the guest accesses would be forced
>> uncached/ordered due to the fact that those mappings are forced
>> PAGE_S2_DEVICE at stage 2 (as you mention), and would also be
>> misidentified by the current code (due to the PAGE_S2_DEVICE
>> attributes)
>>
>
> ok, but such pages should never be swapped out by the host, so I think
> we're still ok here.
>

Yes, I think so, and the patch does not change how that case is handled anyway.

> Will you send an updated proper patch to the list?  You can add my
> RB if you want.
>

Thanks. I will resend with your R-b and Pavel's Tested-by added.

-- 
Ard.