* [kvm-unit-tests PATCH v3 01/18] lib: arm/arm64: Remove unnecessary dcache maintenance operations
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 02/18] lib: arm: Add proper data synchronization barriers for TLBIs Alexandru Elisei
` (17 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
On ARMv7 with multiprocessing extensions (which are mandated by the
virtualization extensions [1]), and on ARMv8, translation table walks are
coherent [2, 3], which means that no dcache maintenance operations are
required when changing the tables. Remove the maintenance operations so
that we do only the minimum required to ensure correctness.
Translation table walks are coherent if the memory where the tables
themselves reside have the same shareability and cacheability attributes
as the translation table walks. For ARMv8, this is already the case, and
it is only a matter of removing the cache operations.
However, for ARMv7, translation table walks were being configured as
Non-shareable (TTBCR.SH0 = 0b00) and Non-cacheable
(TTBCR.{I,O}RGN0 = 0b00). Fix that by marking them as Inner Shareable,
Normal memory, Inner and Outer Write-Back Write-Allocate Cacheable.
Because translation table walks are now coherent on arm, replace the
TLBIMVAA operation with TLBIMVAAIS in flush_tlb_page, which acts on the
Inner Shareable domain instead of being private to the PE.
The functions that update the translation table are called when the MMU
is off, or to modify permissions, in the case of the cache test, so
break-before-make is not necessary.
[1] ARM DDI 0406C.d, section B1.7
[2] ARM DDI 0406C.d, section B3.3.1
[3] ARM DDI 0487E.a, section D13.2.72
[4] ARM DDI 0487E.a, section K11.5.3
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/mmu.h | 4 ++--
lib/arm/asm/pgtable-hwdef.h | 8 ++++++++
lib/arm/mmu.c | 14 +-------------
arm/cstart.S | 7 +++++--
4 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/lib/arm/asm/mmu.h b/lib/arm/asm/mmu.h
index 915c2b07dead..361f3cdcc3d5 100644
--- a/lib/arm/asm/mmu.h
+++ b/lib/arm/asm/mmu.h
@@ -31,8 +31,8 @@ static inline void flush_tlb_all(void)
static inline void flush_tlb_page(unsigned long vaddr)
{
- /* TLBIMVAA */
- asm volatile("mcr p15, 0, %0, c8, c7, 3" :: "r" (vaddr));
+ /* TLBIMVAAIS */
+ asm volatile("mcr p15, 0, %0, c8, c3, 3" :: "r" (vaddr));
dsb();
isb();
}
diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
index c08e6e2c01b4..4f24c78ee011 100644
--- a/lib/arm/asm/pgtable-hwdef.h
+++ b/lib/arm/asm/pgtable-hwdef.h
@@ -108,4 +108,12 @@
#define PHYS_MASK_SHIFT (40)
#define PHYS_MASK ((_AC(1, ULL) << PHYS_MASK_SHIFT) - 1)
+#define TTBCR_IRGN0_WBWA (_AC(1, UL) << 8)
+#define TTBCR_ORGN0_WBWA (_AC(1, UL) << 10)
+#define TTBCR_SH0_SHARED (_AC(3, UL) << 12)
+#define TTBCR_IRGN1_WBWA (_AC(1, UL) << 24)
+#define TTBCR_ORGN1_WBWA (_AC(1, UL) << 26)
+#define TTBCR_SH1_SHARED (_AC(3, UL) << 28)
+#define TTBCR_EAE (_AC(1, UL) << 31)
+
#endif /* _ASMARM_PGTABLE_HWDEF_H_ */
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 78db22e6af14..5c31c00ccb31 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -73,17 +73,6 @@ void mmu_disable(void)
asm_mmu_disable();
}
-static void flush_entry(pgd_t *pgtable, uintptr_t vaddr)
-{
- pgd_t *pgd = pgd_offset(pgtable, vaddr);
- pmd_t *pmd = pmd_offset(pgd, vaddr);
-
- flush_dcache_addr((ulong)pgd);
- flush_dcache_addr((ulong)pmd);
- flush_dcache_addr((ulong)pte_offset(pmd, vaddr));
- flush_tlb_page(vaddr);
-}
-
static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
{
pgd_t *pgd = pgd_offset(pgtable, vaddr);
@@ -98,7 +87,7 @@ static pteval_t *install_pte(pgd_t *pgtable, uintptr_t vaddr, pteval_t pte)
pteval_t *p_pte = get_pte(pgtable, vaddr);
*p_pte = pte;
- flush_entry(pgtable, vaddr);
+ flush_tlb_page(vaddr);
return p_pte;
}
@@ -148,7 +137,6 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
pgd_val(*pgd) = paddr;
pgd_val(*pgd) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
pgd_val(*pgd) |= pgprot_val(prot);
- flush_dcache_addr((ulong)pgd);
flush_tlb_page(vaddr);
}
}
diff --git a/arm/cstart.S b/arm/cstart.S
index 114726feab82..2c81d39a666b 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -9,6 +9,7 @@
#include <auxinfo.h>
#include <asm/thread_info.h>
#include <asm/asm-offsets.h>
+#include <asm/pgtable-hwdef.h>
#include <asm/ptrace.h>
#include <asm/sysreg.h>
@@ -154,9 +155,11 @@ halt:
.globl asm_mmu_enable
asm_mmu_enable:
/* TTBCR */
- mrc p15, 0, r2, c2, c0, 2
- orr r2, #(1 << 31) @ TTB_EAE
+ ldr r2, =(TTBCR_EAE | \
+ TTBCR_SH0_SHARED | \
+ TTBCR_IRGN0_WBWA | TTBCR_ORGN0_WBWA)
mcr p15, 0, r2, c2, c0, 2
+ isb
/* MAIR */
ldr r2, =PRRR
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 02/18] lib: arm: Add proper data synchronization barriers for TLBIs
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 01/18] lib: arm/arm64: Remove unnecessary dcache maintenance operations Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 03/18] lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h Alexandru Elisei
` (16 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
We need to issue a DSB before doing TLB invalidation to make sure that the
table walker sees the new VA mapping after the TLBI finishes. For
flush_tlb_page, we do a DSB ISHST (synchronization barrier for writes in
the Inner Shareable domain) because translation table walks are now
coherent for arm. For local_flush_tlb_all, we only need to affect the
Non-shareable domain, and we do a DSB NSHST. We need a synchronization
barrier here, and not a memory ordering barrier, because a table walk is
not a memory operation and therefore not affected by the DMB.
For the same reasons, we downgrade the full system DSB after the TLBI to a
DSB ISH (synchronization barrier for reads and writes in the Inner
Shareable domain), and, respectively, DSB NSH (in the Non-shareable
domain).
With these two changes, our TLB maintenance functions now match what Linux
does in __flush_tlb_kernel_page, and, respectively, in local_flush_tlb_all.
A similar change was implemented in Linux commit 62cbbc42e001 ("ARM: tlb:
reduce scope of barrier domains for TLB invalidation").
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/mmu.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/arm/asm/mmu.h b/lib/arm/asm/mmu.h
index 361f3cdcc3d5..2bf8965ed35e 100644
--- a/lib/arm/asm/mmu.h
+++ b/lib/arm/asm/mmu.h
@@ -17,9 +17,10 @@
static inline void local_flush_tlb_all(void)
{
+ dsb(nshst);
/* TLBIALL */
asm volatile("mcr p15, 0, %0, c8, c7, 0" :: "r" (0));
- dsb();
+ dsb(nsh);
isb();
}
@@ -31,9 +32,10 @@ static inline void flush_tlb_all(void)
static inline void flush_tlb_page(unsigned long vaddr)
{
+ dsb(ishst);
/* TLBIMVAAIS */
asm volatile("mcr p15, 0, %0, c8, c3, 3" :: "r" (vaddr));
- dsb();
+ dsb(ish);
isb();
}
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 03/18] lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 01/18] lib: arm/arm64: Remove unnecessary dcache maintenance operations Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 02/18] lib: arm: Add proper data synchronization barriers for TLBIs Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-02 18:03 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 04/18] lib: arm/arm64: Use WRITE_ONCE to update the translation tables Alexandru Elisei
` (15 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm
Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin,
mark.rutland, Laurent Vivier, Thomas Huth, David Hildenbrand
Add the WRITE_ONCE and READ_ONCE macros which are used to prevent the
compiler from optimizing a store or a load, respectively, into something
else.
Cc: Drew Jones <drjones@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/linux/compiler.h | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
create mode 100644 lib/linux/compiler.h
diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h
new file mode 100644
index 000000000000..2d72f18c36e5
--- /dev/null
+++ b/lib/linux/compiler.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Taken from Linux commit 219d54332a09 ("Linux 5.4"), from the file
+ * tools/include/linux/compiler.h, with minor changes.
+ */
+#ifndef __LINUX_COMPILER_H
+#define __LINUX_COMPILER_H
+
+#ifndef __ASSEMBLY__
+
+#include <stdint.h>
+
+#define barrier() asm volatile("" : : : "memory")
+
+#define __always_inline inline __attribute__((always_inline))
+
+static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
+{
+ switch (size) {
+ case 1: *(uint8_t *)res = *(volatile uint8_t *)p; break;
+ case 2: *(uint16_t *)res = *(volatile uint16_t *)p; break;
+ case 4: *(uint32_t *)res = *(volatile uint32_t *)p; break;
+ case 8: *(uint64_t *)res = *(volatile uint64_t *)p; break;
+ default:
+ barrier();
+ __builtin_memcpy((void *)res, (const void *)p, size);
+ barrier();
+ }
+}
+
+/*
+ * Prevent the compiler from merging or refetching reads or writes. The
+ * compiler is also forbidden from reordering successive instances of
+ * READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
+ * particular ordering. One way to make the compiler aware of ordering is to
+ * put the two invocations of READ_ONCE or WRITE_ONCE in different C
+ * statements.
+ *
+ * These two macros will also work on aggregate data types like structs or
+ * unions. If the size of the accessed data type exceeds the word size of
+ * the machine (e.g., 32 bits or 64 bits) READ_ONCE() and WRITE_ONCE() will
+ * fall back to memcpy and print a compile-time warning.
+ *
+ * Their two major use cases are: (1) Mediating communication between
+ * process-level code and irq/NMI handlers, all running on the same CPU,
+ * and (2) Ensuring that the compiler does not fold, spindle, or otherwise
+ * mutilate accesses that either do not require ordering or that interact
+ * with an explicit memory barrier or atomic instruction that provides the
+ * required ordering.
+ */
+
+#define READ_ONCE(x) \
+({ \
+ union { typeof(x) __val; char __c[1]; } __u = \
+ { .__c = { 0 } }; \
+ __read_once_size(&(x), __u.__c, sizeof(x)); \
+ __u.__val; \
+})
+
+static __always_inline void __write_once_size(volatile void *p, void *res, int size)
+{
+ switch (size) {
+ case 1: *(volatile uint8_t *) p = *(uint8_t *) res; break;
+ case 2: *(volatile uint16_t *) p = *(uint16_t *) res; break;
+ case 4: *(volatile uint32_t *) p = *(uint32_t *) res; break;
+ case 8: *(volatile uint64_t *) p = *(uint64_t *) res; break;
+ default:
+ barrier();
+ __builtin_memcpy((void *)p, (const void *)res, size);
+ barrier();
+ }
+}
+
+#define WRITE_ONCE(x, val) \
+({ \
+ union { typeof(x) __val; char __c[1]; } __u = \
+ { .__val = (val) }; \
+ __write_once_size(&(x), __u.__c, sizeof(x)); \
+ __u.__val; \
+})
+
+#endif /* !__ASSEMBLY__ */
+#endif /* !__LINUX_COMPILER_H */
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 03/18] lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 03/18] lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h Alexandru Elisei
@ 2020-01-02 18:03 ` Andre Przywara
0 siblings, 0 replies; 47+ messages in thread
From: Andre Przywara @ 2020-01-02 18:03 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland,
Laurent Vivier, Thomas Huth, David Hildenbrand
On Tue, 31 Dec 2019 16:09:34 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> Add the WRITE_ONCE and READ_ONCE macros which are used to prevent the
> compiler from optimizing a store or a load, respectively, into something
> else.
Compared to the Linux version and found to be equivalent:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> Cc: Drew Jones <drjones@redhat.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> lib/linux/compiler.h | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 83 insertions(+)
> create mode 100644 lib/linux/compiler.h
>
> diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h
> new file mode 100644
> index 000000000000..2d72f18c36e5
> --- /dev/null
> +++ b/lib/linux/compiler.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Taken from Linux commit 219d54332a09 ("Linux 5.4"), from the file
> + * tools/include/linux/compiler.h, with minor changes.
> + */
> +#ifndef __LINUX_COMPILER_H
> +#define __LINUX_COMPILER_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <stdint.h>
> +
> +#define barrier() asm volatile("" : : : "memory")
> +
> +#define __always_inline inline __attribute__((always_inline))
> +
> +static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
> +{
> + switch (size) {
> + case 1: *(uint8_t *)res = *(volatile uint8_t *)p; break;
> + case 2: *(uint16_t *)res = *(volatile uint16_t *)p; break;
> + case 4: *(uint32_t *)res = *(volatile uint32_t *)p; break;
> + case 8: *(uint64_t *)res = *(volatile uint64_t *)p; break;
> + default:
> + barrier();
> + __builtin_memcpy((void *)res, (const void *)p, size);
> + barrier();
> + }
> +}
> +
> +/*
> + * Prevent the compiler from merging or refetching reads or writes. The
> + * compiler is also forbidden from reordering successive instances of
> + * READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
> + * particular ordering. One way to make the compiler aware of ordering is to
> + * put the two invocations of READ_ONCE or WRITE_ONCE in different C
> + * statements.
> + *
> + * These two macros will also work on aggregate data types like structs or
> + * unions. If the size of the accessed data type exceeds the word size of
> + * the machine (e.g., 32 bits or 64 bits) READ_ONCE() and WRITE_ONCE() will
> + * fall back to memcpy and print a compile-time warning.
> + *
> + * Their two major use cases are: (1) Mediating communication between
> + * process-level code and irq/NMI handlers, all running on the same CPU,
> + * and (2) Ensuring that the compiler does not fold, spindle, or otherwise
> + * mutilate accesses that either do not require ordering or that interact
> + * with an explicit memory barrier or atomic instruction that provides the
> + * required ordering.
> + */
> +
> +#define READ_ONCE(x) \
> +({ \
> + union { typeof(x) __val; char __c[1]; } __u = \
> + { .__c = { 0 } }; \
> + __read_once_size(&(x), __u.__c, sizeof(x)); \
> + __u.__val; \
> +})
> +
> +static __always_inline void __write_once_size(volatile void *p, void *res, int size)
> +{
> + switch (size) {
> + case 1: *(volatile uint8_t *) p = *(uint8_t *) res; break;
> + case 2: *(volatile uint16_t *) p = *(uint16_t *) res; break;
> + case 4: *(volatile uint32_t *) p = *(uint32_t *) res; break;
> + case 8: *(volatile uint64_t *) p = *(uint64_t *) res; break;
> + default:
> + barrier();
> + __builtin_memcpy((void *)p, (const void *)res, size);
> + barrier();
> + }
> +}
> +
> +#define WRITE_ONCE(x, val) \
> +({ \
> + union { typeof(x) __val; char __c[1]; } __u = \
> + { .__val = (val) }; \
> + __write_once_size(&(x), __u.__c, sizeof(x)); \
> + __u.__val; \
> +})
> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* !__LINUX_COMPILER_H */
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 04/18] lib: arm/arm64: Use WRITE_ONCE to update the translation tables
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (2 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 03/18] lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-02 18:06 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 05/18] lib: arm/arm64: Remove unused CPU_OFF parameter Alexandru Elisei
` (14 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
Use WRITE_ONCE to prevent store tearing when updating an entry in the
translation tables. Without WRITE_ONCE, the compiler, even though it is
unlikely, can emit several stores when changing the table, and we might
end up with bogus TLB entries.
It's worth noting that the existing code is mostly fine without any
changes because the translation tables are updated in one of the
following situations:
- When the tables are being created with the MMU off, which means no TLB
caching is being performed.
- When new page table entries are added as a result of vmalloc'ing a
stack for a secondary CPU, which doesn't happen very often.
- When clearing the PTE_USER bit for the cache test, and store tearing
has no effect on the table walker because there are no intermediate
values between bit values 0 and 1. We still use WRITE_ONCE in this case
for consistency.
However, the functions are global and there is nothing preventing someone
from writing a test that uses them in a different scenario. Let's make
sure that when that happens, there will be no breakage once in a blue
moon.
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/pgtable.h | 12 ++++++++----
lib/arm64/asm/pgtable.h | 7 +++++--
lib/arm/mmu.c | 19 +++++++++++++------
3 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
index 241dff69b38a..794514b8c927 100644
--- a/lib/arm/asm/pgtable.h
+++ b/lib/arm/asm/pgtable.h
@@ -19,6 +19,8 @@
* because we always allocate their pages with alloc_page(), and
* alloc_page() always returns identity mapped pages.
*/
+#include <linux/compiler.h>
+
#define pgtable_va(x) ((void *)(unsigned long)(x))
#define pgtable_pa(x) ((unsigned long)(x))
@@ -58,8 +60,9 @@ static inline pmd_t *pmd_alloc_one(void)
static inline pmd_t *pmd_alloc(pgd_t *pgd, unsigned long addr)
{
if (pgd_none(*pgd)) {
- pmd_t *pmd = pmd_alloc_one();
- pgd_val(*pgd) = pgtable_pa(pmd) | PMD_TYPE_TABLE;
+ pgd_t entry;
+ pgd_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
+ WRITE_ONCE(*pgd, entry);
}
return pmd_offset(pgd, addr);
}
@@ -84,8 +87,9 @@ static inline pte_t *pte_alloc_one(void)
static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
{
if (pmd_none(*pmd)) {
- pte_t *pte = pte_alloc_one();
- pmd_val(*pmd) = pgtable_pa(pte) | PMD_TYPE_TABLE;
+ pmd_t entry;
+ pmd_val(entry) = pgtable_pa(pte_alloc_one()) | PMD_TYPE_TABLE;
+ WRITE_ONCE(*pmd, entry);
}
return pte_offset(pmd, addr);
}
diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
index ee0a2c88cc18..dbf9e7253b71 100644
--- a/lib/arm64/asm/pgtable.h
+++ b/lib/arm64/asm/pgtable.h
@@ -18,6 +18,8 @@
#include <asm/page.h>
#include <asm/pgtable-hwdef.h>
+#include <linux/compiler.h>
+
/*
* We can convert va <=> pa page table addresses with simple casts
* because we always allocate their pages with alloc_page(), and
@@ -66,8 +68,9 @@ static inline pte_t *pte_alloc_one(void)
static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
{
if (pmd_none(*pmd)) {
- pte_t *pte = pte_alloc_one();
- pmd_val(*pmd) = pgtable_pa(pte) | PMD_TYPE_TABLE;
+ pmd_t entry;
+ pmd_val(entry) = pgtable_pa(pte_alloc_one()) | PMD_TYPE_TABLE;
+ WRITE_ONCE(*pmd, entry);
}
return pte_offset(pmd, addr);
}
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 5c31c00ccb31..86a829966a3c 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -17,6 +17,8 @@
#include <asm/pgtable-hwdef.h>
#include <asm/pgtable.h>
+#include <linux/compiler.h>
+
extern unsigned long etext;
pgd_t *mmu_idmap;
@@ -86,7 +88,7 @@ static pteval_t *install_pte(pgd_t *pgtable, uintptr_t vaddr, pteval_t pte)
{
pteval_t *p_pte = get_pte(pgtable, vaddr);
- *p_pte = pte;
+ WRITE_ONCE(*p_pte, pte);
flush_tlb_page(vaddr);
return p_pte;
}
@@ -131,12 +133,15 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
phys_addr_t paddr = phys_start & PGDIR_MASK;
uintptr_t vaddr = virt_offset & PGDIR_MASK;
uintptr_t virt_end = phys_end - paddr + vaddr;
+ pgd_t *pgd;
+ pgd_t entry;
for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
- pgd_t *pgd = pgd_offset(pgtable, vaddr);
- pgd_val(*pgd) = paddr;
- pgd_val(*pgd) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
- pgd_val(*pgd) |= pgprot_val(prot);
+ pgd_val(entry) = paddr;
+ pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
+ pgd_val(entry) |= pgprot_val(prot);
+ pgd = pgd_offset(pgtable, vaddr);
+ WRITE_ONCE(*pgd, entry);
flush_tlb_page(vaddr);
}
}
@@ -210,6 +215,7 @@ void mmu_clear_user(unsigned long vaddr)
{
pgd_t *pgtable;
pteval_t *pte;
+ pteval_t entry;
if (!mmu_enabled())
return;
@@ -217,6 +223,7 @@ void mmu_clear_user(unsigned long vaddr)
pgtable = current_thread_info()->pgtable;
pte = get_pte(pgtable, vaddr);
- *pte &= ~PTE_USER;
+ entry = *pte & ~PTE_USER;
+ WRITE_ONCE(*pte, entry);
flush_tlb_page(vaddr);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 04/18] lib: arm/arm64: Use WRITE_ONCE to update the translation tables
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 04/18] lib: arm/arm64: Use WRITE_ONCE to update the translation tables Alexandru Elisei
@ 2020-01-02 18:06 ` Andre Przywara
0 siblings, 0 replies; 47+ messages in thread
From: Andre Przywara @ 2020-01-02 18:06 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:35 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> Use WRITE_ONCE to prevent store tearing when updating an entry in the
> translation tables. Without WRITE_ONCE, the compiler, even though it is
> unlikely, can emit several stores when changing the table, and we might
> end up with bogus TLB entries.
>
> It's worth noting that the existing code is mostly fine without any
> changes because the translation tables are updated in one of the
> following situations:
>
> - When the tables are being created with the MMU off, which means no TLB
> caching is being performed.
>
> - When new page table entries are added as a result of vmalloc'ing a
> stack for a secondary CPU, which doesn't happen very often.
>
> - When clearing the PTE_USER bit for the cache test, and store tearing
> has no effect on the table walker because there are no intermediate
> values between bit values 0 and 1. We still use WRITE_ONCE in this case
> for consistency.
>
> However, the functions are global and there is nothing preventing someone
> from writing a test that uses them in a different scenario. Let's make
> sure that when that happens, there will be no breakage once in a blue
> moon.
I haven't checked whether there are more places where this would be needed, but it seems the right thing to do, also the changes below look valid.
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> lib/arm/asm/pgtable.h | 12 ++++++++----
> lib/arm64/asm/pgtable.h | 7 +++++--
> lib/arm/mmu.c | 19 +++++++++++++------
> 3 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> index 241dff69b38a..794514b8c927 100644
> --- a/lib/arm/asm/pgtable.h
> +++ b/lib/arm/asm/pgtable.h
> @@ -19,6 +19,8 @@
> * because we always allocate their pages with alloc_page(), and
> * alloc_page() always returns identity mapped pages.
> */
> +#include <linux/compiler.h>
> +
> #define pgtable_va(x) ((void *)(unsigned long)(x))
> #define pgtable_pa(x) ((unsigned long)(x))
>
> @@ -58,8 +60,9 @@ static inline pmd_t *pmd_alloc_one(void)
> static inline pmd_t *pmd_alloc(pgd_t *pgd, unsigned long addr)
> {
> if (pgd_none(*pgd)) {
> - pmd_t *pmd = pmd_alloc_one();
> - pgd_val(*pgd) = pgtable_pa(pmd) | PMD_TYPE_TABLE;
> + pgd_t entry;
> + pgd_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pgd, entry);
> }
> return pmd_offset(pgd, addr);
> }
> @@ -84,8 +87,9 @@ static inline pte_t *pte_alloc_one(void)
> static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
> {
> if (pmd_none(*pmd)) {
> - pte_t *pte = pte_alloc_one();
> - pmd_val(*pmd) = pgtable_pa(pte) | PMD_TYPE_TABLE;
> + pmd_t entry;
> + pmd_val(entry) = pgtable_pa(pte_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pmd, entry);
> }
> return pte_offset(pmd, addr);
> }
> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> index ee0a2c88cc18..dbf9e7253b71 100644
> --- a/lib/arm64/asm/pgtable.h
> +++ b/lib/arm64/asm/pgtable.h
> @@ -18,6 +18,8 @@
> #include <asm/page.h>
> #include <asm/pgtable-hwdef.h>
>
> +#include <linux/compiler.h>
> +
> /*
> * We can convert va <=> pa page table addresses with simple casts
> * because we always allocate their pages with alloc_page(), and
> @@ -66,8 +68,9 @@ static inline pte_t *pte_alloc_one(void)
> static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
> {
> if (pmd_none(*pmd)) {
> - pte_t *pte = pte_alloc_one();
> - pmd_val(*pmd) = pgtable_pa(pte) | PMD_TYPE_TABLE;
> + pmd_t entry;
> + pmd_val(entry) = pgtable_pa(pte_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pmd, entry);
> }
> return pte_offset(pmd, addr);
> }
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 5c31c00ccb31..86a829966a3c 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -17,6 +17,8 @@
> #include <asm/pgtable-hwdef.h>
> #include <asm/pgtable.h>
>
> +#include <linux/compiler.h>
> +
> extern unsigned long etext;
>
> pgd_t *mmu_idmap;
> @@ -86,7 +88,7 @@ static pteval_t *install_pte(pgd_t *pgtable, uintptr_t vaddr, pteval_t pte)
> {
> pteval_t *p_pte = get_pte(pgtable, vaddr);
>
> - *p_pte = pte;
> + WRITE_ONCE(*p_pte, pte);
> flush_tlb_page(vaddr);
> return p_pte;
> }
> @@ -131,12 +133,15 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
> phys_addr_t paddr = phys_start & PGDIR_MASK;
> uintptr_t vaddr = virt_offset & PGDIR_MASK;
> uintptr_t virt_end = phys_end - paddr + vaddr;
> + pgd_t *pgd;
> + pgd_t entry;
>
> for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
> - pgd_t *pgd = pgd_offset(pgtable, vaddr);
> - pgd_val(*pgd) = paddr;
> - pgd_val(*pgd) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> - pgd_val(*pgd) |= pgprot_val(prot);
> + pgd_val(entry) = paddr;
> + pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> + pgd_val(entry) |= pgprot_val(prot);
> + pgd = pgd_offset(pgtable, vaddr);
> + WRITE_ONCE(*pgd, entry);
> flush_tlb_page(vaddr);
> }
> }
> @@ -210,6 +215,7 @@ void mmu_clear_user(unsigned long vaddr)
> {
> pgd_t *pgtable;
> pteval_t *pte;
> + pteval_t entry;
>
> if (!mmu_enabled())
> return;
> @@ -217,6 +223,7 @@ void mmu_clear_user(unsigned long vaddr)
> pgtable = current_thread_info()->pgtable;
> pte = get_pte(pgtable, vaddr);
>
> - *pte &= ~PTE_USER;
> + entry = *pte & ~PTE_USER;
> + WRITE_ONCE(*pte, entry);
> flush_tlb_page(vaddr);
> }
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 05/18] lib: arm/arm64: Remove unused CPU_OFF parameter
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (3 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 04/18] lib: arm/arm64: Use WRITE_ONCE to update the translation tables Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-02 18:11 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors Alexandru Elisei
` (13 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
The first version of PSCI required an argument for CPU_OFF, the power_state
argument, which was removed in version 0.2 of the specification [1].
kvm-unit-tests supports PSCI 0.2, and KVM ignores any CPU_OFF parameters,
so let's remove the PSCI_POWER_STATE_TYPE_POWER_DOWN parameter.
[1] ARM DEN 0022D, section 7.3.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/psci.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/arm/psci.c b/lib/arm/psci.c
index c3d399064ae3..936c83948b6a 100644
--- a/lib/arm/psci.c
+++ b/lib/arm/psci.c
@@ -40,11 +40,9 @@ int cpu_psci_cpu_boot(unsigned int cpu)
return err;
}
-#define PSCI_POWER_STATE_TYPE_POWER_DOWN (1U << 16)
void cpu_psci_cpu_die(void)
{
- int err = psci_invoke(PSCI_0_2_FN_CPU_OFF,
- PSCI_POWER_STATE_TYPE_POWER_DOWN, 0, 0);
+ int err = psci_invoke(PSCI_0_2_FN_CPU_OFF, 0, 0, 0);
printf("CPU%d unable to power off (error = %d)\n", smp_processor_id(), err);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 05/18] lib: arm/arm64: Remove unused CPU_OFF parameter
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 05/18] lib: arm/arm64: Remove unused CPU_OFF parameter Alexandru Elisei
@ 2020-01-02 18:11 ` Andre Przywara
0 siblings, 0 replies; 47+ messages in thread
From: Andre Przywara @ 2020-01-02 18:11 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:36 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> The first version of PSCI required an argument for CPU_OFF, the power_state
> argument, which was removed in version 0.2 of the specification [1].
> kvm-unit-tests supports PSCI 0.2,
You mean kvm-unit-tests *require* PSCI v0.2? I only see explicit checks for that in the PSCI portion of selftests, but not in the other tests using PSCI, for secondary core bringup. Judging by the code we seem to *rely* on the presence of PSCI >= v0.2 via smc (for instance by assuming the v0.2 function IDs), even though this is not documented.
I guess there is little point to provide support for PSCI v0.1, so we should just document this, possibly referring to the explicit PSCI test.
> and KVM ignores any CPU_OFF parameters,
> so let's remove the PSCI_POWER_STATE_TYPE_POWER_DOWN parameter.
>
> [1] ARM DEN 0022D, section 7.3.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Anyway, in case we decide on requiring PSCI v0.2, this seems to be right, so:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> lib/arm/psci.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/lib/arm/psci.c b/lib/arm/psci.c
> index c3d399064ae3..936c83948b6a 100644
> --- a/lib/arm/psci.c
> +++ b/lib/arm/psci.c
> @@ -40,11 +40,9 @@ int cpu_psci_cpu_boot(unsigned int cpu)
> return err;
> }
>
> -#define PSCI_POWER_STATE_TYPE_POWER_DOWN (1U << 16)
> void cpu_psci_cpu_die(void)
> {
> - int err = psci_invoke(PSCI_0_2_FN_CPU_OFF,
> - PSCI_POWER_STATE_TYPE_POWER_DOWN, 0, 0);
> + int err = psci_invoke(PSCI_0_2_FN_CPU_OFF, 0, 0, 0);
> printf("CPU%d unable to power off (error = %d)\n", smp_processor_id(), err);
> }
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (4 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 05/18] lib: arm/arm64: Remove unused CPU_OFF parameter Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-02 18:11 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 07/18] lib: arm/arm64: Add missing include for alloc_page.h in pgtable.h Alexandru Elisei
` (12 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
done by setting the entry point for the CPU_ON call to the physical address
of the C function cpu_psci_cpu_die.
The compiler is well within its rights to use the stack when generating
code for cpu_psci_cpu_die. However, because no stack initialization has
been done, the stack pointer is zero, as set by KVM when creating the VCPU.
This causes a data abort without a change in exception level. The VBAR_EL1
register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
and we end up trying to fetch instructions from address 0x200.
At this point, a stage 2 instruction abort is generated which is taken to
KVM. KVM interprets this as an instruction fetch from an I/O region, and
injects a prefetch abort into the guest. Prefetch abort is a synchronous
exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
abort while fetching the instruction to service the said abort.
cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
provide an assembly implementation for the function which will serve as the
entry point for CPU_ON.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/cstart.S | 7 +++++++
arm/cstart64.S | 7 +++++++
arm/psci.c | 5 +++--
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/arm/cstart.S b/arm/cstart.S
index 2c81d39a666b..dfef48e4dbb2 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -7,6 +7,7 @@
*/
#define __ASSEMBLY__
#include <auxinfo.h>
+#include <linux/psci.h>
#include <asm/thread_info.h>
#include <asm/asm-offsets.h>
#include <asm/pgtable-hwdef.h>
@@ -139,6 +140,12 @@ secondary_entry:
blx r0
b do_idle
+.global asm_cpu_psci_cpu_die
+asm_cpu_psci_cpu_die:
+ ldr r0, =PSCI_0_2_FN_CPU_OFF
+ hvc #0
+ b .
+
.globl halt
halt:
1: wfi
diff --git a/arm/cstart64.S b/arm/cstart64.S
index b0e8baa1a23a..c98842f11e90 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -7,6 +7,7 @@
*/
#define __ASSEMBLY__
#include <auxinfo.h>
+#include <linux/psci.h>
#include <asm/asm-offsets.h>
#include <asm/ptrace.h>
#include <asm/processor.h>
@@ -128,6 +129,12 @@ secondary_entry:
blr x0
b do_idle
+.globl asm_cpu_psci_cpu_die
+asm_cpu_psci_cpu_die:
+ ldr x0, =PSCI_0_2_FN_CPU_OFF
+ hvc #0
+ b .
+
.globl halt
halt:
1: wfi
diff --git a/arm/psci.c b/arm/psci.c
index 5c1accb6cea4..c45a39c7d6e8 100644
--- a/arm/psci.c
+++ b/arm/psci.c
@@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
static cpumask_t cpu_on_ready, cpu_on_done;
static volatile int cpu_on_start;
+extern void asm_cpu_psci_cpu_die(void);
static void cpu_on_secondary_entry(void)
{
int cpu = smp_processor_id();
@@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
cpumask_set_cpu(cpu, &cpu_on_ready);
while (!cpu_on_start)
cpu_relax();
- cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+ cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
cpumask_set_cpu(cpu, &cpu_on_done);
}
@@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
cpu_on_start = 1;
smp_mb();
- cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+ cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
cpumask_set_cpu(0, &cpu_on_done);
while (!cpumask_full(&cpu_on_done))
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors Alexandru Elisei
@ 2020-01-02 18:11 ` Andre Przywara
2020-01-03 15:31 ` Andrew Jones
2020-01-06 10:41 ` Alexandru Elisei
0 siblings, 2 replies; 47+ messages in thread
From: Andre Przywara @ 2020-01-02 18:11 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:37 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
> done by setting the entry point for the CPU_ON call to the physical address
> of the C function cpu_psci_cpu_die.
>
> The compiler is well within its rights to use the stack when generating
> code for cpu_psci_cpu_die.
I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
One more thing below ...
> However, because no stack initialization has
> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
> This causes a data abort without a change in exception level. The VBAR_EL1
> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
> and we end up trying to fetch instructions from address 0x200.
>
> At this point, a stage 2 instruction abort is generated which is taken to
> KVM. KVM interprets this as an instruction fetch from an I/O region, and
> injects a prefetch abort into the guest. Prefetch abort is a synchronous
> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
> abort while fetching the instruction to service the said abort.
>
> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
> provide an assembly implementation for the function which will serve as the
> entry point for CPU_ON.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/cstart.S | 7 +++++++
> arm/cstart64.S | 7 +++++++
> arm/psci.c | 5 +++--
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arm/cstart.S b/arm/cstart.S
> index 2c81d39a666b..dfef48e4dbb2 100644
> --- a/arm/cstart.S
> +++ b/arm/cstart.S
> @@ -7,6 +7,7 @@
> */
> #define __ASSEMBLY__
> #include <auxinfo.h>
> +#include <linux/psci.h>
> #include <asm/thread_info.h>
> #include <asm/asm-offsets.h>
> #include <asm/pgtable-hwdef.h>
> @@ -139,6 +140,12 @@ secondary_entry:
> blx r0
> b do_idle
>
> +.global asm_cpu_psci_cpu_die
> +asm_cpu_psci_cpu_die:
> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> + hvc #0
> + b .
I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
At the very least it's a change in behaviour (ignoring the missing printf).
So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
Cheers,
Andre
> +
> .globl halt
> halt:
> 1: wfi
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index b0e8baa1a23a..c98842f11e90 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -7,6 +7,7 @@
> */
> #define __ASSEMBLY__
> #include <auxinfo.h>
> +#include <linux/psci.h>
> #include <asm/asm-offsets.h>
> #include <asm/ptrace.h>
> #include <asm/processor.h>
> @@ -128,6 +129,12 @@ secondary_entry:
> blr x0
> b do_idle
>
> +.globl asm_cpu_psci_cpu_die
> +asm_cpu_psci_cpu_die:
> + ldr x0, =PSCI_0_2_FN_CPU_OFF
> + hvc #0
> + b .
> +
> .globl halt
> halt:
> 1: wfi
> diff --git a/arm/psci.c b/arm/psci.c
> index 5c1accb6cea4..c45a39c7d6e8 100644
> --- a/arm/psci.c
> +++ b/arm/psci.c
> @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
> static cpumask_t cpu_on_ready, cpu_on_done;
> static volatile int cpu_on_start;
>
> +extern void asm_cpu_psci_cpu_die(void);
> static void cpu_on_secondary_entry(void)
> {
> int cpu = smp_processor_id();
> @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
> cpumask_set_cpu(cpu, &cpu_on_ready);
> while (!cpu_on_start)
> cpu_relax();
> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> cpumask_set_cpu(cpu, &cpu_on_done);
> }
>
> @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
> cpu_on_start = 1;
> smp_mb();
>
> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> cpumask_set_cpu(0, &cpu_on_done);
>
> while (!cpumask_full(&cpu_on_done))
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-02 18:11 ` Andre Przywara
@ 2020-01-03 15:31 ` Andrew Jones
2020-01-06 11:02 ` Alexandru Elisei
2020-01-06 10:41 ` Alexandru Elisei
1 sibling, 1 reply; 47+ messages in thread
From: Andrew Jones @ 2020-01-03 15:31 UTC (permalink / raw)
To: Andre Przywara
Cc: Alexandru Elisei, kvm, pbonzini, maz, vladimir.murzin, mark.rutland
On Thu, Jan 02, 2020 at 06:11:21PM +0000, Andre Przywara wrote:
> On Tue, 31 Dec 2019 16:09:37 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
> > The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
> > done by setting the entry point for the CPU_ON call to the physical address
> > of the C function cpu_psci_cpu_die.
> >
> > The compiler is well within its rights to use the stack when generating
> > code for cpu_psci_cpu_die.
>
> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
I think the test just doesn't care that the CPU is in an infinite
exception loop. Indeed we should probably put the CPU into an
infinite loop instead of attempting to call PSCI die with it, as
the status of a dying or dead CPU may conflict with our expected
status of the test.
>
> One more thing below ...
>
> > However, because no stack initialization has
> > been done, the stack pointer is zero, as set by KVM when creating the VCPU.
> > This causes a data abort without a change in exception level. The VBAR_EL1
> > register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
> > and we end up trying to fetch instructions from address 0x200.
> >
> > At this point, a stage 2 instruction abort is generated which is taken to
> > KVM. KVM interprets this as an instruction fetch from an I/O region, and
> > injects a prefetch abort into the guest. Prefetch abort is a synchronous
> > exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
> > which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
> > abort while fetching the instruction to service the said abort.
> >
> > cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
> > provide an assembly implementation for the function which will serve as the
> > entry point for CPU_ON.
> >
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> > arm/cstart.S | 7 +++++++
> > arm/cstart64.S | 7 +++++++
> > arm/psci.c | 5 +++--
> > 3 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/arm/cstart.S b/arm/cstart.S
> > index 2c81d39a666b..dfef48e4dbb2 100644
> > --- a/arm/cstart.S
> > +++ b/arm/cstart.S
> > @@ -7,6 +7,7 @@
> > */
> > #define __ASSEMBLY__
> > #include <auxinfo.h>
> > +#include <linux/psci.h>
> > #include <asm/thread_info.h>
> > #include <asm/asm-offsets.h>
> > #include <asm/pgtable-hwdef.h>
> > @@ -139,6 +140,12 @@ secondary_entry:
> > blx r0
> > b do_idle
> >
> > +.global asm_cpu_psci_cpu_die
> > +asm_cpu_psci_cpu_die:
> > + ldr r0, =PSCI_0_2_FN_CPU_OFF
> > + hvc #0
> > + b .
>
> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
>
> At the very least it's a change in behaviour (ignoring the missing printf).
> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
If we were to keep this function, then I agree we should zero the
registers, but as I said above, I think the proper fix for this issue is
to just not call PSCI die. Rather we should drop into an infinite loop,
which also doesn't use the stack. Maybe something like this will work
diff --git a/arm/psci.c b/arm/psci.c
index 5c1accb6cea4..74c179d4976c 100644
--- a/arm/psci.c
+++ b/arm/psci.c
@@ -79,10 +79,14 @@ static void cpu_on_secondary_entry(void)
cpumask_set_cpu(cpu, &cpu_on_ready);
while (!cpu_on_start)
cpu_relax();
- cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+ cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
cpumask_set_cpu(cpu, &cpu_on_done);
}
+/*
+ * This test expects CPU1 to not have previously been boot,
+ * and when this test completes CPU1 will be stuck in halt.
+ */
static bool psci_cpu_on_test(void)
{
bool failed = false;
@@ -104,7 +108,7 @@ static bool psci_cpu_on_test(void)
cpu_on_start = 1;
smp_mb();
- cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+ cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
cpumask_set_cpu(0, &cpu_on_done);
while (!cpumask_full(&cpu_on_done))
Thanks,
drew
>
> Cheers,
> Andre
>
> > +
> > .globl halt
> > halt:
> > 1: wfi
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index b0e8baa1a23a..c98842f11e90 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -7,6 +7,7 @@
> > */
> > #define __ASSEMBLY__
> > #include <auxinfo.h>
> > +#include <linux/psci.h>
> > #include <asm/asm-offsets.h>
> > #include <asm/ptrace.h>
> > #include <asm/processor.h>
> > @@ -128,6 +129,12 @@ secondary_entry:
> > blr x0
> > b do_idle
> >
> > +.globl asm_cpu_psci_cpu_die
> > +asm_cpu_psci_cpu_die:
> > + ldr x0, =PSCI_0_2_FN_CPU_OFF
> > + hvc #0
> > + b .
> > +
> > .globl halt
> > halt:
> > 1: wfi
> > diff --git a/arm/psci.c b/arm/psci.c
> > index 5c1accb6cea4..c45a39c7d6e8 100644
> > --- a/arm/psci.c
> > +++ b/arm/psci.c
> > @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
> > static cpumask_t cpu_on_ready, cpu_on_done;
> > static volatile int cpu_on_start;
> >
> > +extern void asm_cpu_psci_cpu_die(void);
> > static void cpu_on_secondary_entry(void)
> > {
> > int cpu = smp_processor_id();
> > @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
> > cpumask_set_cpu(cpu, &cpu_on_ready);
> > while (!cpu_on_start)
> > cpu_relax();
> > - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> > + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> > cpumask_set_cpu(cpu, &cpu_on_done);
> > }
> >
> > @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
> > cpu_on_start = 1;
> > smp_mb();
> >
> > - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> > + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> > cpumask_set_cpu(0, &cpu_on_done);
> >
> > while (!cpumask_full(&cpu_on_done))
>
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-03 15:31 ` Andrew Jones
@ 2020-01-06 11:02 ` Alexandru Elisei
2020-01-06 13:03 ` Andrew Jones
0 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 11:02 UTC (permalink / raw)
To: Andrew Jones, Andre Przywara
Cc: kvm, pbonzini, maz, vladimir.murzin, mark.rutland
Hi,
On 1/3/20 3:31 PM, Andrew Jones wrote:
> On Thu, Jan 02, 2020 at 06:11:21PM +0000, Andre Przywara wrote:
>> On Tue, 31 Dec 2019 16:09:37 +0000
>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>
>> Hi,
>>
>>> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
>>> done by setting the entry point for the CPU_ON call to the physical address
>>> of the C function cpu_psci_cpu_die.
>>>
>>> The compiler is well within its rights to use the stack when generating
>>> code for cpu_psci_cpu_die.
>> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
> I think the test just doesn't care that the CPU is in an infinite
> exception loop. Indeed we should probably put the CPU into an
> infinite loop instead of attempting to call PSCI die with it, as
> the status of a dying or dead CPU may conflict with our expected
> status of the test.
I don't like this idea, I'll explain below why.
>> One more thing below ...
>>
>>> However, because no stack initialization has
>>> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
>>> This causes a data abort without a change in exception level. The VBAR_EL1
>>> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
>>> and we end up trying to fetch instructions from address 0x200.
>>>
>>> At this point, a stage 2 instruction abort is generated which is taken to
>>> KVM. KVM interprets this as an instruction fetch from an I/O region, and
>>> injects a prefetch abort into the guest. Prefetch abort is a synchronous
>>> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
>>> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
>>> abort while fetching the instruction to service the said abort.
>>>
>>> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
>>> provide an assembly implementation for the function which will serve as the
>>> entry point for CPU_ON.
>>>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>> arm/cstart.S | 7 +++++++
>>> arm/cstart64.S | 7 +++++++
>>> arm/psci.c | 5 +++--
>>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arm/cstart.S b/arm/cstart.S
>>> index 2c81d39a666b..dfef48e4dbb2 100644
>>> --- a/arm/cstart.S
>>> +++ b/arm/cstart.S
>>> @@ -7,6 +7,7 @@
>>> */
>>> #define __ASSEMBLY__
>>> #include <auxinfo.h>
>>> +#include <linux/psci.h>
>>> #include <asm/thread_info.h>
>>> #include <asm/asm-offsets.h>
>>> #include <asm/pgtable-hwdef.h>
>>> @@ -139,6 +140,12 @@ secondary_entry:
>>> blx r0
>>> b do_idle
>>>
>>> +.global asm_cpu_psci_cpu_die
>>> +asm_cpu_psci_cpu_die:
>>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
>>> + hvc #0
>>> + b .
>> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
>> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
>>
>> At the very least it's a change in behaviour (ignoring the missing printf).
>> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
> If we were to keep this function, then I agree we should zero the
> registers, but as I said above, I think the proper fix for this issue is
Regarding zero'ing unused arguments, I've explained my point of view in my reply
to Andre.
> to just not call PSCI die. Rather we should drop into an infinite loop,
> which also doesn't use the stack. Maybe something like this will work
>
> diff --git a/arm/psci.c b/arm/psci.c
> index 5c1accb6cea4..74c179d4976c 100644
> --- a/arm/psci.c
> +++ b/arm/psci.c
> @@ -79,10 +79,14 @@ static void cpu_on_secondary_entry(void)
> cpumask_set_cpu(cpu, &cpu_on_ready);
> while (!cpu_on_start)
> cpu_relax();
> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
> cpumask_set_cpu(cpu, &cpu_on_done);
> }
>
> +/*
> + * This test expects CPU1 to not have previously been boot,
> + * and when this test completes CPU1 will be stuck in halt.
> + */
> static bool psci_cpu_on_test(void)
> {
> bool failed = false;
> @@ -104,7 +108,7 @@ static bool psci_cpu_on_test(void)
> cpu_on_start = 1;
> smp_mb();
>
> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
> cpumask_set_cpu(0, &cpu_on_done);
>
> while (!cpumask_full(&cpu_on_done))
Trying to turn a CPU on and off concurrently from separate threads seems like a
nifty test to me, and good for catching possible races. With your version, 2
threads are enough to get all possible results: success and CPU already on.
Besides that, if my memory serves me right, this exact test was the reason for a
refactoring of the VCPU reset code, in commit 03fdfb269009 ("KVM: arm64: Don't
write junk to sysregs on reset").
My preference would be to keep calling CPU_ON and CPU_OFF repeatedly.
Thanks,
Alex
> Thanks,
> drew
>
>> Cheers,
>> Andre
>>
>>> +
>>> .globl halt
>>> halt:
>>> 1: wfi
>>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>>> index b0e8baa1a23a..c98842f11e90 100644
>>> --- a/arm/cstart64.S
>>> +++ b/arm/cstart64.S
>>> @@ -7,6 +7,7 @@
>>> */
>>> #define __ASSEMBLY__
>>> #include <auxinfo.h>
>>> +#include <linux/psci.h>
>>> #include <asm/asm-offsets.h>
>>> #include <asm/ptrace.h>
>>> #include <asm/processor.h>
>>> @@ -128,6 +129,12 @@ secondary_entry:
>>> blr x0
>>> b do_idle
>>>
>>> +.globl asm_cpu_psci_cpu_die
>>> +asm_cpu_psci_cpu_die:
>>> + ldr x0, =PSCI_0_2_FN_CPU_OFF
>>> + hvc #0
>>> + b .
>>> +
>>> .globl halt
>>> halt:
>>> 1: wfi
>>> diff --git a/arm/psci.c b/arm/psci.c
>>> index 5c1accb6cea4..c45a39c7d6e8 100644
>>> --- a/arm/psci.c
>>> +++ b/arm/psci.c
>>> @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
>>> static cpumask_t cpu_on_ready, cpu_on_done;
>>> static volatile int cpu_on_start;
>>>
>>> +extern void asm_cpu_psci_cpu_die(void);
>>> static void cpu_on_secondary_entry(void)
>>> {
>>> int cpu = smp_processor_id();
>>> @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
>>> cpumask_set_cpu(cpu, &cpu_on_ready);
>>> while (!cpu_on_start)
>>> cpu_relax();
>>> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>>> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
>>> cpumask_set_cpu(cpu, &cpu_on_done);
>>> }
>>>
>>> @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
>>> cpu_on_start = 1;
>>> smp_mb();
>>>
>>> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>>> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
>>> cpumask_set_cpu(0, &cpu_on_done);
>>>
>>> while (!cpumask_full(&cpu_on_done))
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 11:02 ` Alexandru Elisei
@ 2020-01-06 13:03 ` Andrew Jones
2020-01-06 14:03 ` Alexandru Elisei
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Jones @ 2020-01-06 13:03 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Andre Przywara, kvm, pbonzini, maz, vladimir.murzin, mark.rutland
On Mon, Jan 06, 2020 at 11:02:16AM +0000, Alexandru Elisei wrote:
> Hi,
>
> On 1/3/20 3:31 PM, Andrew Jones wrote:
> > On Thu, Jan 02, 2020 at 06:11:21PM +0000, Andre Przywara wrote:
> >> On Tue, 31 Dec 2019 16:09:37 +0000
> >> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >>
> >> Hi,
> >>
> >>> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
> >>> done by setting the entry point for the CPU_ON call to the physical address
> >>> of the C function cpu_psci_cpu_die.
> >>>
> >>> The compiler is well within its rights to use the stack when generating
> >>> code for cpu_psci_cpu_die.
> >> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
> > I think the test just doesn't care that the CPU is in an infinite
> > exception loop. Indeed we should probably put the CPU into an
> > infinite loop instead of attempting to call PSCI die with it, as
> > the status of a dying or dead CPU may conflict with our expected
> > status of the test.
>
> I don't like this idea, I'll explain below why.
>
> >> One more thing below ...
> >>
> >>> However, because no stack initialization has
> >>> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
> >>> This causes a data abort without a change in exception level. The VBAR_EL1
> >>> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
> >>> and we end up trying to fetch instructions from address 0x200.
> >>>
> >>> At this point, a stage 2 instruction abort is generated which is taken to
> >>> KVM. KVM interprets this as an instruction fetch from an I/O region, and
> >>> injects a prefetch abort into the guest. Prefetch abort is a synchronous
> >>> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
> >>> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
> >>> abort while fetching the instruction to service the said abort.
> >>>
> >>> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
> >>> provide an assembly implementation for the function which will serve as the
> >>> entry point for CPU_ON.
> >>>
> >>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>> ---
> >>> arm/cstart.S | 7 +++++++
> >>> arm/cstart64.S | 7 +++++++
> >>> arm/psci.c | 5 +++--
> >>> 3 files changed, 17 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arm/cstart.S b/arm/cstart.S
> >>> index 2c81d39a666b..dfef48e4dbb2 100644
> >>> --- a/arm/cstart.S
> >>> +++ b/arm/cstart.S
> >>> @@ -7,6 +7,7 @@
> >>> */
> >>> #define __ASSEMBLY__
> >>> #include <auxinfo.h>
> >>> +#include <linux/psci.h>
> >>> #include <asm/thread_info.h>
> >>> #include <asm/asm-offsets.h>
> >>> #include <asm/pgtable-hwdef.h>
> >>> @@ -139,6 +140,12 @@ secondary_entry:
> >>> blx r0
> >>> b do_idle
> >>>
> >>> +.global asm_cpu_psci_cpu_die
> >>> +asm_cpu_psci_cpu_die:
> >>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> >>> + hvc #0
> >>> + b .
> >> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
> >> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
> >>
> >> At the very least it's a change in behaviour (ignoring the missing printf).
> >> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
> > If we were to keep this function, then I agree we should zero the
> > registers, but as I said above, I think the proper fix for this issue is
>
> Regarding zero'ing unused arguments, I've explained my point of view in my reply
> to Andre.
>
> > to just not call PSCI die. Rather we should drop into an infinite loop,
> > which also doesn't use the stack. Maybe something like this will work
> >
> > diff --git a/arm/psci.c b/arm/psci.c
> > index 5c1accb6cea4..74c179d4976c 100644
> > --- a/arm/psci.c
> > +++ b/arm/psci.c
> > @@ -79,10 +79,14 @@ static void cpu_on_secondary_entry(void)
> > cpumask_set_cpu(cpu, &cpu_on_ready);
> > while (!cpu_on_start)
> > cpu_relax();
> > - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> > + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
> > cpumask_set_cpu(cpu, &cpu_on_done);
> > }
> >
> > +/*
> > + * This test expects CPU1 to not have previously been boot,
> > + * and when this test completes CPU1 will be stuck in halt.
> > + */
> > static bool psci_cpu_on_test(void)
> > {
> > bool failed = false;
> > @@ -104,7 +108,7 @@ static bool psci_cpu_on_test(void)
> > cpu_on_start = 1;
> > smp_mb();
> >
> > - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> > + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
> > cpumask_set_cpu(0, &cpu_on_done);
> >
> > while (!cpumask_full(&cpu_on_done))
>
> Trying to turn a CPU on and off concurrently from separate threads seems like a
> nifty test to me, and good for catching possible races. With your version, 2
> threads are enough to get all possible results: success and CPU already on.
>
> Besides that, if my memory serves me right, this exact test was the reason for a
> refactoring of the VCPU reset code, in commit 03fdfb269009 ("KVM: arm64: Don't
> write junk to sysregs on reset").
It wasn't because there was a CPU_OFF involved. Two simultaneous CPU_ON's
was enough. The first, successful CPU_ON reset the target VCPU causing the
second CPU_ON to fail to look it up by MPIDR, resulting in
PSCI_RET_INVALID_PARAMS.
>
> My preference would be to keep calling CPU_ON and CPU_OFF repeatedly.
>
Your analysis showed we never called CPU_OFF, because CPU1 got stuck
in an exception loop. So we haven't actually done a repeated ON/OFF
test yet. That's not a bad idea, but I also like a test that ensures
a single successful CPU_ON, like the patch below would ensure if we
halted instead of CPU_OFF'ed.
It should be possible to have both tests if we do the CPU_OFF test first.
Thanks,
drew
diff --git a/arm/psci.c b/arm/psci.c
index 5c1accb6cea4..8a24860bde28 100644
--- a/arm/psci.c
+++ b/arm/psci.c
@@ -86,7 +86,7 @@ static void cpu_on_secondary_entry(void)
static bool psci_cpu_on_test(void)
{
bool failed = false;
- int cpu;
+ int cpu, ret_success = 0;
cpumask_set_cpu(1, &cpu_on_ready);
cpumask_set_cpu(1, &cpu_on_done);
@@ -113,7 +113,12 @@ static bool psci_cpu_on_test(void)
for_each_present_cpu(cpu) {
if (cpu == 1)
continue;
- if (cpu_on_ret[cpu] != PSCI_RET_SUCCESS && cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
+ if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) {
+ if (++ret_success > 1) {
+ report_info("more than one cpu_on success");
+ failed = true;
+ }
+ } else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
report_info("unexpected cpu_on return value: caller=CPU%d, ret=%d", cpu, cpu_on_ret[cpu]);
failed = true;
}
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 13:03 ` Andrew Jones
@ 2020-01-06 14:03 ` Alexandru Elisei
0 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 14:03 UTC (permalink / raw)
To: Andrew Jones
Cc: Andre Przywara, kvm, pbonzini, maz, vladimir.murzin, mark.rutland
Hi,
On 1/6/20 1:03 PM, Andrew Jones wrote:
> On Mon, Jan 06, 2020 at 11:02:16AM +0000, Alexandru Elisei wrote:
>> Hi,
>>
>> On 1/3/20 3:31 PM, Andrew Jones wrote:
>>> On Thu, Jan 02, 2020 at 06:11:21PM +0000, Andre Przywara wrote:
>>>> On Tue, 31 Dec 2019 16:09:37 +0000
>>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>>> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
>>>>> done by setting the entry point for the CPU_ON call to the physical address
>>>>> of the C function cpu_psci_cpu_die.
>>>>>
>>>>> The compiler is well within its rights to use the stack when generating
>>>>> code for cpu_psci_cpu_die.
>>>> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
>>> I think the test just doesn't care that the CPU is in an infinite
>>> exception loop. Indeed we should probably put the CPU into an
>>> infinite loop instead of attempting to call PSCI die with it, as
>>> the status of a dying or dead CPU may conflict with our expected
>>> status of the test.
>> I don't like this idea, I'll explain below why.
>>
>>>> One more thing below ...
>>>>
>>>>> However, because no stack initialization has
>>>>> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
>>>>> This causes a data abort without a change in exception level. The VBAR_EL1
>>>>> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
>>>>> and we end up trying to fetch instructions from address 0x200.
>>>>>
>>>>> At this point, a stage 2 instruction abort is generated which is taken to
>>>>> KVM. KVM interprets this as an instruction fetch from an I/O region, and
>>>>> injects a prefetch abort into the guest. Prefetch abort is a synchronous
>>>>> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
>>>>> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
>>>>> abort while fetching the instruction to service the said abort.
>>>>>
>>>>> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
>>>>> provide an assembly implementation for the function which will serve as the
>>>>> entry point for CPU_ON.
>>>>>
>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>>> ---
>>>>> arm/cstart.S | 7 +++++++
>>>>> arm/cstart64.S | 7 +++++++
>>>>> arm/psci.c | 5 +++--
>>>>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arm/cstart.S b/arm/cstart.S
>>>>> index 2c81d39a666b..dfef48e4dbb2 100644
>>>>> --- a/arm/cstart.S
>>>>> +++ b/arm/cstart.S
>>>>> @@ -7,6 +7,7 @@
>>>>> */
>>>>> #define __ASSEMBLY__
>>>>> #include <auxinfo.h>
>>>>> +#include <linux/psci.h>
>>>>> #include <asm/thread_info.h>
>>>>> #include <asm/asm-offsets.h>
>>>>> #include <asm/pgtable-hwdef.h>
>>>>> @@ -139,6 +140,12 @@ secondary_entry:
>>>>> blx r0
>>>>> b do_idle
>>>>>
>>>>> +.global asm_cpu_psci_cpu_die
>>>>> +asm_cpu_psci_cpu_die:
>>>>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
>>>>> + hvc #0
>>>>> + b .
>>>> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
>>>> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
>>>>
>>>> At the very least it's a change in behaviour (ignoring the missing printf).
>>>> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
>>> If we were to keep this function, then I agree we should zero the
>>> registers, but as I said above, I think the proper fix for this issue is
>> Regarding zero'ing unused arguments, I've explained my point of view in my reply
>> to Andre.
>>
>>> to just not call PSCI die. Rather we should drop into an infinite loop,
>>> which also doesn't use the stack. Maybe something like this will work
>>>
>>> diff --git a/arm/psci.c b/arm/psci.c
>>> index 5c1accb6cea4..74c179d4976c 100644
>>> --- a/arm/psci.c
>>> +++ b/arm/psci.c
>>> @@ -79,10 +79,14 @@ static void cpu_on_secondary_entry(void)
>>> cpumask_set_cpu(cpu, &cpu_on_ready);
>>> while (!cpu_on_start)
>>> cpu_relax();
>>> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>>> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
>>> cpumask_set_cpu(cpu, &cpu_on_done);
>>> }
>>>
>>> +/*
>>> + * This test expects CPU1 to not have previously been boot,
>>> + * and when this test completes CPU1 will be stuck in halt.
>>> + */
>>> static bool psci_cpu_on_test(void)
>>> {
>>> bool failed = false;
>>> @@ -104,7 +108,7 @@ static bool psci_cpu_on_test(void)
>>> cpu_on_start = 1;
>>> smp_mb();
>>>
>>> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>>> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
>>> cpumask_set_cpu(0, &cpu_on_done);
>>>
>>> while (!cpumask_full(&cpu_on_done))
>> Trying to turn a CPU on and off concurrently from separate threads seems like a
>> nifty test to me, and good for catching possible races. With your version, 2
>> threads are enough to get all possible results: success and CPU already on.
>>
>> Besides that, if my memory serves me right, this exact test was the reason for a
>> refactoring of the VCPU reset code, in commit 03fdfb269009 ("KVM: arm64: Don't
>> write junk to sysregs on reset").
> It wasn't because there was a CPU_OFF involved. Two simultaneous CPU_ON's
> was enough. The first, successful CPU_ON reset the target VCPU causing the
> second CPU_ON to fail to look it up by MPIDR, resulting in
> PSCI_RET_INVALID_PARAMS.
My bad.
>> My preference would be to keep calling CPU_ON and CPU_OFF repeatedly.
>>
> Your analysis showed we never called CPU_OFF, because CPU1 got stuck
> in an exception loop. So we haven't actually done a repeated ON/OFF
True.
> test yet. That's not a bad idea, but I also like a test that ensures
> a single successful CPU_ON, like the patch below would ensure if we
> halted instead of CPU_OFF'ed.
>
> It should be possible to have both tests if we do the CPU_OFF test first.
>
> Thanks,
> drew
>
>
> diff --git a/arm/psci.c b/arm/psci.c
> index 5c1accb6cea4..8a24860bde28 100644
> --- a/arm/psci.c
> +++ b/arm/psci.c
> @@ -86,7 +86,7 @@ static void cpu_on_secondary_entry(void)
> static bool psci_cpu_on_test(void)
> {
> bool failed = false;
> - int cpu;
> + int cpu, ret_success = 0;
>
> cpumask_set_cpu(1, &cpu_on_ready);
> cpumask_set_cpu(1, &cpu_on_done);
> @@ -113,7 +113,12 @@ static bool psci_cpu_on_test(void)
> for_each_present_cpu(cpu) {
> if (cpu == 1)
> continue;
> - if (cpu_on_ret[cpu] != PSCI_RET_SUCCESS && cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
> + if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) {
> + if (++ret_success > 1) {
> + report_info("more than one cpu_on success");
> + failed = true;
> + }
> + } else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
> report_info("unexpected cpu_on return value: caller=CPU%d, ret=%d", cpu, cpu_on_ret[cpu]);
> failed = true;
> }
>
Your patch looks fine to me. Tested it together with changing the CPU_ON address
to halt and running it 1000 times on Ampere EMAG:
Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
I'm fine with dropping my patch from the series and sending it as an enhancement
after the other fixes get merged.
Thanks,
Alex
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-02 18:11 ` Andre Przywara
2020-01-03 15:31 ` Andrew Jones
@ 2020-01-06 10:41 ` Alexandru Elisei
2020-01-06 11:17 ` Andre Przywara
2020-01-06 11:41 ` Mark Rutland
1 sibling, 2 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 10:41 UTC (permalink / raw)
To: Andre Przywara, mark.rutland; +Cc: kvm, pbonzini, drjones, maz, vladimir.murzin
Hi Andre,
Thank you for reviewing the patches!
On 1/2/20 6:11 PM, Andre Przywara wrote:
> On Tue, 31 Dec 2019 16:09:37 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
>> done by setting the entry point for the CPU_ON call to the physical address
>> of the C function cpu_psci_cpu_die.
>>
>> The compiler is well within its rights to use the stack when generating
>> code for cpu_psci_cpu_die.
> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
The test checks for the return value of the CPU_ON function. What the CPU does
while it's live is inconsequential.
> One more thing below ...
>
>> However, because no stack initialization has
>> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
>> This causes a data abort without a change in exception level. The VBAR_EL1
>> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
>> and we end up trying to fetch instructions from address 0x200.
>>
>> At this point, a stage 2 instruction abort is generated which is taken to
>> KVM. KVM interprets this as an instruction fetch from an I/O region, and
>> injects a prefetch abort into the guest. Prefetch abort is a synchronous
>> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
>> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
>> abort while fetching the instruction to service the said abort.
>>
>> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
>> provide an assembly implementation for the function which will serve as the
>> entry point for CPU_ON.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> arm/cstart.S | 7 +++++++
>> arm/cstart64.S | 7 +++++++
>> arm/psci.c | 5 +++--
>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arm/cstart.S b/arm/cstart.S
>> index 2c81d39a666b..dfef48e4dbb2 100644
>> --- a/arm/cstart.S
>> +++ b/arm/cstart.S
>> @@ -7,6 +7,7 @@
>> */
>> #define __ASSEMBLY__
>> #include <auxinfo.h>
>> +#include <linux/psci.h>
>> #include <asm/thread_info.h>
>> #include <asm/asm-offsets.h>
>> #include <asm/pgtable-hwdef.h>
>> @@ -139,6 +140,12 @@ secondary_entry:
>> blx r0
>> b do_idle
>>
>> +.global asm_cpu_psci_cpu_die
>> +asm_cpu_psci_cpu_die:
>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
>> + hvc #0
>> + b .
> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
The SMC calling convention only specifies the values for the arguments that are
used by a function, not the values for all possible arguments. kvm-unit-tests sets
the other arguments to 0 because the function prototype that does the actual SMC
call takes 4 arguments. The value 0 is a random value that was chosen for those
unused parameters. For example, it could have been a random number on each call.
Let me put it another way. Suggesting that unused arguments should be set to 0 is
the same as suggesting that normal C function that adheres to procedure call
standard for arm64 should always have 8 arguments, and for a particular function
that doesn't use all of them, they should be set to 0 by the caller.
@Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
wants to chime in on this.
Thanks,
Alex
> At the very least it's a change in behaviour (ignoring the missing printf).
> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
>
> Cheers,
> Andre
>
>> +
>> .globl halt
>> halt:
>> 1: wfi
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index b0e8baa1a23a..c98842f11e90 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -7,6 +7,7 @@
>> */
>> #define __ASSEMBLY__
>> #include <auxinfo.h>
>> +#include <linux/psci.h>
>> #include <asm/asm-offsets.h>
>> #include <asm/ptrace.h>
>> #include <asm/processor.h>
>> @@ -128,6 +129,12 @@ secondary_entry:
>> blr x0
>> b do_idle
>>
>> +.globl asm_cpu_psci_cpu_die
>> +asm_cpu_psci_cpu_die:
>> + ldr x0, =PSCI_0_2_FN_CPU_OFF
>> + hvc #0
>> + b .
>> +
>> .globl halt
>> halt:
>> 1: wfi
>> diff --git a/arm/psci.c b/arm/psci.c
>> index 5c1accb6cea4..c45a39c7d6e8 100644
>> --- a/arm/psci.c
>> +++ b/arm/psci.c
>> @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
>> static cpumask_t cpu_on_ready, cpu_on_done;
>> static volatile int cpu_on_start;
>>
>> +extern void asm_cpu_psci_cpu_die(void);
>> static void cpu_on_secondary_entry(void)
>> {
>> int cpu = smp_processor_id();
>> @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
>> cpumask_set_cpu(cpu, &cpu_on_ready);
>> while (!cpu_on_start)
>> cpu_relax();
>> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
>> cpumask_set_cpu(cpu, &cpu_on_done);
>> }
>>
>> @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
>> cpu_on_start = 1;
>> smp_mb();
>>
>> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
>> cpumask_set_cpu(0, &cpu_on_done);
>>
>> while (!cpumask_full(&cpu_on_done))
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 10:41 ` Alexandru Elisei
@ 2020-01-06 11:17 ` Andre Przywara
2020-01-06 11:28 ` Alexandru Elisei
2020-01-06 11:36 ` Mark Rutland
2020-01-06 11:41 ` Mark Rutland
1 sibling, 2 replies; 47+ messages in thread
From: Andre Przywara @ 2020-01-06 11:17 UTC (permalink / raw)
To: Alexandru Elisei
Cc: mark.rutland, kvm, pbonzini, drjones, maz, vladimir.murzin
On Mon, 6 Jan 2020 10:41:55 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> Hi Andre,
>
> Thank you for reviewing the patches!
>
> On 1/2/20 6:11 PM, Andre Przywara wrote:
> > On Tue, 31 Dec 2019 16:09:37 +0000
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >
> > Hi,
> >
> >> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
> >> done by setting the entry point for the CPU_ON call to the physical address
> >> of the C function cpu_psci_cpu_die.
> >>
> >> The compiler is well within its rights to use the stack when generating
> >> code for cpu_psci_cpu_die.
> > I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
>
> The test checks for the return value of the CPU_ON function. What the CPU does
> while it's live is inconsequential.
OK, I see.
> > One more thing below ...
> >
> >> However, because no stack initialization has
> >> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
> >> This causes a data abort without a change in exception level. The VBAR_EL1
> >> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
> >> and we end up trying to fetch instructions from address 0x200.
> >>
> >> At this point, a stage 2 instruction abort is generated which is taken to
> >> KVM. KVM interprets this as an instruction fetch from an I/O region, and
> >> injects a prefetch abort into the guest. Prefetch abort is a synchronous
> >> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
> >> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
> >> abort while fetching the instruction to service the said abort.
> >>
> >> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
> >> provide an assembly implementation for the function which will serve as the
> >> entry point for CPU_ON.
> >>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >> arm/cstart.S | 7 +++++++
> >> arm/cstart64.S | 7 +++++++
> >> arm/psci.c | 5 +++--
> >> 3 files changed, 17 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arm/cstart.S b/arm/cstart.S
> >> index 2c81d39a666b..dfef48e4dbb2 100644
> >> --- a/arm/cstart.S
> >> +++ b/arm/cstart.S
> >> @@ -7,6 +7,7 @@
> >> */
> >> #define __ASSEMBLY__
> >> #include <auxinfo.h>
> >> +#include <linux/psci.h>
> >> #include <asm/thread_info.h>
> >> #include <asm/asm-offsets.h>
> >> #include <asm/pgtable-hwdef.h>
> >> @@ -139,6 +140,12 @@ secondary_entry:
> >> blx r0
> >> b do_idle
> >>
> >> +.global asm_cpu_psci_cpu_die
> >> +asm_cpu_psci_cpu_die:
> >> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> >> + hvc #0
> >> + b .
> > I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
> > I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
>
> The SMC calling convention only specifies the values for the arguments that are
> used by a function, not the values for all possible arguments. kvm-unit-tests sets
> the other arguments to 0 because the function prototype that does the actual SMC
> call takes 4 arguments. The value 0 is a random value that was chosen for those
> unused parameters. For example, it could have been a random number on each call.
>
> Let me put it another way. Suggesting that unused arguments should be set to 0 is
> the same as suggesting that normal C function that adheres to procedure call
> standard for arm64 should always have 8 arguments, and for a particular function
> that doesn't use all of them, they should be set to 0 by the caller.
I understand that, but was wondering if the SMCCC would mandate stricter requirements. After all every PSCI call from Linux goes through a function which clears all unused input parameters.
Cheers,
Andre.
> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
> wants to chime in on this.
>
> Thanks,
> Alex
> > At the very least it's a change in behaviour (ignoring the missing printf).
> > So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
> >
> > Cheers,
> > Andre
> >
> >> +
> >> .globl halt
> >> halt:
> >> 1: wfi
> >> diff --git a/arm/cstart64.S b/arm/cstart64.S
> >> index b0e8baa1a23a..c98842f11e90 100644
> >> --- a/arm/cstart64.S
> >> +++ b/arm/cstart64.S
> >> @@ -7,6 +7,7 @@
> >> */
> >> #define __ASSEMBLY__
> >> #include <auxinfo.h>
> >> +#include <linux/psci.h>
> >> #include <asm/asm-offsets.h>
> >> #include <asm/ptrace.h>
> >> #include <asm/processor.h>
> >> @@ -128,6 +129,12 @@ secondary_entry:
> >> blr x0
> >> b do_idle
> >>
> >> +.globl asm_cpu_psci_cpu_die
> >> +asm_cpu_psci_cpu_die:
> >> + ldr x0, =PSCI_0_2_FN_CPU_OFF
> >> + hvc #0
> >> + b .
> >> +
> >> .globl halt
> >> halt:
> >> 1: wfi
> >> diff --git a/arm/psci.c b/arm/psci.c
> >> index 5c1accb6cea4..c45a39c7d6e8 100644
> >> --- a/arm/psci.c
> >> +++ b/arm/psci.c
> >> @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
> >> static cpumask_t cpu_on_ready, cpu_on_done;
> >> static volatile int cpu_on_start;
> >>
> >> +extern void asm_cpu_psci_cpu_die(void);
> >> static void cpu_on_secondary_entry(void)
> >> {
> >> int cpu = smp_processor_id();
> >> @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
> >> cpumask_set_cpu(cpu, &cpu_on_ready);
> >> while (!cpu_on_start)
> >> cpu_relax();
> >> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> >> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> >> cpumask_set_cpu(cpu, &cpu_on_done);
> >> }
> >>
> >> @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
> >> cpu_on_start = 1;
> >> smp_mb();
> >>
> >> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> >> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> >> cpumask_set_cpu(0, &cpu_on_done);
> >>
> >> while (!cpumask_full(&cpu_on_done))
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 11:17 ` Andre Przywara
@ 2020-01-06 11:28 ` Alexandru Elisei
2020-01-06 11:36 ` Mark Rutland
1 sibling, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 11:28 UTC (permalink / raw)
To: Andre Przywara; +Cc: mark.rutland, kvm, pbonzini, drjones, maz, vladimir.murzin
Hi,
On 1/6/20 11:17 AM, Andre Przywara wrote:
> On Mon, 6 Jan 2020 10:41:55 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
>> Hi Andre,
>>
>> Thank you for reviewing the patches!
>>
>> On 1/2/20 6:11 PM, Andre Przywara wrote:
>>> On Tue, 31 Dec 2019 16:09:37 +0000
>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>
>>> Hi,
>>>
>>>> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
>>>> done by setting the entry point for the CPU_ON call to the physical address
>>>> of the C function cpu_psci_cpu_die.
>>>>
>>>> The compiler is well within its rights to use the stack when generating
>>>> code for cpu_psci_cpu_die.
>>> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
>> The test checks for the return value of the CPU_ON function. What the CPU does
>> while it's live is inconsequential.
> OK, I see.
>
>>> One more thing below ...
>>>
>>>> However, because no stack initialization has
>>>> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
>>>> This causes a data abort without a change in exception level. The VBAR_EL1
>>>> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
>>>> and we end up trying to fetch instructions from address 0x200.
>>>>
>>>> At this point, a stage 2 instruction abort is generated which is taken to
>>>> KVM. KVM interprets this as an instruction fetch from an I/O region, and
>>>> injects a prefetch abort into the guest. Prefetch abort is a synchronous
>>>> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
>>>> which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
>>>> abort while fetching the instruction to service the said abort.
>>>>
>>>> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
>>>> provide an assembly implementation for the function which will serve as the
>>>> entry point for CPU_ON.
>>>>
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> ---
>>>> arm/cstart.S | 7 +++++++
>>>> arm/cstart64.S | 7 +++++++
>>>> arm/psci.c | 5 +++--
>>>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arm/cstart.S b/arm/cstart.S
>>>> index 2c81d39a666b..dfef48e4dbb2 100644
>>>> --- a/arm/cstart.S
>>>> +++ b/arm/cstart.S
>>>> @@ -7,6 +7,7 @@
>>>> */
>>>> #define __ASSEMBLY__
>>>> #include <auxinfo.h>
>>>> +#include <linux/psci.h>
>>>> #include <asm/thread_info.h>
>>>> #include <asm/asm-offsets.h>
>>>> #include <asm/pgtable-hwdef.h>
>>>> @@ -139,6 +140,12 @@ secondary_entry:
>>>> blx r0
>>>> b do_idle
>>>>
>>>> +.global asm_cpu_psci_cpu_die
>>>> +asm_cpu_psci_cpu_die:
>>>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
>>>> + hvc #0
>>>> + b .
>>> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
>>> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
>> The SMC calling convention only specifies the values for the arguments that are
>> used by a function, not the values for all possible arguments. kvm-unit-tests sets
>> the other arguments to 0 because the function prototype that does the actual SMC
>> call takes 4 arguments. The value 0 is a random value that was chosen for those
>> unused parameters. For example, it could have been a random number on each call.
>>
>> Let me put it another way. Suggesting that unused arguments should be set to 0 is
>> the same as suggesting that normal C function that adheres to procedure call
>> standard for arm64 should always have 8 arguments, and for a particular function
>> that doesn't use all of them, they should be set to 0 by the caller.
> I understand that, but was wondering if the SMCCC would mandate stricter requirements. After all every PSCI call from Linux goes through a function which clears all unused input parameters.
I don't think that's a good idea. kvm-unit-tests is designed to test correctness.
Doing more than it is strictly necessary might hide real issues.
SMCCC mandating stricter requirements could happen at some point, yes. But I think
KVM/qemu dropping support for current versions of PSCI is a long way into the
future. In the meantime, I would not make decisions based on hypothetical
situations that might or might not happen.
Thanks,
Alex
>
> Cheers,
> Andre.
>
>> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
>> wants to chime in on this.
>>
>> Thanks,
>> Alex
>>> At the very least it's a change in behaviour (ignoring the missing printf).
>>> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
>>>
>>> Cheers,
>>> Andre
>>>
>>>> +
>>>> .globl halt
>>>> halt:
>>>> 1: wfi
>>>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>>>> index b0e8baa1a23a..c98842f11e90 100644
>>>> --- a/arm/cstart64.S
>>>> +++ b/arm/cstart64.S
>>>> @@ -7,6 +7,7 @@
>>>> */
>>>> #define __ASSEMBLY__
>>>> #include <auxinfo.h>
>>>> +#include <linux/psci.h>
>>>> #include <asm/asm-offsets.h>
>>>> #include <asm/ptrace.h>
>>>> #include <asm/processor.h>
>>>> @@ -128,6 +129,12 @@ secondary_entry:
>>>> blr x0
>>>> b do_idle
>>>>
>>>> +.globl asm_cpu_psci_cpu_die
>>>> +asm_cpu_psci_cpu_die:
>>>> + ldr x0, =PSCI_0_2_FN_CPU_OFF
>>>> + hvc #0
>>>> + b .
>>>> +
>>>> .globl halt
>>>> halt:
>>>> 1: wfi
>>>> diff --git a/arm/psci.c b/arm/psci.c
>>>> index 5c1accb6cea4..c45a39c7d6e8 100644
>>>> --- a/arm/psci.c
>>>> +++ b/arm/psci.c
>>>> @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
>>>> static cpumask_t cpu_on_ready, cpu_on_done;
>>>> static volatile int cpu_on_start;
>>>>
>>>> +extern void asm_cpu_psci_cpu_die(void);
>>>> static void cpu_on_secondary_entry(void)
>>>> {
>>>> int cpu = smp_processor_id();
>>>> @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
>>>> cpumask_set_cpu(cpu, &cpu_on_ready);
>>>> while (!cpu_on_start)
>>>> cpu_relax();
>>>> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>>>> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
>>>> cpumask_set_cpu(cpu, &cpu_on_done);
>>>> }
>>>>
>>>> @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
>>>> cpu_on_start = 1;
>>>> smp_mb();
>>>>
>>>> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
>>>> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
>>>> cpumask_set_cpu(0, &cpu_on_done);
>>>>
>>>> while (!cpumask_full(&cpu_on_done))
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 11:17 ` Andre Przywara
2020-01-06 11:28 ` Alexandru Elisei
@ 2020-01-06 11:36 ` Mark Rutland
1 sibling, 0 replies; 47+ messages in thread
From: Mark Rutland @ 2020-01-06 11:36 UTC (permalink / raw)
To: Andre Przywara
Cc: Alexandru Elisei, kvm, pbonzini, drjones, maz, vladimir.murzin
On Mon, Jan 06, 2020 at 11:17:16AM +0000, Andre Przywara wrote:
> On Mon, 6 Jan 2020 10:41:55 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> > Hi Andre,
> >
> > Thank you for reviewing the patches!
> >
> > On 1/2/20 6:11 PM, Andre Przywara wrote:
> > >> +.global asm_cpu_psci_cpu_die
> > >> +asm_cpu_psci_cpu_die:
> > >> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> > >> + hvc #0
> > >> + b .
> > > I am wondering if this implementation is actually too simple. Both
> > > the current implementation and the kernel clear at least the first
> > > three arguments to 0. I failed to find a requirement for doing
> > > this (nothing in the SMCCC or the PSCI spec), but I guess it would
> > > make sense when looking at forward compatibility.
This is a Linux implementation detail, and is not intended to be an
SMCCC contract detail. To be generic to all SMCCC calls, the invocation
functions have to take the largest set of potential arguments, and
callers have to pass /something/ for the unused values.
It would be perfectly valid for callers to pass the result of
get_random_long() instead. The SMCCC implementation should not derive
any meaning from registers which are not arguments to the call in
question.
> > The SMC calling convention only specifies the values for the arguments that are
> > used by a function, not the values for all possible arguments. kvm-unit-tests sets
> > the other arguments to 0 because the function prototype that does the actual SMC
> > call takes 4 arguments. The value 0 is a random value that was chosen for those
> > unused parameters. For example, it could have been a random number on each call.
> >
> > Let me put it another way. Suggesting that unused arguments should be set to 0 is
> > the same as suggesting that normal C function that adheres to procedure call
> > standard for arm64 should always have 8 arguments, and for a particular function
> > that doesn't use all of them, they should be set to 0 by the caller.
>
> I understand that, but was wondering if the SMCCC would mandate
> stricter requirements. After all every PSCI call from Linux goes
> through a function which clears all unused input parameters.
Callers are not deliberately clearingh the unused parameters, but rather
passing arbitrary values because they are forced to.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 10:41 ` Alexandru Elisei
2020-01-06 11:17 ` Andre Przywara
@ 2020-01-06 11:41 ` Mark Rutland
2020-01-06 13:17 ` Andrew Jones
1 sibling, 1 reply; 47+ messages in thread
From: Mark Rutland @ 2020-01-06 11:41 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Andre Przywara, kvm, pbonzini, drjones, maz, vladimir.murzin
On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote:
> On 1/2/20 6:11 PM, Andre Przywara wrote:
> > On Tue, 31 Dec 2019 16:09:37 +0000
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >> +.global asm_cpu_psci_cpu_die
> >> +asm_cpu_psci_cpu_die:
> >> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> >> + hvc #0
> >> + b .
> > I am wondering if this implementation is actually too simple. Both
> > the current implementation and the kernel clear at least the first
> > three arguments to 0.
> > I failed to find a requirement for doing this (nothing in the SMCCC
> > or the PSCI spec), but I guess it would make sense when looking at
> > forward compatibility.
>
> The SMC calling convention only specifies the values for the arguments that are
> used by a function, not the values for all possible arguments. kvm-unit-tests sets
> the other arguments to 0 because the function prototype that does the actual SMC
> call takes 4 arguments. The value 0 is a random value that was chosen for those
> unused parameters. For example, it could have been a random number on each call.
That's correct.
A caller can leave arbitrary values in non-argument registers, in the
same manner as a caller of an AAPCS function. The callee should not
consume those values as they are not arguments.
> Let me put it another way. Suggesting that unused arguments should be set to 0 is
> the same as suggesting that normal C function that adheres to procedure call
> standard for arm64 should always have 8 arguments, and for a particular function
> that doesn't use all of them, they should be set to 0 by the caller.
Heh, same rationale. :)
> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
> wants to chime in on this.
>
> Thanks,
> Alex
> > At the very least it's a change in behaviour (ignoring the missing printf).
> > So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
There's no need to zero non-argument registers, and it could potentially
mask bugs in callees, so I don't think it's a good idea to do so.
If you really want to test that the callee is robust and correct, it
would be better to randomize the content of non-argument regsiters to
fuzz the callee.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 11:41 ` Mark Rutland
@ 2020-01-06 13:17 ` Andrew Jones
2020-01-06 14:12 ` Alexandru Elisei
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Jones @ 2020-01-06 13:17 UTC (permalink / raw)
To: Mark Rutland
Cc: Alexandru Elisei, Andre Przywara, kvm, pbonzini, maz, vladimir.murzin
On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote:
> On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote:
> > On 1/2/20 6:11 PM, Andre Przywara wrote:
> > > On Tue, 31 Dec 2019 16:09:37 +0000
> > > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > >> +.global asm_cpu_psci_cpu_die
> > >> +asm_cpu_psci_cpu_die:
> > >> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> > >> + hvc #0
> > >> + b .
> > > I am wondering if this implementation is actually too simple. Both
> > > the current implementation and the kernel clear at least the first
> > > three arguments to 0.
> > > I failed to find a requirement for doing this (nothing in the SMCCC
> > > or the PSCI spec), but I guess it would make sense when looking at
> > > forward compatibility.
> >
> > The SMC calling convention only specifies the values for the arguments that are
> > used by a function, not the values for all possible arguments. kvm-unit-tests sets
> > the other arguments to 0 because the function prototype that does the actual SMC
> > call takes 4 arguments. The value 0 is a random value that was chosen for those
> > unused parameters. For example, it could have been a random number on each call.
>
> That's correct.
>
> A caller can leave arbitrary values in non-argument registers, in the
> same manner as a caller of an AAPCS function. The callee should not
> consume those values as they are not arguments.
>
> > Let me put it another way. Suggesting that unused arguments should be set to 0 is
> > the same as suggesting that normal C function that adheres to procedure call
> > standard for arm64 should always have 8 arguments, and for a particular function
> > that doesn't use all of them, they should be set to 0 by the caller.
>
> Heh, same rationale. :)
This is a good rationale for the function to not zero parameters.
>
> > @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
> > wants to chime in on this.
> >
> > Thanks,
> > Alex
> > > At the very least it's a change in behaviour (ignoring the missing printf).
> > > So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
>
> There's no need to zero non-argument registers, and it could potentially
> mask bugs in callees, so I don't think it's a good idea to do so.
>
> If you really want to test that the callee is robust and correct, it
> would be better to randomize the content of non-argument regsiters to
> fuzz the callee.
>
But this indicates there is risk that we'll be less robust if we don't
zero the parameters. Since this function is a common utility function and
kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody
wants to try and target, then if there's any chance that zeroing unused
parameters is more robust, I believe we should do that here. If we want to
test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write
explicit test cases to do that.
I can't speak to the risk of not zeroing, but due to the way we've been
calling PSCI functions with C, I can say up until now we always have.
Thanks,
drew
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 13:17 ` Andrew Jones
@ 2020-01-06 14:12 ` Alexandru Elisei
2020-01-06 15:20 ` Andrew Jones
0 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 14:12 UTC (permalink / raw)
To: Andrew Jones, Mark Rutland
Cc: Andre Przywara, kvm, pbonzini, maz, vladimir.murzin
Hi,
On 1/6/20 1:17 PM, Andrew Jones wrote:
> On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote:
>> On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote:
>>> On 1/2/20 6:11 PM, Andre Przywara wrote:
>>>> On Tue, 31 Dec 2019 16:09:37 +0000
>>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>>> +.global asm_cpu_psci_cpu_die
>>>>> +asm_cpu_psci_cpu_die:
>>>>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
>>>>> + hvc #0
>>>>> + b .
>>>> I am wondering if this implementation is actually too simple. Both
>>>> the current implementation and the kernel clear at least the first
>>>> three arguments to 0.
>>>> I failed to find a requirement for doing this (nothing in the SMCCC
>>>> or the PSCI spec), but I guess it would make sense when looking at
>>>> forward compatibility.
>>> The SMC calling convention only specifies the values for the arguments that are
>>> used by a function, not the values for all possible arguments. kvm-unit-tests sets
>>> the other arguments to 0 because the function prototype that does the actual SMC
>>> call takes 4 arguments. The value 0 is a random value that was chosen for those
>>> unused parameters. For example, it could have been a random number on each call.
>> That's correct.
>>
>> A caller can leave arbitrary values in non-argument registers, in the
>> same manner as a caller of an AAPCS function. The callee should not
>> consume those values as they are not arguments.
>>
>>> Let me put it another way. Suggesting that unused arguments should be set to 0 is
>>> the same as suggesting that normal C function that adheres to procedure call
>>> standard for arm64 should always have 8 arguments, and for a particular function
>>> that doesn't use all of them, they should be set to 0 by the caller.
>> Heh, same rationale. :)
> This is a good rationale for the function to not zero parameters.
>
>>> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
>>> wants to chime in on this.
>>>
>>> Thanks,
>>> Alex
>>>> At the very least it's a change in behaviour (ignoring the missing printf).
>>>> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
>> There's no need to zero non-argument registers, and it could potentially
>> mask bugs in callees, so I don't think it's a good idea to do so.
>>
>> If you really want to test that the callee is robust and correct, it
>> would be better to randomize the content of non-argument regsiters to
>> fuzz the callee.
>>
> But this indicates there is risk that we'll be less robust if we don't
> zero the parameters. Since this function is a common utility function and
> kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody
> wants to try and target, then if there's any chance that zeroing unused
> parameters is more robust, I believe we should do that here. If we want to
We agree that zero'ing unused parameters is not required by the specification,
right? After that, I think it depends how you see kvm-unit-tests. I am of the
opinion that as a testing tool, if not zero'ing parameters (I'm not talking here
about fuzzing) causes an error in whatever piece of software you are running, then
that piece of software is not specification compliant and kvm-unit-tests has done
its job.
Either way, I'm not advocating changing our PSCI code. I'm fine with dropping this
patch for now (I mentioned this in another thread), so we can resume this
conversation when I rework it.
Thanks,
Alex
> test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write
> explicit test cases to do that.
>
> I can't speak to the risk of not zeroing, but due to the way we've been
> calling PSCI functions with C, I can say up until now we always have.
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors
2020-01-06 14:12 ` Alexandru Elisei
@ 2020-01-06 15:20 ` Andrew Jones
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Jones @ 2020-01-06 15:20 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Mark Rutland, Andre Przywara, kvm, pbonzini, maz, vladimir.murzin
On Mon, Jan 06, 2020 at 02:12:46PM +0000, Alexandru Elisei wrote:
> Hi,
>
> On 1/6/20 1:17 PM, Andrew Jones wrote:
> > On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote:
> >> On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote:
> >>> On 1/2/20 6:11 PM, Andre Przywara wrote:
> >>>> On Tue, 31 Dec 2019 16:09:37 +0000
> >>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >>>>> +.global asm_cpu_psci_cpu_die
> >>>>> +asm_cpu_psci_cpu_die:
> >>>>> + ldr r0, =PSCI_0_2_FN_CPU_OFF
> >>>>> + hvc #0
> >>>>> + b .
> >>>> I am wondering if this implementation is actually too simple. Both
> >>>> the current implementation and the kernel clear at least the first
> >>>> three arguments to 0.
> >>>> I failed to find a requirement for doing this (nothing in the SMCCC
> >>>> or the PSCI spec), but I guess it would make sense when looking at
> >>>> forward compatibility.
> >>> The SMC calling convention only specifies the values for the arguments that are
> >>> used by a function, not the values for all possible arguments. kvm-unit-tests sets
> >>> the other arguments to 0 because the function prototype that does the actual SMC
> >>> call takes 4 arguments. The value 0 is a random value that was chosen for those
> >>> unused parameters. For example, it could have been a random number on each call.
> >> That's correct.
> >>
> >> A caller can leave arbitrary values in non-argument registers, in the
> >> same manner as a caller of an AAPCS function. The callee should not
> >> consume those values as they are not arguments.
> >>
> >>> Let me put it another way. Suggesting that unused arguments should be set to 0 is
> >>> the same as suggesting that normal C function that adheres to procedure call
> >>> standard for arm64 should always have 8 arguments, and for a particular function
> >>> that doesn't use all of them, they should be set to 0 by the caller.
> >> Heh, same rationale. :)
> > This is a good rationale for the function to not zero parameters.
> >
> >>> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he
> >>> wants to chime in on this.
> >>>
> >>> Thanks,
> >>> Alex
> >>>> At the very least it's a change in behaviour (ignoring the missing printf).
> >>>> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
> >> There's no need to zero non-argument registers, and it could potentially
> >> mask bugs in callees, so I don't think it's a good idea to do so.
> >>
> >> If you really want to test that the callee is robust and correct, it
> >> would be better to randomize the content of non-argument regsiters to
> >> fuzz the callee.
> >>
> > But this indicates there is risk that we'll be less robust if we don't
> > zero the parameters. Since this function is a common utility function and
> > kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody
> > wants to try and target, then if there's any chance that zeroing unused
> > parameters is more robust, I believe we should do that here. If we want to
>
> We agree that zero'ing unused parameters is not required by the specification,
> right?
Definitely
> After that, I think it depends how you see kvm-unit-tests. I am of the
> opinion that as a testing tool, if not zero'ing parameters (I'm not talking here
> about fuzzing) causes an error in whatever piece of software you are running, then
> that piece of software is not specification compliant and kvm-unit-tests has done
> its job.
We generally do our best to make sure the supporting code in the framework
is robust and fails cleanly (usually with asserts). If there's reason to
believe that the SUT may not implement a specification correctly, but we
have test results proving it at least works under certain constrains, then
we should probably keep the support code within those constraints. We can
also write independent test cases to check that the SUT implements the
specification completely. The reason for this approach is because not
every user of kvm-unit-tests is willing to debug a random, potentially
difficult to isolate failure when they're attempting to use the framework
to test something unrelated.
>
> Either way, I'm not advocating changing our PSCI code. I'm fine with dropping this
> patch for now (I mentioned this in another thread), so we can resume this
> conversation when I rework it.
Sounds good. And, like I said, I can't speak to the risk of not zeroing.
Perhaps there's not enough concern here to bother with it. Maybe we could
write those PSCI fuzzing tests sooner than later, confirm that on a
reasonable sample of targets that there's no risk in not zeroing, and then
implement all supporting functions as we wish.
Thanks,
drew
>
> Thanks,
> Alex
> > test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write
> > explicit test cases to do that.
> >
> > I can't speak to the risk of not zeroing, but due to the way we've been
> > calling PSCI functions with C, I can say up until now we always have.
> >
> > Thanks,
> > drew
> >
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 07/18] lib: arm/arm64: Add missing include for alloc_page.h in pgtable.h
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (5 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 06/18] arm/arm64: psci: Don't run C code without stack or vectors Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 08/18] lib: arm: Implement flush_tlb_all Alexandru Elisei
` (11 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
pgtable.h is used only by mmu.c, where it is included after alloc_page.h.
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/pgtable.h | 1 +
lib/arm64/asm/pgtable.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
index 794514b8c927..e7f967071980 100644
--- a/lib/arm/asm/pgtable.h
+++ b/lib/arm/asm/pgtable.h
@@ -13,6 +13,7 @@
*
* This work is licensed under the terms of the GNU GPL, version 2.
*/
+#include <alloc_page.h>
/*
* We can convert va <=> pa page table addresses with simple casts
diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
index dbf9e7253b71..6412d67759e4 100644
--- a/lib/arm64/asm/pgtable.h
+++ b/lib/arm64/asm/pgtable.h
@@ -14,6 +14,7 @@
* This work is licensed under the terms of the GNU GPL, version 2.
*/
#include <alloc.h>
+#include <alloc_page.h>
#include <asm/setup.h>
#include <asm/page.h>
#include <asm/pgtable-hwdef.h>
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 08/18] lib: arm: Implement flush_tlb_all
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (6 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 07/18] lib: arm/arm64: Add missing include for alloc_page.h in pgtable.h Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 09/18] lib: arm/arm64: Teach mmu_clear_user about block mappings Alexandru Elisei
` (10 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
flush_tlb_all performs a TLBIALL, which invalidates the entire TLB and
affects only the executing PE; translation table walks are now Inner
Shareable, so execute a TLBIALLIS (invalidate TLB Inner Shareable) instead.
TLBIALLIS is the equivalent of TLBIALL [1] when the multiprocessing
extensions are implemented, which are mandated by the virtualization
extensions.
Also add the necessary barriers to tlb_flush_all and a comment to
flush_dcache_addr stating what instruction is uses (unsurprisingly, it's
DCCIMVAC, which does a dcache clean and invalidate by VA to PoC).
[1] ARM DDI 0406C.d, section B3.10.6
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/mmu.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/lib/arm/asm/mmu.h b/lib/arm/asm/mmu.h
index 2bf8965ed35e..122874b8aebe 100644
--- a/lib/arm/asm/mmu.h
+++ b/lib/arm/asm/mmu.h
@@ -26,8 +26,11 @@ static inline void local_flush_tlb_all(void)
static inline void flush_tlb_all(void)
{
- //TODO
- local_flush_tlb_all();
+ dsb(ishst);
+ /* TLBIALLIS */
+ asm volatile("mcr p15, 0, %0, c8, c3, 0" :: "r" (0));
+ dsb(ish);
+ isb();
}
static inline void flush_tlb_page(unsigned long vaddr)
@@ -41,6 +44,7 @@ static inline void flush_tlb_page(unsigned long vaddr)
static inline void flush_dcache_addr(unsigned long vaddr)
{
+ /* DCCIMVAC */
asm volatile("mcr p15, 0, %0, c7, c14, 1" :: "r" (vaddr));
}
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 09/18] lib: arm/arm64: Teach mmu_clear_user about block mappings
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (7 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 08/18] lib: arm: Implement flush_tlb_all Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 10/18] arm/arm64: selftest: Add prefetch abort test Alexandru Elisei
` (9 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
kvm-unit-tests uses block mappings, so let's expand the mmu_clear_user
function to handle those as well.
Now that the function knows about block mappings, we cannot simply
assume that if an address isn't mapped we can map it as a regular page.
Change the semantics of the function to fail quite loudly if the address
isn't mapped, and shift the burden on the caller to map the address as a
page or block mapping before calling mmu_clear_user.
Also make mmu_clear_user more flexible by adding a pgtable parameter,
instead of assuming that the change always applies to the current
translation tables.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/mmu-api.h | 2 +-
lib/arm/asm/pgtable-hwdef.h | 3 +++
lib/arm/asm/pgtable.h | 7 +++++++
lib/arm64/asm/pgtable-hwdef.h | 3 +++
lib/arm64/asm/pgtable.h | 7 +++++++
lib/arm/mmu.c | 26 +++++++++++++++++++-------
arm/cache.c | 3 ++-
7 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
index 8fe85ba31ec9..2bbe1faea900 100644
--- a/lib/arm/asm/mmu-api.h
+++ b/lib/arm/asm/mmu-api.h
@@ -22,5 +22,5 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
phys_addr_t phys_start, phys_addr_t phys_end,
pgprot_t prot);
-extern void mmu_clear_user(unsigned long vaddr);
+extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
#endif
diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
index 4f24c78ee011..4107e188014a 100644
--- a/lib/arm/asm/pgtable-hwdef.h
+++ b/lib/arm/asm/pgtable-hwdef.h
@@ -14,6 +14,8 @@
#define PGDIR_SIZE (_AC(1,UL) << PGDIR_SHIFT)
#define PGDIR_MASK (~((1 << PGDIR_SHIFT) - 1))
+#define PGD_VALID (_AT(pgdval_t, 1) << 0)
+
#define PTRS_PER_PTE 512
#define PTRS_PER_PMD 512
@@ -54,6 +56,7 @@
#define PMD_TYPE_FAULT (_AT(pmdval_t, 0) << 0)
#define PMD_TYPE_TABLE (_AT(pmdval_t, 3) << 0)
#define PMD_TYPE_SECT (_AT(pmdval_t, 1) << 0)
+#define PMD_SECT_VALID (_AT(pmdval_t, 1) << 0)
#define PMD_TABLE_BIT (_AT(pmdval_t, 1) << 1)
#define PMD_BIT4 (_AT(pmdval_t, 0))
#define PMD_DOMAIN(x) (_AT(pmdval_t, 0))
diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
index e7f967071980..078dd16fa799 100644
--- a/lib/arm/asm/pgtable.h
+++ b/lib/arm/asm/pgtable.h
@@ -29,6 +29,13 @@
#define pmd_none(pmd) (!pmd_val(pmd))
#define pte_none(pte) (!pte_val(pte))
+#define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
+#define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
+#define pte_valid(pte) (pte_val(pte) & L_PTE_VALID)
+
+#define pmd_huge(pmd) \
+ ((pmd_val(pmd) & PMD_TYPE_MASK) == PMD_TYPE_SECT)
+
#define pgd_index(addr) \
(((addr) >> PGDIR_SHIFT) & (PTRS_PER_PGD - 1))
#define pgd_offset(pgtable, addr) ((pgtable) + pgd_index(addr))
diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
index 045a3ce12645..33524899e5fa 100644
--- a/lib/arm64/asm/pgtable-hwdef.h
+++ b/lib/arm64/asm/pgtable-hwdef.h
@@ -22,6 +22,8 @@
#define PGDIR_MASK (~(PGDIR_SIZE-1))
#define PTRS_PER_PGD (1 << (VA_BITS - PGDIR_SHIFT))
+#define PGD_VALID (_AT(pgdval_t, 1) << 0)
+
/* From include/asm-generic/pgtable-nopmd.h */
#define PMD_SHIFT PGDIR_SHIFT
#define PTRS_PER_PMD 1
@@ -71,6 +73,7 @@
#define PTE_TYPE_MASK (_AT(pteval_t, 3) << 0)
#define PTE_TYPE_FAULT (_AT(pteval_t, 0) << 0)
#define PTE_TYPE_PAGE (_AT(pteval_t, 3) << 0)
+#define PTE_VALID (_AT(pteval_t, 1) << 0)
#define PTE_TABLE_BIT (_AT(pteval_t, 1) << 1)
#define PTE_USER (_AT(pteval_t, 1) << 6) /* AP[1] */
#define PTE_RDONLY (_AT(pteval_t, 1) << 7) /* AP[2] */
diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
index 6412d67759e4..e577d9cf304e 100644
--- a/lib/arm64/asm/pgtable.h
+++ b/lib/arm64/asm/pgtable.h
@@ -33,6 +33,13 @@
#define pmd_none(pmd) (!pmd_val(pmd))
#define pte_none(pte) (!pte_val(pte))
+#define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
+#define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
+#define pte_valid(pte) (pte_val(pte) & PTE_VALID)
+
+#define pmd_huge(pmd) \
+ ((pmd_val(pmd) & PMD_TYPE_MASK) == PMD_TYPE_SECT)
+
#define pgd_index(addr) \
(((addr) >> PGDIR_SHIFT) & (PTRS_PER_PGD - 1))
#define pgd_offset(pgtable, addr) ((pgtable) + pgd_index(addr))
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 86a829966a3c..928a3702c563 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -211,19 +211,31 @@ unsigned long __phys_to_virt(phys_addr_t addr)
return addr;
}
-void mmu_clear_user(unsigned long vaddr)
+void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
{
- pgd_t *pgtable;
- pteval_t *pte;
- pteval_t entry;
+ pgd_t *pgd;
+ pmd_t *pmd;
+ pte_t *pte;
if (!mmu_enabled())
return;
- pgtable = current_thread_info()->pgtable;
- pte = get_pte(pgtable, vaddr);
+ pgd = pgd_offset(pgtable, vaddr);
+ assert(pgd_valid(*pgd));
+ pmd = pmd_offset(pgd, vaddr);
+ assert(pmd_valid(*pmd));
+
+ if (pmd_huge(*pmd)) {
+ pmd_t entry = __pmd(pmd_val(*pmd) & ~PMD_SECT_USER);
+ WRITE_ONCE(*pmd, entry);
+ goto out_flush_tlb;
+ }
- entry = *pte & ~PTE_USER;
+ pte = pte_offset(pmd, vaddr);
+ assert(pte_valid(*pte));
+ pte_t entry = __pte(pte_val(*pte) & ~PTE_USER);
WRITE_ONCE(*pte, entry);
+
+out_flush_tlb:
flush_tlb_page(vaddr);
}
diff --git a/arm/cache.c b/arm/cache.c
index 13dc5d52d40c..2756066fd4e9 100644
--- a/arm/cache.c
+++ b/arm/cache.c
@@ -2,6 +2,7 @@
#include <alloc_page.h>
#include <asm/mmu.h>
#include <asm/processor.h>
+#include <asm/thread_info.h>
#define NTIMES (1 << 16)
@@ -47,7 +48,7 @@ static void check_code_generation(bool dcache_clean, bool icache_inval)
bool success;
/* Make sure we can execute from a writable page */
- mmu_clear_user((unsigned long)code);
+ mmu_clear_user(current_thread_info()->pgtable, (unsigned long)code);
sctlr = read_sysreg(sctlr_el1);
if (sctlr & SCTLR_EL1_WXN) {
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 10/18] arm/arm64: selftest: Add prefetch abort test
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (8 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 09/18] lib: arm/arm64: Teach mmu_clear_user about block mappings Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-06 9:24 ` Andrew Jones
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 11/18] arm64: timer: Write to ICENABLER to disable timer IRQ Alexandru Elisei
` (8 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
When a guest tries to execute code from MMIO memory, KVM injects an
external abort into that guest. We have now fixed the psci test to not
fetch instructions from the I/O region, and it's not that often that a
guest misbehaves in such a way. Let's expand our coverage by adding a
proper test targetting this corner case.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm64/asm/esr.h | 3 ++
arm/selftest.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/lib/arm64/asm/esr.h b/lib/arm64/asm/esr.h
index 8e5af4d90767..8c351631b0a0 100644
--- a/lib/arm64/asm/esr.h
+++ b/lib/arm64/asm/esr.h
@@ -44,4 +44,7 @@
#define ESR_EL1_EC_BKPT32 (0x38)
#define ESR_EL1_EC_BRK64 (0x3C)
+#define ESR_EL1_FSC_MASK (0x3F)
+#define ESR_EL1_FSC_EXTABT (0x10)
+
#endif /* _ASMARM64_ESR_H_ */
diff --git a/arm/selftest.c b/arm/selftest.c
index 89759cf9f592..11dd432f4e6f 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -16,6 +16,8 @@
#include <asm/psci.h>
#include <asm/smp.h>
#include <asm/barrier.h>
+#include <asm/mmu.h>
+#include <asm/pgtable.h>
static cpumask_t ready, valid;
@@ -68,6 +70,7 @@ static void check_setup(int argc, char **argv)
static struct pt_regs expected_regs;
static bool und_works;
static bool svc_works;
+static bool pabt_works;
#if defined(__arm__)
/*
* Capture the current register state and execute an instruction
@@ -91,7 +94,7 @@ static bool svc_works;
"str r1, [r0, #" xstr(S_PC) "]\n" \
excptn_insn "\n" \
post_insns "\n" \
- :: "r" (&expected_regs) : "r0", "r1")
+ :: "r" (&expected_regs) : "r0", "r1", "r2")
static bool check_regs(struct pt_regs *regs)
{
@@ -171,6 +174,55 @@ static void user_psci_system_off(struct pt_regs *regs)
{
__user_psci_system_off();
}
+
+static void check_pabt_exit(void)
+{
+ install_exception_handler(EXCPTN_PABT, NULL);
+
+ report(pabt_works, "pabt");
+ exit(report_summary());
+}
+
+/* The page below the vmalloc region at 3G, assuming that PAGE_SIZE = 4K. */
+#define PABT_ADDR 0xbffff000
+static void pabt_handler(struct pt_regs *regs)
+{
+ expected_regs.ARM_pc = PABT_ADDR;
+ pabt_works = check_regs(regs);
+
+ regs->ARM_pc = (unsigned long)&check_pabt_exit;
+}
+
+static void check_pabt(void)
+{
+ unsigned long sctlr;
+
+ if (PABT_ADDR < __phys_end) {
+ report_skip("pabt: physical memory overlap");
+ return;
+ }
+
+ mmu_set_range_ptes(current_thread_info()->pgtable, PABT_ADDR,
+ PABT_ADDR, PABT_ADDR + PAGE_SIZE, __pgprot(PTE_WBWA));
+
+ /* Make sure we can actually execute from a writable region */
+ asm volatile("mrc p15, 0, %0, c1, c0, 0": "=r" (sctlr));
+ if (sctlr & CR_ST) {
+ sctlr &= ~CR_ST;
+ asm volatile("mcr p15, 0, %0, c1, c0, 0" :: "r" (sctlr));
+ isb();
+ /*
+ * Required according to the sequence in ARM DDI 0406C.d, page
+ * B3-1358.
+ */
+ flush_tlb_all();
+ }
+
+ install_exception_handler(EXCPTN_PABT, pabt_handler);
+
+ test_exception("ldr r2, =" xstr(PABT_ADDR), "bx r2", "");
+ __builtin_unreachable();
+}
#elif defined(__aarch64__)
/*
@@ -212,7 +264,7 @@ static void user_psci_system_off(struct pt_regs *regs)
"stp x0, x1, [x1]\n" \
"1:" excptn_insn "\n" \
post_insns "\n" \
- :: "r" (&expected_regs) : "x0", "x1")
+ :: "r" (&expected_regs) : "x0", "x1", "x2")
static bool check_regs(struct pt_regs *regs)
{
@@ -288,6 +340,60 @@ static bool check_svc(void)
return svc_works;
}
+static void check_pabt_exit(void)
+{
+ install_exception_handler(EL1H_SYNC, ESR_EL1_EC_IABT_EL1, NULL);
+
+ report(pabt_works, "pabt");
+ exit(report_summary());
+}
+
+/* The page below the MMIO region at 256G, assuming that PAGE_SIZE = 64K. */
+#define PABT_ADDR 0x3fffff0000
+static void pabt_handler(struct pt_regs *regs, unsigned int esr)
+{
+ bool is_extabt;
+
+ expected_regs.pc = PABT_ADDR;
+ is_extabt = (esr & ESR_EL1_FSC_MASK) == ESR_EL1_FSC_EXTABT;
+ pabt_works = check_regs(regs) && is_extabt;
+
+ regs->pc = (u64)&check_pabt_exit;
+}
+
+static void check_pabt(void)
+{
+ enum vector v = check_vector_prep();
+ unsigned long sctlr;
+
+ if (PABT_ADDR < __phys_end) {
+ report_skip("pabt: physical memory overlap");
+ return;
+ }
+
+ /*
+ * According to ARM DDI 0487E.a, table D5-33, footnote c, all regions
+ * writable at EL0 are treated as PXN. Map the page without the user bit
+ * set.
+ */
+ mmu_set_range_ptes(current_thread_info()->pgtable, PABT_ADDR,
+ PABT_ADDR, PABT_ADDR + PAGE_SIZE, __pgprot(PTE_WBWA));
+
+ /* Make sure we can actually execute from a writable region */
+ sctlr = read_sysreg(sctlr_el1);
+ if (sctlr & SCTLR_EL1_WXN) {
+ write_sysreg(sctlr & ~SCTLR_EL1_WXN, sctlr_el1);
+ isb();
+ /* SCTLR_EL1.WXN is permitted to be cached in a TLB. */
+ flush_tlb_all();
+ }
+
+ install_exception_handler(v, ESR_EL1_EC_IABT_EL1, pabt_handler);
+
+ test_exception("ldr x2, =" xstr(PABT_ADDR), "br x2", "");
+ __builtin_unreachable();
+}
+
static void user_psci_system_off(struct pt_regs *regs, unsigned int esr)
{
__user_psci_system_off();
@@ -305,6 +411,8 @@ static void check_vectors(void *arg __unused)
install_exception_handler(EL0_SYNC_64, ESR_EL1_EC_UNKNOWN,
user_psci_system_off);
#endif
+ } else {
+ check_pabt();
}
exit(report_summary());
}
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 10/18] arm/arm64: selftest: Add prefetch abort test
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 10/18] arm/arm64: selftest: Add prefetch abort test Alexandru Elisei
@ 2020-01-06 9:24 ` Andrew Jones
2020-01-06 11:03 ` Alexandru Elisei
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Jones @ 2020-01-06 9:24 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, maz, andre.przywara, vladimir.murzin, mark.rutland
On Tue, Dec 31, 2019 at 04:09:41PM +0000, Alexandru Elisei wrote:
> When a guest tries to execute code from MMIO memory, KVM injects an
> external abort into that guest. We have now fixed the psci test to not
> fetch instructions from the I/O region, and it's not that often that a
> guest misbehaves in such a way. Let's expand our coverage by adding a
> proper test targetting this corner case.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> lib/arm64/asm/esr.h | 3 ++
> arm/selftest.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 113 insertions(+), 2 deletions(-)
>
I like this test, but I have a few idea on how to make it more robust.
I'll send something out for review soon.
Thanks,
drew
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [kvm-unit-tests PATCH v3 10/18] arm/arm64: selftest: Add prefetch abort test
2020-01-06 9:24 ` Andrew Jones
@ 2020-01-06 11:03 ` Alexandru Elisei
0 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 11:03 UTC (permalink / raw)
To: Andrew Jones
Cc: kvm, pbonzini, maz, andre.przywara, vladimir.murzin, mark.rutland
Hi,
On 1/6/20 9:24 AM, Andrew Jones wrote:
> On Tue, Dec 31, 2019 at 04:09:41PM +0000, Alexandru Elisei wrote:
>> When a guest tries to execute code from MMIO memory, KVM injects an
>> external abort into that guest. We have now fixed the psci test to not
>> fetch instructions from the I/O region, and it's not that often that a
>> guest misbehaves in such a way. Let's expand our coverage by adding a
>> proper test targetting this corner case.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> lib/arm64/asm/esr.h | 3 ++
>> arm/selftest.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 113 insertions(+), 2 deletions(-)
>>
> I like this test, but I have a few idea on how to make it more robust.
> I'll send something out for review soon.
Great, looking forward to your ideas :)
Thanks,
Alex
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 11/18] arm64: timer: Write to ICENABLER to disable timer IRQ
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (9 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 10/18] arm/arm64: selftest: Add prefetch abort test Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-03 13:36 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 12/18] arm64: timer: EOIR the interrupt after masking the timer Alexandru Elisei
` (7 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
According the Generic Interrupt Controller versions 2, 3 and 4 architecture
specifications, a write of 0 to the GIC{D,R}_ISENABLER{,0} registers is
ignored; this is also how KVM emulates the corresponding register. Write
instead to the ICENABLER register when disabling the timer interrupt.
Note that fortunately for us, the timer test was still working as intended
because KVM does the sensible thing and all interrupts are disabled by
default when creating a VM.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/gic-v3.h | 1 +
lib/arm/asm/gic.h | 1 +
arm/timer.c | 22 +++++++++++-----------
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 347be2f9da17..0dc838b3ab2d 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -31,6 +31,7 @@
/* Re-Distributor registers, offsets from SGI_base */
#define GICR_IGROUPR0 GICD_IGROUPR
#define GICR_ISENABLER0 GICD_ISENABLER
+#define GICR_ICENABLER0 GICD_ICENABLER
#define GICR_IPRIORITYR0 GICD_IPRIORITYR
#define ICC_SGI1R_AFFINITY_1_SHIFT 16
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 1fc10a096259..09826fd5bc29 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -15,6 +15,7 @@
#define GICD_IIDR 0x0008
#define GICD_IGROUPR 0x0080
#define GICD_ISENABLER 0x0100
+#define GICD_ICENABLER 0x0180
#define GICD_ISPENDR 0x0200
#define GICD_ICPENDR 0x0280
#define GICD_ISACTIVER 0x0300
diff --git a/arm/timer.c b/arm/timer.c
index b30fd6b6d90b..f390e8e65d31 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -17,6 +17,9 @@
#define ARCH_TIMER_CTL_ISTATUS (1 << 2)
static void *gic_ispendr;
+static void *gic_isenabler;
+static void *gic_icenabler;
+
static bool ptimer_unsupported;
static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
@@ -132,19 +135,12 @@ static struct timer_info ptimer_info = {
static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
{
- u32 val = 0;
+ u32 val = 1 << PPI(info->irq);
if (enabled)
- val = 1 << PPI(info->irq);
-
- switch (gic_version()) {
- case 2:
- writel(val, gicv2_dist_base() + GICD_ISENABLER + 0);
- break;
- case 3:
- writel(val, gicv3_sgi_base() + GICR_ISENABLER0);
- break;
- }
+ writel(val, gic_isenabler);
+ else
+ writel(val, gic_icenabler);
}
static void irq_handler(struct pt_regs *regs)
@@ -306,9 +302,13 @@ static void test_init(void)
switch (gic_version()) {
case 2:
gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
+ gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
+ gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
break;
case 3:
gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
+ gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
+ gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
break;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 11/18] arm64: timer: Write to ICENABLER to disable timer IRQ
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 11/18] arm64: timer: Write to ICENABLER to disable timer IRQ Alexandru Elisei
@ 2020-01-03 13:36 ` Andre Przywara
0 siblings, 0 replies; 47+ messages in thread
From: Andre Przywara @ 2020-01-03 13:36 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:42 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> According the Generic Interrupt Controller versions 2, 3 and 4 architecture
> specifications, a write of 0 to the GIC{D,R}_ISENABLER{,0} registers is
> ignored; this is also how KVM emulates the corresponding register. Write
> instead to the ICENABLER register when disabling the timer interrupt.
>
> Note that fortunately for us, the timer test was still working as intended
> because KVM does the sensible thing and all interrupts are disabled by
> default when creating a VM.
Indeed, good catch!
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> lib/arm/asm/gic-v3.h | 1 +
> lib/arm/asm/gic.h | 1 +
> arm/timer.c | 22 +++++++++++-----------
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 347be2f9da17..0dc838b3ab2d 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -31,6 +31,7 @@
> /* Re-Distributor registers, offsets from SGI_base */
> #define GICR_IGROUPR0 GICD_IGROUPR
> #define GICR_ISENABLER0 GICD_ISENABLER
> +#define GICR_ICENABLER0 GICD_ICENABLER
> #define GICR_IPRIORITYR0 GICD_IPRIORITYR
>
> #define ICC_SGI1R_AFFINITY_1_SHIFT 16
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a096259..09826fd5bc29 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -15,6 +15,7 @@
> #define GICD_IIDR 0x0008
> #define GICD_IGROUPR 0x0080
> #define GICD_ISENABLER 0x0100
> +#define GICD_ICENABLER 0x0180
> #define GICD_ISPENDR 0x0200
> #define GICD_ICPENDR 0x0280
> #define GICD_ISACTIVER 0x0300
> diff --git a/arm/timer.c b/arm/timer.c
> index b30fd6b6d90b..f390e8e65d31 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -17,6 +17,9 @@
> #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
>
> static void *gic_ispendr;
> +static void *gic_isenabler;
> +static void *gic_icenabler;
> +
> static bool ptimer_unsupported;
>
> static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
> @@ -132,19 +135,12 @@ static struct timer_info ptimer_info = {
>
> static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
> {
> - u32 val = 0;
> + u32 val = 1 << PPI(info->irq);
>
> if (enabled)
> - val = 1 << PPI(info->irq);
> -
> - switch (gic_version()) {
> - case 2:
> - writel(val, gicv2_dist_base() + GICD_ISENABLER + 0);
> - break;
> - case 3:
> - writel(val, gicv3_sgi_base() + GICR_ISENABLER0);
> - break;
> - }
> + writel(val, gic_isenabler);
> + else
> + writel(val, gic_icenabler);
> }
>
> static void irq_handler(struct pt_regs *regs)
> @@ -306,9 +302,13 @@ static void test_init(void)
> switch (gic_version()) {
> case 2:
> gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
> + gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
> + gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
> break;
> case 3:
> gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
> + gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
> + gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
> break;
> }
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 12/18] arm64: timer: EOIR the interrupt after masking the timer
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (10 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 11/18] arm64: timer: Write to ICENABLER to disable timer IRQ Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-03 13:36 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked Alexandru Elisei
` (6 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
Writing to the EOIR register before masking the HW mapped timer
interrupt can cause taking another timer interrupt immediately after
exception return. This doesn't happen all the time, because KVM
reevaluates the state of pending HW mapped level sensitive interrupts on
each guest exit. If the second interrupt is pending and a guest exit
occurs after masking the timer interrupt and before the ERET (which
restores PSTATE.I), then KVM removes it.
Move the write after the IMASK bit has been set to prevent this from
happening.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/timer.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arm/timer.c b/arm/timer.c
index f390e8e65d31..67e95ede24ef 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -149,8 +149,8 @@ static void irq_handler(struct pt_regs *regs)
u32 irqstat = gic_read_iar();
u32 irqnr = gic_iar_irqnr(irqstat);
- if (irqnr != GICC_INT_SPURIOUS)
- gic_write_eoir(irqstat);
+ if (irqnr == GICC_INT_SPURIOUS)
+ return;
if (irqnr == PPI(vtimer_info.irq)) {
info = &vtimer_info;
@@ -162,7 +162,11 @@ static void irq_handler(struct pt_regs *regs)
}
info->write_ctl(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE);
+ isb();
+
info->irq_received = true;
+
+ gic_write_eoir(irqstat);
}
static bool gic_timer_pending(struct timer_info *info)
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 12/18] arm64: timer: EOIR the interrupt after masking the timer
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 12/18] arm64: timer: EOIR the interrupt after masking the timer Alexandru Elisei
@ 2020-01-03 13:36 ` Andre Przywara
2020-01-06 11:35 ` Alexandru Elisei
0 siblings, 1 reply; 47+ messages in thread
From: Andre Przywara @ 2020-01-03 13:36 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:43 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> Writing to the EOIR register before masking the HW mapped timer
> interrupt can cause taking another timer interrupt immediately after
> exception return. This doesn't happen all the time, because KVM
> reevaluates the state of pending HW mapped level sensitive interrupts on
> each guest exit. If the second interrupt is pending and a guest exit
> occurs after masking the timer interrupt and before the ERET (which
> restores PSTATE.I), then KVM removes it.
>
> Move the write after the IMASK bit has been set to prevent this from
> happening.
Sounds about right, just one comment below:
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/timer.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index f390e8e65d31..67e95ede24ef 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -149,8 +149,8 @@ static void irq_handler(struct pt_regs *regs)
> u32 irqstat = gic_read_iar();
> u32 irqnr = gic_iar_irqnr(irqstat);
>
> - if (irqnr != GICC_INT_SPURIOUS)
> - gic_write_eoir(irqstat);
> + if (irqnr == GICC_INT_SPURIOUS)
> + return;
>
> if (irqnr == PPI(vtimer_info.irq)) {
> info = &vtimer_info;
> @@ -162,7 +162,11 @@ static void irq_handler(struct pt_regs *regs)
> }
>
> info->write_ctl(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE);
> + isb();
Shall this isb() actually go into the write_[pv]timer_ctl() implementation? I see one other call where we enable the timer without an isb() afterwards. Plus I am not sure we wouldn't need it as well for disabling the timers?
Cheers,
Andre.
> +
> info->irq_received = true;
> +
> + gic_write_eoir(irqstat);
> }
>
> static bool gic_timer_pending(struct timer_info *info)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 12/18] arm64: timer: EOIR the interrupt after masking the timer
2020-01-03 13:36 ` Andre Przywara
@ 2020-01-06 11:35 ` Alexandru Elisei
0 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 11:35 UTC (permalink / raw)
To: Andre Przywara; +Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
Hi,
On 1/3/20 1:36 PM, Andre Przywara wrote:
> On Tue, 31 Dec 2019 16:09:43 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> Writing to the EOIR register before masking the HW mapped timer
>> interrupt can cause taking another timer interrupt immediately after
>> exception return. This doesn't happen all the time, because KVM
>> reevaluates the state of pending HW mapped level sensitive interrupts on
>> each guest exit. If the second interrupt is pending and a guest exit
>> occurs after masking the timer interrupt and before the ERET (which
>> restores PSTATE.I), then KVM removes it.
>>
>> Move the write after the IMASK bit has been set to prevent this from
>> happening.
> Sounds about right, just one comment below:
>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> arm/timer.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arm/timer.c b/arm/timer.c
>> index f390e8e65d31..67e95ede24ef 100644
>> --- a/arm/timer.c
>> +++ b/arm/timer.c
>> @@ -149,8 +149,8 @@ static void irq_handler(struct pt_regs *regs)
>> u32 irqstat = gic_read_iar();
>> u32 irqnr = gic_iar_irqnr(irqstat);
>>
>> - if (irqnr != GICC_INT_SPURIOUS)
>> - gic_write_eoir(irqstat);
>> + if (irqnr == GICC_INT_SPURIOUS)
>> + return;
>>
>> if (irqnr == PPI(vtimer_info.irq)) {
>> info = &vtimer_info;
>> @@ -162,7 +162,11 @@ static void irq_handler(struct pt_regs *regs)
>> }
>>
>> info->write_ctl(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE);
>> + isb();
> Shall this isb() actually go into the write_[pv]timer_ctl() implementation? I see one other call where we enable the timer without an isb() afterwards. Plus I am not sure we wouldn't need it as well for disabling the timers?
Good catch. From ARM DDI 0487E.a glossary, the section "Context synchronization
event":
"All direct and indirect writes to System registers that are made before the
Context synchronization event affect any instruction, including a direct read,
that appears in program order after the instruction causing the Context
synchronization event."
Based on that, I'll add an ISB after a control register write.
Thanks,
Alex
>
> Cheers,
> Andre.
>
>> +
>> info->irq_received = true;
>> +
>> + gic_write_eoir(irqstat);
>> }
>>
>> static bool gic_timer_pending(struct timer_info *info)
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (11 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 12/18] arm64: timer: EOIR the interrupt after masking the timer Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-03 13:37 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 14/18] lib: arm/arm64: Refuse to disable the MMU with non-identity stack pointer Alexandru Elisei
` (5 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
When the timer is disabled (the *_CTL_EL0.ENABLE bit is clear) or the
timer interrupt is masked at the timer level (the *_CTL_EL0.IMASK bit is
set), timer interrupts must not be pending or asserted by the VGIC.
However, only when the timer interrupt is masked, we can still check
that the timer condition is met by reading the *_CTL_EL0.ISTATUS bit.
This test was used to discover a bug and test the fix introduced by KVM
commit 16e604a437c8 ("KVM: arm/arm64: vgic: Reevaluate level sensitive
interrupts on enable").
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/timer.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arm/timer.c b/arm/timer.c
index 67e95ede24ef..a0b57afd4fe4 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -230,9 +230,17 @@ static void test_timer(struct timer_info *info)
/* Disable the timer again and prepare to take interrupts */
info->write_ctl(0);
+ isb();
+ info->irq_received = false;
set_timer_irq_enabled(info, true);
+ report(!info->irq_received, "no interrupt when timer is disabled");
report(!gic_timer_pending(info), "interrupt signal no longer pending");
+ info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
+ isb();
+ report(!gic_timer_pending(info), "interrupt signal not pending");
+ report(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS, "timer condition met");
+
report(test_cval_10msec(info), "latency within 10 ms");
report(info->irq_received, "interrupt received");
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked Alexandru Elisei
@ 2020-01-03 13:37 ` Andre Przywara
2020-01-06 13:22 ` Alexandru Elisei
0 siblings, 1 reply; 47+ messages in thread
From: Andre Przywara @ 2020-01-03 13:37 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:44 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> When the timer is disabled (the *_CTL_EL0.ENABLE bit is clear) or the
> timer interrupt is masked at the timer level (the *_CTL_EL0.IMASK bit is
> set), timer interrupts must not be pending or asserted by the VGIC.
> However, only when the timer interrupt is masked, we can still check
> that the timer condition is met by reading the *_CTL_EL0.ISTATUS bit.
>
> This test was used to discover a bug and test the fix introduced by KVM
> commit 16e604a437c8 ("KVM: arm/arm64: vgic: Reevaluate level sensitive
> interrupts on enable").
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/timer.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index 67e95ede24ef..a0b57afd4fe4 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -230,9 +230,17 @@ static void test_timer(struct timer_info *info)
>
> /* Disable the timer again and prepare to take interrupts */
> info->write_ctl(0);
> + isb();
> + info->irq_received = false;
> set_timer_irq_enabled(info, true);
Are we too impatient here? There does not seem to be a barrier after the write to the ISENABLER register, so I wonder if we need at least a dsb() here? I think in other occasions (GIC test) we even wait for some significant amount of time to allow interrupts to trigger (or not).
> + report(!info->irq_received, "no interrupt when timer is disabled");
> report(!gic_timer_pending(info), "interrupt signal no longer pending");
>
> + info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> + isb();
> + report(!gic_timer_pending(info), "interrupt signal not pending");
> + report(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS, "timer condition met");
> +
> report(test_cval_10msec(info), "latency within 10 ms");
> report(info->irq_received, "interrupt received");
Not part of your patch, but is this kind of evaluation of the irq_received actually valid? Does the compiler know that this gets set in another part of the code (the IRQ handler)? Do we need some synchronisation or barrier here to prevent the compiler from optimising or reordering the access to irq_received?
Cheers,
Andre.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked
2020-01-03 13:37 ` Andre Przywara
@ 2020-01-06 13:22 ` Alexandru Elisei
0 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 13:22 UTC (permalink / raw)
To: Andre Przywara; +Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
Hi,
On 1/3/20 1:37 PM, Andre Przywara wrote:
> On Tue, 31 Dec 2019 16:09:44 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> When the timer is disabled (the *_CTL_EL0.ENABLE bit is clear) or the
>> timer interrupt is masked at the timer level (the *_CTL_EL0.IMASK bit is
>> set), timer interrupts must not be pending or asserted by the VGIC.
>> However, only when the timer interrupt is masked, we can still check
>> that the timer condition is met by reading the *_CTL_EL0.ISTATUS bit.
>>
>> This test was used to discover a bug and test the fix introduced by KVM
>> commit 16e604a437c8 ("KVM: arm/arm64: vgic: Reevaluate level sensitive
>> interrupts on enable").
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> arm/timer.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/arm/timer.c b/arm/timer.c
>> index 67e95ede24ef..a0b57afd4fe4 100644
>> --- a/arm/timer.c
>> +++ b/arm/timer.c
>> @@ -230,9 +230,17 @@ static void test_timer(struct timer_info *info)
>>
>> /* Disable the timer again and prepare to take interrupts */
>> info->write_ctl(0);
>> + isb();
>> + info->irq_received = false;
>> set_timer_irq_enabled(info, true);
> Are we too impatient here? There does not seem to be a barrier after the write to the ISENABLER register, so I wonder if we need at least a dsb() here? I think in other occasions (GIC test) we even wait for some significant amount of time to allow interrupts to trigger (or not).
I'm not sure exactly what is going on with regards to the GIC write. The gicv3
driver tries to read the RWP (Register Write Pending) register after the ISENABLER
write until it times out. The gicv2 driver doesn't do anything after the write to
ISENABLER. As far as enabling/disabling interrupts, I think it would be best to
have a library function for that, that can deal with the differences between GIC
versions. In practice, this isn't an issue for the GIC because accesses are
trapped and a write always completes after the instruction finishes (and RWP is
always clear, to indicate that there are no writes in progress).
As for waiting for interrupts to trigger, I see no reason why we shouldn't do
that. I have some local patches that fix several issues with the GIC and refactor
check_acked, I'll get back to them after these fixes get merged.
>> + report(!info->irq_received, "no interrupt when timer is disabled");
>> report(!gic_timer_pending(info), "interrupt signal no longer pending");
>>
>> + info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
>> + isb();
>> + report(!gic_timer_pending(info), "interrupt signal not pending");
>> + report(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS, "timer condition met");
>> +
>> report(test_cval_10msec(info), "latency within 10 ms");
>> report(info->irq_received, "interrupt received");
> Not part of your patch, but is this kind of evaluation of the irq_received actually valid? Does the compiler know that this gets set in another part of the code (the IRQ handler)? Do we need some synchronisation or barrier here to prevent the compiler from optimising or reordering the access to irq_received?
We don't need a synchronization or memory barrier because the test is single
threaded. Exception entry and ERET are context synchronization events, so we don't
need an ISB when setting/reading irq_received.
But I think you're right about the compiler possibly messing with accesses to the
irq_received field. I'll mark it as volatile.
Thanks,
Alex
>
> Cheers,
> Andre.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 14/18] lib: arm/arm64: Refuse to disable the MMU with non-identity stack pointer
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (12 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 13/18] arm64: timer: Test behavior when timer disabled or masked Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off Alexandru Elisei
` (4 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
When the MMU is off, all addresses are physical addresses. If the stack
pointer is not an identity mapped address (the virtual address is not the
same as the physical address), then we end up trying to access an invalid
memory region. This can happen if we call mmu_disable from a secondary CPU,
which has its stack allocated from the vmalloc region.
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/mmu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 928a3702c563..111e3a52591a 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -68,8 +68,12 @@ void mmu_enable(pgd_t *pgtable)
extern void asm_mmu_disable(void);
void mmu_disable(void)
{
+ unsigned long sp = current_stack_pointer;
int cpu = current_thread_info()->cpu;
+ assert_msg(__virt_to_phys(sp) == sp,
+ "Attempting to disable MMU with non-identity mapped stack");
+
mmu_mark_disabled(cpu);
asm_mmu_disable();
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (13 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 14/18] lib: arm/arm64: Refuse to disable the MMU with non-identity stack pointer Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-03 16:49 ` Andre Przywara
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 16/18] arm: cstart64.S: Downgrade TLBI to non-shareable in asm_mmu_enable Alexandru Elisei
` (3 subsequent siblings)
18 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
When the MMU is off, data accesses are to Device nGnRnE memory on arm64 [1]
or to Strongly-Ordered memory on arm [2]. This means that the accesses are
non-cacheable.
Perform a dcache clean to PoC so we can read the newer values from the
cache after we turn the MMU off, instead of the stale values from memory.
Perform an invalidation so we can access the data written to memory after
we turn the MMU back on. This prevents reading back the stale values we
cleaned from the cache when we turned the MMU off.
Data caches are PIPT and the VAs are translated using the current
translation tables, or an identity mapping (what Arm calls a "flat
mapping") when the MMU is off [1, 2]. Do the clean + invalidate when the
MMU is off so we don't depend on the current translation tables and we can
make sure that the operation applies to the entire physical memory.
The patch was tested by hacking arm/selftest.c:
+#include <alloc_page.h>
+#include <asm/mmu.h>
int main(int argc, char **argv)
{
+ int *x = alloc_page();
+
report_prefix_push("selftest");
+ *x = 0x42;
+ mmu_disable();
+ report("read back value written with MMU on", *x == 0x42);
+ *x = 0x50;
+ mmu_enable(current_thread_info()->pgtable);
+ report("read back value written with MMU off", *x == 0x50);
+
if (argc < 2)
report_abort("no test specified");
Without the fix, the first report fails, and the test usually hangs before
the second report. This is because mmu_enable pushes the LR register on the
stack when the MMU is off, which means that the value will be written to
memory. However, after asm_mmu_enable, the MMU is enabled, and we read it
back from the dcache, thus getting garbage.
With the fix, the two reports pass.
[1] ARM DDI 0487E.a, section D5.2.9
[2] ARM DDI 0406C.d, section B3.2.1
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/processor.h | 6 ++++++
lib/arm64/asm/processor.h | 6 ++++++
lib/arm/processor.c | 10 ++++++++++
lib/arm/setup.c | 2 ++
lib/arm64/processor.c | 11 +++++++++++
arm/cstart.S | 22 ++++++++++++++++++++++
arm/cstart64.S | 23 +++++++++++++++++++++++
7 files changed, 80 insertions(+)
diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
index a8c4628da818..4684fb4755b3 100644
--- a/lib/arm/asm/processor.h
+++ b/lib/arm/asm/processor.h
@@ -9,6 +9,11 @@
#include <asm/sysreg.h>
#include <asm/barrier.h>
+#define CTR_DMINLINE_SHIFT 16
+#define CTR_DMINLINE_MASK (0xf << 16)
+#define CTR_DMINLINE(x) \
+ (((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT)
+
enum vector {
EXCPTN_RST,
EXCPTN_UND,
@@ -25,6 +30,7 @@ typedef void (*exception_fn)(struct pt_regs *);
extern void install_exception_handler(enum vector v, exception_fn fn);
extern void show_regs(struct pt_regs *regs);
+extern void init_dcache_line_size(void);
static inline unsigned long current_cpsr(void)
{
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 1d9223f728a5..fd508c02f30d 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -16,6 +16,11 @@
#define SCTLR_EL1_A (1 << 1)
#define SCTLR_EL1_M (1 << 0)
+#define CTR_EL0_DMINLINE_SHIFT 16
+#define CTR_EL0_DMINLINE_MASK (0xf << 16)
+#define CTR_EL0_DMINLINE(x) \
+ (((x) & CTR_EL0_DMINLINE_MASK) >> CTR_EL0_DMINLINE_SHIFT)
+
#ifndef __ASSEMBLY__
#include <asm/ptrace.h>
#include <asm/esr.h>
@@ -60,6 +65,7 @@ extern void vector_handlers_default_init(vector_fn *handlers);
extern void show_regs(struct pt_regs *regs);
extern bool get_far(unsigned int esr, unsigned long *far);
+extern void init_dcache_line_size(void);
static inline unsigned long current_level(void)
{
diff --git a/lib/arm/processor.c b/lib/arm/processor.c
index 773337e6d3b7..c57657c5ea53 100644
--- a/lib/arm/processor.c
+++ b/lib/arm/processor.c
@@ -25,6 +25,8 @@ static const char *vector_names[] = {
"rst", "und", "svc", "pabt", "dabt", "addrexcptn", "irq", "fiq"
};
+unsigned int dcache_line_size;
+
void show_regs(struct pt_regs *regs)
{
unsigned long flags;
@@ -145,3 +147,11 @@ bool is_user(void)
{
return current_thread_info()->flags & TIF_USER_MODE;
}
+void init_dcache_line_size(void)
+{
+ u32 ctr;
+
+ asm volatile("mrc p15, 0, %0, c0, c0, 1" : "=r" (ctr));
+ /* DminLine is log2 of the number of words in the smallest cache line */
+ dcache_line_size = 1 << (CTR_DMINLINE(ctr) + 2);
+}
diff --git a/lib/arm/setup.c b/lib/arm/setup.c
index 4f02fca85607..54fc19a20942 100644
--- a/lib/arm/setup.c
+++ b/lib/arm/setup.c
@@ -20,6 +20,7 @@
#include <asm/thread_info.h>
#include <asm/setup.h>
#include <asm/page.h>
+#include <asm/processor.h>
#include <asm/smp.h>
#include "io.h"
@@ -63,6 +64,7 @@ static void cpu_init(void)
ret = dt_for_each_cpu_node(cpu_set, NULL);
assert(ret == 0);
set_cpu_online(0, true);
+ init_dcache_line_size();
}
static void mem_init(phys_addr_t freemem_start)
diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
index 2a024e3f4e9d..f28066d40145 100644
--- a/lib/arm64/processor.c
+++ b/lib/arm64/processor.c
@@ -62,6 +62,8 @@ static const char *ec_names[EC_MAX] = {
[ESR_EL1_EC_BRK64] = "BRK64",
};
+unsigned int dcache_line_size;
+
void show_regs(struct pt_regs *regs)
{
int i;
@@ -257,3 +259,12 @@ bool is_user(void)
{
return current_thread_info()->flags & TIF_USER_MODE;
}
+
+void init_dcache_line_size(void)
+{
+ u64 ctr;
+
+ ctr = read_sysreg(ctr_el0);
+ /* DminLine is log2 of the number of words in the smallest cache line */
+ dcache_line_size = 1 << (CTR_EL0_DMINLINE(ctr) + 2);
+}
diff --git a/arm/cstart.S b/arm/cstart.S
index dfef48e4dbb2..3c2a3bcde61a 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -188,6 +188,20 @@ asm_mmu_enable:
mov pc, lr
+.macro dcache_clean_inval domain, start, end, tmp1, tmp2
+ ldr \tmp1, =dcache_line_size
+ ldr \tmp1, [\tmp1]
+ sub \tmp2, \tmp1, #1
+ bic \start, \start, \tmp2
+9998:
+ /* DCCIMVAC */
+ mcr p15, 0, \start, c7, c14, 1
+ add \start, \start, \tmp1
+ cmp \start, \end
+ blo 9998b
+ dsb \domain
+.endm
+
.globl asm_mmu_disable
asm_mmu_disable:
/* SCTLR */
@@ -195,6 +209,14 @@ asm_mmu_disable:
bic r0, #CR_M
mcr p15, 0, r0, c1, c0, 0
isb
+
+ ldr r0, =__phys_offset
+ ldr r0, [r0]
+ ldr r1, =__phys_end
+ ldr r1, [r1]
+ dcache_clean_inval sy, r0, r1, r2, r3
+ isb
+
mov pc, lr
/*
diff --git a/arm/cstart64.S b/arm/cstart64.S
index c98842f11e90..f41ffa3bc6c2 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -201,12 +201,35 @@ asm_mmu_enable:
ret
+/* Taken with small changes from arch/arm64/incluse/asm/assembler.h */
+.macro dcache_by_line_op op, domain, start, end, tmp1, tmp2
+ adrp \tmp1, dcache_line_size
+ ldr \tmp1, [\tmp1, :lo12:dcache_line_size]
+ sub \tmp2, \tmp1, #1
+ bic \start, \start, \tmp2
+9998:
+ dc \op , \start
+ add \start, \start, \tmp1
+ cmp \start, \end
+ b.lo 9998b
+ dsb \domain
+.endm
+
.globl asm_mmu_disable
asm_mmu_disable:
mrs x0, sctlr_el1
bic x0, x0, SCTLR_EL1_M
msr sctlr_el1, x0
isb
+
+ /* Clean + invalidate the entire memory */
+ adrp x0, __phys_offset
+ ldr x0, [x0, :lo12:__phys_offset]
+ adrp x1, __phys_end
+ ldr x1, [x1, :lo12:__phys_end]
+ dcache_by_line_op civac, sy, x0, x1, x2, x3
+ isb
+
ret
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off Alexandru Elisei
@ 2020-01-03 16:49 ` Andre Przywara
2020-01-06 14:27 ` Alexandru Elisei
0 siblings, 1 reply; 47+ messages in thread
From: Andre Przywara @ 2020-01-03 16:49 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
On Tue, 31 Dec 2019 16:09:46 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> When the MMU is off, data accesses are to Device nGnRnE memory on arm64 [1]
> or to Strongly-Ordered memory on arm [2]. This means that the accesses are
> non-cacheable.
>
> Perform a dcache clean to PoC so we can read the newer values from the
> cache after we turn the MMU off, instead of the stale values from memory.
Wow, did we really not do this before?
> Perform an invalidation so we can access the data written to memory after
> we turn the MMU back on. This prevents reading back the stale values we
> cleaned from the cache when we turned the MMU off.
>
> Data caches are PIPT and the VAs are translated using the current
> translation tables, or an identity mapping (what Arm calls a "flat
> mapping") when the MMU is off [1, 2]. Do the clean + invalidate when the
> MMU is off so we don't depend on the current translation tables and we can
> make sure that the operation applies to the entire physical memory.
The intention of the patch is very much valid, I am just wondering if there is any reason why you do the cache line size determination in (quite some lines of) C?
Given that you only use that in asm, wouldn't it be much easier to read the CTR register there, just before you actually use it? The actual CTR read is (inline) assembly anyway, so you just need the mask/shift/add in asm as well. You could draw inspiration from here, for instance:
https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/cpu/armv8/cache.S#L132
> The patch was tested by hacking arm/selftest.c:
>
> +#include <alloc_page.h>
> +#include <asm/mmu.h>
> int main(int argc, char **argv)
> {
> + int *x = alloc_page();
> +
> report_prefix_push("selftest");
>
> + *x = 0x42;
> + mmu_disable();
> + report("read back value written with MMU on", *x == 0x42);
> + *x = 0x50;
> + mmu_enable(current_thread_info()->pgtable);
> + report("read back value written with MMU off", *x == 0x50);
> +
Shall this be a new test then as well? At least to avoid regressions in kvm-unit-tests itself? But also to test for proper MMU-off and cache inval operations inside guests?
Cheers,
Andre
> if (argc < 2)
> report_abort("no test specified");
>
> Without the fix, the first report fails, and the test usually hangs before
> the second report. This is because mmu_enable pushes the LR register on the
> stack when the MMU is off, which means that the value will be written to
> memory. However, after asm_mmu_enable, the MMU is enabled, and we read it
> back from the dcache, thus getting garbage.
>
> With the fix, the two reports pass.
>
> [1] ARM DDI 0487E.a, section D5.2.9
> [2] ARM DDI 0406C.d, section B3.2.1
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> lib/arm/asm/processor.h | 6 ++++++
> lib/arm64/asm/processor.h | 6 ++++++
> lib/arm/processor.c | 10 ++++++++++
> lib/arm/setup.c | 2 ++
> lib/arm64/processor.c | 11 +++++++++++
> arm/cstart.S | 22 ++++++++++++++++++++++
> arm/cstart64.S | 23 +++++++++++++++++++++++
> 7 files changed, 80 insertions(+)
>
> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
> index a8c4628da818..4684fb4755b3 100644
> --- a/lib/arm/asm/processor.h
> +++ b/lib/arm/asm/processor.h
> @@ -9,6 +9,11 @@
> #include <asm/sysreg.h>
> #include <asm/barrier.h>
>
> +#define CTR_DMINLINE_SHIFT 16
> +#define CTR_DMINLINE_MASK (0xf << 16)
> +#define CTR_DMINLINE(x) \
> + (((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT)
> +
> enum vector {
> EXCPTN_RST,
> EXCPTN_UND,
> @@ -25,6 +30,7 @@ typedef void (*exception_fn)(struct pt_regs *);
> extern void install_exception_handler(enum vector v, exception_fn fn);
>
> extern void show_regs(struct pt_regs *regs);
> +extern void init_dcache_line_size(void);
>
> static inline unsigned long current_cpsr(void)
> {
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 1d9223f728a5..fd508c02f30d 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -16,6 +16,11 @@
> #define SCTLR_EL1_A (1 << 1)
> #define SCTLR_EL1_M (1 << 0)
>
> +#define CTR_EL0_DMINLINE_SHIFT 16
> +#define CTR_EL0_DMINLINE_MASK (0xf << 16)
> +#define CTR_EL0_DMINLINE(x) \
> + (((x) & CTR_EL0_DMINLINE_MASK) >> CTR_EL0_DMINLINE_SHIFT)
> +
> #ifndef __ASSEMBLY__
> #include <asm/ptrace.h>
> #include <asm/esr.h>
> @@ -60,6 +65,7 @@ extern void vector_handlers_default_init(vector_fn *handlers);
>
> extern void show_regs(struct pt_regs *regs);
> extern bool get_far(unsigned int esr, unsigned long *far);
> +extern void init_dcache_line_size(void);
>
> static inline unsigned long current_level(void)
> {
> diff --git a/lib/arm/processor.c b/lib/arm/processor.c
> index 773337e6d3b7..c57657c5ea53 100644
> --- a/lib/arm/processor.c
> +++ b/lib/arm/processor.c
> @@ -25,6 +25,8 @@ static const char *vector_names[] = {
> "rst", "und", "svc", "pabt", "dabt", "addrexcptn", "irq", "fiq"
> };
>
> +unsigned int dcache_line_size;
> +
> void show_regs(struct pt_regs *regs)
> {
> unsigned long flags;
> @@ -145,3 +147,11 @@ bool is_user(void)
> {
> return current_thread_info()->flags & TIF_USER_MODE;
> }
> +void init_dcache_line_size(void)
> +{
> + u32 ctr;
> +
> + asm volatile("mrc p15, 0, %0, c0, c0, 1" : "=r" (ctr));
> + /* DminLine is log2 of the number of words in the smallest cache line */
> + dcache_line_size = 1 << (CTR_DMINLINE(ctr) + 2);
> +}
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index 4f02fca85607..54fc19a20942 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -20,6 +20,7 @@
> #include <asm/thread_info.h>
> #include <asm/setup.h>
> #include <asm/page.h>
> +#include <asm/processor.h>
> #include <asm/smp.h>
>
> #include "io.h"
> @@ -63,6 +64,7 @@ static void cpu_init(void)
> ret = dt_for_each_cpu_node(cpu_set, NULL);
> assert(ret == 0);
> set_cpu_online(0, true);
> + init_dcache_line_size();
> }
>
> static void mem_init(phys_addr_t freemem_start)
> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
> index 2a024e3f4e9d..f28066d40145 100644
> --- a/lib/arm64/processor.c
> +++ b/lib/arm64/processor.c
> @@ -62,6 +62,8 @@ static const char *ec_names[EC_MAX] = {
> [ESR_EL1_EC_BRK64] = "BRK64",
> };
>
> +unsigned int dcache_line_size;
> +
> void show_regs(struct pt_regs *regs)
> {
> int i;
> @@ -257,3 +259,12 @@ bool is_user(void)
> {
> return current_thread_info()->flags & TIF_USER_MODE;
> }
> +
> +void init_dcache_line_size(void)
> +{
> + u64 ctr;
> +
> + ctr = read_sysreg(ctr_el0);
> + /* DminLine is log2 of the number of words in the smallest cache line */
> + dcache_line_size = 1 << (CTR_EL0_DMINLINE(ctr) + 2);
> +}
> diff --git a/arm/cstart.S b/arm/cstart.S
> index dfef48e4dbb2..3c2a3bcde61a 100644
> --- a/arm/cstart.S
> +++ b/arm/cstart.S
> @@ -188,6 +188,20 @@ asm_mmu_enable:
>
> mov pc, lr
>
> +.macro dcache_clean_inval domain, start, end, tmp1, tmp2
> + ldr \tmp1, =dcache_line_size
> + ldr \tmp1, [\tmp1]
> + sub \tmp2, \tmp1, #1
> + bic \start, \start, \tmp2
> +9998:
> + /* DCCIMVAC */
> + mcr p15, 0, \start, c7, c14, 1
> + add \start, \start, \tmp1
> + cmp \start, \end
> + blo 9998b
> + dsb \domain
> +.endm
> +
> .globl asm_mmu_disable
> asm_mmu_disable:
> /* SCTLR */
> @@ -195,6 +209,14 @@ asm_mmu_disable:
> bic r0, #CR_M
> mcr p15, 0, r0, c1, c0, 0
> isb
> +
> + ldr r0, =__phys_offset
> + ldr r0, [r0]
> + ldr r1, =__phys_end
> + ldr r1, [r1]
> + dcache_clean_inval sy, r0, r1, r2, r3
> + isb
> +
> mov pc, lr
>
> /*
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index c98842f11e90..f41ffa3bc6c2 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -201,12 +201,35 @@ asm_mmu_enable:
>
> ret
>
> +/* Taken with small changes from arch/arm64/incluse/asm/assembler.h */
> +.macro dcache_by_line_op op, domain, start, end, tmp1, tmp2
> + adrp \tmp1, dcache_line_size
> + ldr \tmp1, [\tmp1, :lo12:dcache_line_size]
> + sub \tmp2, \tmp1, #1
> + bic \start, \start, \tmp2
> +9998:
> + dc \op , \start
> + add \start, \start, \tmp1
> + cmp \start, \end
> + b.lo 9998b
> + dsb \domain
> +.endm
> +
> .globl asm_mmu_disable
> asm_mmu_disable:
> mrs x0, sctlr_el1
> bic x0, x0, SCTLR_EL1_M
> msr sctlr_el1, x0
> isb
> +
> + /* Clean + invalidate the entire memory */
> + adrp x0, __phys_offset
> + ldr x0, [x0, :lo12:__phys_offset]
> + adrp x1, __phys_end
> + ldr x1, [x1, :lo12:__phys_end]
> + dcache_by_line_op civac, sy, x0, x1, x2, x3
> + isb
> +
> ret
>
> /*
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off
2020-01-03 16:49 ` Andre Przywara
@ 2020-01-06 14:27 ` Alexandru Elisei
2020-01-06 16:28 ` Andrew Jones
0 siblings, 1 reply; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-06 14:27 UTC (permalink / raw)
To: Andre Przywara; +Cc: kvm, pbonzini, drjones, maz, vladimir.murzin, mark.rutland
Hi,
On 1/3/20 4:49 PM, Andre Przywara wrote:
> On Tue, 31 Dec 2019 16:09:46 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> When the MMU is off, data accesses are to Device nGnRnE memory on arm64 [1]
>> or to Strongly-Ordered memory on arm [2]. This means that the accesses are
>> non-cacheable.
>>
>> Perform a dcache clean to PoC so we can read the newer values from the
>> cache after we turn the MMU off, instead of the stale values from memory.
> Wow, did we really not do this before?
>
>> Perform an invalidation so we can access the data written to memory after
>> we turn the MMU back on. This prevents reading back the stale values we
>> cleaned from the cache when we turned the MMU off.
>>
>> Data caches are PIPT and the VAs are translated using the current
>> translation tables, or an identity mapping (what Arm calls a "flat
>> mapping") when the MMU is off [1, 2]. Do the clean + invalidate when the
>> MMU is off so we don't depend on the current translation tables and we can
>> make sure that the operation applies to the entire physical memory.
> The intention of the patch is very much valid, I am just wondering if there is any reason why you do the cache line size determination in (quite some lines of) C?
> Given that you only use that in asm, wouldn't it be much easier to read the CTR register there, just before you actually use it? The actual CTR read is (inline) assembly anyway, so you just need the mask/shift/add in asm as well. You could draw inspiration from here, for instance:
> https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/cpu/armv8/cache.S#L132
Computing the dcache line size in assembly is how Linux does it as well. I chose
to do it in C because I like to avoid using assembly as much as possible. But I
have no strong preference in keeping it in C. Andrew, what do you think? Should
the cache line size be computed in C or in assembly, in asm_mmu_disable?
>
>> The patch was tested by hacking arm/selftest.c:
>>
>> +#include <alloc_page.h>
>> +#include <asm/mmu.h>
>> int main(int argc, char **argv)
>> {
>> + int *x = alloc_page();
>> +
>> report_prefix_push("selftest");
>>
>> + *x = 0x42;
>> + mmu_disable();
>> + report("read back value written with MMU on", *x == 0x42);
>> + *x = 0x50;
>> + mmu_enable(current_thread_info()->pgtable);
>> + report("read back value written with MMU off", *x == 0x50);
>> +
> Shall this be a new test then as well? At least to avoid regressions in kvm-unit-tests itself? But also to test for proper MMU-off and cache inval operations inside guests?
I'm not sure what you mean by proper MMU-off and cache inval operations. KVM (at
the moment) doesn't trap toggling the MMU off and on, KVM stops trapping SCTLR
after you turn the MMU on for the first time (take a look at kvm_toggle_cache from
virt/kvm/arm/mmu.c). And the cache maintenance operations don't require KVM
involvement.
As for testing for regressions, improper CMOs when you turn the MMU off will be
very visible for any test that actually turns the MMU off. I spotted it when I
wrote a two line test that turned the MMU off and called report, the function was
obviously broken because it wasn't keeping track properly of the number of passed
tests. I don't think adding a test to check for MMU toggling is worth it.
>
> Cheers,
> Andre
>
>> if (argc < 2)
>> report_abort("no test specified");
>>
>> Without the fix, the first report fails, and the test usually hangs before
>> the second report. This is because mmu_enable pushes the LR register on the
>> stack when the MMU is off, which means that the value will be written to
>> memory. However, after asm_mmu_enable, the MMU is enabled, and we read it
>> back from the dcache, thus getting garbage.
>>
>> With the fix, the two reports pass.
>>
>> [1] ARM DDI 0487E.a, section D5.2.9
>> [2] ARM DDI 0406C.d, section B3.2.1
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> lib/arm/asm/processor.h | 6 ++++++
>> lib/arm64/asm/processor.h | 6 ++++++
>> lib/arm/processor.c | 10 ++++++++++
>> lib/arm/setup.c | 2 ++
>> lib/arm64/processor.c | 11 +++++++++++
>> arm/cstart.S | 22 ++++++++++++++++++++++
>> arm/cstart64.S | 23 +++++++++++++++++++++++
>> 7 files changed, 80 insertions(+)
>>
>> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
>> index a8c4628da818..4684fb4755b3 100644
>> --- a/lib/arm/asm/processor.h
>> +++ b/lib/arm/asm/processor.h
>> @@ -9,6 +9,11 @@
>> #include <asm/sysreg.h>
>> #include <asm/barrier.h>
>>
>> +#define CTR_DMINLINE_SHIFT 16
>> +#define CTR_DMINLINE_MASK (0xf << 16)
>> +#define CTR_DMINLINE(x) \
>> + (((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT)
>> +
>> enum vector {
>> EXCPTN_RST,
>> EXCPTN_UND,
>> @@ -25,6 +30,7 @@ typedef void (*exception_fn)(struct pt_regs *);
>> extern void install_exception_handler(enum vector v, exception_fn fn);
>>
>> extern void show_regs(struct pt_regs *regs);
>> +extern void init_dcache_line_size(void);
>>
>> static inline unsigned long current_cpsr(void)
>> {
>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>> index 1d9223f728a5..fd508c02f30d 100644
>> --- a/lib/arm64/asm/processor.h
>> +++ b/lib/arm64/asm/processor.h
>> @@ -16,6 +16,11 @@
>> #define SCTLR_EL1_A (1 << 1)
>> #define SCTLR_EL1_M (1 << 0)
>>
>> +#define CTR_EL0_DMINLINE_SHIFT 16
>> +#define CTR_EL0_DMINLINE_MASK (0xf << 16)
>> +#define CTR_EL0_DMINLINE(x) \
>> + (((x) & CTR_EL0_DMINLINE_MASK) >> CTR_EL0_DMINLINE_SHIFT)
>> +
>> #ifndef __ASSEMBLY__
>> #include <asm/ptrace.h>
>> #include <asm/esr.h>
>> @@ -60,6 +65,7 @@ extern void vector_handlers_default_init(vector_fn *handlers);
>>
>> extern void show_regs(struct pt_regs *regs);
>> extern bool get_far(unsigned int esr, unsigned long *far);
>> +extern void init_dcache_line_size(void);
>>
>> static inline unsigned long current_level(void)
>> {
>> diff --git a/lib/arm/processor.c b/lib/arm/processor.c
>> index 773337e6d3b7..c57657c5ea53 100644
>> --- a/lib/arm/processor.c
>> +++ b/lib/arm/processor.c
>> @@ -25,6 +25,8 @@ static const char *vector_names[] = {
>> "rst", "und", "svc", "pabt", "dabt", "addrexcptn", "irq", "fiq"
>> };
>>
>> +unsigned int dcache_line_size;
>> +
>> void show_regs(struct pt_regs *regs)
>> {
>> unsigned long flags;
>> @@ -145,3 +147,11 @@ bool is_user(void)
>> {
>> return current_thread_info()->flags & TIF_USER_MODE;
>> }
>> +void init_dcache_line_size(void)
>> +{
>> + u32 ctr;
>> +
>> + asm volatile("mrc p15, 0, %0, c0, c0, 1" : "=r" (ctr));
>> + /* DminLine is log2 of the number of words in the smallest cache line */
>> + dcache_line_size = 1 << (CTR_DMINLINE(ctr) + 2);
>> +}
>> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
>> index 4f02fca85607..54fc19a20942 100644
>> --- a/lib/arm/setup.c
>> +++ b/lib/arm/setup.c
>> @@ -20,6 +20,7 @@
>> #include <asm/thread_info.h>
>> #include <asm/setup.h>
>> #include <asm/page.h>
>> +#include <asm/processor.h>
>> #include <asm/smp.h>
>>
>> #include "io.h"
>> @@ -63,6 +64,7 @@ static void cpu_init(void)
>> ret = dt_for_each_cpu_node(cpu_set, NULL);
>> assert(ret == 0);
>> set_cpu_online(0, true);
>> + init_dcache_line_size();
>> }
>>
>> static void mem_init(phys_addr_t freemem_start)
>> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
>> index 2a024e3f4e9d..f28066d40145 100644
>> --- a/lib/arm64/processor.c
>> +++ b/lib/arm64/processor.c
>> @@ -62,6 +62,8 @@ static const char *ec_names[EC_MAX] = {
>> [ESR_EL1_EC_BRK64] = "BRK64",
>> };
>>
>> +unsigned int dcache_line_size;
>> +
>> void show_regs(struct pt_regs *regs)
>> {
>> int i;
>> @@ -257,3 +259,12 @@ bool is_user(void)
>> {
>> return current_thread_info()->flags & TIF_USER_MODE;
>> }
>> +
>> +void init_dcache_line_size(void)
>> +{
>> + u64 ctr;
>> +
>> + ctr = read_sysreg(ctr_el0);
>> + /* DminLine is log2 of the number of words in the smallest cache line */
>> + dcache_line_size = 1 << (CTR_EL0_DMINLINE(ctr) + 2);
>> +}
>> diff --git a/arm/cstart.S b/arm/cstart.S
>> index dfef48e4dbb2..3c2a3bcde61a 100644
>> --- a/arm/cstart.S
>> +++ b/arm/cstart.S
>> @@ -188,6 +188,20 @@ asm_mmu_enable:
>>
>> mov pc, lr
>>
>> +.macro dcache_clean_inval domain, start, end, tmp1, tmp2
>> + ldr \tmp1, =dcache_line_size
>> + ldr \tmp1, [\tmp1]
>> + sub \tmp2, \tmp1, #1
>> + bic \start, \start, \tmp2
>> +9998:
>> + /* DCCIMVAC */
>> + mcr p15, 0, \start, c7, c14, 1
>> + add \start, \start, \tmp1
>> + cmp \start, \end
>> + blo 9998b
>> + dsb \domain
>> +.endm
>> +
>> .globl asm_mmu_disable
>> asm_mmu_disable:
>> /* SCTLR */
>> @@ -195,6 +209,14 @@ asm_mmu_disable:
>> bic r0, #CR_M
>> mcr p15, 0, r0, c1, c0, 0
>> isb
>> +
>> + ldr r0, =__phys_offset
>> + ldr r0, [r0]
>> + ldr r1, =__phys_end
>> + ldr r1, [r1]
>> + dcache_clean_inval sy, r0, r1, r2, r3
>> + isb
>> +
>> mov pc, lr
>>
>> /*
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index c98842f11e90..f41ffa3bc6c2 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -201,12 +201,35 @@ asm_mmu_enable:
>>
>> ret
>>
>> +/* Taken with small changes from arch/arm64/incluse/asm/assembler.h */
>> +.macro dcache_by_line_op op, domain, start, end, tmp1, tmp2
>> + adrp \tmp1, dcache_line_size
>> + ldr \tmp1, [\tmp1, :lo12:dcache_line_size]
>> + sub \tmp2, \tmp1, #1
>> + bic \start, \start, \tmp2
>> +9998:
>> + dc \op , \start
>> + add \start, \start, \tmp1
>> + cmp \start, \end
>> + b.lo 9998b
>> + dsb \domain
>> +.endm
>> +
>> .globl asm_mmu_disable
>> asm_mmu_disable:
>> mrs x0, sctlr_el1
>> bic x0, x0, SCTLR_EL1_M
>> msr sctlr_el1, x0
>> isb
>> +
>> + /* Clean + invalidate the entire memory */
>> + adrp x0, __phys_offset
>> + ldr x0, [x0, :lo12:__phys_offset]
>> + adrp x1, __phys_end
>> + ldr x1, [x1, :lo12:__phys_end]
>> + dcache_by_line_op civac, sy, x0, x1, x2, x3
>> + isb
>> +
>> ret
>>
>> /*
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off
2020-01-06 14:27 ` Alexandru Elisei
@ 2020-01-06 16:28 ` Andrew Jones
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Jones @ 2020-01-06 16:28 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Andre Przywara, kvm, pbonzini, maz, vladimir.murzin, mark.rutland
On Mon, Jan 06, 2020 at 02:27:31PM +0000, Alexandru Elisei wrote:
> Hi,
>
> On 1/3/20 4:49 PM, Andre Przywara wrote:
> > On Tue, 31 Dec 2019 16:09:46 +0000
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >
> > Hi,
> >
> >> When the MMU is off, data accesses are to Device nGnRnE memory on arm64 [1]
> >> or to Strongly-Ordered memory on arm [2]. This means that the accesses are
> >> non-cacheable.
> >>
> >> Perform a dcache clean to PoC so we can read the newer values from the
> >> cache after we turn the MMU off, instead of the stale values from memory.
> > Wow, did we really not do this before?
> >
> >> Perform an invalidation so we can access the data written to memory after
> >> we turn the MMU back on. This prevents reading back the stale values we
> >> cleaned from the cache when we turned the MMU off.
> >>
> >> Data caches are PIPT and the VAs are translated using the current
> >> translation tables, or an identity mapping (what Arm calls a "flat
> >> mapping") when the MMU is off [1, 2]. Do the clean + invalidate when the
> >> MMU is off so we don't depend on the current translation tables and we can
> >> make sure that the operation applies to the entire physical memory.
> > The intention of the patch is very much valid, I am just wondering if there is any reason why you do the cache line size determination in (quite some lines of) C?
> > Given that you only use that in asm, wouldn't it be much easier to read the CTR register there, just before you actually use it? The actual CTR read is (inline) assembly anyway, so you just need the mask/shift/add in asm as well. You could draw inspiration from here, for instance:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/cpu/armv8/cache.S#L132
>
> Computing the dcache line size in assembly is how Linux does it as well. I chose
> to do it in C because I like to avoid using assembly as much as possible. But I
> have no strong preference in keeping it in C. Andrew, what do you think? Should
> the cache line size be computed in C or in assembly, in asm_mmu_disable?
I also prefer to minimize the amount of assembly and to minimize the
amount of code in general. For something like this I probably wouldn't
have introduced the macros, unless there's reason to believe unit tests
will also make use of them. Instead, I would just introduce get_ctr()
to avoid #ifdef's and then put the calculation directly into cpu_init()
like below. However I don't have a strong opinion here, so whatever
makes you guys happy :-)
Thanks,
drew
diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
index a8c4628da818..ae7e3816e676 100644
--- a/lib/arm/asm/processor.h
+++ b/lib/arm/asm/processor.h
@@ -64,6 +64,7 @@ extern bool is_user(void);
#define CNTVCT __ACCESS_CP15_64(1, c14)
#define CNTFRQ __ACCESS_CP15(c14, 0, c0, 0)
+#define CTR __ACCESS_CP15(c0, 0, c0, 1)
static inline u64 get_cntvct(void)
{
@@ -76,4 +77,11 @@ static inline u32 get_cntfrq(void)
return read_sysreg(CNTFRQ);
}
+static inline u32 get_ctr(void)
+{
+ return read_sysreg(CTR);
+}
+
+extern u32 dcache_line_size;
+
#endif /* _ASMARM_PROCESSOR_H_ */
diff --git a/lib/arm/setup.c b/lib/arm/setup.c
index 4f02fca85607..11b9cc9602ea 100644
--- a/lib/arm/setup.c
+++ b/lib/arm/setup.c
@@ -35,6 +35,8 @@ int nr_cpus;
struct mem_region mem_regions[NR_MEM_REGIONS];
phys_addr_t __phys_offset, __phys_end;
+u32 dcache_line_size;
+
int mpidr_to_cpu(uint64_t mpidr)
{
int i;
@@ -59,6 +61,8 @@ static void cpu_init(void)
{
int ret;
+ dcache_line_size = 4 << ((get_ctr() >> 16) & 0xf);
+
nr_cpus = 0;
ret = dt_for_each_cpu_node(cpu_set, NULL);
assert(ret == 0);
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 1d9223f728a5..5cba1591eda7 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -105,5 +105,12 @@ static inline u32 get_cntfrq(void)
return read_sysreg(cntfrq_el0);
}
+static inline u32 get_ctr(void)
+{
+ return read_sysreg(ctr_el0);
+}
+
+extern u32 dcache_line_size;
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASMARM64_PROCESSOR_H_ */
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [kvm-unit-tests PATCH v3 16/18] arm: cstart64.S: Downgrade TLBI to non-shareable in asm_mmu_enable
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (14 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 15/18] arm/arm64: Perform dcache clean + invalidate after turning MMU off Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 17/18] arm/arm64: Invalidate TLB before enabling MMU Alexandru Elisei
` (2 subsequent siblings)
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
There's really no need to invalidate the TLB entries for all CPUs when
enabling the MMU for the current CPU, so use the non-shareable version of
the TLBI operation (and downgrade the DSB accordingly).
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/cstart64.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arm/cstart64.S b/arm/cstart64.S
index f41ffa3bc6c2..87bf873795a1 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -167,8 +167,8 @@ halt:
.globl asm_mmu_enable
asm_mmu_enable:
ic iallu // I+BTB cache invalidate
- tlbi vmalle1is // invalidate I + D TLBs
- dsb ish
+ tlbi vmalle1 // invalidate I + D TLBs
+ dsb nsh
/* TCR */
ldr x1, =TCR_TxSZ(VA_BITS) | \
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 17/18] arm/arm64: Invalidate TLB before enabling MMU
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (15 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 16/18] arm: cstart64.S: Downgrade TLBI to non-shareable in asm_mmu_enable Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 18/18] arm: cstart64.S: Remove icache invalidation from asm_mmu_enable Alexandru Elisei
2020-01-06 9:28 ` [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Andrew Jones
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
Let's invalidate the TLB before enabling the MMU, not after, so we don't
accidently use a stale TLB mapping. For arm, we add a TLBIALL operation,
which applies only to the PE that executed the instruction [1]. For arm64,
we already do that in asm_mmu_enable.
We now find ourselves in a situation where we issue an extra invalidation
after asm_mmu_enable returns. Remove this redundant call to tlb_flush_all.
[1] ARM DDI 0406C.d, section B3.10.6
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/mmu.c | 1 -
arm/cstart.S | 4 ++++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 111e3a52591a..5fb56180d334 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -59,7 +59,6 @@ void mmu_enable(pgd_t *pgtable)
struct thread_info *info = current_thread_info();
asm_mmu_enable(__pa(pgtable));
- flush_tlb_all();
info->pgtable = pgtable;
mmu_mark_enabled(info->cpu);
diff --git a/arm/cstart.S b/arm/cstart.S
index 3c2a3bcde61a..32b2b4f03098 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -161,6 +161,10 @@ halt:
.equ NMRR, 0xff000004 @ MAIR1 (from Linux kernel)
.globl asm_mmu_enable
asm_mmu_enable:
+ /* TLBIALL */
+ mcr p15, 0, r2, c8, c7, 0
+ dsb nsh
+
/* TTBCR */
ldr r2, =(TTBCR_EAE | \
TTBCR_SH0_SHARED | \
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [kvm-unit-tests PATCH v3 18/18] arm: cstart64.S: Remove icache invalidation from asm_mmu_enable
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (16 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 17/18] arm/arm64: Invalidate TLB before enabling MMU Alexandru Elisei
@ 2019-12-31 16:09 ` Alexandru Elisei
2020-01-06 9:28 ` [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Andrew Jones
18 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2019-12-31 16:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones, maz, andre.przywara, vladimir.murzin, mark.rutland
According to the ARM ARM [1]:
"In Armv8, any permitted instruction cache implementation can be
described as implementing the IVIPT Extension to the Arm architecture.
The formal definition of the Arm IVIPT Extension is that it reduces the
instruction cache maintenance requirement to the following condition:
Instruction cache maintenance is required only after writing new data to
a PA that holds an instruction".
We never patch instructions in the boot path, so remove the icache
invalidation from asm_mmu_enable. Tests that modify instructions (like
the cache test) should have their own icache maintenance operations.
[1] ARM DDI 0487E.a, section D5.11.2 "Instruction caches"
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/cstart64.S | 1 -
1 file changed, 1 deletion(-)
diff --git a/arm/cstart64.S b/arm/cstart64.S
index 87bf873795a1..7e7f8b2e8f0b 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -166,7 +166,6 @@ halt:
.globl asm_mmu_enable
asm_mmu_enable:
- ic iallu // I+BTB cache invalidate
tlbi vmalle1 // invalidate I + D TLBs
dsb nsh
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes
2019-12-31 16:09 [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Alexandru Elisei
` (17 preceding siblings ...)
2019-12-31 16:09 ` [kvm-unit-tests PATCH v3 18/18] arm: cstart64.S: Remove icache invalidation from asm_mmu_enable Alexandru Elisei
@ 2020-01-06 9:28 ` Andrew Jones
2020-01-09 10:01 ` Alexandru Elisei
18 siblings, 1 reply; 47+ messages in thread
From: Andrew Jones @ 2020-01-06 9:28 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, pbonzini, maz, andre.przywara, vladimir.murzin, mark.rutland
On Tue, Dec 31, 2019 at 04:09:31PM +0000, Alexandru Elisei wrote:
> This is a combination of the fixes from my EL2 series [1] and other new
> fixes. I've rebased the series on top of 2c6589bc4e8b ("Update AMD
> instructions to conform to LLVM assembler"), which means that I had to
> switch the order of parameters for the report function.
>
> This time around I tried to do a better job at testing. I've ran
> kvm-unit-tests in the following configurations:
>
> - with kvmtool, on an arm64 host kernel: 64 and 32 bit tests, with GICv3
> (on an Ampere eMAG) and GICv2 (on a AMD Seattle box).
>
> - with qemu, on an arm64 host kernel:
> a. with accel=kvm, 64 and 32 bit tests, with GICv3 (Ampere eMAG) and
> GICv2 (Seattle).
> b. with accel=tcg, 64 and 32 bit tests, on the Ampere eMAG machine.
>
> I didn't run the 32 bit tests under a 32 bit host kernel because I don't
> have a 32 bit arm board at hand at the moment. It's also worth noting that
> when I tried running the selftest-vectors-kernel tests on an ancient
> version of qemu (QEMU emulator version 2.5.0 (Debian
> 1:2.5+dfsg-5ubuntu10.42)) I got the following error:
>
> $ arm/run arm/selftest.flat -append vectors-kernel
> /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/selftest.flat -append vectors-kernel # -initrd /tmp/tmp.zNO1kWtmuM
> PASS: selftest: vectors-kernel: und
> PASS: selftest: vectors-kernel: svc
> qemu: fatal: Trying to execute code outside RAM or ROM at 0x0000003fffff0000
>
> PC=0000003fffff0000 SP=00000000400aff70
> X00=00000000400805a0 X01=0000000040092f20 X02=0000003fffff0000 X03=0000000040092f20
> X04=0000000000000010 X05=00000000400aff40 X06=00000000400aff70 X07=00000000400aff70
> X08=00000000400afde0 X09=ffffff80ffffffc8 X10=00000000400afe20 X11=00000000400afe20
> X12=00000000400b0000 X13=00000000400afeac X14=00000000400b0000 X15=0000000000000000
> X16=0000000000000000 X17=0000000000000000 X18=0000000000000000 X19=0000000040092000
> X20=0000000000000004 X21=0000000040092e98 X22=0000000040092f20 X23=0000000000000000
> X24=0000000000000000 X25=0000000000000000 X26=0000000000000000 X27=0000000000000000
> X28=0000000000000000 X29=0000000000000000 X30=000000004008052c
> PSTATE=800003c5 N--- EL1h
> q00=0000000000000000:0000000000000000 q01=0000000000000000:0000000000000000
> q02=0000000000000000:0000000000000000 q03=0000000000000000:0000000000000000
> q04=0000000000000000:0000000000000000 q05=0000000000000000:0000000000000000
> q06=0000000000000000:0000000000000000 q07=0000000000000000:0000000000000000
> q08=0000000000000000:0000000000000000 q09=0000000000000000:0000000000000000
> q10=0000000000000000:0000000000000000 q11=0000000000000000:0000000000000000
> q12=0000000000000000:0000000000000000 q13=0000000000000000:0000000000000000
> q14=0000000000000000:0000000000000000 q15=0000000000000000:0000000000000000
> q16=0000000000000000:0000000000000000 q17=0000000000000000:0000000000000000
> q18=0000000000000000:0000000000000000 q19=0000000000000000:0000000000000000
> q20=0000000000000000:0000000000000000 q21=0000000000000000:0000000000000000
> q22=0000000000000000:0000000000000000 q23=0000000000000000:0000000000000000
> q24=0000000000000000:0000000000000000 q25=0000000000000000:0000000000000000
> q26=0000000000000000:0000000000000000 q27=0000000000000000:0000000000000000
> q28=0000000000000000:0000000000000000 q29=0000000000000000:0000000000000000
> q30=0000000000000000:0000000000000000 q31=0000000000000000:0000000000000000
> FPCR: 00000000 FPSR: 00000000
> QEMU Aborted
>
> I'm not sure if we support such an old version of qemu. If we do, please
> let me know, and I'll try to come up with a solution. I am reluctant to
> drop the prefetch abort test because it uncovered a bug in the nested
> virtualization patches.
>
> Summary of the patches:
> * Patch 1 adds coherent translation table walks for ARMv7 and removes
> unneeded dcache maintenance.
> * Patches 2-4 make translation table updates more robust.
> * Patches 5-6 fix a pretty serious bug in our PSCI test, which was causing
> an infinite loop of prefetch aborts.
> * Patches 7-10 add a proper test for prefetch aborts. The test now uses
> mmu_clear_user.
> * Patches 11-13 are fixes for the timer test.
> * Patches 14-15 fix turning the MMU off.
> * Patches 16-18 are small fixes to make the code more robust, and perhaps
> more important, remove unnecessary operations that might hide real bugs
> in KVM.
>
> Patches 1-4, 9, 18 are new. The rest are taken from the EL2 series, and
> I've kept the Reviewed-by tag where appropriate.
>
> Changes in v3:
> * Implemented review comments.
> * Minor cosmetic changes to the commit messages here and there.
> * Removed the duplicate DSB ISHST that I had added to mmu.c in patch #1.
> flush_tlb_page already has the needed barriers.
> * Replaced patch #2 "lib: arm64: Remove barriers before TLB operations"
> with "lib: arm: Add proper data synchronization barriers for TLBIs".
> I've decided to keep the needed barriers in the flush_tlb_* functions, to
> match what the kernel does.
> * Added a missing DSB ISHST in flush_tlb_all in patch #8 "lib: arm:
> Implement flush_tlb_all"
> * The address for the prefetch abort test is now in hexadecimal to prevent
> a compile error.
> * Added information about the KVM bug that patch #13 "arm64: timer: Test
> behavior when timer disabled or masked" helped find.
> * Explained in the commit message for #15 how to reproduce some of the
> errors that I was seeing without the patch.
>
> Changes in v2:
> * Fixed the prefetch abort test on QEMU by changing the address used to
> cause the abort.
>
> [1] https://www.spinics.net/lists/kvm/msg196797.html
>
> Alexandru Elisei (18):
> lib: arm/arm64: Remove unnecessary dcache maintenance operations
> lib: arm: Add proper data synchronization barriers for TLBIs
> lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h
> lib: arm/arm64: Use WRITE_ONCE to update the translation tables
> lib: arm/arm64: Remove unused CPU_OFF parameter
> arm/arm64: psci: Don't run C code without stack or vectors
> lib: arm/arm64: Add missing include for alloc_page.h in pgtable.h
> lib: arm: Implement flush_tlb_all
> lib: arm/arm64: Teach mmu_clear_user about block mappings
> arm/arm64: selftest: Add prefetch abort test
> arm64: timer: Write to ICENABLER to disable timer IRQ
> arm64: timer: EOIR the interrupt after masking the timer
> arm64: timer: Test behavior when timer disabled or masked
> lib: arm/arm64: Refuse to disable the MMU with non-identity stack
> pointer
> arm/arm64: Perform dcache clean + invalidate after turning MMU off
> arm: cstart64.S: Downgrade TLBI to non-shareable in asm_mmu_enable
> arm/arm64: Invalidate TLB before enabling MMU
> arm: cstart64.S: Remove icache invalidation from asm_mmu_enable
>
> lib/linux/compiler.h | 83 +++++++++++++++++++++++++++++++
> lib/arm/asm/gic-v3.h | 1 +
> lib/arm/asm/gic.h | 1 +
> lib/arm/asm/mmu-api.h | 2 +-
> lib/arm/asm/mmu.h | 18 ++++---
> lib/arm/asm/pgtable-hwdef.h | 11 +++++
> lib/arm/asm/pgtable.h | 20 ++++++--
> lib/arm/asm/processor.h | 6 +++
> lib/arm64/asm/esr.h | 3 ++
> lib/arm64/asm/pgtable-hwdef.h | 3 ++
> lib/arm64/asm/pgtable.h | 15 +++++-
> lib/arm64/asm/processor.h | 6 +++
> lib/arm/mmu.c | 60 ++++++++++++----------
> lib/arm/processor.c | 10 ++++
> lib/arm/psci.c | 4 +-
> lib/arm/setup.c | 2 +
> lib/arm64/processor.c | 11 +++++
> arm/cstart.S | 40 ++++++++++++++-
> arm/cstart64.S | 35 +++++++++++--
> arm/cache.c | 3 +-
> arm/psci.c | 5 +-
> arm/selftest.c | 112 +++++++++++++++++++++++++++++++++++++++++-
> arm/timer.c | 38 +++++++++-----
> 23 files changed, 425 insertions(+), 64 deletions(-)
> create mode 100644 lib/linux/compiler.h
>
> --
> 2.7.4
>
Thanks Alexandru. I'm queuing everything except
arm/arm64: psci: Don't run C code without stack or vectors
arm/arm64: selftest: Add prefetch abort test
arm64: timer: EOIR the interrupt after masking the timer
arm64: timer: Test behavior when timer disabled or masked
arm/arm64: Perform dcache clean + invalidate after turning MMU off
as those had comments from Andre and myself to address.
Thanks,
drew
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes
2020-01-06 9:28 ` [kvm-unit-tests PATCH v3 00/18] arm/arm64: Various fixes Andrew Jones
@ 2020-01-09 10:01 ` Alexandru Elisei
0 siblings, 0 replies; 47+ messages in thread
From: Alexandru Elisei @ 2020-01-09 10:01 UTC (permalink / raw)
To: Andrew Jones
Cc: kvm, pbonzini, maz, andre.przywara, vladimir.murzin, mark.rutland
Hi,
On 1/6/20 9:28 AM, Andrew Jones wrote:
> On Tue, Dec 31, 2019 at 04:09:31PM +0000, Alexandru Elisei wrote:
>> This is a combination of the fixes from my EL2 series [1] and other new
>> fixes. I've rebased the series on top of 2c6589bc4e8b ("Update AMD
>> instructions to conform to LLVM assembler"), which means that I had to
>> switch the order of parameters for the report function.
>>
>> This time around I tried to do a better job at testing. I've ran
>> kvm-unit-tests in the following configurations:
>>
>> - with kvmtool, on an arm64 host kernel: 64 and 32 bit tests, with GICv3
>> (on an Ampere eMAG) and GICv2 (on a AMD Seattle box).
>>
>> - with qemu, on an arm64 host kernel:
>> a. with accel=kvm, 64 and 32 bit tests, with GICv3 (Ampere eMAG) and
>> GICv2 (Seattle).
>> b. with accel=tcg, 64 and 32 bit tests, on the Ampere eMAG machine.
>>
>> I didn't run the 32 bit tests under a 32 bit host kernel because I don't
>> have a 32 bit arm board at hand at the moment. It's also worth noting that
>> when I tried running the selftest-vectors-kernel tests on an ancient
>> version of qemu (QEMU emulator version 2.5.0 (Debian
>> 1:2.5+dfsg-5ubuntu10.42)) I got the following error:
>>
>> $ arm/run arm/selftest.flat -append vectors-kernel
>> /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/selftest.flat -append vectors-kernel # -initrd /tmp/tmp.zNO1kWtmuM
>> PASS: selftest: vectors-kernel: und
>> PASS: selftest: vectors-kernel: svc
>> qemu: fatal: Trying to execute code outside RAM or ROM at 0x0000003fffff0000
>>
>> PC=0000003fffff0000 SP=00000000400aff70
>> X00=00000000400805a0 X01=0000000040092f20 X02=0000003fffff0000 X03=0000000040092f20
>> X04=0000000000000010 X05=00000000400aff40 X06=00000000400aff70 X07=00000000400aff70
>> X08=00000000400afde0 X09=ffffff80ffffffc8 X10=00000000400afe20 X11=00000000400afe20
>> X12=00000000400b0000 X13=00000000400afeac X14=00000000400b0000 X15=0000000000000000
>> X16=0000000000000000 X17=0000000000000000 X18=0000000000000000 X19=0000000040092000
>> X20=0000000000000004 X21=0000000040092e98 X22=0000000040092f20 X23=0000000000000000
>> X24=0000000000000000 X25=0000000000000000 X26=0000000000000000 X27=0000000000000000
>> X28=0000000000000000 X29=0000000000000000 X30=000000004008052c
>> PSTATE=800003c5 N--- EL1h
>> q00=0000000000000000:0000000000000000 q01=0000000000000000:0000000000000000
>> q02=0000000000000000:0000000000000000 q03=0000000000000000:0000000000000000
>> q04=0000000000000000:0000000000000000 q05=0000000000000000:0000000000000000
>> q06=0000000000000000:0000000000000000 q07=0000000000000000:0000000000000000
>> q08=0000000000000000:0000000000000000 q09=0000000000000000:0000000000000000
>> q10=0000000000000000:0000000000000000 q11=0000000000000000:0000000000000000
>> q12=0000000000000000:0000000000000000 q13=0000000000000000:0000000000000000
>> q14=0000000000000000:0000000000000000 q15=0000000000000000:0000000000000000
>> q16=0000000000000000:0000000000000000 q17=0000000000000000:0000000000000000
>> q18=0000000000000000:0000000000000000 q19=0000000000000000:0000000000000000
>> q20=0000000000000000:0000000000000000 q21=0000000000000000:0000000000000000
>> q22=0000000000000000:0000000000000000 q23=0000000000000000:0000000000000000
>> q24=0000000000000000:0000000000000000 q25=0000000000000000:0000000000000000
>> q26=0000000000000000:0000000000000000 q27=0000000000000000:0000000000000000
>> q28=0000000000000000:0000000000000000 q29=0000000000000000:0000000000000000
>> q30=0000000000000000:0000000000000000 q31=0000000000000000:0000000000000000
>> FPCR: 00000000 FPSR: 00000000
>> QEMU Aborted
>>
>> I'm not sure if we support such an old version of qemu. If we do, please
>> let me know, and I'll try to come up with a solution. I am reluctant to
>> drop the prefetch abort test because it uncovered a bug in the nested
>> virtualization patches.
>>
>> Summary of the patches:
>> * Patch 1 adds coherent translation table walks for ARMv7 and removes
>> unneeded dcache maintenance.
>> * Patches 2-4 make translation table updates more robust.
>> * Patches 5-6 fix a pretty serious bug in our PSCI test, which was causing
>> an infinite loop of prefetch aborts.
>> * Patches 7-10 add a proper test for prefetch aborts. The test now uses
>> mmu_clear_user.
>> * Patches 11-13 are fixes for the timer test.
>> * Patches 14-15 fix turning the MMU off.
>> * Patches 16-18 are small fixes to make the code more robust, and perhaps
>> more important, remove unnecessary operations that might hide real bugs
>> in KVM.
>>
>> Patches 1-4, 9, 18 are new. The rest are taken from the EL2 series, and
>> I've kept the Reviewed-by tag where appropriate.
>>
>> Changes in v3:
>> * Implemented review comments.
>> * Minor cosmetic changes to the commit messages here and there.
>> * Removed the duplicate DSB ISHST that I had added to mmu.c in patch #1.
>> flush_tlb_page already has the needed barriers.
>> * Replaced patch #2 "lib: arm64: Remove barriers before TLB operations"
>> with "lib: arm: Add proper data synchronization barriers for TLBIs".
>> I've decided to keep the needed barriers in the flush_tlb_* functions, to
>> match what the kernel does.
>> * Added a missing DSB ISHST in flush_tlb_all in patch #8 "lib: arm:
>> Implement flush_tlb_all"
>> * The address for the prefetch abort test is now in hexadecimal to prevent
>> a compile error.
>> * Added information about the KVM bug that patch #13 "arm64: timer: Test
>> behavior when timer disabled or masked" helped find.
>> * Explained in the commit message for #15 how to reproduce some of the
>> errors that I was seeing without the patch.
>>
>> Changes in v2:
>> * Fixed the prefetch abort test on QEMU by changing the address used to
>> cause the abort.
>>
>> [1] https://www.spinics.net/lists/kvm/msg196797.html
>>
>> Alexandru Elisei (18):
>> lib: arm/arm64: Remove unnecessary dcache maintenance operations
>> lib: arm: Add proper data synchronization barriers for TLBIs
>> lib: Add WRITE_ONCE and READ_ONCE implementations in compiler.h
>> lib: arm/arm64: Use WRITE_ONCE to update the translation tables
>> lib: arm/arm64: Remove unused CPU_OFF parameter
>> arm/arm64: psci: Don't run C code without stack or vectors
>> lib: arm/arm64: Add missing include for alloc_page.h in pgtable.h
>> lib: arm: Implement flush_tlb_all
>> lib: arm/arm64: Teach mmu_clear_user about block mappings
>> arm/arm64: selftest: Add prefetch abort test
>> arm64: timer: Write to ICENABLER to disable timer IRQ
>> arm64: timer: EOIR the interrupt after masking the timer
>> arm64: timer: Test behavior when timer disabled or masked
>> lib: arm/arm64: Refuse to disable the MMU with non-identity stack
>> pointer
>> arm/arm64: Perform dcache clean + invalidate after turning MMU off
>> arm: cstart64.S: Downgrade TLBI to non-shareable in asm_mmu_enable
>> arm/arm64: Invalidate TLB before enabling MMU
>> arm: cstart64.S: Remove icache invalidation from asm_mmu_enable
>>
>> lib/linux/compiler.h | 83 +++++++++++++++++++++++++++++++
>> lib/arm/asm/gic-v3.h | 1 +
>> lib/arm/asm/gic.h | 1 +
>> lib/arm/asm/mmu-api.h | 2 +-
>> lib/arm/asm/mmu.h | 18 ++++---
>> lib/arm/asm/pgtable-hwdef.h | 11 +++++
>> lib/arm/asm/pgtable.h | 20 ++++++--
>> lib/arm/asm/processor.h | 6 +++
>> lib/arm64/asm/esr.h | 3 ++
>> lib/arm64/asm/pgtable-hwdef.h | 3 ++
>> lib/arm64/asm/pgtable.h | 15 +++++-
>> lib/arm64/asm/processor.h | 6 +++
>> lib/arm/mmu.c | 60 ++++++++++++----------
>> lib/arm/processor.c | 10 ++++
>> lib/arm/psci.c | 4 +-
>> lib/arm/setup.c | 2 +
>> lib/arm64/processor.c | 11 +++++
>> arm/cstart.S | 40 ++++++++++++++-
>> arm/cstart64.S | 35 +++++++++++--
>> arm/cache.c | 3 +-
>> arm/psci.c | 5 +-
>> arm/selftest.c | 112 +++++++++++++++++++++++++++++++++++++++++-
>> arm/timer.c | 38 +++++++++-----
>> 23 files changed, 425 insertions(+), 64 deletions(-)
>> create mode 100644 lib/linux/compiler.h
>>
>> --
>> 2.7.4
>>
> Thanks Alexandru. I'm queuing everything except
>
> arm/arm64: psci: Don't run C code without stack or vectors
> arm/arm64: selftest: Add prefetch abort test
> arm64: timer: EOIR the interrupt after masking the timer
> arm64: timer: Test behavior when timer disabled or masked
> arm/arm64: Perform dcache clean + invalidate after turning MMU off
>
> as those had comments from Andre and myself to address.
I noticed that Paolo merged the pull request, I'll send the remaining patches as v4.
Thanks,
Alex
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 47+ messages in thread