[kvm-unit-tests PATCH 0/2] arm64: Add support for configuring the translation granule

[kvm-unit-tests PATCH 0/2] arm64: Add support for configuring the translation granule

[Nikos Nikoleris]

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

[kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Nikos Nikoleris]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Nikos Nikoleris]

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
> 

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Alexandru Elisei]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Alexandru Elisei]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Alexandru Elisei]

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

Re: [kvm-unit-tests PATCH 1/2] arm64: Add support for configuring the translation granule

[Nikos Nikoleris]

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

[kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Nikos Nikoleris]

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

Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Andrew Jones]

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_ */

Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Nikos Nikoleris]

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_ */
> 

Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Alexandru Elisei]

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

Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Nikos Nikoleris]

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
> 

Re: [kvm-unit-tests PATCH 2/2] arm64: Check if the configured translation granule is supported

[Andrew Jones]

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