* [kvm-unit-tests PATCH 0/2] arm64: Add support for configuring the translation granule
@ 2020-11-02 11:34 Nikos Nikoleris
2020-11-02 11:34 ` [kvm-unit-tests PATCH 1/2] " Nikos Nikoleris
2020-11-02 11:34 ` [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported Nikos Nikoleris
0 siblings, 2 replies; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-02 11:34 UTC (permalink / raw)
To: kvm
Cc: mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei, drjones
Hi all,
This is an update to the patch that allows a user to change the
translation granule in arm64. Special thanks to Drew and Alex for
having a look at the code and their suggestions.
v1: https://lore.kernel.org/kvm/006a19c0-cdf7-e76c-8335-03034bea9c7e@arm.com/T
Changes in v2:
- Change the configure option from page-shift to page-size
- Check and warn if the configured granule is not supported
Thanks,
Nikos
Nikos Nikoleris (2):
arm64: Add support for configuring the translation granule
arm64: Check if the configured translation granule is supported
configure | 26 +++++++++++++
lib/arm/asm/page.h | 4 ++
lib/arm/asm/pgtable-hwdef.h | 4 ++
lib/arm/asm/pgtable.h | 6 +++
lib/arm/asm/thread_info.h | 4 +-
lib/arm64/asm/page.h | 25 ++++++++++---
lib/arm64/asm/pgtable-hwdef.h | 38 +++++++++++++------
lib/arm64/asm/pgtable.h | 69 +++++++++++++++++++++++++++++++++--
lib/arm64/asm/processor.h | 65 +++++++++++++++++++++++++++++++++
lib/arm/mmu.c | 29 ++++++++++-----
arm/cstart64.S | 10 ++++-
11 files changed, 248 insertions(+), 32 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-02 11:34 [kvm-unit-tests PATCH 0/2] arm64: Add support for configuring the translation granule Nikos Nikoleris
@ 2020-11-02 11:34 ` Nikos Nikoleris
2020-11-03 13:04 ` Andrew Jones
2020-11-03 16:21 ` Alexandru Elisei
2020-11-02 11:34 ` [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported Nikos Nikoleris
1 sibling, 2 replies; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-02 11:34 UTC (permalink / raw)
To: kvm
Cc: mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei, drjones
Make the translation granule configurable for arm64. arm64 supports
page sizes of 4K, 16K and 64K. By default, arm64 is configured with
64K pages. configure has been extended with a new argument:
--page-shift=(12|14|16)
which allows the user to set the page shift and therefore the page
size for arm64. Using the --page-shift for any other architecture
results an error message.
To allow for smaller page sizes and 42b VA, this change adds support
for 4-level and 3-level page tables. At compile time, we determine how
many levels in the page tables we needed.
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
configure | 26 +++++++++++++
lib/arm/asm/page.h | 4 ++
lib/arm/asm/pgtable-hwdef.h | 4 ++
lib/arm/asm/pgtable.h | 6 +++
lib/arm/asm/thread_info.h | 4 +-
lib/arm64/asm/page.h | 25 ++++++++++---
lib/arm64/asm/pgtable-hwdef.h | 38 +++++++++++++------
lib/arm64/asm/pgtable.h | 69 +++++++++++++++++++++++++++++++++--
lib/arm/mmu.c | 26 ++++++++-----
arm/cstart64.S | 10 ++++-
10 files changed, 180 insertions(+), 32 deletions(-)
diff --git a/configure b/configure
index 706aab5..9c6bb2c 100755
--- a/configure
+++ b/configure
@@ -25,6 +25,7 @@ vmm="qemu"
errata_force=0
erratatxt="$srcdir/errata.txt"
host_key_document=
+page_size=
usage() {
cat <<-EOF
@@ -50,6 +51,8 @@ usage() {
--host-key-document=HOST_KEY_DOCUMENT
Specify the machine-specific host-key document for creating
a PVM image with 'genprotimg' (s390x only)
+ --page-size=PAGE_SIZE
+ Specify the page size (translation granule) (arm64 only)
EOF
exit 1
}
@@ -105,6 +108,9 @@ while [[ "$1" = -* ]]; do
--host-key-document)
host_key_document="$arg"
;;
+ --page-size)
+ page_size="$arg"
+ ;;
--help)
usage
;;
@@ -123,6 +129,25 @@ arch_name=$arch
[ "$arch" = "aarch64" ] && arch="arm64"
[ "$arch_name" = "arm64" ] && arch_name="aarch64"
+if [ -z "$page_size" ]; then
+ [ "$arch" = "arm64" ] && page_size="65536"
+ [ "$arch" = "arm" ] && page_size="4096"
+else
+ if [ "$arch" != "arm64" ]; then
+ echo "--page-size is not supported for $arch"
+ usage
+ fi
+
+ if [ "${page_size: -1}" = "K" ] || [ "${page_size: -1}" = "k" ]; then
+ page_size=$[ ${page_size%?} * 1024 ]
+ fi
+ if [ "$page_size" != "4096" ] && [ "$page_size" != "16384" ] &&
+ [ "$page_size" != "65536" ]; then
+ echo "arm64 doesn't support page size of $page_size"
+ usage
+ fi
+fi
+
[ -z "$processor" ] && processor="$arch"
if [ "$processor" = "arm64" ]; then
@@ -254,6 +279,7 @@ cat <<EOF >> lib/config.h
#define CONFIG_UART_EARLY_BASE ${arm_uart_early_addr}
#define CONFIG_ERRATA_FORCE ${errata_force}
+#define CONFIG_PAGE_SIZE ${page_size}
EOF
fi
diff --git a/lib/arm/asm/page.h b/lib/arm/asm/page.h
index 039c9f7..ae0ac2c 100644
--- a/lib/arm/asm/page.h
+++ b/lib/arm/asm/page.h
@@ -29,6 +29,10 @@ typedef struct { pteval_t pgprot; } pgprot_t;
#define pgd_val(x) ((x).pgd)
#define pgprot_val(x) ((x).pgprot)
+/* For compatibility with arm64 page tables */
+#define pud_t pgd_t
+#define pud_val(x) pgd_val(x)
+
#define __pte(x) ((pte_t) { (x) } )
#define __pmd(x) ((pmd_t) { (x) } )
#define __pgd(x) ((pgd_t) { (x) } )
diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
index 4107e18..fe1d854 100644
--- a/lib/arm/asm/pgtable-hwdef.h
+++ b/lib/arm/asm/pgtable-hwdef.h
@@ -19,6 +19,10 @@
#define PTRS_PER_PTE 512
#define PTRS_PER_PMD 512
+/* For compatibility with arm64 page tables */
+#define PUD_SIZE PGDIR_SIZE
+#define PUD_MASK PGDIR_MASK
+
#define PMD_SHIFT 21
#define PMD_SIZE (_AC(1,UL) << PMD_SHIFT)
#define PMD_MASK (~((1 << PMD_SHIFT) - 1))
diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
index 078dd16..4759d82 100644
--- a/lib/arm/asm/pgtable.h
+++ b/lib/arm/asm/pgtable.h
@@ -53,6 +53,12 @@ static inline pmd_t *pgd_page_vaddr(pgd_t pgd)
return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
}
+/* For compatibility with arm64 page tables */
+#define pud_valid(pud) pgd_valid(pud)
+#define pud_offset(pgd, addr) ((pud_t *)pgd)
+#define pud_free(pud)
+#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
+
#define pmd_index(addr) \
(((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
#define pmd_offset(pgd, addr) \
diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
index 80ab395..eaa7258 100644
--- a/lib/arm/asm/thread_info.h
+++ b/lib/arm/asm/thread_info.h
@@ -14,10 +14,12 @@
#define THREAD_SHIFT PAGE_SHIFT
#define THREAD_SIZE PAGE_SIZE
#define THREAD_MASK PAGE_MASK
+#define THREAD_ALIGNMENT PAGE_SIZE
#else
#define THREAD_SHIFT MIN_THREAD_SHIFT
#define THREAD_SIZE (_AC(1,UL) << THREAD_SHIFT)
#define THREAD_MASK (~(THREAD_SIZE-1))
+#define THREAD_ALIGNMENT THREAD_SIZE
#endif
#ifndef __ASSEMBLY__
@@ -38,7 +40,7 @@
static inline void *thread_stack_alloc(void)
{
- void *sp = memalign(PAGE_SIZE, THREAD_SIZE);
+ void *sp = memalign(THREAD_ALIGNMENT, THREAD_SIZE);
return sp + THREAD_START_SP;
}
diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
index 46af552..2a06207 100644
--- a/lib/arm64/asm/page.h
+++ b/lib/arm64/asm/page.h
@@ -10,38 +10,51 @@
* This work is licensed under the terms of the GNU GPL, version 2.
*/
+#include <config.h>
#include <linux/const.h>
-#define PGTABLE_LEVELS 2
#define VA_BITS 42
+#define PAGE_SIZE CONFIG_PAGE_SIZE
+#if PAGE_SIZE == 65536
#define PAGE_SHIFT 16
-#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
+#elif PAGE_SIZE == 16384
+#define PAGE_SHIFT 14
+#elif PAGE_SIZE == 4096
+#define PAGE_SHIFT 12
+#else
+#error Unsupported PAGE_SIZE
+#endif
#define PAGE_MASK (~(PAGE_SIZE-1))
+#define PGTABLE_LEVELS (((VA_BITS) - 4) / (PAGE_SHIFT - 3))
+
#ifndef __ASSEMBLY__
#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
typedef u64 pteval_t;
typedef u64 pmdval_t;
+typedef u64 pudval_t;
typedef u64 pgdval_t;
typedef struct { pteval_t pte; } pte_t;
+typedef struct { pmdval_t pmd; } pmd_t;
+typedef struct { pudval_t pud; } pud_t;
typedef struct { pgdval_t pgd; } pgd_t;
typedef struct { pteval_t pgprot; } pgprot_t;
#define pte_val(x) ((x).pte)
+#define pmd_val(x) ((x).pmd)
+#define pud_val(x) ((x).pud)
#define pgd_val(x) ((x).pgd)
#define pgprot_val(x) ((x).pgprot)
#define __pte(x) ((pte_t) { (x) } )
+#define __pmd(x) ((pmd_t) { (x) } )
+#define __pud(x) ((pud_t) { (x) } )
#define __pgd(x) ((pgd_t) { (x) } )
#define __pgprot(x) ((pgprot_t) { (x) } )
-typedef struct { pgd_t pgd; } pmd_t;
-#define pmd_val(x) (pgd_val((x).pgd))
-#define __pmd(x) ((pmd_t) { __pgd(x) } )
-
#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
#define __pa(x) __virt_to_phys((unsigned long)(x))
diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
index 3352489..3b6b0d6 100644
--- a/lib/arm64/asm/pgtable-hwdef.h
+++ b/lib/arm64/asm/pgtable-hwdef.h
@@ -9,38 +9,54 @@
* This work is licensed under the terms of the GNU GPL, version 2.
*/
+#include <asm/page.h>
+
#define UL(x) _AC(x, UL)
+#define PGTABLE_LEVEL_SHIFT(n) ((PAGE_SHIFT - 3) * (4 - (n)) + 3)
#define PTRS_PER_PTE (1 << (PAGE_SHIFT - 3))
+#if PGTABLE_LEVELS > 2
+#define PMD_SHIFT PGTABLE_LEVEL_SHIFT(2)
+#define PTRS_PER_PMD PTRS_PER_PTE
+#define PMD_SIZE (UL(1) << PMD_SHIFT)
+#define PMD_MASK (~(PMD_SIZE-1))
+#endif
+
+#if PGTABLE_LEVELS > 3
+#define PUD_SHIFT PGTABLE_LEVEL_SHIFT(1)
+#define PTRS_PER_PUD PTRS_PER_PTE
+#define PUD_SIZE (UL(1) << PUD_SHIFT)
+#define PUD_MASK (~(PUD_SIZE-1))
+#else
+#define PUD_SIZE PGDIR_SIZE
+#define PUD_MASK PGDIR_MASK
+#endif
+
+#define PUD_VALID (_AT(pudval_t, 1) << 0)
+
/*
* PGDIR_SHIFT determines the size a top-level page table entry can map
* (depending on the configuration, this level can be 0, 1 or 2).
*/
-#define PGDIR_SHIFT ((PAGE_SHIFT - 3) * PGTABLE_LEVELS + 3)
+#define PGDIR_SHIFT PGTABLE_LEVEL_SHIFT(4 - PGTABLE_LEVELS)
#define PGDIR_SIZE (_AC(1, UL) << PGDIR_SHIFT)
#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
-#define PMD_SIZE (UL(1) << PMD_SHIFT)
-#define PMD_MASK (~(PMD_SIZE-1))
-
/*
* Section address mask and size definitions.
*/
-#define SECTION_SHIFT PMD_SHIFT
-#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
-#define SECTION_MASK (~(SECTION_SIZE-1))
+#define SECTION_SHIFT PMD_SHIFT
+#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
+#define SECTION_MASK (~(SECTION_SIZE-1))
/*
* Hardware page table definitions.
*
- * Level 1 descriptor (PMD).
+ * Level 0,1,2 descriptor (PGD, PUD and PMD).
*/
#define PMD_TYPE_MASK (_AT(pmdval_t, 3) << 0)
#define PMD_TYPE_FAULT (_AT(pmdval_t, 0) << 0)
diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
index e577d9c..c7632ae 100644
--- a/lib/arm64/asm/pgtable.h
+++ b/lib/arm64/asm/pgtable.h
@@ -30,10 +30,12 @@
#define pgtable_pa(x) ((unsigned long)(x))
#define pgd_none(pgd) (!pgd_val(pgd))
+#define pud_none(pud) (!pud_val(pud))
#define pmd_none(pmd) (!pmd_val(pmd))
#define pte_none(pte) (!pte_val(pte))
#define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
+#define pud_valid(pud) (pud_val(pud) & PUD_VALID)
#define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
#define pte_valid(pte) (pte_val(pte) & PTE_VALID)
@@ -52,15 +54,76 @@ static inline pgd_t *pgd_alloc(void)
return pgd;
}
-#define pmd_offset(pgd, addr) ((pmd_t *)pgd)
-#define pmd_free(pmd)
-#define pmd_alloc(pgd, addr) pmd_offset(pgd, addr)
+static inline pud_t *pgd_page_vaddr(pgd_t pgd)
+{
+ return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
+}
+
+static inline pmd_t *pud_page_vaddr(pud_t pud)
+{
+ return pgtable_va(pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK);
+}
+
static inline pte_t *pmd_page_vaddr(pmd_t pmd)
{
return pgtable_va(pmd_val(pmd) & PHYS_MASK & (s32)PAGE_MASK);
}
+#if PGTABLE_LEVELS > 2
+#define pmd_index(addr) \
+ (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
+#define pmd_offset(pud, addr) \
+ (pud_page_vaddr(*(pud)) + pmd_index(addr))
+#define pmd_free(pmd) free_page(pmd)
+static inline pmd_t *pmd_alloc_one(void)
+{
+ assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
+ pmd_t *pmd = alloc_page();
+ return pmd;
+}
+static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
+{
+ if (pud_none(*pud)) {
+ pud_t entry;
+ pud_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
+ WRITE_ONCE(*pud, entry);
+ }
+ return pmd_offset(pud, addr);
+}
+#else
+#define pmd_offset(pud, addr) ((pmd_t *)pud)
+#define pmd_free(pmd)
+#define pmd_alloc(pud, addr) pmd_offset(pud, addr)
+#endif
+
+#if PGTABLE_LEVELS > 3
+#define pud_index(addr) \
+ (((addr) >> PUD_SHIFT) & (PTRS_PER_PUD - 1))
+#define pud_offset(pgd, addr) \
+ (pgd_page_vaddr(*(pgd)) + pud_index(addr))
+#define pud_free(pud) free_page(pud)
+static inline pud_t *pud_alloc_one(void)
+{
+ assert(PTRS_PER_PMD * sizeof(pud_t) == PAGE_SIZE);
+ pud_t *pud = alloc_page();
+ return pud;
+}
+static inline pud_t *pud_alloc(pgd_t *pgd, unsigned long addr)
+{
+ if (pgd_none(*pgd)) {
+ pgd_t entry;
+ pgd_val(entry) = pgtable_pa(pud_alloc_one()) | PMD_TYPE_TABLE;
+ WRITE_ONCE(*pgd, entry);
+ }
+ return pud_offset(pgd, addr);
+}
+#else
+#define pud_offset(pgd, addr) ((pud_t *)pgd)
+#define pud_free(pud)
+#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
+#endif
+
#define pte_index(addr) \
(((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
#define pte_offset(pmd, addr) \
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 540a1e8..6d1c75b 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -81,7 +81,8 @@ void mmu_disable(void)
static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
{
pgd_t *pgd = pgd_offset(pgtable, vaddr);
- pmd_t *pmd = pmd_alloc(pgd, vaddr);
+ pud_t *pud = pud_alloc(pgd, vaddr);
+ pmd_t *pmd = pmd_alloc(pud, vaddr);
pte_t *pte = pte_alloc(pmd, vaddr);
return &pte_val(*pte);
@@ -133,18 +134,20 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
phys_addr_t phys_start, phys_addr_t phys_end,
pgprot_t prot)
{
- phys_addr_t paddr = phys_start & PGDIR_MASK;
- uintptr_t vaddr = virt_offset & PGDIR_MASK;
+ phys_addr_t paddr = phys_start & PUD_MASK;
+ uintptr_t vaddr = virt_offset & PUD_MASK;
uintptr_t virt_end = phys_end - paddr + vaddr;
pgd_t *pgd;
- pgd_t entry;
+ pud_t *pud;
+ pud_t entry;
- for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
- pgd_val(entry) = paddr;
- pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
- pgd_val(entry) |= pgprot_val(prot);
+ for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
+ pud_val(entry) = paddr;
+ pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
+ pud_val(entry) |= pgprot_val(prot);
pgd = pgd_offset(pgtable, vaddr);
- WRITE_ONCE(*pgd, entry);
+ pud = pud_alloc(pgd, vaddr);
+ WRITE_ONCE(*pud, entry);
flush_tlb_page(vaddr);
}
}
@@ -207,6 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
{
pgd_t *pgd;
+ pud_t *pud;
pmd_t *pmd;
pte_t *pte;
@@ -215,7 +219,9 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
pgd = pgd_offset(pgtable, vaddr);
assert(pgd_valid(*pgd));
- pmd = pmd_offset(pgd, vaddr);
+ pud = pud_offset(pgd, vaddr);
+ assert(pud_valid(*pud));
+ pmd = pmd_offset(pud, vaddr);
assert(pmd_valid(*pmd));
if (pmd_huge(*pmd)) {
diff --git a/arm/cstart64.S b/arm/cstart64.S
index ffdd49f..cedc678 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -157,6 +157,14 @@ halt:
*/
#define MAIR(attr, mt) ((attr) << ((mt) * 8))
+#if PAGE_SIZE == 65536
+#define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
+#elif PAGE_SIZE == 16384
+#define TCR_TG_FLAGS TCR_TG0_16K | TCR_TG1_16K
+#elif PAGE_SIZE == 4096
+#define TCR_TG_FLAGS TCR_TG0_4K | TCR_TG1_4K
+#endif
+
.globl asm_mmu_enable
asm_mmu_enable:
tlbi vmalle1 // invalidate I + D TLBs
@@ -164,7 +172,7 @@ asm_mmu_enable:
/* TCR */
ldr x1, =TCR_TxSZ(VA_BITS) | \
- TCR_TG0_64K | TCR_TG1_64K | \
+ TCR_TG_FLAGS | \
TCR_IRGN_WBWA | TCR_ORGN_WBWA | \
TCR_SHARED
mrs x2, id_aa64mmfr0_el1
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-02 11:34 [kvm-unit-tests PATCH 0/2] arm64: Add support for configuring the translation granule Nikos Nikoleris
2020-11-02 11:34 ` [kvm-unit-tests PATCH 1/2] " Nikos Nikoleris
@ 2020-11-02 11:34 ` Nikos Nikoleris
2020-11-03 10:02 ` Andrew Jones
1 sibling, 1 reply; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-02 11:34 UTC (permalink / raw)
To: kvm
Cc: mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei, drjones
Now that we can change the translation granule at will, and since
arm64 implementations can support a subset of the architecturally
defined granules, we need to check and warn the user if the configured
granule is not supported.
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
lib/arm64/asm/processor.h | 65 +++++++++++++++++++++++++++++++++++++++
lib/arm/mmu.c | 3 ++
2 files changed, 68 insertions(+)
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 02665b8..0eac928 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -117,5 +117,70 @@ static inline u64 get_ctr(void)
extern u32 dcache_line_size;
+static inline unsigned long get_id_aa64mmfr0_el1(void)
+{
+ unsigned long mmfr0;
+ asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
+ return mmfr0;
+}
+
+/* From arch/arm64/include/asm/cpufeature.h */
+static inline unsigned int
+cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
+{
+ return (u64)(features << (64 - width - field)) >> (64 - width);
+}
+
+#define ID_AA64MMFR0_TGRAN4_SHIFT 28
+#define ID_AA64MMFR0_TGRAN64_SHIFT 24
+#define ID_AA64MMFR0_TGRAN16_SHIFT 20
+#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
+#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
+#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
+
+static inline bool system_supports_64kb_granule(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = get_id_aa64mmfr0_el1();
+ val = cpuid_feature_extract_unsigned_field_width(
+ mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT,4);
+
+ return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
+}
+
+static inline bool system_supports_16kb_granule(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = get_id_aa64mmfr0_el1();
+ val = cpuid_feature_extract_unsigned_field_width(
+ mmfr0, ID_AA64MMFR0_TGRAN16_SHIFT, 4);
+
+ return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
+}
+
+static inline bool system_supports_4kb_granule(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = get_id_aa64mmfr0_el1();
+ val = cpuid_feature_extract_unsigned_field_width(
+ mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT, 4);
+
+ return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
+}
+
+#if PAGE_SIZE == 65536
+#define system_supports_configured_granule system_supports_64kb_granule
+#elif PAGE_SIZE == 16384
+#define system_supports_configured_granule system_supports_16kb_granule
+#elif PAGE_SIZE == 4096
+#define system_supports_configured_granule system_supports_4kb_granule
+#endif
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASMARM64_PROCESSOR_H_ */
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 6d1c75b..51fa745 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -163,6 +163,9 @@ void *setup_mmu(phys_addr_t phys_end)
#ifdef __aarch64__
init_alloc_vpage((void*)(4ul << 30));
+
+ assert_msg(system_supports_configured_granule(),
+ "Unsupported translation granule %d\n", PAGE_SIZE);
#endif
mmu_idmap = alloc_page();
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-02 11:34 ` [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported Nikos Nikoleris
@ 2020-11-03 10:02 ` Andrew Jones
2020-11-03 10:21 ` Nikos Nikoleris
2020-11-03 17:03 ` Alexandru Elisei
0 siblings, 2 replies; 17+ messages in thread
From: Andrew Jones @ 2020-11-03 10:02 UTC (permalink / raw)
To: Nikos Nikoleris
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei
On Mon, Nov 02, 2020 at 11:34:44AM +0000, Nikos Nikoleris wrote:
> Now that we can change the translation granule at will, and since
> arm64 implementations can support a subset of the architecturally
> defined granules, we need to check and warn the user if the configured
> granule is not supported.
nit: it'd be better for this patch to come before the last patch.
>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
> lib/arm64/asm/processor.h | 65 +++++++++++++++++++++++++++++++++++++++
> lib/arm/mmu.c | 3 ++
> 2 files changed, 68 insertions(+)
>
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 02665b8..0eac928 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -117,5 +117,70 @@ static inline u64 get_ctr(void)
>
> extern u32 dcache_line_size;
>
> +static inline unsigned long get_id_aa64mmfr0_el1(void)
> +{
> + unsigned long mmfr0;
> + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
> + return mmfr0;
> +}
> +
> +/* From arch/arm64/include/asm/cpufeature.h */
> +static inline unsigned int
> +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
> +{
> + return (u64)(features << (64 - width - field)) >> (64 - width);
> +}
> +
> +#define ID_AA64MMFR0_TGRAN4_SHIFT 28
> +#define ID_AA64MMFR0_TGRAN64_SHIFT 24
> +#define ID_AA64MMFR0_TGRAN16_SHIFT 20
> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
> +
> +static inline bool system_supports_64kb_granule(void)
> +{
> + u64 mmfr0;
> + u32 val;
> +
> + mmfr0 = get_id_aa64mmfr0_el1();
> + val = cpuid_feature_extract_unsigned_field_width(
> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT,4);
> +
> + return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
> +}
> +
> +static inline bool system_supports_16kb_granule(void)
> +{
> + u64 mmfr0;
> + u32 val;
> +
> + mmfr0 = get_id_aa64mmfr0_el1();
> + val = cpuid_feature_extract_unsigned_field_width(
> + mmfr0, ID_AA64MMFR0_TGRAN16_SHIFT, 4);
> +
> + return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
> +}
> +
> +static inline bool system_supports_4kb_granule(void)
> +{
> + u64 mmfr0;
> + u32 val;
> +
> + mmfr0 = get_id_aa64mmfr0_el1();
> + val = cpuid_feature_extract_unsigned_field_width(
> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT, 4);
> +
> + return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
> +}
> +
> +#if PAGE_SIZE == 65536
> +#define system_supports_configured_granule system_supports_64kb_granule
> +#elif PAGE_SIZE == 16384
> +#define system_supports_configured_granule system_supports_16kb_granule
> +#elif PAGE_SIZE == 4096
> +#define system_supports_configured_granule system_supports_4kb_granule
> +#endif
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASMARM64_PROCESSOR_H_ */
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 6d1c75b..51fa745 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -163,6 +163,9 @@ void *setup_mmu(phys_addr_t phys_end)
>
> #ifdef __aarch64__
> init_alloc_vpage((void*)(4ul << 30));
> +
> + assert_msg(system_supports_configured_granule(),
> + "Unsupported translation granule %d\n", PAGE_SIZE);
^
needs '%ld' to compile
> #endif
>
> mmu_idmap = alloc_page();
> --
> 2.17.1
>
I don't think we need the three separate functions. How about just
doing the following diff?
Thanks,
drew
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 540a1e842d5b..fef62f5a9866 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -160,6 +160,9 @@ void *setup_mmu(phys_addr_t phys_end)
#ifdef __aarch64__
init_alloc_vpage((void*)(4ul << 30));
+
+ assert_msg(system_supports_granule(PAGE_SIZE),
+ "Unsupported translation granule: %ld\n", PAGE_SIZE);
#endif
mmu_idmap = alloc_page();
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 02665b84cc7e..dc493d1686bc 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -117,5 +117,21 @@ static inline u64 get_ctr(void)
extern u32 dcache_line_size;
+static inline unsigned long get_id_aa64mmfr0_el1(void)
+{
+ unsigned long mmfr0;
+ asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
+ return mmfr0;
+}
+
+static inline bool system_supports_granule(size_t granule)
+{
+ u64 mmfr0 = get_id_aa64mmfr0_el1();
+
+ return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
+ (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
+ (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
+}
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASMARM64_PROCESSOR_H_ */
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-03 10:02 ` Andrew Jones
@ 2020-11-03 10:21 ` Nikos Nikoleris
2020-11-03 17:03 ` Alexandru Elisei
1 sibling, 0 replies; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-03 10:21 UTC (permalink / raw)
To: Andrew Jones
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei
Hi Drew,
On 03/11/2020 10:02, Andrew Jones wrote:
> On Mon, Nov 02, 2020 at 11:34:44AM +0000, Nikos Nikoleris wrote:
>> Now that we can change the translation granule at will, and since
>> arm64 implementations can support a subset of the architecturally
>> defined granules, we need to check and warn the user if the configured
>> granule is not supported.
>
> nit: it'd be better for this patch to come before the last patch.
>
Ack, I will re-order them.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>> lib/arm64/asm/processor.h | 65 +++++++++++++++++++++++++++++++++++++++
>> lib/arm/mmu.c | 3 ++
>> 2 files changed, 68 insertions(+)
>>
>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>> index 02665b8..0eac928 100644
>> --- a/lib/arm64/asm/processor.h
>> +++ b/lib/arm64/asm/processor.h
>> @@ -117,5 +117,70 @@ static inline u64 get_ctr(void)
>>
>> extern u32 dcache_line_size;
>>
>> +static inline unsigned long get_id_aa64mmfr0_el1(void)
>> +{
>> + unsigned long mmfr0;
>> + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
>> + return mmfr0;
>> +}
>> +
>> +/* From arch/arm64/include/asm/cpufeature.h */
>> +static inline unsigned int
>> +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
>> +{
>> + return (u64)(features << (64 - width - field)) >> (64 - width);
>> +}
>> +
>> +#define ID_AA64MMFR0_TGRAN4_SHIFT 28
>> +#define ID_AA64MMFR0_TGRAN64_SHIFT 24
>> +#define ID_AA64MMFR0_TGRAN16_SHIFT 20
>> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
>> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
>> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
>> +
>> +static inline bool system_supports_64kb_granule(void)
>> +{
>> + u64 mmfr0;
>> + u32 val;
>> +
>> + mmfr0 = get_id_aa64mmfr0_el1();
>> + val = cpuid_feature_extract_unsigned_field_width(
>> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT,4);
>> +
>> + return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
>> +}
>> +
>> +static inline bool system_supports_16kb_granule(void)
>> +{
>> + u64 mmfr0;
>> + u32 val;
>> +
>> + mmfr0 = get_id_aa64mmfr0_el1();
>> + val = cpuid_feature_extract_unsigned_field_width(
>> + mmfr0, ID_AA64MMFR0_TGRAN16_SHIFT, 4);
>> +
>> + return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
>> +}
>> +
>> +static inline bool system_supports_4kb_granule(void)
>> +{
>> + u64 mmfr0;
>> + u32 val;
>> +
>> + mmfr0 = get_id_aa64mmfr0_el1();
>> + val = cpuid_feature_extract_unsigned_field_width(
>> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT, 4);
>> +
>> + return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
>> +}
>> +
>> +#if PAGE_SIZE == 65536
>> +#define system_supports_configured_granule system_supports_64kb_granule
>> +#elif PAGE_SIZE == 16384
>> +#define system_supports_configured_granule system_supports_16kb_granule
>> +#elif PAGE_SIZE == 4096
>> +#define system_supports_configured_granule system_supports_4kb_granule
>> +#endif
>> +
>> #endif /* !__ASSEMBLY__ */
>> #endif /* _ASMARM64_PROCESSOR_H_ */
>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>> index 6d1c75b..51fa745 100644
>> --- a/lib/arm/mmu.c
>> +++ b/lib/arm/mmu.c
>> @@ -163,6 +163,9 @@ void *setup_mmu(phys_addr_t phys_end)
>>
>> #ifdef __aarch64__
>> init_alloc_vpage((void*)(4ul << 30));
>> +
>> + assert_msg(system_supports_configured_granule(),
>> + "Unsupported translation granule %d\n", PAGE_SIZE);
> ^
> needs '%ld' to compile
>> #endif
>>
>> mmu_idmap = alloc_page();
>> --
>> 2.17.1
>>
>
> I don't think we need the three separate functions. How about just
> doing the following diff?
>
Makes sense, I was looking at how we do it in the kernel and got carried
away. We don't need to do that much at compile time.
Thanks for the review, I will included your suggestions in v3.
Thanks,
Nikos
> Thanks,
> drew
>
>
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 540a1e842d5b..fef62f5a9866 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -160,6 +160,9 @@ void *setup_mmu(phys_addr_t phys_end)
>
> #ifdef __aarch64__
> init_alloc_vpage((void*)(4ul << 30));
> +
> + assert_msg(system_supports_granule(PAGE_SIZE),
> + "Unsupported translation granule: %ld\n", PAGE_SIZE);
> #endif
>
> mmu_idmap = alloc_page();
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 02665b84cc7e..dc493d1686bc 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -117,5 +117,21 @@ static inline u64 get_ctr(void)
>
> extern u32 dcache_line_size;
>
> +static inline unsigned long get_id_aa64mmfr0_el1(void)
> +{
> + unsigned long mmfr0;
> + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
> + return mmfr0;
> +}
> +
> +static inline bool system_supports_granule(size_t granule)
> +{
> + u64 mmfr0 = get_id_aa64mmfr0_el1();
> +
> + return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
> + (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
> + (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
> +}
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASMARM64_PROCESSOR_H_ */
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-02 11:34 ` [kvm-unit-tests PATCH 1/2] " Nikos Nikoleris
@ 2020-11-03 13:04 ` Andrew Jones
2020-11-03 15:49 ` Nikos Nikoleris
2020-11-03 16:21 ` Alexandru Elisei
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Jones @ 2020-11-03 13:04 UTC (permalink / raw)
To: Nikos Nikoleris
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei
On Mon, Nov 02, 2020 at 11:34:43AM +0000, Nikos Nikoleris wrote:
> Make the translation granule configurable for arm64. arm64 supports
> page sizes of 4K, 16K and 64K. By default, arm64 is configured with
> 64K pages. configure has been extended with a new argument:
>
> --page-shift=(12|14|16)
--page-size=PAGE_SIZE
>
> which allows the user to set the page shift and therefore the page
> size for arm64.
which allows the user to set the page size for arm64.
> Using the --page-shift for any other architecture
--page-size
> results an error message.
>
> To allow for smaller page sizes and 42b VA, this change adds support
> for 4-level and 3-level page tables. At compile time, we determine how
> many levels in the page tables we needed.
>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
> configure | 26 +++++++++++++
> lib/arm/asm/page.h | 4 ++
> lib/arm/asm/pgtable-hwdef.h | 4 ++
> lib/arm/asm/pgtable.h | 6 +++
> lib/arm/asm/thread_info.h | 4 +-
> lib/arm64/asm/page.h | 25 ++++++++++---
> lib/arm64/asm/pgtable-hwdef.h | 38 +++++++++++++------
> lib/arm64/asm/pgtable.h | 69 +++++++++++++++++++++++++++++++++--
> lib/arm/mmu.c | 26 ++++++++-----
> arm/cstart64.S | 10 ++++-
> 10 files changed, 180 insertions(+), 32 deletions(-)
>
> diff --git a/configure b/configure
> index 706aab5..9c6bb2c 100755
> --- a/configure
> +++ b/configure
> @@ -25,6 +25,7 @@ vmm="qemu"
> errata_force=0
> erratatxt="$srcdir/errata.txt"
> host_key_document=
> +page_size=
>
> usage() {
> cat <<-EOF
> @@ -50,6 +51,8 @@ usage() {
> --host-key-document=HOST_KEY_DOCUMENT
> Specify the machine-specific host-key document for creating
> a PVM image with 'genprotimg' (s390x only)
> + --page-size=PAGE_SIZE
> + Specify the page size (translation granule) (arm64 only)
> EOF
> exit 1
> }
> @@ -105,6 +108,9 @@ while [[ "$1" = -* ]]; do
> --host-key-document)
> host_key_document="$arg"
> ;;
> + --page-size)
> + page_size="$arg"
> + ;;
> --help)
> usage
> ;;
> @@ -123,6 +129,25 @@ arch_name=$arch
> [ "$arch" = "aarch64" ] && arch="arm64"
> [ "$arch_name" = "arm64" ] && arch_name="aarch64"
>
> +if [ -z "$page_size" ]; then
> + [ "$arch" = "arm64" ] && page_size="65536"
> + [ "$arch" = "arm" ] && page_size="4096"
> +else
> + if [ "$arch" != "arm64" ]; then
> + echo "--page-size is not supported for $arch"
> + usage
> + fi
> +
> + if [ "${page_size: -1}" = "K" ] || [ "${page_size: -1}" = "k" ]; then
> + page_size=$[ ${page_size%?} * 1024 ]
$[...] is legacy syntax. Please use $((...))
> + fi
> + if [ "$page_size" != "4096" ] && [ "$page_size" != "16384" ] &&
> + [ "$page_size" != "65536" ]; then
> + echo "arm64 doesn't support page size of $page_size"
> + usage
> + fi
> +fi
> +
> [ -z "$processor" ] && processor="$arch"
>
> if [ "$processor" = "arm64" ]; then
> @@ -254,6 +279,7 @@ cat <<EOF >> lib/config.h
>
> #define CONFIG_UART_EARLY_BASE ${arm_uart_early_addr}
> #define CONFIG_ERRATA_FORCE ${errata_force}
> +#define CONFIG_PAGE_SIZE ${page_size}
>
> EOF
> fi
> diff --git a/lib/arm/asm/page.h b/lib/arm/asm/page.h
> index 039c9f7..ae0ac2c 100644
> --- a/lib/arm/asm/page.h
> +++ b/lib/arm/asm/page.h
> @@ -29,6 +29,10 @@ typedef struct { pteval_t pgprot; } pgprot_t;
> #define pgd_val(x) ((x).pgd)
> #define pgprot_val(x) ((x).pgprot)
>
> +/* For compatibility with arm64 page tables */
> +#define pud_t pgd_t
> +#define pud_val(x) pgd_val(x)
> +
> #define __pte(x) ((pte_t) { (x) } )
> #define __pmd(x) ((pmd_t) { (x) } )
> #define __pgd(x) ((pgd_t) { (x) } )
> diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
> index 4107e18..fe1d854 100644
> --- a/lib/arm/asm/pgtable-hwdef.h
> +++ b/lib/arm/asm/pgtable-hwdef.h
> @@ -19,6 +19,10 @@
> #define PTRS_PER_PTE 512
> #define PTRS_PER_PMD 512
>
> +/* For compatibility with arm64 page tables */
> +#define PUD_SIZE PGDIR_SIZE
> +#define PUD_MASK PGDIR_MASK
> +
> #define PMD_SHIFT 21
> #define PMD_SIZE (_AC(1,UL) << PMD_SHIFT)
> #define PMD_MASK (~((1 << PMD_SHIFT) - 1))
> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> index 078dd16..4759d82 100644
> --- a/lib/arm/asm/pgtable.h
> +++ b/lib/arm/asm/pgtable.h
> @@ -53,6 +53,12 @@ static inline pmd_t *pgd_page_vaddr(pgd_t pgd)
> return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
> }
>
> +/* For compatibility with arm64 page tables */
> +#define pud_valid(pud) pgd_valid(pud)
> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
> +#define pud_free(pud)
> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
Please watch the spaces vs. tabs. This comment applies to the
whole patch. There are many places.
> +
> #define pmd_index(addr) \
> (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
> #define pmd_offset(pgd, addr) \
> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
> index 80ab395..eaa7258 100644
> --- a/lib/arm/asm/thread_info.h
> +++ b/lib/arm/asm/thread_info.h
> @@ -14,10 +14,12 @@
> #define THREAD_SHIFT PAGE_SHIFT
> #define THREAD_SIZE PAGE_SIZE
> #define THREAD_MASK PAGE_MASK
> +#define THREAD_ALIGNMENT PAGE_SIZE
> #else
> #define THREAD_SHIFT MIN_THREAD_SHIFT
> #define THREAD_SIZE (_AC(1,UL) << THREAD_SHIFT)
> #define THREAD_MASK (~(THREAD_SIZE-1))
> +#define THREAD_ALIGNMENT THREAD_SIZE
> #endif
>
> #ifndef __ASSEMBLY__
> @@ -38,7 +40,7 @@
>
> static inline void *thread_stack_alloc(void)
> {
> - void *sp = memalign(PAGE_SIZE, THREAD_SIZE);
> + void *sp = memalign(THREAD_ALIGNMENT, THREAD_SIZE);
> return sp + THREAD_START_SP;
> }
>
> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
> index 46af552..2a06207 100644
> --- a/lib/arm64/asm/page.h
> +++ b/lib/arm64/asm/page.h
> @@ -10,38 +10,51 @@
> * This work is licensed under the terms of the GNU GPL, version 2.
> */
>
> +#include <config.h>
> #include <linux/const.h>
>
> -#define PGTABLE_LEVELS 2
> #define VA_BITS 42
Let's bump VA_BITS to 48 while we're at it.
>
> +#define PAGE_SIZE CONFIG_PAGE_SIZE
I see now how we had '%d' in the other patch for PAGE_SIZE
instead of %ld. To keep a UL like it is for arm and x86,
then we can add the UL to CONFIG_PAGE_SIZE in configure.
> +#if PAGE_SIZE == 65536
> #define PAGE_SHIFT 16
> -#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> +#elif PAGE_SIZE == 16384
> +#define PAGE_SHIFT 14
> +#elif PAGE_SIZE == 4096
> +#define PAGE_SHIFT 12
> +#else
> +#error Unsupported PAGE_SIZE
> +#endif
> #define PAGE_MASK (~(PAGE_SIZE-1))
>
> +#define PGTABLE_LEVELS (((VA_BITS) - 4) / (PAGE_SHIFT - 3))
Please replace the above define with
/*
* Since a page table descriptor is 8 bytes we have (PAGE_SHIFT - 3) bits
* of virtual address at each page table level. So, to get the number of
* page table levels needed, we round up the division of the number of
* address bits (VA_BITS - PAGE_SHIFT) by (PAGE_SHIFT - 3).
*/
#define PGTABLE_LEVELS \
(((VA_BITS - PAGE_SHIFT) + ((PAGE_SHIFT - 3) - 1)) / (PAGE_SHIFT - 3))
> +
> #ifndef __ASSEMBLY__
>
> #define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
>
> typedef u64 pteval_t;
> typedef u64 pmdval_t;
> +typedef u64 pudval_t;
> typedef u64 pgdval_t;
> typedef struct { pteval_t pte; } pte_t;
> +typedef struct { pmdval_t pmd; } pmd_t;
> +typedef struct { pudval_t pud; } pud_t;
> typedef struct { pgdval_t pgd; } pgd_t;
> typedef struct { pteval_t pgprot; } pgprot_t;
>
> #define pte_val(x) ((x).pte)
> +#define pmd_val(x) ((x).pmd)
> +#define pud_val(x) ((x).pud)
> #define pgd_val(x) ((x).pgd)
> #define pgprot_val(x) ((x).pgprot)
>
> #define __pte(x) ((pte_t) { (x) } )
> +#define __pmd(x) ((pmd_t) { (x) } )
> +#define __pud(x) ((pud_t) { (x) } )
> #define __pgd(x) ((pgd_t) { (x) } )
> #define __pgprot(x) ((pgprot_t) { (x) } )
>
> -typedef struct { pgd_t pgd; } pmd_t;
> -#define pmd_val(x) (pgd_val((x).pgd))
> -#define __pmd(x) ((pmd_t) { __pgd(x) } )
> -
> #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
> #define __pa(x) __virt_to_phys((unsigned long)(x))
>
> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
> index 3352489..3b6b0d6 100644
> --- a/lib/arm64/asm/pgtable-hwdef.h
> +++ b/lib/arm64/asm/pgtable-hwdef.h
> @@ -9,38 +9,54 @@
> * This work is licensed under the terms of the GNU GPL, version 2.
> */
>
> +#include <asm/page.h>
> +
> #define UL(x) _AC(x, UL)
>
> +#define PGTABLE_LEVEL_SHIFT(n) ((PAGE_SHIFT - 3) * (4 - (n)) + 3)
Please replace the above define with
/*
* The number of bits a given page table level, n (where n=0 is the top),
* maps is ((max_n - n) - 1) * nr_bits_per_level + PAGE_SHIFT. Since a page
* table descriptor is 8 bytes we have (PAGE_SHIFT - 3) bits per level. We
* also have a maximum of 4 page table levels. Hence,
*/
#define PGTABLE_LEVEL_SHIFT(n) \
(((4 - (n)) - 1) * (PAGE_SHIFT - 3) + PAGE_SHIFT)
> #define PTRS_PER_PTE (1 << (PAGE_SHIFT - 3))
>
> +#if PGTABLE_LEVELS > 2
> +#define PMD_SHIFT PGTABLE_LEVEL_SHIFT(2)
> +#define PTRS_PER_PMD PTRS_PER_PTE
> +#define PMD_SIZE (UL(1) << PMD_SHIFT)
> +#define PMD_MASK (~(PMD_SIZE-1))
> +#endif
> +
> +#if PGTABLE_LEVELS > 3
> +#define PUD_SHIFT PGTABLE_LEVEL_SHIFT(1)
> +#define PTRS_PER_PUD PTRS_PER_PTE
> +#define PUD_SIZE (UL(1) << PUD_SHIFT)
> +#define PUD_MASK (~(PUD_SIZE-1))
> +#else
> +#define PUD_SIZE PGDIR_SIZE
> +#define PUD_MASK PGDIR_MASK
> +#endif
> +
> +#define PUD_VALID (_AT(pudval_t, 1) << 0)
> +
> /*
> * PGDIR_SHIFT determines the size a top-level page table entry can map
> * (depending on the configuration, this level can be 0, 1 or 2).
> */
> -#define PGDIR_SHIFT ((PAGE_SHIFT - 3) * PGTABLE_LEVELS + 3)
> +#define PGDIR_SHIFT PGTABLE_LEVEL_SHIFT(4 - PGTABLE_LEVELS)
> #define PGDIR_SIZE (_AC(1, UL) << PGDIR_SHIFT)
> #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
> -#define PMD_SIZE (UL(1) << PMD_SHIFT)
> -#define PMD_MASK (~(PMD_SIZE-1))
> -
> /*
> * Section address mask and size definitions.
> */
> -#define SECTION_SHIFT PMD_SHIFT
> -#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
> -#define SECTION_MASK (~(SECTION_SIZE-1))
> +#define SECTION_SHIFT PMD_SHIFT
> +#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
> +#define SECTION_MASK (~(SECTION_SIZE-1))
>
> /*
> * Hardware page table definitions.
> *
> - * Level 1 descriptor (PMD).
> + * Level 0,1,2 descriptor (PGD, PUD and PMD).
> */
> #define PMD_TYPE_MASK (_AT(pmdval_t, 3) << 0)
> #define PMD_TYPE_FAULT (_AT(pmdval_t, 0) << 0)
> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> index e577d9c..c7632ae 100644
> --- a/lib/arm64/asm/pgtable.h
> +++ b/lib/arm64/asm/pgtable.h
> @@ -30,10 +30,12 @@
> #define pgtable_pa(x) ((unsigned long)(x))
>
> #define pgd_none(pgd) (!pgd_val(pgd))
> +#define pud_none(pud) (!pud_val(pud))
> #define pmd_none(pmd) (!pmd_val(pmd))
> #define pte_none(pte) (!pte_val(pte))
>
> #define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
> +#define pud_valid(pud) (pud_val(pud) & PUD_VALID)
> #define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
> #define pte_valid(pte) (pte_val(pte) & PTE_VALID)
>
> @@ -52,15 +54,76 @@ static inline pgd_t *pgd_alloc(void)
> return pgd;
> }
>
> -#define pmd_offset(pgd, addr) ((pmd_t *)pgd)
> -#define pmd_free(pmd)
> -#define pmd_alloc(pgd, addr) pmd_offset(pgd, addr)
> +static inline pud_t *pgd_page_vaddr(pgd_t pgd)
> +{
> + return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
> +}
> +
> +static inline pmd_t *pud_page_vaddr(pud_t pud)
> +{
> + return pgtable_va(pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK);
> +}
> +
>
> static inline pte_t *pmd_page_vaddr(pmd_t pmd)
> {
> return pgtable_va(pmd_val(pmd) & PHYS_MASK & (s32)PAGE_MASK);
> }
>
> +#if PGTABLE_LEVELS > 2
> +#define pmd_index(addr) \
> + (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
> +#define pmd_offset(pud, addr) \
> + (pud_page_vaddr(*(pud)) + pmd_index(addr))
> +#define pmd_free(pmd) free_page(pmd)
> +static inline pmd_t *pmd_alloc_one(void)
> +{
> + assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
> + pmd_t *pmd = alloc_page();
> + return pmd;
> +}
> +static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
> +{
> + if (pud_none(*pud)) {
> + pud_t entry;
> + pud_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pud, entry);
> + }
> + return pmd_offset(pud, addr);
> +}
> +#else
> +#define pmd_offset(pud, addr) ((pmd_t *)pud)
> +#define pmd_free(pmd)
> +#define pmd_alloc(pud, addr) pmd_offset(pud, addr)
> +#endif
> +
> +#if PGTABLE_LEVELS > 3
> +#define pud_index(addr) \
> + (((addr) >> PUD_SHIFT) & (PTRS_PER_PUD - 1))
> +#define pud_offset(pgd, addr) \
> + (pgd_page_vaddr(*(pgd)) + pud_index(addr))
> +#define pud_free(pud) free_page(pud)
> +static inline pud_t *pud_alloc_one(void)
> +{
> + assert(PTRS_PER_PMD * sizeof(pud_t) == PAGE_SIZE);
PTRS_PER_PUD
> + pud_t *pud = alloc_page();
> + return pud;
> +}
> +static inline pud_t *pud_alloc(pgd_t *pgd, unsigned long addr)
> +{
> + if (pgd_none(*pgd)) {
> + pgd_t entry;
> + pgd_val(entry) = pgtable_pa(pud_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pgd, entry);
> + }
> + return pud_offset(pgd, addr);
> +}
> +#else
> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
> +#define pud_free(pud)
> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
> +#endif
> +
> #define pte_index(addr) \
> (((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
> #define pte_offset(pmd, addr) \
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 540a1e8..6d1c75b 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -81,7 +81,8 @@ void mmu_disable(void)
> static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
> {
> pgd_t *pgd = pgd_offset(pgtable, vaddr);
> - pmd_t *pmd = pmd_alloc(pgd, vaddr);
> + pud_t *pud = pud_alloc(pgd, vaddr);
> + pmd_t *pmd = pmd_alloc(pud, vaddr);
> pte_t *pte = pte_alloc(pmd, vaddr);
>
> return &pte_val(*pte);
> @@ -133,18 +134,20 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
> phys_addr_t phys_start, phys_addr_t phys_end,
> pgprot_t prot)
> {
> - phys_addr_t paddr = phys_start & PGDIR_MASK;
> - uintptr_t vaddr = virt_offset & PGDIR_MASK;
> + phys_addr_t paddr = phys_start & PUD_MASK;
> + uintptr_t vaddr = virt_offset & PUD_MASK;
> uintptr_t virt_end = phys_end - paddr + vaddr;
> pgd_t *pgd;
> - pgd_t entry;
> + pud_t *pud;
> + pud_t entry;
>
> - for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
> - pgd_val(entry) = paddr;
> - pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> - pgd_val(entry) |= pgprot_val(prot);
> + for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
> + pud_val(entry) = paddr;
> + pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> + pud_val(entry) |= pgprot_val(prot);
> pgd = pgd_offset(pgtable, vaddr);
> - WRITE_ONCE(*pgd, entry);
> + pud = pud_alloc(pgd, vaddr);
> + WRITE_ONCE(*pud, entry);
> flush_tlb_page(vaddr);
> }
> }
> @@ -207,6 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
> void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
> {
> pgd_t *pgd;
> + pud_t *pud;
> pmd_t *pmd;
> pte_t *pte;
>
> @@ -215,7 +219,9 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>
> pgd = pgd_offset(pgtable, vaddr);
> assert(pgd_valid(*pgd));
> - pmd = pmd_offset(pgd, vaddr);
> + pud = pud_offset(pgd, vaddr);
> + assert(pud_valid(*pud));
> + pmd = pmd_offset(pud, vaddr);
> assert(pmd_valid(*pmd));
>
> if (pmd_huge(*pmd)) {
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index ffdd49f..cedc678 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -157,6 +157,14 @@ halt:
> */
> #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>
> +#if PAGE_SIZE == 65536
> +#define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
> +#elif PAGE_SIZE == 16384
> +#define TCR_TG_FLAGS TCR_TG0_16K | TCR_TG1_16K
> +#elif PAGE_SIZE == 4096
> +#define TCR_TG_FLAGS TCR_TG0_4K | TCR_TG1_4K
> +#endif
> +
> .globl asm_mmu_enable
> asm_mmu_enable:
> tlbi vmalle1 // invalidate I + D TLBs
> @@ -164,7 +172,7 @@ asm_mmu_enable:
>
> /* TCR */
> ldr x1, =TCR_TxSZ(VA_BITS) | \
> - TCR_TG0_64K | TCR_TG1_64K | \
> + TCR_TG_FLAGS | \
> TCR_IRGN_WBWA | TCR_ORGN_WBWA | \
> TCR_SHARED
> mrs x2, id_aa64mmfr0_el1
> --
> 2.17.1
>
Testing results looked pretty good. Everywhere I tried (TCG, seattle,
mustang, thunderx) the 4K and 64K pages worked. Most places the 16K
pages were rejected. The thunderx allowed them, but then the tests
hung. I checked what was going on with the QEMU monitor. While writing
the page tables it got an exception. I have a feeling that's a host
problem rather than a problem with this patch though.
Thanks,
drew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-03 13:04 ` Andrew Jones
@ 2020-11-03 15:49 ` Nikos Nikoleris
2020-11-03 16:10 ` Andrew Jones
0 siblings, 1 reply; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-03 15:49 UTC (permalink / raw)
To: Andrew Jones
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei
On 03/11/2020 13:04, Andrew Jones wrote:
> On Mon, Nov 02, 2020 at 11:34:43AM +0000, Nikos Nikoleris wrote:
>> Make the translation granule configurable for arm64. arm64 supports
>> page sizes of 4K, 16K and 64K. By default, arm64 is configured with
>> 64K pages. configure has been extended with a new argument:
>>
>> --page-shift=(12|14|16)
>
> --page-size=PAGE_SIZE
>
>>
>> which allows the user to set the page shift and therefore the page
>> size for arm64.
>
> which allows the user to set the page size for arm64.
>
>> Using the --page-shift for any other architecture
>
> --page-size
>
>> results an error message.
>>
>> To allow for smaller page sizes and 42b VA, this change adds support
>> for 4-level and 3-level page tables. At compile time, we determine how
>> many levels in the page tables we needed.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>> configure | 26 +++++++++++++
>> lib/arm/asm/page.h | 4 ++
>> lib/arm/asm/pgtable-hwdef.h | 4 ++
>> lib/arm/asm/pgtable.h | 6 +++
>> lib/arm/asm/thread_info.h | 4 +-
>> lib/arm64/asm/page.h | 25 ++++++++++---
>> lib/arm64/asm/pgtable-hwdef.h | 38 +++++++++++++------
>> lib/arm64/asm/pgtable.h | 69 +++++++++++++++++++++++++++++++++--
>> lib/arm/mmu.c | 26 ++++++++-----
>> arm/cstart64.S | 10 ++++-
>> 10 files changed, 180 insertions(+), 32 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 706aab5..9c6bb2c 100755
>> --- a/configure
>> +++ b/configure
>> @@ -25,6 +25,7 @@ vmm="qemu"
>> errata_force=0
>> erratatxt="$srcdir/errata.txt"
>> host_key_document=
>> +page_size=
>>
>> usage() {
>> cat <<-EOF
>> @@ -50,6 +51,8 @@ usage() {
>> --host-key-document=HOST_KEY_DOCUMENT
>> Specify the machine-specific host-key document for creating
>> a PVM image with 'genprotimg' (s390x only)
>> + --page-size=PAGE_SIZE
>> + Specify the page size (translation granule) (arm64 only)
>> EOF
>> exit 1
>> }
>> @@ -105,6 +108,9 @@ while [[ "$1" = -* ]]; do
>> --host-key-document)
>> host_key_document="$arg"
>> ;;
>> + --page-size)
>> + page_size="$arg"
>> + ;;
>> --help)
>> usage
>> ;;
>> @@ -123,6 +129,25 @@ arch_name=$arch
>> [ "$arch" = "aarch64" ] && arch="arm64"
>> [ "$arch_name" = "arm64" ] && arch_name="aarch64"
>>
>> +if [ -z "$page_size" ]; then
>> + [ "$arch" = "arm64" ] && page_size="65536"
>> + [ "$arch" = "arm" ] && page_size="4096"
>> +else
>> + if [ "$arch" != "arm64" ]; then
>> + echo "--page-size is not supported for $arch"
>> + usage
>> + fi
>> +
>> + if [ "${page_size: -1}" = "K" ] || [ "${page_size: -1}" = "k" ]; then
>> + page_size=$[ ${page_size%?} * 1024 ]
>
> $[...] is legacy syntax. Please use $((...))
>
>> + fi
>> + if [ "$page_size" != "4096" ] && [ "$page_size" != "16384" ] &&
>> + [ "$page_size" != "65536" ]; then
>> + echo "arm64 doesn't support page size of $page_size"
>> + usage
>> + fi
>> +fi
>> +
>> [ -z "$processor" ] && processor="$arch"
>>
>> if [ "$processor" = "arm64" ]; then
>> @@ -254,6 +279,7 @@ cat <<EOF >> lib/config.h
>>
>> #define CONFIG_UART_EARLY_BASE ${arm_uart_early_addr}
>> #define CONFIG_ERRATA_FORCE ${errata_force}
>> +#define CONFIG_PAGE_SIZE ${page_size}
>>
>> EOF
>> fi
>> diff --git a/lib/arm/asm/page.h b/lib/arm/asm/page.h
>> index 039c9f7..ae0ac2c 100644
>> --- a/lib/arm/asm/page.h
>> +++ b/lib/arm/asm/page.h
>> @@ -29,6 +29,10 @@ typedef struct { pteval_t pgprot; } pgprot_t;
>> #define pgd_val(x) ((x).pgd)
>> #define pgprot_val(x) ((x).pgprot)
>>
>> +/* For compatibility with arm64 page tables */
>> +#define pud_t pgd_t
>> +#define pud_val(x) pgd_val(x)
>> +
>> #define __pte(x) ((pte_t) { (x) } )
>> #define __pmd(x) ((pmd_t) { (x) } )
>> #define __pgd(x) ((pgd_t) { (x) } )
>> diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
>> index 4107e18..fe1d854 100644
>> --- a/lib/arm/asm/pgtable-hwdef.h
>> +++ b/lib/arm/asm/pgtable-hwdef.h
>> @@ -19,6 +19,10 @@
>> #define PTRS_PER_PTE 512
>> #define PTRS_PER_PMD 512
>>
>> +/* For compatibility with arm64 page tables */
>> +#define PUD_SIZE PGDIR_SIZE
>> +#define PUD_MASK PGDIR_MASK
>> +
>> #define PMD_SHIFT 21
>> #define PMD_SIZE (_AC(1,UL) << PMD_SHIFT)
>> #define PMD_MASK (~((1 << PMD_SHIFT) - 1))
>> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
>> index 078dd16..4759d82 100644
>> --- a/lib/arm/asm/pgtable.h
>> +++ b/lib/arm/asm/pgtable.h
>> @@ -53,6 +53,12 @@ static inline pmd_t *pgd_page_vaddr(pgd_t pgd)
>> return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
>> }
>>
>> +/* For compatibility with arm64 page tables */
>> +#define pud_valid(pud) pgd_valid(pud)
>> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
>> +#define pud_free(pud)
>> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
>
> Please watch the spaces vs. tabs. This comment applies to the
> whole patch. There are many places.
>
Apologies, I will go through the patch and fix this.
>> +
>> #define pmd_index(addr) \
>> (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
>> #define pmd_offset(pgd, addr) \
>> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
>> index 80ab395..eaa7258 100644
>> --- a/lib/arm/asm/thread_info.h
>> +++ b/lib/arm/asm/thread_info.h
>> @@ -14,10 +14,12 @@
>> #define THREAD_SHIFT PAGE_SHIFT
>> #define THREAD_SIZE PAGE_SIZE
>> #define THREAD_MASK PAGE_MASK
>> +#define THREAD_ALIGNMENT PAGE_SIZE
>> #else
>> #define THREAD_SHIFT MIN_THREAD_SHIFT
>> #define THREAD_SIZE (_AC(1,UL) << THREAD_SHIFT)
>> #define THREAD_MASK (~(THREAD_SIZE-1))
>> +#define THREAD_ALIGNMENT THREAD_SIZE
>> #endif
>>
>> #ifndef __ASSEMBLY__
>> @@ -38,7 +40,7 @@
>>
>> static inline void *thread_stack_alloc(void)
>> {
>> - void *sp = memalign(PAGE_SIZE, THREAD_SIZE);
>> + void *sp = memalign(THREAD_ALIGNMENT, THREAD_SIZE);
>> return sp + THREAD_START_SP;
>> }
>>
>> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
>> index 46af552..2a06207 100644
>> --- a/lib/arm64/asm/page.h
>> +++ b/lib/arm64/asm/page.h
>> @@ -10,38 +10,51 @@
>> * This work is licensed under the terms of the GNU GPL, version 2.
>> */
>>
>> +#include <config.h>
>> #include <linux/const.h>
>>
>> -#define PGTABLE_LEVELS 2
>> #define VA_BITS 42
>
> Let's bump VA_BITS to 48 while we're at it.
>
>>
>> +#define PAGE_SIZE CONFIG_PAGE_SIZE
>
> I see now how we had '%d' in the other patch for PAGE_SIZE
> instead of %ld. To keep a UL like it is for arm and x86,
> then we can add the UL to CONFIG_PAGE_SIZE in configure.
>
I realised the problem as soon as I reorderd the two changes. I have now
added UL to CONFIG_PAGE_SIZE.
>> +#if PAGE_SIZE == 65536
>> #define PAGE_SHIFT 16
>> -#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
>> +#elif PAGE_SIZE == 16384
>> +#define PAGE_SHIFT 14
>> +#elif PAGE_SIZE == 4096
>> +#define PAGE_SHIFT 12
I've also reordered things a little in <libclat.h> so that I can use
SZ_4K, SZ_16K and SZ_64K here too.
>> +#else
>> +#error Unsupported PAGE_SIZE
>> +#endif
>> #define PAGE_MASK (~(PAGE_SIZE-1))
>>
>> +#define PGTABLE_LEVELS (((VA_BITS) - 4) / (PAGE_SHIFT - 3))
>
> Please replace the above define with
>
> /*
> * Since a page table descriptor is 8 bytes we have (PAGE_SHIFT - 3) bits
> * of virtual address at each page table level. So, to get the number of
> * page table levels needed, we round up the division of the number of
> * address bits (VA_BITS - PAGE_SHIFT) by (PAGE_SHIFT - 3).
> */
> #define PGTABLE_LEVELS \
> (((VA_BITS - PAGE_SHIFT) + ((PAGE_SHIFT - 3) - 1)) / (PAGE_SHIFT - 3))
>
>> +
>> #ifndef __ASSEMBLY__
>>
>> #define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
>>
>> typedef u64 pteval_t;
>> typedef u64 pmdval_t;
>> +typedef u64 pudval_t;
>> typedef u64 pgdval_t;
>> typedef struct { pteval_t pte; } pte_t;
>> +typedef struct { pmdval_t pmd; } pmd_t;
>> +typedef struct { pudval_t pud; } pud_t;
>> typedef struct { pgdval_t pgd; } pgd_t;
>> typedef struct { pteval_t pgprot; } pgprot_t;
>>
>> #define pte_val(x) ((x).pte)
>> +#define pmd_val(x) ((x).pmd)
>> +#define pud_val(x) ((x).pud)
>> #define pgd_val(x) ((x).pgd)
>> #define pgprot_val(x) ((x).pgprot)
>>
>> #define __pte(x) ((pte_t) { (x) } )
>> +#define __pmd(x) ((pmd_t) { (x) } )
>> +#define __pud(x) ((pud_t) { (x) } )
>> #define __pgd(x) ((pgd_t) { (x) } )
>> #define __pgprot(x) ((pgprot_t) { (x) } )
>>
>> -typedef struct { pgd_t pgd; } pmd_t;
>> -#define pmd_val(x) (pgd_val((x).pgd))
>> -#define __pmd(x) ((pmd_t) { __pgd(x) } )
>> -
>> #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
>> #define __pa(x) __virt_to_phys((unsigned long)(x))
>>
>> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
>> index 3352489..3b6b0d6 100644
>> --- a/lib/arm64/asm/pgtable-hwdef.h
>> +++ b/lib/arm64/asm/pgtable-hwdef.h
>> @@ -9,38 +9,54 @@
>> * This work is licensed under the terms of the GNU GPL, version 2.
>> */
>>
>> +#include <asm/page.h>
>> +
>> #define UL(x) _AC(x, UL)
>>
>> +#define PGTABLE_LEVEL_SHIFT(n) ((PAGE_SHIFT - 3) * (4 - (n)) + 3)
>
> Please replace the above define with
>
> /*
> * The number of bits a given page table level, n (where n=0 is the top),
> * maps is ((max_n - n) - 1) * nr_bits_per_level + PAGE_SHIFT. Since a page
> * table descriptor is 8 bytes we have (PAGE_SHIFT - 3) bits per level. We
> * also have a maximum of 4 page table levels. Hence,
> */
> #define PGTABLE_LEVEL_SHIFT(n) \
> (((4 - (n)) - 1) * (PAGE_SHIFT - 3) + PAGE_SHIFT)
>
>> #define PTRS_PER_PTE (1 << (PAGE_SHIFT - 3))
>>
>> +#if PGTABLE_LEVELS > 2
>> +#define PMD_SHIFT PGTABLE_LEVEL_SHIFT(2)
>> +#define PTRS_PER_PMD PTRS_PER_PTE
>> +#define PMD_SIZE (UL(1) << PMD_SHIFT)
>> +#define PMD_MASK (~(PMD_SIZE-1))
>> +#endif
>> +
>> +#if PGTABLE_LEVELS > 3
>> +#define PUD_SHIFT PGTABLE_LEVEL_SHIFT(1)
>> +#define PTRS_PER_PUD PTRS_PER_PTE
>> +#define PUD_SIZE (UL(1) << PUD_SHIFT)
>> +#define PUD_MASK (~(PUD_SIZE-1))
>> +#else
>> +#define PUD_SIZE PGDIR_SIZE
>> +#define PUD_MASK PGDIR_MASK
>> +#endif
>> +
>> +#define PUD_VALID (_AT(pudval_t, 1) << 0)
>> +
>> /*
>> * PGDIR_SHIFT determines the size a top-level page table entry can map
>> * (depending on the configuration, this level can be 0, 1 or 2).
>> */
>> -#define PGDIR_SHIFT ((PAGE_SHIFT - 3) * PGTABLE_LEVELS + 3)
>> +#define PGDIR_SHIFT PGTABLE_LEVEL_SHIFT(4 - PGTABLE_LEVELS)
>> #define PGDIR_SIZE (_AC(1, UL) << PGDIR_SHIFT)
>> #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
>> -#define PMD_SIZE (UL(1) << PMD_SHIFT)
>> -#define PMD_MASK (~(PMD_SIZE-1))
>> -
>> /*
>> * Section address mask and size definitions.
>> */
>> -#define SECTION_SHIFT PMD_SHIFT
>> -#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
>> -#define SECTION_MASK (~(SECTION_SIZE-1))
>> +#define SECTION_SHIFT PMD_SHIFT
>> +#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
>> +#define SECTION_MASK (~(SECTION_SIZE-1))
>>
>> /*
>> * Hardware page table definitions.
>> *
>> - * Level 1 descriptor (PMD).
>> + * Level 0,1,2 descriptor (PGD, PUD and PMD).
>> */
>> #define PMD_TYPE_MASK (_AT(pmdval_t, 3) << 0)
>> #define PMD_TYPE_FAULT (_AT(pmdval_t, 0) << 0)
>> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
>> index e577d9c..c7632ae 100644
>> --- a/lib/arm64/asm/pgtable.h
>> +++ b/lib/arm64/asm/pgtable.h
>> @@ -30,10 +30,12 @@
>> #define pgtable_pa(x) ((unsigned long)(x))
>>
>> #define pgd_none(pgd) (!pgd_val(pgd))
>> +#define pud_none(pud) (!pud_val(pud))
>> #define pmd_none(pmd) (!pmd_val(pmd))
>> #define pte_none(pte) (!pte_val(pte))
>>
>> #define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
>> +#define pud_valid(pud) (pud_val(pud) & PUD_VALID)
>> #define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
>> #define pte_valid(pte) (pte_val(pte) & PTE_VALID)
>>
>> @@ -52,15 +54,76 @@ static inline pgd_t *pgd_alloc(void)
>> return pgd;
>> }
>>
>> -#define pmd_offset(pgd, addr) ((pmd_t *)pgd)
>> -#define pmd_free(pmd)
>> -#define pmd_alloc(pgd, addr) pmd_offset(pgd, addr)
>> +static inline pud_t *pgd_page_vaddr(pgd_t pgd)
>> +{
>> + return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
>> +}
>> +
>> +static inline pmd_t *pud_page_vaddr(pud_t pud)
>> +{
>> + return pgtable_va(pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK);
>> +}
>> +
>>
>> static inline pte_t *pmd_page_vaddr(pmd_t pmd)
>> {
>> return pgtable_va(pmd_val(pmd) & PHYS_MASK & (s32)PAGE_MASK);
>> }
>>
>> +#if PGTABLE_LEVELS > 2
>> +#define pmd_index(addr) \
>> + (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
>> +#define pmd_offset(pud, addr) \
>> + (pud_page_vaddr(*(pud)) + pmd_index(addr))
>> +#define pmd_free(pmd) free_page(pmd)
>> +static inline pmd_t *pmd_alloc_one(void)
>> +{
>> + assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
>> + pmd_t *pmd = alloc_page();
>> + return pmd;
>> +}
>> +static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
>> +{
>> + if (pud_none(*pud)) {
>> + pud_t entry;
>> + pud_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
>> + WRITE_ONCE(*pud, entry);
>> + }
>> + return pmd_offset(pud, addr);
>> +}
>> +#else
>> +#define pmd_offset(pud, addr) ((pmd_t *)pud)
>> +#define pmd_free(pmd)
>> +#define pmd_alloc(pud, addr) pmd_offset(pud, addr)
>> +#endif
>> +
>> +#if PGTABLE_LEVELS > 3
>> +#define pud_index(addr) \
>> + (((addr) >> PUD_SHIFT) & (PTRS_PER_PUD - 1))
>> +#define pud_offset(pgd, addr) \
>> + (pgd_page_vaddr(*(pgd)) + pud_index(addr))
>> +#define pud_free(pud) free_page(pud)
>> +static inline pud_t *pud_alloc_one(void)
>> +{
>> + assert(PTRS_PER_PMD * sizeof(pud_t) == PAGE_SIZE);
>
> PTRS_PER_PUD
>
>> + pud_t *pud = alloc_page();
>> + return pud;
>> +}
>> +static inline pud_t *pud_alloc(pgd_t *pgd, unsigned long addr)
>> +{
>> + if (pgd_none(*pgd)) {
>> + pgd_t entry;
>> + pgd_val(entry) = pgtable_pa(pud_alloc_one()) | PMD_TYPE_TABLE;
>> + WRITE_ONCE(*pgd, entry);
>> + }
>> + return pud_offset(pgd, addr);
>> +}
>> +#else
>> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
>> +#define pud_free(pud)
>> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
>> +#endif
>> +
>> #define pte_index(addr) \
>> (((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
>> #define pte_offset(pmd, addr) \
>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>> index 540a1e8..6d1c75b 100644
>> --- a/lib/arm/mmu.c
>> +++ b/lib/arm/mmu.c
>> @@ -81,7 +81,8 @@ void mmu_disable(void)
>> static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
>> {
>> pgd_t *pgd = pgd_offset(pgtable, vaddr);
>> - pmd_t *pmd = pmd_alloc(pgd, vaddr);
>> + pud_t *pud = pud_alloc(pgd, vaddr);
>> + pmd_t *pmd = pmd_alloc(pud, vaddr);
>> pte_t *pte = pte_alloc(pmd, vaddr);
>>
>> return &pte_val(*pte);
>> @@ -133,18 +134,20 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
>> phys_addr_t phys_start, phys_addr_t phys_end,
>> pgprot_t prot)
>> {
>> - phys_addr_t paddr = phys_start & PGDIR_MASK;
>> - uintptr_t vaddr = virt_offset & PGDIR_MASK;
>> + phys_addr_t paddr = phys_start & PUD_MASK;
>> + uintptr_t vaddr = virt_offset & PUD_MASK;
>> uintptr_t virt_end = phys_end - paddr + vaddr;
>> pgd_t *pgd;
>> - pgd_t entry;
>> + pud_t *pud;
>> + pud_t entry;
>>
>> - for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
>> - pgd_val(entry) = paddr;
>> - pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
>> - pgd_val(entry) |= pgprot_val(prot);
>> + for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
>> + pud_val(entry) = paddr;
>> + pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
>> + pud_val(entry) |= pgprot_val(prot);
>> pgd = pgd_offset(pgtable, vaddr);
>> - WRITE_ONCE(*pgd, entry);
>> + pud = pud_alloc(pgd, vaddr);
>> + WRITE_ONCE(*pud, entry);
>> flush_tlb_page(vaddr);
>> }
>> }
>> @@ -207,6 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
>> void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>> {
>> pgd_t *pgd;
>> + pud_t *pud;
>> pmd_t *pmd;
>> pte_t *pte;
>>
>> @@ -215,7 +219,9 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>>
>> pgd = pgd_offset(pgtable, vaddr);
>> assert(pgd_valid(*pgd));
>> - pmd = pmd_offset(pgd, vaddr);
>> + pud = pud_offset(pgd, vaddr);
>> + assert(pud_valid(*pud));
>> + pmd = pmd_offset(pud, vaddr);
>> assert(pmd_valid(*pmd));
>>
>> if (pmd_huge(*pmd)) {
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index ffdd49f..cedc678 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -157,6 +157,14 @@ halt:
>> */
>> #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>>
>> +#if PAGE_SIZE == 65536
>> +#define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
>> +#elif PAGE_SIZE == 16384
>> +#define TCR_TG_FLAGS TCR_TG0_16K | TCR_TG1_16K
>> +#elif PAGE_SIZE == 4096
>> +#define TCR_TG_FLAGS TCR_TG0_4K | TCR_TG1_4K
>> +#endif
>> +
>> .globl asm_mmu_enable
>> asm_mmu_enable:
>> tlbi vmalle1 // invalidate I + D TLBs
>> @@ -164,7 +172,7 @@ asm_mmu_enable:
>>
>> /* TCR */
>> ldr x1, =TCR_TxSZ(VA_BITS) | \
>> - TCR_TG0_64K | TCR_TG1_64K | \
>> + TCR_TG_FLAGS | \
>> TCR_IRGN_WBWA | TCR_ORGN_WBWA | \
>> TCR_SHARED
>> mrs x2, id_aa64mmfr0_el1
>> --
>> 2.17.1
>>
>
> Testing results looked pretty good. Everywhere I tried (TCG, seattle,
> mustang, thunderx) the 4K and 64K pages worked. Most places the 16K
> pages were rejected. The thunderx allowed them, but then the tests
> hung. I checked what was going on with the QEMU monitor. While writing
> the page tables it got an exception. I have a feeling that's a host
> problem rather than a problem with this patch though.
>
I've also tested 4K and 64K (TCG, ThunderX2, N1 SDP). When I configured
16K pages, I hit the assertion in all 3 cases.
Thanks for the review! I will post an update soon.
Thanks,
Nikos
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-03 15:49 ` Nikos Nikoleris
@ 2020-11-03 16:10 ` Andrew Jones
2020-11-03 16:25 ` Alexandru Elisei
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Jones @ 2020-11-03 16:10 UTC (permalink / raw)
To: Nikos Nikoleris
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara,
alexandru.elisei
On Tue, Nov 03, 2020 at 03:49:32PM +0000, Nikos Nikoleris wrote:
> > > diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
> > > index 46af552..2a06207 100644
> > > --- a/lib/arm64/asm/page.h
> > > +++ b/lib/arm64/asm/page.h
> > > @@ -10,38 +10,51 @@
> > > * This work is licensed under the terms of the GNU GPL, version 2.
> > > */
> > > +#include <config.h>
> > > #include <linux/const.h>
> > > -#define PGTABLE_LEVELS 2
> > > #define VA_BITS 42
> >
> > Let's bump VA_BITS to 48 while we're at it.
I tried my suggestion to go to 48 VA bits, but it seems to break
things for 64K pages.
> >
> > > +#define PAGE_SIZE CONFIG_PAGE_SIZE
> >
> > I see now how we had '%d' in the other patch for PAGE_SIZE
> > instead of %ld. To keep a UL like it is for arm and x86,
> > then we can add the UL to CONFIG_PAGE_SIZE in configure.
> >
>
> I realised the problem as soon as I reorderd the two changes. I have now
> added UL to CONFIG_PAGE_SIZE.
>
> > > +#if PAGE_SIZE == 65536
> > > #define PAGE_SHIFT 16
> > > -#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> > > +#elif PAGE_SIZE == 16384
> > > +#define PAGE_SHIFT 14
> > > +#elif PAGE_SIZE == 4096
> > > +#define PAGE_SHIFT 12
>
> I've also reordered things a little in <libclat.h> so that I can use SZ_4K,
> SZ_16K and SZ_64K here too.
Sounds good to me, as long as we can have PAGE_SIZE and PAGE_SHIFT
defined in assembly too.
Thanks,
drew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-02 11:34 ` [kvm-unit-tests PATCH 1/2] " Nikos Nikoleris
2020-11-03 13:04 ` Andrew Jones
@ 2020-11-03 16:21 ` Alexandru Elisei
2020-11-03 17:14 ` Nikos Nikoleris
1 sibling, 1 reply; 17+ messages in thread
From: Alexandru Elisei @ 2020-11-03 16:21 UTC (permalink / raw)
To: Nikos Nikoleris, kvm
Cc: mark.rutland, jade.alglave, luc.maranget, andre.przywara, drjones
Hi Nikos,
In the subject, you forgot increment the series version to v2. Not a big deal, but
it makes it obvious which is the latest version.
kvm-unit-tests hangs when using 16k pages, I believe it's because we're using a
block mapping at the wrong level; comments below.
On 11/2/20 11:34 AM, Nikos Nikoleris wrote:
> Make the translation granule configurable for arm64. arm64 supports
> page sizes of 4K, 16K and 64K. By default, arm64 is configured with
> 64K pages. configure has been extended with a new argument:
>
> --page-shift=(12|14|16)
>
> which allows the user to set the page shift and therefore the page
> size for arm64. Using the --page-shift for any other architecture
> results an error message.
>
> To allow for smaller page sizes and 42b VA, this change adds support
> for 4-level and 3-level page tables. At compile time, we determine how
> many levels in the page tables we needed.
>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
> configure | 26 +++++++++++++
> lib/arm/asm/page.h | 4 ++
> lib/arm/asm/pgtable-hwdef.h | 4 ++
> lib/arm/asm/pgtable.h | 6 +++
> lib/arm/asm/thread_info.h | 4 +-
> lib/arm64/asm/page.h | 25 ++++++++++---
> lib/arm64/asm/pgtable-hwdef.h | 38 +++++++++++++------
> lib/arm64/asm/pgtable.h | 69 +++++++++++++++++++++++++++++++++--
> lib/arm/mmu.c | 26 ++++++++-----
> arm/cstart64.S | 10 ++++-
> 10 files changed, 180 insertions(+), 32 deletions(-)
>
> diff --git a/configure b/configure
> index 706aab5..9c6bb2c 100755
> --- a/configure
> +++ b/configure
> @@ -25,6 +25,7 @@ vmm="qemu"
> errata_force=0
> erratatxt="$srcdir/errata.txt"
> host_key_document=
> +page_size=
>
> usage() {
> cat <<-EOF
> @@ -50,6 +51,8 @@ usage() {
> --host-key-document=HOST_KEY_DOCUMENT
> Specify the machine-specific host-key document for creating
> a PVM image with 'genprotimg' (s390x only)
> + --page-size=PAGE_SIZE
> + Specify the page size (translation granule) (arm64 only)
It would be nice to have an example here regarding the expected format for
PAGE_SIZE, maybe something similar to:
"Specify the page size (translation granule) (4k, 16k or 64k, default is 64k,
arm64 only)"
The page size ends up being transformed into an integer, so it will be less likely
for an user to be confused about the input format.
> EOF
> exit 1
> }
> @@ -105,6 +108,9 @@ while [[ "$1" = -* ]]; do
> --host-key-document)
> host_key_document="$arg"
> ;;
> + --page-size)
> + page_size="$arg"
> + ;;
> --help)
> usage
> ;;
> @@ -123,6 +129,25 @@ arch_name=$arch
> [ "$arch" = "aarch64" ] && arch="arm64"
> [ "$arch_name" = "arm64" ] && arch_name="aarch64"
>
> +if [ -z "$page_size" ]; then
> + [ "$arch" = "arm64" ] && page_size="65536"
> + [ "$arch" = "arm" ] && page_size="4096"
> +else
> + if [ "$arch" != "arm64" ]; then
> + echo "--page-size is not supported for $arch"
> + usage
> + fi
> +
> + if [ "${page_size: -1}" = "K" ] || [ "${page_size: -1}" = "k" ]; then
> + page_size=$[ ${page_size%?} * 1024 ]
> + fi
> + if [ "$page_size" != "4096" ] && [ "$page_size" != "16384" ] &&
> + [ "$page_size" != "65536" ]; then
> + echo "arm64 doesn't support page size of $page_size"
> + usage
> + fi
> +fi
> +
> [ -z "$processor" ] && processor="$arch"
>
> if [ "$processor" = "arm64" ]; then
> @@ -254,6 +279,7 @@ cat <<EOF >> lib/config.h
>
> #define CONFIG_UART_EARLY_BASE ${arm_uart_early_addr}
> #define CONFIG_ERRATA_FORCE ${errata_force}
> +#define CONFIG_PAGE_SIZE ${page_size}
>
> EOF
> fi
> diff --git a/lib/arm/asm/page.h b/lib/arm/asm/page.h
> index 039c9f7..ae0ac2c 100644
> --- a/lib/arm/asm/page.h
> +++ b/lib/arm/asm/page.h
> @@ -29,6 +29,10 @@ typedef struct { pteval_t pgprot; } pgprot_t;
> #define pgd_val(x) ((x).pgd)
> #define pgprot_val(x) ((x).pgprot)
>
> +/* For compatibility with arm64 page tables */
> +#define pud_t pgd_t
> +#define pud_val(x) pgd_val(x)
> +
> #define __pte(x) ((pte_t) { (x) } )
> #define __pmd(x) ((pmd_t) { (x) } )
> #define __pgd(x) ((pgd_t) { (x) } )
> diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
> index 4107e18..fe1d854 100644
> --- a/lib/arm/asm/pgtable-hwdef.h
> +++ b/lib/arm/asm/pgtable-hwdef.h
> @@ -19,6 +19,10 @@
> #define PTRS_PER_PTE 512
> #define PTRS_PER_PMD 512
>
> +/* For compatibility with arm64 page tables */
> +#define PUD_SIZE PGDIR_SIZE
> +#define PUD_MASK PGDIR_MASK
> +
> #define PMD_SHIFT 21
> #define PMD_SIZE (_AC(1,UL) << PMD_SHIFT)
> #define PMD_MASK (~((1 << PMD_SHIFT) - 1))
> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> index 078dd16..4759d82 100644
> --- a/lib/arm/asm/pgtable.h
> +++ b/lib/arm/asm/pgtable.h
> @@ -53,6 +53,12 @@ static inline pmd_t *pgd_page_vaddr(pgd_t pgd)
> return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
> }
>
> +/* For compatibility with arm64 page tables */
> +#define pud_valid(pud) pgd_valid(pud)
> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
> +#define pud_free(pud)
> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
> +
> #define pmd_index(addr) \
> (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
> #define pmd_offset(pgd, addr) \
> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
> index 80ab395..eaa7258 100644
> --- a/lib/arm/asm/thread_info.h
> +++ b/lib/arm/asm/thread_info.h
> @@ -14,10 +14,12 @@
> #define THREAD_SHIFT PAGE_SHIFT
> #define THREAD_SIZE PAGE_SIZE
> #define THREAD_MASK PAGE_MASK
> +#define THREAD_ALIGNMENT PAGE_SIZE
> #else
> #define THREAD_SHIFT MIN_THREAD_SHIFT
> #define THREAD_SIZE (_AC(1,UL) << THREAD_SHIFT)
> #define THREAD_MASK (~(THREAD_SIZE-1))
> +#define THREAD_ALIGNMENT THREAD_SIZE
> #endif
>
> #ifndef __ASSEMBLY__
> @@ -38,7 +40,7 @@
>
> static inline void *thread_stack_alloc(void)
> {
> - void *sp = memalign(PAGE_SIZE, THREAD_SIZE);
> + void *sp = memalign(THREAD_ALIGNMENT, THREAD_SIZE);
> return sp + THREAD_START_SP;
> }
>
> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
> index 46af552..2a06207 100644
> --- a/lib/arm64/asm/page.h
> +++ b/lib/arm64/asm/page.h
> @@ -10,38 +10,51 @@
> * This work is licensed under the terms of the GNU GPL, version 2.
> */
>
> +#include <config.h>
> #include <linux/const.h>
>
> -#define PGTABLE_LEVELS 2
> #define VA_BITS 42
>
> +#define PAGE_SIZE CONFIG_PAGE_SIZE
> +#if PAGE_SIZE == 65536
> #define PAGE_SHIFT 16
> -#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> +#elif PAGE_SIZE == 16384
> +#define PAGE_SHIFT 14
> +#elif PAGE_SIZE == 4096
> +#define PAGE_SHIFT 12
> +#else
> +#error Unsupported PAGE_SIZE
> +#endif
> #define PAGE_MASK (~(PAGE_SIZE-1))
>
> +#define PGTABLE_LEVELS (((VA_BITS) - 4) / (PAGE_SHIFT - 3))
> +
> #ifndef __ASSEMBLY__
>
> #define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
>
> typedef u64 pteval_t;
> typedef u64 pmdval_t;
> +typedef u64 pudval_t;
> typedef u64 pgdval_t;
> typedef struct { pteval_t pte; } pte_t;
> +typedef struct { pmdval_t pmd; } pmd_t;
> +typedef struct { pudval_t pud; } pud_t;
> typedef struct { pgdval_t pgd; } pgd_t;
> typedef struct { pteval_t pgprot; } pgprot_t;
>
> #define pte_val(x) ((x).pte)
> +#define pmd_val(x) ((x).pmd)
> +#define pud_val(x) ((x).pud)
> #define pgd_val(x) ((x).pgd)
> #define pgprot_val(x) ((x).pgprot)
>
> #define __pte(x) ((pte_t) { (x) } )
> +#define __pmd(x) ((pmd_t) { (x) } )
> +#define __pud(x) ((pud_t) { (x) } )
> #define __pgd(x) ((pgd_t) { (x) } )
> #define __pgprot(x) ((pgprot_t) { (x) } )
>
> -typedef struct { pgd_t pgd; } pmd_t;
> -#define pmd_val(x) (pgd_val((x).pgd))
> -#define __pmd(x) ((pmd_t) { __pgd(x) } )
> -
> #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
> #define __pa(x) __virt_to_phys((unsigned long)(x))
>
> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
> index 3352489..3b6b0d6 100644
> --- a/lib/arm64/asm/pgtable-hwdef.h
> +++ b/lib/arm64/asm/pgtable-hwdef.h
> @@ -9,38 +9,54 @@
> * This work is licensed under the terms of the GNU GPL, version 2.
> */
>
> +#include <asm/page.h>
> +
> #define UL(x) _AC(x, UL)
>
> +#define PGTABLE_LEVEL_SHIFT(n) ((PAGE_SHIFT - 3) * (4 - (n)) + 3)
> #define PTRS_PER_PTE (1 << (PAGE_SHIFT - 3))
>
> +#if PGTABLE_LEVELS > 2
> +#define PMD_SHIFT PGTABLE_LEVEL_SHIFT(2)
> +#define PTRS_PER_PMD PTRS_PER_PTE
> +#define PMD_SIZE (UL(1) << PMD_SHIFT)
> +#define PMD_MASK (~(PMD_SIZE-1))
> +#endif
For consistency, I think we should have these definitions even when PGTABLE_LEVELS
<= 2, same as we do for puds below.
> +
> +#if PGTABLE_LEVELS > 3
> +#define PUD_SHIFT PGTABLE_LEVEL_SHIFT(1)
> +#define PTRS_PER_PUD PTRS_PER_PTE
> +#define PUD_SIZE (UL(1) << PUD_SHIFT)
> +#define PUD_MASK (~(PUD_SIZE-1))
> +#else
> +#define PUD_SIZE PGDIR_SIZE
> +#define PUD_MASK PGDIR_MASK
> +#endif
> +
> +#define PUD_VALID (_AT(pudval_t, 1) << 0)
> +
> /*
> * PGDIR_SHIFT determines the size a top-level page table entry can map
> * (depending on the configuration, this level can be 0, 1 or 2).
> */
> -#define PGDIR_SHIFT ((PAGE_SHIFT - 3) * PGTABLE_LEVELS + 3)
> +#define PGDIR_SHIFT PGTABLE_LEVEL_SHIFT(4 - PGTABLE_LEVELS)
> #define PGDIR_SIZE (_AC(1, UL) << PGDIR_SHIFT)
> #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
> -#define PMD_SIZE (UL(1) << PMD_SHIFT)
> -#define PMD_MASK (~(PMD_SIZE-1))
> -
> /*
> * Section address mask and size definitions.
> */
> -#define SECTION_SHIFT PMD_SHIFT
> -#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
> -#define SECTION_MASK (~(SECTION_SIZE-1))
> +#define SECTION_SHIFT PMD_SHIFT
> +#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
> +#define SECTION_MASK (~(SECTION_SIZE-1))
>
> /*
> * Hardware page table definitions.
> *
> - * Level 1 descriptor (PMD).
> + * Level 0,1,2 descriptor (PGD, PUD and PMD).
> */
> #define PMD_TYPE_MASK (_AT(pmdval_t, 3) << 0)
> #define PMD_TYPE_FAULT (_AT(pmdval_t, 0) << 0)
> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> index e577d9c..c7632ae 100644
> --- a/lib/arm64/asm/pgtable.h
> +++ b/lib/arm64/asm/pgtable.h
> @@ -30,10 +30,12 @@
> #define pgtable_pa(x) ((unsigned long)(x))
>
> #define pgd_none(pgd) (!pgd_val(pgd))
> +#define pud_none(pud) (!pud_val(pud))
> #define pmd_none(pmd) (!pmd_val(pmd))
> #define pte_none(pte) (!pte_val(pte))
>
> #define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
> +#define pud_valid(pud) (pud_val(pud) & PUD_VALID)
> #define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
> #define pte_valid(pte) (pte_val(pte) & PTE_VALID)
>
> @@ -52,15 +54,76 @@ static inline pgd_t *pgd_alloc(void)
> return pgd;
> }
>
> -#define pmd_offset(pgd, addr) ((pmd_t *)pgd)
> -#define pmd_free(pmd)
> -#define pmd_alloc(pgd, addr) pmd_offset(pgd, addr)
> +static inline pud_t *pgd_page_vaddr(pgd_t pgd)
> +{
> + return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
Aren't all numbers treated by gcc like integers? Which means that for arm64 they
are s32 already? I see the cast was present in pte_page_vaddr(), am I missing
something?
> +}
> +
> +static inline pmd_t *pud_page_vaddr(pud_t pud)
> +{
> + return pgtable_va(pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK);
> +}
> +
Stray newline.
>
> static inline pte_t *pmd_page_vaddr(pmd_t pmd)
> {
> return pgtable_va(pmd_val(pmd) & PHYS_MASK & (s32)PAGE_MASK);
> }
>
> +#if PGTABLE_LEVELS > 2
> +#define pmd_index(addr) \
> + (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
> +#define pmd_offset(pud, addr) \
> + (pud_page_vaddr(*(pud)) + pmd_index(addr))
> +#define pmd_free(pmd) free_page(pmd)
> +static inline pmd_t *pmd_alloc_one(void)
> +{
> + assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
> + pmd_t *pmd = alloc_page();
> + return pmd;
> +}
> +static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
> +{
> + if (pud_none(*pud)) {
> + pud_t entry;
> + pud_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pud, entry);
> + }
> + return pmd_offset(pud, addr);
> +}
> +#else
> +#define pmd_offset(pud, addr) ((pmd_t *)pud)
> +#define pmd_free(pmd)
> +#define pmd_alloc(pud, addr) pmd_offset(pud, addr)
> +#endif
> +
> +#if PGTABLE_LEVELS > 3
> +#define pud_index(addr) \
> + (((addr) >> PUD_SHIFT) & (PTRS_PER_PUD - 1))
> +#define pud_offset(pgd, addr) \
> + (pgd_page_vaddr(*(pgd)) + pud_index(addr))
> +#define pud_free(pud) free_page(pud)
> +static inline pud_t *pud_alloc_one(void)
> +{
> + assert(PTRS_PER_PMD * sizeof(pud_t) == PAGE_SIZE);
> + pud_t *pud = alloc_page();
> + return pud;
> +}
> +static inline pud_t *pud_alloc(pgd_t *pgd, unsigned long addr)
> +{
> + if (pgd_none(*pgd)) {
> + pgd_t entry;
> + pgd_val(entry) = pgtable_pa(pud_alloc_one()) | PMD_TYPE_TABLE;
> + WRITE_ONCE(*pgd, entry);
> + }
> + return pud_offset(pgd, addr);
> +}
> +#else
> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
> +#define pud_free(pud)
> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
> +#endif
> +
> #define pte_index(addr) \
> (((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
> #define pte_offset(pmd, addr) \
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 540a1e8..6d1c75b 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -81,7 +81,8 @@ void mmu_disable(void)
> static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
> {
> pgd_t *pgd = pgd_offset(pgtable, vaddr);
> - pmd_t *pmd = pmd_alloc(pgd, vaddr);
> + pud_t *pud = pud_alloc(pgd, vaddr);
> + pmd_t *pmd = pmd_alloc(pud, vaddr);
> pte_t *pte = pte_alloc(pmd, vaddr);
>
> return &pte_val(*pte);
> @@ -133,18 +134,20 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
> phys_addr_t phys_start, phys_addr_t phys_end,
> pgprot_t prot)
> {
> - phys_addr_t paddr = phys_start & PGDIR_MASK;
> - uintptr_t vaddr = virt_offset & PGDIR_MASK;
> + phys_addr_t paddr = phys_start & PUD_MASK;
> + uintptr_t vaddr = virt_offset & PUD_MASK;
> uintptr_t virt_end = phys_end - paddr + vaddr;
> pgd_t *pgd;
> - pgd_t entry;
> + pud_t *pud;
> + pud_t entry;
>
> - for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
> - pgd_val(entry) = paddr;
> - pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> - pgd_val(entry) |= pgprot_val(prot);
> + for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
> + pud_val(entry) = paddr;
> + pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> + pud_val(entry) |= pgprot_val(prot);
> pgd = pgd_offset(pgtable, vaddr);
> - WRITE_ONCE(*pgd, entry);
> + pud = pud_alloc(pgd, vaddr);
> + WRITE_ONCE(*pud, entry);
I'm looking at Figure D5-9, page D5-2568 from D5-2568, and it looks to me like for
16k pages the minimum level where a block mapping is allowed is level 2 (aka pmd);
for 16k pages I get PGTABLE_LEVELS=3, which means the code will created a block
mapping at the pud level. Judging from figure D5-12, 64k pages have the same
requirement (unless ARMv8.2-LPA is implemented), but we don't run into this issue
there because PGTABLE_LEVELS=2 in that case.
I tried running a test using qemu TCG with --page-size=16k and the test hangs
(didn't investigate exactly where):
$ ./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu- --page-size=64k &&
make -j24 && arm/run arm/cache.flat
Thanks,
Alex
> flush_tlb_page(vaddr);
> }
> }
> @@ -207,6 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
> void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
> {
> pgd_t *pgd;
> + pud_t *pud;
> pmd_t *pmd;
> pte_t *pte;
>
> @@ -215,7 +219,9 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>
> pgd = pgd_offset(pgtable, vaddr);
> assert(pgd_valid(*pgd));
> - pmd = pmd_offset(pgd, vaddr);
> + pud = pud_offset(pgd, vaddr);
> + assert(pud_valid(*pud));
> + pmd = pmd_offset(pud, vaddr);
> assert(pmd_valid(*pmd));
>
> if (pmd_huge(*pmd)) {
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index ffdd49f..cedc678 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -157,6 +157,14 @@ halt:
> */
> #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>
> +#if PAGE_SIZE == 65536
> +#define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
> +#elif PAGE_SIZE == 16384
> +#define TCR_TG_FLAGS TCR_TG0_16K | TCR_TG1_16K
> +#elif PAGE_SIZE == 4096
> +#define TCR_TG_FLAGS TCR_TG0_4K | TCR_TG1_4K
> +#endif
> +
> .globl asm_mmu_enable
> asm_mmu_enable:
> tlbi vmalle1 // invalidate I + D TLBs
> @@ -164,7 +172,7 @@ asm_mmu_enable:
>
> /* TCR */
> ldr x1, =TCR_TxSZ(VA_BITS) | \
> - TCR_TG0_64K | TCR_TG1_64K | \
> + TCR_TG_FLAGS | \
> TCR_IRGN_WBWA | TCR_ORGN_WBWA | \
> TCR_SHARED
> mrs x2, id_aa64mmfr0_el1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-03 16:10 ` Andrew Jones
@ 2020-11-03 16:25 ` Alexandru Elisei
2020-11-03 16:39 ` Andrew Jones
0 siblings, 1 reply; 17+ messages in thread
From: Alexandru Elisei @ 2020-11-03 16:25 UTC (permalink / raw)
To: Andrew Jones, Nikos Nikoleris
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara
Hi,
On 11/3/20 4:10 PM, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 03:49:32PM +0000, Nikos Nikoleris wrote:
>>>> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
>>>> index 46af552..2a06207 100644
>>>> --- a/lib/arm64/asm/page.h
>>>> +++ b/lib/arm64/asm/page.h
>>>> @@ -10,38 +10,51 @@
>>>> * This work is licensed under the terms of the GNU GPL, version 2.
>>>> */
>>>> +#include <config.h>
>>>> #include <linux/const.h>
>>>> -#define PGTABLE_LEVELS 2
>>>> #define VA_BITS 42
>>> Let's bump VA_BITS to 48 while we're at it.
> I tried my suggestion to go to 48 VA bits, but it seems to break
> things for 64K pages.
I believe that is because we end up with PGTABLE_LEVELS=3 and in
mmu_set_ranges_sect() we try to install a block mapping at the PUD level, which is
forbidden by the architecture.
I think the easiest fix for that is to always try to install block mapping at the
pmd level. The diff below fixed all errors (with 16k and 64k pages):
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 6d1c75b00eaa..d33948a8a06a 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -134,20 +134,22 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
phys_addr_t phys_start, phys_addr_t phys_end,
pgprot_t prot)
{
- phys_addr_t paddr = phys_start & PUD_MASK;
- uintptr_t vaddr = virt_offset & PUD_MASK;
+ phys_addr_t paddr = phys_start & PMD_MASK;
+ uintptr_t vaddr = virt_offset & PMD_MASK;
uintptr_t virt_end = phys_end - paddr + vaddr;
pgd_t *pgd;
pud_t *pud;
- pud_t entry;
+ pmd_t *pmd;
+ pmd_t entry;
- for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
- pud_val(entry) = paddr;
- pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
- pud_val(entry) |= pgprot_val(prot);
+ for (; vaddr < virt_end; vaddr += PMD_SIZE, paddr += PMD_SIZE) {
+ pmd_val(entry) = paddr;
+ pmd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
+ pmd_val(entry) |= pgprot_val(prot);
pgd = pgd_offset(pgtable, vaddr);
pud = pud_alloc(pgd, vaddr);
- WRITE_ONCE(*pud, entry);
+ pmd = pmd_alloc(pud, vaddr);
+ WRITE_ONCE(*pmd, entry);
flush_tlb_page(vaddr);
}
}
diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
index 2a06207444aa..f649f56bf16f 100644
--- a/lib/arm64/asm/page.h
+++ b/lib/arm64/asm/page.h
@@ -13,7 +13,7 @@
#include <config.h>
#include <linux/const.h>
-#define VA_BITS 42
+#define VA_BITS 46
#define PAGE_SIZE CONFIG_PAGE_SIZE
#if PAGE_SIZE == 65536
Thanks,
Alex
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-03 16:25 ` Alexandru Elisei
@ 2020-11-03 16:39 ` Andrew Jones
2020-11-03 16:46 ` Alexandru Elisei
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Jones @ 2020-11-03 16:39 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Nikos Nikoleris, kvm, mark.rutland, jade.alglave, luc.maranget,
andre.przywara
On Tue, Nov 03, 2020 at 04:25:15PM +0000, Alexandru Elisei wrote:
> Hi,
>
> On 11/3/20 4:10 PM, Andrew Jones wrote:
> > On Tue, Nov 03, 2020 at 03:49:32PM +0000, Nikos Nikoleris wrote:
> >>>> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
> >>>> index 46af552..2a06207 100644
> >>>> --- a/lib/arm64/asm/page.h
> >>>> +++ b/lib/arm64/asm/page.h
> >>>> @@ -10,38 +10,51 @@
> >>>> * This work is licensed under the terms of the GNU GPL, version 2.
> >>>> */
> >>>> +#include <config.h>
> >>>> #include <linux/const.h>
> >>>> -#define PGTABLE_LEVELS 2
> >>>> #define VA_BITS 42
> >>> Let's bump VA_BITS to 48 while we're at it.
> > I tried my suggestion to go to 48 VA bits, but it seems to break
> > things for 64K pages.
>
> I believe that is because we end up with PGTABLE_LEVELS=3 and in
> mmu_set_ranges_sect() we try to install a block mapping at the PUD level, which is
> forbidden by the architecture.
>
> I think the easiest fix for that is to always try to install block mapping at the
> pmd level. The diff below fixed all errors (with 16k and 64k pages):
>
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 6d1c75b00eaa..d33948a8a06a 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -134,20 +134,22 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
> phys_addr_t phys_start, phys_addr_t phys_end,
> pgprot_t prot)
> {
> - phys_addr_t paddr = phys_start & PUD_MASK;
> - uintptr_t vaddr = virt_offset & PUD_MASK;
> + phys_addr_t paddr = phys_start & PMD_MASK;
> + uintptr_t vaddr = virt_offset & PMD_MASK;
> uintptr_t virt_end = phys_end - paddr + vaddr;
> pgd_t *pgd;
> pud_t *pud;
> - pud_t entry;
> + pmd_t *pmd;
> + pmd_t entry;
>
> - for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
> - pud_val(entry) = paddr;
> - pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> - pud_val(entry) |= pgprot_val(prot);
> + for (; vaddr < virt_end; vaddr += PMD_SIZE, paddr += PMD_SIZE) {
> + pmd_val(entry) = paddr;
> + pmd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
> + pmd_val(entry) |= pgprot_val(prot);
> pgd = pgd_offset(pgtable, vaddr);
> pud = pud_alloc(pgd, vaddr);
> - WRITE_ONCE(*pud, entry);
> + pmd = pmd_alloc(pud, vaddr);
> + WRITE_ONCE(*pmd, entry);
> flush_tlb_page(vaddr);
Nice work! This resolves my issue as well. And, I can run with 16K pages
on my thunderx now too.
> }
> }
> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
> index 2a06207444aa..f649f56bf16f 100644
> --- a/lib/arm64/asm/page.h
> +++ b/lib/arm64/asm/page.h
> @@ -13,7 +13,7 @@
> #include <config.h>
> #include <linux/const.h>
>
> -#define VA_BITS 42
> +#define VA_BITS 46
Let's use 48.
>
> #define PAGE_SIZE CONFIG_PAGE_SIZE
> #if PAGE_SIZE == 65536
>
> Thanks,
>
> Alex
>
Thanks,
drew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-03 16:39 ` Andrew Jones
@ 2020-11-03 16:46 ` Alexandru Elisei
0 siblings, 0 replies; 17+ messages in thread
From: Alexandru Elisei @ 2020-11-03 16:46 UTC (permalink / raw)
To: Andrew Jones
Cc: Nikos Nikoleris, kvm, mark.rutland, jade.alglave, luc.maranget,
andre.przywara
Hi Andrew,
On 11/3/20 4:39 PM, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 04:25:15PM +0000, Alexandru Elisei wrote:
>> Hi,
>>
>> On 11/3/20 4:10 PM, Andrew Jones wrote:
>>> On Tue, Nov 03, 2020 at 03:49:32PM +0000, Nikos Nikoleris wrote:
>>>>>> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
>>>>>> index 46af552..2a06207 100644
>>>>>> --- a/lib/arm64/asm/page.h
>>>>>> +++ b/lib/arm64/asm/page.h
>>>>>> @@ -10,38 +10,51 @@
>>>>>> * This work is licensed under the terms of the GNU GPL, version 2.
>>>>>> */
>>>>>> +#include <config.h>
>>>>>> #include <linux/const.h>
>>>>>> -#define PGTABLE_LEVELS 2
>>>>>> #define VA_BITS 42
>>>>> Let's bump VA_BITS to 48 while we're at it.
>>> I tried my suggestion to go to 48 VA bits, but it seems to break
>>> things for 64K pages.
>> I believe that is because we end up with PGTABLE_LEVELS=3 and in
>> mmu_set_ranges_sect() we try to install a block mapping at the PUD level, which is
>> forbidden by the architecture.
>>
>> I think the easiest fix for that is to always try to install block mapping at the
>> pmd level. The diff below fixed all errors (with 16k and 64k pages):
>> [..]
> Let's use 48.
>
Yes, that was a typo.
Thanks,
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-03 10:02 ` Andrew Jones
2020-11-03 10:21 ` Nikos Nikoleris
@ 2020-11-03 17:03 ` Alexandru Elisei
2020-11-03 17:36 ` Andrew Jones
1 sibling, 1 reply; 17+ messages in thread
From: Alexandru Elisei @ 2020-11-03 17:03 UTC (permalink / raw)
To: Andrew Jones, Nikos Nikoleris
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara
Hi,
On 11/3/20 10:02 AM, Andrew Jones wrote:
> On Mon, Nov 02, 2020 at 11:34:44AM +0000, Nikos Nikoleris wrote:
>> Now that we can change the translation granule at will, and since
>> arm64 implementations can support a subset of the architecturally
>> defined granules, we need to check and warn the user if the configured
>> granule is not supported.
> nit: it'd be better for this patch to come before the last patch.
>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>> lib/arm64/asm/processor.h | 65 +++++++++++++++++++++++++++++++++++++++
>> lib/arm/mmu.c | 3 ++
>> 2 files changed, 68 insertions(+)
>>
>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>> index 02665b8..0eac928 100644
>> --- a/lib/arm64/asm/processor.h
>> +++ b/lib/arm64/asm/processor.h
>> @@ -117,5 +117,70 @@ static inline u64 get_ctr(void)
>>
>> extern u32 dcache_line_size;
>>
>> +static inline unsigned long get_id_aa64mmfr0_el1(void)
>> +{
>> + unsigned long mmfr0;
>> + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
>> + return mmfr0;
>> +}
>> +
>> +/* From arch/arm64/include/asm/cpufeature.h */
>> +static inline unsigned int
>> +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
>> +{
>> + return (u64)(features << (64 - width - field)) >> (64 - width);
>> +}
>> +
>> +#define ID_AA64MMFR0_TGRAN4_SHIFT 28
>> +#define ID_AA64MMFR0_TGRAN64_SHIFT 24
>> +#define ID_AA64MMFR0_TGRAN16_SHIFT 20
>> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
>> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
>> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
>> +
>> +static inline bool system_supports_64kb_granule(void)
>> +{
>> + u64 mmfr0;
>> + u32 val;
>> +
>> + mmfr0 = get_id_aa64mmfr0_el1();
>> + val = cpuid_feature_extract_unsigned_field_width(
>> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT,4);
>> +
>> + return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
>> +}
>> +
>> +static inline bool system_supports_16kb_granule(void)
>> +{
>> + u64 mmfr0;
>> + u32 val;
>> +
>> + mmfr0 = get_id_aa64mmfr0_el1();
>> + val = cpuid_feature_extract_unsigned_field_width(
>> + mmfr0, ID_AA64MMFR0_TGRAN16_SHIFT, 4);
>> +
>> + return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
>> +}
>> +
>> +static inline bool system_supports_4kb_granule(void)
>> +{
>> + u64 mmfr0;
>> + u32 val;
>> +
>> + mmfr0 = get_id_aa64mmfr0_el1();
>> + val = cpuid_feature_extract_unsigned_field_width(
>> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT, 4);
>> +
>> + return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
>> +}
>> +
>> +#if PAGE_SIZE == 65536
>> +#define system_supports_configured_granule system_supports_64kb_granule
>> +#elif PAGE_SIZE == 16384
>> +#define system_supports_configured_granule system_supports_16kb_granule
>> +#elif PAGE_SIZE == 4096
>> +#define system_supports_configured_granule system_supports_4kb_granule
>> +#endif
>> +
>> #endif /* !__ASSEMBLY__ */
>> #endif /* _ASMARM64_PROCESSOR_H_ */
>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>> index 6d1c75b..51fa745 100644
>> --- a/lib/arm/mmu.c
>> +++ b/lib/arm/mmu.c
>> @@ -163,6 +163,9 @@ void *setup_mmu(phys_addr_t phys_end)
>>
>> #ifdef __aarch64__
>> init_alloc_vpage((void*)(4ul << 30));
>> +
>> + assert_msg(system_supports_configured_granule(),
>> + "Unsupported translation granule %d\n", PAGE_SIZE);
> ^
> needs '%ld' to compile
>> #endif
>>
>> mmu_idmap = alloc_page();
>> --
>> 2.17.1
>>
> I don't think we need the three separate functions. How about just
> doing the following diff?
>
> Thanks,
> drew
>
>
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 540a1e842d5b..fef62f5a9866 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -160,6 +160,9 @@ void *setup_mmu(phys_addr_t phys_end)
>
> #ifdef __aarch64__
> init_alloc_vpage((void*)(4ul << 30));
> +
> + assert_msg(system_supports_granule(PAGE_SIZE),
> + "Unsupported translation granule: %ld\n", PAGE_SIZE);
> #endif
>
> mmu_idmap = alloc_page();
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 02665b84cc7e..dc493d1686bc 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -117,5 +117,21 @@ static inline u64 get_ctr(void)
>
> extern u32 dcache_line_size;
>
> +static inline unsigned long get_id_aa64mmfr0_el1(void)
> +{
> + unsigned long mmfr0;
> + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
> + return mmfr0;
I think we can safely use read_sysreg here, the other functions in this file do.
> +}
> +
> +static inline bool system_supports_granule(size_t granule)
> +{
> + u64 mmfr0 = get_id_aa64mmfr0_el1();
> +
> + return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
> + (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
> + (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
> +}
Or we can turn it into a switch statement and keep all the field defines. Either
way looks good to me (funny how tgran16 stands out).
Thanks,
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule
2020-11-03 16:21 ` Alexandru Elisei
@ 2020-11-03 17:14 ` Nikos Nikoleris
0 siblings, 0 replies; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-03 17:14 UTC (permalink / raw)
To: Alexandru Elisei, kvm
Cc: mark.rutland, jade.alglave, luc.maranget, andre.przywara, drjones
Hi Alex,
On 03/11/2020 16:21, Alexandru Elisei wrote:
> Hi Nikos,
>
> In the subject, you forgot increment the series version to v2. Not a big deal, but
> it makes it obvious which is the latest version.
>
Sorry about that, I will fix in v3.
> kvm-unit-tests hangs when using 16k pages, I believe it's because we're using a
> block mapping at the wrong level; comments below.
>
> On 11/2/20 11:34 AM, Nikos Nikoleris wrote:
>> Make the translation granule configurable for arm64. arm64 supports
>> page sizes of 4K, 16K and 64K. By default, arm64 is configured with
>> 64K pages. configure has been extended with a new argument:
>>
>> --page-shift=(12|14|16)
>>
>> which allows the user to set the page shift and therefore the page
>> size for arm64. Using the --page-shift for any other architecture
>> results an error message.
>>
>> To allow for smaller page sizes and 42b VA, this change adds support
>> for 4-level and 3-level page tables. At compile time, we determine how
>> many levels in the page tables we needed.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>> configure | 26 +++++++++++++
>> lib/arm/asm/page.h | 4 ++
>> lib/arm/asm/pgtable-hwdef.h | 4 ++
>> lib/arm/asm/pgtable.h | 6 +++
>> lib/arm/asm/thread_info.h | 4 +-
>> lib/arm64/asm/page.h | 25 ++++++++++---
>> lib/arm64/asm/pgtable-hwdef.h | 38 +++++++++++++------
>> lib/arm64/asm/pgtable.h | 69 +++++++++++++++++++++++++++++++++--
>> lib/arm/mmu.c | 26 ++++++++-----
>> arm/cstart64.S | 10 ++++-
>> 10 files changed, 180 insertions(+), 32 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 706aab5..9c6bb2c 100755
>> --- a/configure
>> +++ b/configure
>> @@ -25,6 +25,7 @@ vmm="qemu"
>> errata_force=0
>> erratatxt="$srcdir/errata.txt"
>> host_key_document=
>> +page_size=
>>
>> usage() {
>> cat <<-EOF
>> @@ -50,6 +51,8 @@ usage() {
>> --host-key-document=HOST_KEY_DOCUMENT
>> Specify the machine-specific host-key document for creating
>> a PVM image with 'genprotimg' (s390x only)
>> + --page-size=PAGE_SIZE
>> + Specify the page size (translation granule) (arm64 only)
>
> It would be nice to have an example here regarding the expected format for
> PAGE_SIZE, maybe something similar to:
>
> "Specify the page size (translation granule) (4k, 16k or 64k, default is 64k,
> arm64 only)"
>
> The page size ends up being transformed into an integer, so it will be less likely
> for an user to be confused about the input format.
>
>> EOF
>> exit 1
>> }
>> @@ -105,6 +108,9 @@ while [[ "$1" = -* ]]; do
>> --host-key-document)
>> host_key_document="$arg"
>> ;;
>> + --page-size)
>> + page_size="$arg"
>> + ;;
>> --help)
>> usage
>> ;;
>> @@ -123,6 +129,25 @@ arch_name=$arch
>> [ "$arch" = "aarch64" ] && arch="arm64"
>> [ "$arch_name" = "arm64" ] && arch_name="aarch64"
>>
>> +if [ -z "$page_size" ]; then
>> + [ "$arch" = "arm64" ] && page_size="65536"
>> + [ "$arch" = "arm" ] && page_size="4096"
>> +else
>> + if [ "$arch" != "arm64" ]; then
>> + echo "--page-size is not supported for $arch"
>> + usage
>> + fi
>> +
>> + if [ "${page_size: -1}" = "K" ] || [ "${page_size: -1}" = "k" ]; then
>> + page_size=$[ ${page_size%?} * 1024 ]
>> + fi
>> + if [ "$page_size" != "4096" ] && [ "$page_size" != "16384" ] &&
>> + [ "$page_size" != "65536" ]; then
>> + echo "arm64 doesn't support page size of $page_size"
>> + usage
>> + fi
>> +fi
>> +
>> [ -z "$processor" ] && processor="$arch"
>>
>> if [ "$processor" = "arm64" ]; then
>> @@ -254,6 +279,7 @@ cat <<EOF >> lib/config.h
>>
>> #define CONFIG_UART_EARLY_BASE ${arm_uart_early_addr}
>> #define CONFIG_ERRATA_FORCE ${errata_force}
>> +#define CONFIG_PAGE_SIZE ${page_size}
>>
>> EOF
>> fi
>> diff --git a/lib/arm/asm/page.h b/lib/arm/asm/page.h
>> index 039c9f7..ae0ac2c 100644
>> --- a/lib/arm/asm/page.h
>> +++ b/lib/arm/asm/page.h
>> @@ -29,6 +29,10 @@ typedef struct { pteval_t pgprot; } pgprot_t;
>> #define pgd_val(x) ((x).pgd)
>> #define pgprot_val(x) ((x).pgprot)
>>
>> +/* For compatibility with arm64 page tables */
>> +#define pud_t pgd_t
>> +#define pud_val(x) pgd_val(x)
>> +
>> #define __pte(x) ((pte_t) { (x) } )
>> #define __pmd(x) ((pmd_t) { (x) } )
>> #define __pgd(x) ((pgd_t) { (x) } )
>> diff --git a/lib/arm/asm/pgtable-hwdef.h b/lib/arm/asm/pgtable-hwdef.h
>> index 4107e18..fe1d854 100644
>> --- a/lib/arm/asm/pgtable-hwdef.h
>> +++ b/lib/arm/asm/pgtable-hwdef.h
>> @@ -19,6 +19,10 @@
>> #define PTRS_PER_PTE 512
>> #define PTRS_PER_PMD 512
>>
>> +/* For compatibility with arm64 page tables */
>> +#define PUD_SIZE PGDIR_SIZE
>> +#define PUD_MASK PGDIR_MASK
>> +
>> #define PMD_SHIFT 21
>> #define PMD_SIZE (_AC(1,UL) << PMD_SHIFT)
>> #define PMD_MASK (~((1 << PMD_SHIFT) - 1))
>> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
>> index 078dd16..4759d82 100644
>> --- a/lib/arm/asm/pgtable.h
>> +++ b/lib/arm/asm/pgtable.h
>> @@ -53,6 +53,12 @@ static inline pmd_t *pgd_page_vaddr(pgd_t pgd)
>> return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
>> }
>>
>> +/* For compatibility with arm64 page tables */
>> +#define pud_valid(pud) pgd_valid(pud)
>> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
>> +#define pud_free(pud)
>> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
>> +
>> #define pmd_index(addr) \
>> (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
>> #define pmd_offset(pgd, addr) \
>> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
>> index 80ab395..eaa7258 100644
>> --- a/lib/arm/asm/thread_info.h
>> +++ b/lib/arm/asm/thread_info.h
>> @@ -14,10 +14,12 @@
>> #define THREAD_SHIFT PAGE_SHIFT
>> #define THREAD_SIZE PAGE_SIZE
>> #define THREAD_MASK PAGE_MASK
>> +#define THREAD_ALIGNMENT PAGE_SIZE
>> #else
>> #define THREAD_SHIFT MIN_THREAD_SHIFT
>> #define THREAD_SIZE (_AC(1,UL) << THREAD_SHIFT)
>> #define THREAD_MASK (~(THREAD_SIZE-1))
>> +#define THREAD_ALIGNMENT THREAD_SIZE
>> #endif
>>
>> #ifndef __ASSEMBLY__
>> @@ -38,7 +40,7 @@
>>
>> static inline void *thread_stack_alloc(void)
>> {
>> - void *sp = memalign(PAGE_SIZE, THREAD_SIZE);
>> + void *sp = memalign(THREAD_ALIGNMENT, THREAD_SIZE);
>> return sp + THREAD_START_SP;
>> }
>>
>> diff --git a/lib/arm64/asm/page.h b/lib/arm64/asm/page.h
>> index 46af552..2a06207 100644
>> --- a/lib/arm64/asm/page.h
>> +++ b/lib/arm64/asm/page.h
>> @@ -10,38 +10,51 @@
>> * This work is licensed under the terms of the GNU GPL, version 2.
>> */
>>
>> +#include <config.h>
>> #include <linux/const.h>
>>
>> -#define PGTABLE_LEVELS 2
>> #define VA_BITS 42
>>
>> +#define PAGE_SIZE CONFIG_PAGE_SIZE
>> +#if PAGE_SIZE == 65536
>> #define PAGE_SHIFT 16
>> -#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
>> +#elif PAGE_SIZE == 16384
>> +#define PAGE_SHIFT 14
>> +#elif PAGE_SIZE == 4096
>> +#define PAGE_SHIFT 12
>> +#else
>> +#error Unsupported PAGE_SIZE
>> +#endif
>> #define PAGE_MASK (~(PAGE_SIZE-1))
>>
>> +#define PGTABLE_LEVELS (((VA_BITS) - 4) / (PAGE_SHIFT - 3))
>> +
>> #ifndef __ASSEMBLY__
>>
>> #define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
>>
>> typedef u64 pteval_t;
>> typedef u64 pmdval_t;
>> +typedef u64 pudval_t;
>> typedef u64 pgdval_t;
>> typedef struct { pteval_t pte; } pte_t;
>> +typedef struct { pmdval_t pmd; } pmd_t;
>> +typedef struct { pudval_t pud; } pud_t;
>> typedef struct { pgdval_t pgd; } pgd_t;
>> typedef struct { pteval_t pgprot; } pgprot_t;
>>
>> #define pte_val(x) ((x).pte)
>> +#define pmd_val(x) ((x).pmd)
>> +#define pud_val(x) ((x).pud)
>> #define pgd_val(x) ((x).pgd)
>> #define pgprot_val(x) ((x).pgprot)
>>
>> #define __pte(x) ((pte_t) { (x) } )
>> +#define __pmd(x) ((pmd_t) { (x) } )
>> +#define __pud(x) ((pud_t) { (x) } )
>> #define __pgd(x) ((pgd_t) { (x) } )
>> #define __pgprot(x) ((pgprot_t) { (x) } )
>>
>> -typedef struct { pgd_t pgd; } pmd_t;
>> -#define pmd_val(x) (pgd_val((x).pgd))
>> -#define __pmd(x) ((pmd_t) { __pgd(x) } )
>> -
>> #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
>> #define __pa(x) __virt_to_phys((unsigned long)(x))
>>
>> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
>> index 3352489..3b6b0d6 100644
>> --- a/lib/arm64/asm/pgtable-hwdef.h
>> +++ b/lib/arm64/asm/pgtable-hwdef.h
>> @@ -9,38 +9,54 @@
>> * This work is licensed under the terms of the GNU GPL, version 2.
>> */
>>
>> +#include <asm/page.h>
>> +
>> #define UL(x) _AC(x, UL)
>>
>> +#define PGTABLE_LEVEL_SHIFT(n) ((PAGE_SHIFT - 3) * (4 - (n)) + 3)
>> #define PTRS_PER_PTE (1 << (PAGE_SHIFT - 3))
>>
>> +#if PGTABLE_LEVELS > 2
>> +#define PMD_SHIFT PGTABLE_LEVEL_SHIFT(2)
>> +#define PTRS_PER_PMD PTRS_PER_PTE
>> +#define PMD_SIZE (UL(1) << PMD_SHIFT)
>> +#define PMD_MASK (~(PMD_SIZE-1))
>> +#endif
>
> For consistency, I think we should have these definitions even when PGTABLE_LEVELS
> <= 2, same as we do for puds below.
>
>> +
>> +#if PGTABLE_LEVELS > 3
>> +#define PUD_SHIFT PGTABLE_LEVEL_SHIFT(1)
>> +#define PTRS_PER_PUD PTRS_PER_PTE
>> +#define PUD_SIZE (UL(1) << PUD_SHIFT)
>> +#define PUD_MASK (~(PUD_SIZE-1))
>> +#else
>> +#define PUD_SIZE PGDIR_SIZE
>> +#define PUD_MASK PGDIR_MASK
>> +#endif
>> +
>> +#define PUD_VALID (_AT(pudval_t, 1) << 0)
>> +
>> /*
>> * PGDIR_SHIFT determines the size a top-level page table entry can map
>> * (depending on the configuration, this level can be 0, 1 or 2).
>> */
>> -#define PGDIR_SHIFT ((PAGE_SHIFT - 3) * PGTABLE_LEVELS + 3)
>> +#define PGDIR_SHIFT PGTABLE_LEVEL_SHIFT(4 - PGTABLE_LEVELS)
>> #define PGDIR_SIZE (_AC(1, UL) << PGDIR_SHIFT)
>> #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
>> -#define PMD_SIZE (UL(1) << PMD_SHIFT)
>> -#define PMD_MASK (~(PMD_SIZE-1))
>> -
>> /*
>> * Section address mask and size definitions.
>> */
>> -#define SECTION_SHIFT PMD_SHIFT
>> -#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
>> -#define SECTION_MASK (~(SECTION_SIZE-1))
>> +#define SECTION_SHIFT PMD_SHIFT
>> +#define SECTION_SIZE (_AC(1, UL) << SECTION_SHIFT)
>> +#define SECTION_MASK (~(SECTION_SIZE-1))
>>
>> /*
>> * Hardware page table definitions.
>> *
>> - * Level 1 descriptor (PMD).
>> + * Level 0,1,2 descriptor (PGD, PUD and PMD).
>> */
>> #define PMD_TYPE_MASK (_AT(pmdval_t, 3) << 0)
>> #define PMD_TYPE_FAULT (_AT(pmdval_t, 0) << 0)
>> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
>> index e577d9c..c7632ae 100644
>> --- a/lib/arm64/asm/pgtable.h
>> +++ b/lib/arm64/asm/pgtable.h
>> @@ -30,10 +30,12 @@
>> #define pgtable_pa(x) ((unsigned long)(x))
>>
>> #define pgd_none(pgd) (!pgd_val(pgd))
>> +#define pud_none(pud) (!pud_val(pud))
>> #define pmd_none(pmd) (!pmd_val(pmd))
>> #define pte_none(pte) (!pte_val(pte))
>>
>> #define pgd_valid(pgd) (pgd_val(pgd) & PGD_VALID)
>> +#define pud_valid(pud) (pud_val(pud) & PUD_VALID)
>> #define pmd_valid(pmd) (pmd_val(pmd) & PMD_SECT_VALID)
>> #define pte_valid(pte) (pte_val(pte) & PTE_VALID)
>>
>> @@ -52,15 +54,76 @@ static inline pgd_t *pgd_alloc(void)
>> return pgd;
>> }
>>
>> -#define pmd_offset(pgd, addr) ((pmd_t *)pgd)
>> -#define pmd_free(pmd)
>> -#define pmd_alloc(pgd, addr) pmd_offset(pgd, addr)
>> +static inline pud_t *pgd_page_vaddr(pgd_t pgd)
>> +{
>> + return pgtable_va(pgd_val(pgd) & PHYS_MASK & (s32)PAGE_MASK);
>
> Aren't all numbers treated by gcc like integers? Which means that for arm64 they
> are s32 already? I see the cast was present in pte_page_vaddr(), am I missing
> something?
>
Here, we cast to signed 32bit, then there is an implicit sign extention
to 64bit. This is the safest way to mask out irrenevant bits. PAGE_MASK
is unsigned 64bit (UL) so it would work anyway.
>> +}
>> +
>> +static inline pmd_t *pud_page_vaddr(pud_t pud)
>> +{
>> + return pgtable_va(pud_val(pud) & PHYS_MASK & (s32)PAGE_MASK);
>> +}
>> +
>
> Stray newline.
>
>>
>> static inline pte_t *pmd_page_vaddr(pmd_t pmd)
>> {
>> return pgtable_va(pmd_val(pmd) & PHYS_MASK & (s32)PAGE_MASK);
>> }
>>
>> +#if PGTABLE_LEVELS > 2
>> +#define pmd_index(addr) \
>> + (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
>> +#define pmd_offset(pud, addr) \
>> + (pud_page_vaddr(*(pud)) + pmd_index(addr))
>> +#define pmd_free(pmd) free_page(pmd)
>> +static inline pmd_t *pmd_alloc_one(void)
>> +{
>> + assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
>> + pmd_t *pmd = alloc_page();
>> + return pmd;
>> +}
>> +static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
>> +{
>> + if (pud_none(*pud)) {
>> + pud_t entry;
>> + pud_val(entry) = pgtable_pa(pmd_alloc_one()) | PMD_TYPE_TABLE;
>> + WRITE_ONCE(*pud, entry);
>> + }
>> + return pmd_offset(pud, addr);
>> +}
>> +#else
>> +#define pmd_offset(pud, addr) ((pmd_t *)pud)
>> +#define pmd_free(pmd)
>> +#define pmd_alloc(pud, addr) pmd_offset(pud, addr)
>> +#endif
>> +
>> +#if PGTABLE_LEVELS > 3
>> +#define pud_index(addr) \
>> + (((addr) >> PUD_SHIFT) & (PTRS_PER_PUD - 1))
>> +#define pud_offset(pgd, addr) \
>> + (pgd_page_vaddr(*(pgd)) + pud_index(addr))
>> +#define pud_free(pud) free_page(pud)
>> +static inline pud_t *pud_alloc_one(void)
>> +{
>> + assert(PTRS_PER_PMD * sizeof(pud_t) == PAGE_SIZE);
>> + pud_t *pud = alloc_page();
>> + return pud;
>> +}
>> +static inline pud_t *pud_alloc(pgd_t *pgd, unsigned long addr)
>> +{
>> + if (pgd_none(*pgd)) {
>> + pgd_t entry;
>> + pgd_val(entry) = pgtable_pa(pud_alloc_one()) | PMD_TYPE_TABLE;
>> + WRITE_ONCE(*pgd, entry);
>> + }
>> + return pud_offset(pgd, addr);
>> +}
>> +#else
>> +#define pud_offset(pgd, addr) ((pud_t *)pgd)
>> +#define pud_free(pud)
>> +#define pud_alloc(pgd, addr) pud_offset(pgd, addr)
>> +#endif
>> +
>> #define pte_index(addr) \
>> (((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
>> #define pte_offset(pmd, addr) \
>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>> index 540a1e8..6d1c75b 100644
>> --- a/lib/arm/mmu.c
>> +++ b/lib/arm/mmu.c
>> @@ -81,7 +81,8 @@ void mmu_disable(void)
>> static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
>> {
>> pgd_t *pgd = pgd_offset(pgtable, vaddr);
>> - pmd_t *pmd = pmd_alloc(pgd, vaddr);
>> + pud_t *pud = pud_alloc(pgd, vaddr);
>> + pmd_t *pmd = pmd_alloc(pud, vaddr);
>> pte_t *pte = pte_alloc(pmd, vaddr);
>>
>> return &pte_val(*pte);
>> @@ -133,18 +134,20 @@ void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
>> phys_addr_t phys_start, phys_addr_t phys_end,
>> pgprot_t prot)
>> {
>> - phys_addr_t paddr = phys_start & PGDIR_MASK;
>> - uintptr_t vaddr = virt_offset & PGDIR_MASK;
>> + phys_addr_t paddr = phys_start & PUD_MASK;
>> + uintptr_t vaddr = virt_offset & PUD_MASK;
>> uintptr_t virt_end = phys_end - paddr + vaddr;
>> pgd_t *pgd;
>> - pgd_t entry;
>> + pud_t *pud;
>> + pud_t entry;
>>
>> - for (; vaddr < virt_end; vaddr += PGDIR_SIZE, paddr += PGDIR_SIZE) {
>> - pgd_val(entry) = paddr;
>> - pgd_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
>> - pgd_val(entry) |= pgprot_val(prot);
>> + for (; vaddr < virt_end; vaddr += PUD_SIZE, paddr += PUD_SIZE) {
>> + pud_val(entry) = paddr;
>> + pud_val(entry) |= PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S;
>> + pud_val(entry) |= pgprot_val(prot);
>> pgd = pgd_offset(pgtable, vaddr);
>> - WRITE_ONCE(*pgd, entry);
>> + pud = pud_alloc(pgd, vaddr);
>> + WRITE_ONCE(*pud, entry);
>
> I'm looking at Figure D5-9, page D5-2568 from D5-2568, and it looks to me like for
> 16k pages the minimum level where a block mapping is allowed is level 2 (aka pmd);
> for 16k pages I get PGTABLE_LEVELS=3, which means the code will created a block
> mapping at the pud level. Judging from figure D5-12, 64k pages have the same
> requirement (unless ARMv8.2-LPA is implemented), but we don't run into this issue
> there because PGTABLE_LEVELS=2 in that case.
>
> I tried running a test using qemu TCG with --page-size=16k and the test hangs
> (didn't investigate exactly where):
For me TCG uses a cortex-a57 cpu which doesn't support 16K. With the
other patch I hit the assertion.
Thanks,
Nikos
>
> $ ./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu- --page-size=64k &&
> make -j24 && arm/run arm/cache.flat
>
> Thanks,
>
> Alex
>
>> flush_tlb_page(vaddr);
>> }
>> }
>> @@ -207,6 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
>> void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>> {
>> pgd_t *pgd;
>> + pud_t *pud;
>> pmd_t *pmd;
>> pte_t *pte;
>>
>> @@ -215,7 +219,9 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>>
>> pgd = pgd_offset(pgtable, vaddr);
>> assert(pgd_valid(*pgd));
>> - pmd = pmd_offset(pgd, vaddr);
>> + pud = pud_offset(pgd, vaddr);
>> + assert(pud_valid(*pud));
>> + pmd = pmd_offset(pud, vaddr);
>> assert(pmd_valid(*pmd));
>>
>> if (pmd_huge(*pmd)) {
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index ffdd49f..cedc678 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -157,6 +157,14 @@ halt:
>> */
>> #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>>
>> +#if PAGE_SIZE == 65536
>> +#define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
>> +#elif PAGE_SIZE == 16384
>> +#define TCR_TG_FLAGS TCR_TG0_16K | TCR_TG1_16K
>> +#elif PAGE_SIZE == 4096
>> +#define TCR_TG_FLAGS TCR_TG0_4K | TCR_TG1_4K
>> +#endif
>> +
>> .globl asm_mmu_enable
>> asm_mmu_enable:
>> tlbi vmalle1 // invalidate I + D TLBs
>> @@ -164,7 +172,7 @@ asm_mmu_enable:
>>
>> /* TCR */
>> ldr x1, =TCR_TxSZ(VA_BITS) | \
>> - TCR_TG0_64K | TCR_TG1_64K | \
>> + TCR_TG_FLAGS | \
>> TCR_IRGN_WBWA | TCR_ORGN_WBWA | \
>> TCR_SHARED
>> mrs x2, id_aa64mmfr0_el1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-03 17:03 ` Alexandru Elisei
@ 2020-11-03 17:36 ` Andrew Jones
2020-11-03 18:14 ` Nikos Nikoleris
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Jones @ 2020-11-03 17:36 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Nikos Nikoleris, kvm, mark.rutland, jade.alglave, luc.maranget,
andre.przywara
On Tue, Nov 03, 2020 at 05:03:15PM +0000, Alexandru Elisei wrote:
> Hi,
>
> On 11/3/20 10:02 AM, Andrew Jones wrote:
> > On Mon, Nov 02, 2020 at 11:34:44AM +0000, Nikos Nikoleris wrote:
> >> Now that we can change the translation granule at will, and since
> >> arm64 implementations can support a subset of the architecturally
> >> defined granules, we need to check and warn the user if the configured
> >> granule is not supported.
> > nit: it'd be better for this patch to come before the last patch.
> >
> >> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> >> ---
> >> lib/arm64/asm/processor.h | 65 +++++++++++++++++++++++++++++++++++++++
> >> lib/arm/mmu.c | 3 ++
> >> 2 files changed, 68 insertions(+)
> >>
> >> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> >> index 02665b8..0eac928 100644
> >> --- a/lib/arm64/asm/processor.h
> >> +++ b/lib/arm64/asm/processor.h
> >> @@ -117,5 +117,70 @@ static inline u64 get_ctr(void)
> >>
> >> extern u32 dcache_line_size;
> >>
> >> +static inline unsigned long get_id_aa64mmfr0_el1(void)
> >> +{
> >> + unsigned long mmfr0;
> >> + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
> >> + return mmfr0;
> >> +}
> >> +
> >> +/* From arch/arm64/include/asm/cpufeature.h */
> >> +static inline unsigned int
> >> +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
> >> +{
> >> + return (u64)(features << (64 - width - field)) >> (64 - width);
> >> +}
> >> +
> >> +#define ID_AA64MMFR0_TGRAN4_SHIFT 28
> >> +#define ID_AA64MMFR0_TGRAN64_SHIFT 24
> >> +#define ID_AA64MMFR0_TGRAN16_SHIFT 20
> >> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
> >> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
> >> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
> >> +
> >> +static inline bool system_supports_64kb_granule(void)
> >> +{
> >> + u64 mmfr0;
> >> + u32 val;
> >> +
> >> + mmfr0 = get_id_aa64mmfr0_el1();
> >> + val = cpuid_feature_extract_unsigned_field_width(
> >> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT,4);
> >> +
> >> + return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
> >> +}
> >> +
> >> +static inline bool system_supports_16kb_granule(void)
> >> +{
> >> + u64 mmfr0;
> >> + u32 val;
> >> +
> >> + mmfr0 = get_id_aa64mmfr0_el1();
> >> + val = cpuid_feature_extract_unsigned_field_width(
> >> + mmfr0, ID_AA64MMFR0_TGRAN16_SHIFT, 4);
> >> +
> >> + return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
> >> +}
> >> +
> >> +static inline bool system_supports_4kb_granule(void)
> >> +{
> >> + u64 mmfr0;
> >> + u32 val;
> >> +
> >> + mmfr0 = get_id_aa64mmfr0_el1();
> >> + val = cpuid_feature_extract_unsigned_field_width(
> >> + mmfr0, ID_AA64MMFR0_TGRAN4_SHIFT, 4);
> >> +
> >> + return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
> >> +}
> >> +
> >> +#if PAGE_SIZE == 65536
> >> +#define system_supports_configured_granule system_supports_64kb_granule
> >> +#elif PAGE_SIZE == 16384
> >> +#define system_supports_configured_granule system_supports_16kb_granule
> >> +#elif PAGE_SIZE == 4096
> >> +#define system_supports_configured_granule system_supports_4kb_granule
> >> +#endif
> >> +
> >> #endif /* !__ASSEMBLY__ */
> >> #endif /* _ASMARM64_PROCESSOR_H_ */
> >> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> >> index 6d1c75b..51fa745 100644
> >> --- a/lib/arm/mmu.c
> >> +++ b/lib/arm/mmu.c
> >> @@ -163,6 +163,9 @@ void *setup_mmu(phys_addr_t phys_end)
> >>
> >> #ifdef __aarch64__
> >> init_alloc_vpage((void*)(4ul << 30));
> >> +
> >> + assert_msg(system_supports_configured_granule(),
> >> + "Unsupported translation granule %d\n", PAGE_SIZE);
> > ^
> > needs '%ld' to compile
> >> #endif
> >>
> >> mmu_idmap = alloc_page();
> >> --
> >> 2.17.1
> >>
> > I don't think we need the three separate functions. How about just
> > doing the following diff?
> >
> > Thanks,
> > drew
> >
> >
> > diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> > index 540a1e842d5b..fef62f5a9866 100644
> > --- a/lib/arm/mmu.c
> > +++ b/lib/arm/mmu.c
> > @@ -160,6 +160,9 @@ void *setup_mmu(phys_addr_t phys_end)
> >
> > #ifdef __aarch64__
> > init_alloc_vpage((void*)(4ul << 30));
> > +
> > + assert_msg(system_supports_granule(PAGE_SIZE),
> > + "Unsupported translation granule: %ld\n", PAGE_SIZE);
> > #endif
> >
> > mmu_idmap = alloc_page();
> > diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> > index 02665b84cc7e..dc493d1686bc 100644
> > --- a/lib/arm64/asm/processor.h
> > +++ b/lib/arm64/asm/processor.h
> > @@ -117,5 +117,21 @@ static inline u64 get_ctr(void)
> >
> > extern u32 dcache_line_size;
> >
> > +static inline unsigned long get_id_aa64mmfr0_el1(void)
> > +{
> > + unsigned long mmfr0;
> > + asm volatile("mrs %0, id_aa64mmfr0_el1" : "=r" (mmfr0));
> > + return mmfr0;
>
> I think we can safely use read_sysreg here, the other functions in this file do.
Agreed
>
> > +}
> > +
> > +static inline bool system_supports_granule(size_t granule)
> > +{
> > + u64 mmfr0 = get_id_aa64mmfr0_el1();
> > +
> > + return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
> > + (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
> > + (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
> > +}
>
> Or we can turn it into a switch statement and keep all the field defines. Either
> way looks good to me (funny how tgran16 stands out).
>
Keeping the defines is probably a good idea. Whether the function uses
a switch or an expression like above doesn't matter to me much. Keeping
LOC down in the lib/ code is a goal of kvm-unit-tests, but so is
readabilty. If the switch looks better, then let's go that way.
Thanks,
drew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-03 17:36 ` Andrew Jones
@ 2020-11-03 18:14 ` Nikos Nikoleris
2020-11-03 18:23 ` Andrew Jones
0 siblings, 1 reply; 17+ messages in thread
From: Nikos Nikoleris @ 2020-11-03 18:14 UTC (permalink / raw)
To: Andrew Jones, Alexandru Elisei
Cc: kvm, mark.rutland, jade.alglave, luc.maranget, andre.przywara
On 03/11/2020 17:36, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 05:03:15PM +0000, Alexandru Elisei wrote:
>>> +}
>>> +
>>> +static inline bool system_supports_granule(size_t granule)
>>> +{
>>> + u64 mmfr0 = get_id_aa64mmfr0_el1();
>>> +
>>> + return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
>>> + (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
>>> + (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
>>> +}
>>
>> Or we can turn it into a switch statement and keep all the field defines. Either
>> way looks good to me (funny how tgran16 stands out).
>>
>
> Keeping the defines is probably a good idea. Whether the function uses
> a switch or an expression like above doesn't matter to me much. Keeping
> LOC down in the lib/ code is a goal of kvm-unit-tests, but so is
> readabilty. If the switch looks better, then let's go that way.
>
I liked Drew's version in that it was very concise. The new version will
be much longer. If you think it's more readable I'll use that instead.
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 02665b8..430ded3 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -117,5 +117,38 @@ static inline u64 get_ctr(void)
extern u32 dcache_line_size;
+static inline unsigned long get_id_aa64mmfr0_el1(void)
+{
+ return read_sysreg(id_aa64mmfr0_el1);
+}
+
+#define ID_AA64MMFR0_TGRAN4_SHIFT 28
+#define ID_AA64MMFR0_TGRAN64_SHIFT 24
+#define ID_AA64MMFR0_TGRAN16_SHIFT 20
+
+#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
+#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
+#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
+
+static inline bool system_supports_granule(size_t granule)
+{
+ u32 shift;
+ u32 val;
+ u64 mmfr0 = get_id_aa64mmfr0_el1();
+ if (granule == SZ_4K) {
+ shift = ID_AA64MMFR0_TGRAN4_SHIFT;
+ val = ID_AA64MMFR0_TGRAN4_SUPPORTED;
+ } else if (granule == SZ_16K) {
+ shift = ID_AA64MMFR0_TGRAN16_SHIFT;
+ val = ID_AA64MMFR0_TGRAN16_SUPPORTED;
+ } else {
+ assert(granule == SZ_64K);
+ shift = ID_AA64MMFR0_TGRAN64_SHIFT;
+ val = ID_AA64MMFR0_TGRAN64_SUPPORTED;
+ }
+
+ return ((mmfr0 >> shift) & 0xf) == val;
+}
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASMARM64_PROCESSOR_H_ */
Thanks,
Nikos
> Thanks,
> drew
>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported
2020-11-03 18:14 ` Nikos Nikoleris
@ 2020-11-03 18:23 ` Andrew Jones
0 siblings, 0 replies; 17+ messages in thread
From: Andrew Jones @ 2020-11-03 18:23 UTC (permalink / raw)
To: Nikos Nikoleris
Cc: Alexandru Elisei, kvm, mark.rutland, jade.alglave, luc.maranget,
andre.przywara
On Tue, Nov 03, 2020 at 06:14:53PM +0000, Nikos Nikoleris wrote:
> On 03/11/2020 17:36, Andrew Jones wrote:
> > On Tue, Nov 03, 2020 at 05:03:15PM +0000, Alexandru Elisei wrote:
> > > > +}
> > > > +
> > > > +static inline bool system_supports_granule(size_t granule)
> > > > +{
> > > > + u64 mmfr0 = get_id_aa64mmfr0_el1();
> > > > +
> > > > + return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
> > > > + (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
> > > > + (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
> > > > +}
> > >
> > > Or we can turn it into a switch statement and keep all the field defines. Either
> > > way looks good to me (funny how tgran16 stands out).
> > >
> >
> > Keeping the defines is probably a good idea. Whether the function uses
> > a switch or an expression like above doesn't matter to me much. Keeping
> > LOC down in the lib/ code is a goal of kvm-unit-tests, but so is
> > readabilty. If the switch looks better, then let's go that way.
> >
>
> I liked Drew's version in that it was very concise. The new version will be
> much longer. If you think it's more readable I'll use that instead.
>
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 02665b8..430ded3 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -117,5 +117,38 @@ static inline u64 get_ctr(void)
>
> extern u32 dcache_line_size;
>
> +static inline unsigned long get_id_aa64mmfr0_el1(void)
> +{
> + return read_sysreg(id_aa64mmfr0_el1);
> +}
> +
> +#define ID_AA64MMFR0_TGRAN4_SHIFT 28
> +#define ID_AA64MMFR0_TGRAN64_SHIFT 24
> +#define ID_AA64MMFR0_TGRAN16_SHIFT 20
> +
> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
> +
> +static inline bool system_supports_granule(size_t granule)
> +{
> + u32 shift;
> + u32 val;
> + u64 mmfr0 = get_id_aa64mmfr0_el1();
> + if (granule == SZ_4K) {
> + shift = ID_AA64MMFR0_TGRAN4_SHIFT;
> + val = ID_AA64MMFR0_TGRAN4_SUPPORTED;
> + } else if (granule == SZ_16K) {
> + shift = ID_AA64MMFR0_TGRAN16_SHIFT;
> + val = ID_AA64MMFR0_TGRAN16_SUPPORTED;
> + } else {
> + assert(granule == SZ_64K);
> + shift = ID_AA64MMFR0_TGRAN64_SHIFT;
> + val = ID_AA64MMFR0_TGRAN64_SUPPORTED;
> + }
> +
> + return ((mmfr0 >> shift) & 0xf) == val;
> +}
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASMARM64_PROCESSOR_H_ */
>
I'm happy with it either way.
Thanks,
drew
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-11-03 18:23 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 11:34 [kvm-unit-tests PATCH 0/2] arm64: Add support for configuring the translation granule Nikos Nikoleris
2020-11-02 11:34 ` [kvm-unit-tests PATCH 1/2] " Nikos Nikoleris
2020-11-03 13:04 ` Andrew Jones
2020-11-03 15:49 ` Nikos Nikoleris
2020-11-03 16:10 ` Andrew Jones
2020-11-03 16:25 ` Alexandru Elisei
2020-11-03 16:39 ` Andrew Jones
2020-11-03 16:46 ` Alexandru Elisei
2020-11-03 16:21 ` Alexandru Elisei
2020-11-03 17:14 ` Nikos Nikoleris
2020-11-02 11:34 ` [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported Nikos Nikoleris
2020-11-03 10:02 ` Andrew Jones
2020-11-03 10:21 ` Nikos Nikoleris
2020-11-03 17:03 ` Alexandru Elisei
2020-11-03 17:36 ` Andrew Jones
2020-11-03 18:14 ` Nikos Nikoleris
2020-11-03 18:23 ` Andrew Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).