* [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation
@ 2014-04-30 12:15 vijay.kilari
2014-04-30 12:39 ` Julien Grall
2014-05-02 16:09 ` Ian Campbell
0 siblings, 2 replies; 6+ messages in thread
From: vijay.kilari @ 2014-04-30 12:15 UTC (permalink / raw)
To: Ian.Campbell, julien.grall, stefano.stabellini,
stefano.stabellini, xen-devel
Cc: Prasun.Kapoor, Vijaya Kumar K, vijay.kilari
From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
To support 48-bit Physical Address support, add 4-level
page tables for stage 2 translation.By default stage 1 and
stage 2 translation at EL2 are with 4-levels
Configure TCR_EL2.IPS and VTCR_EL2.PS based on platform
supported PA range at runtime
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
xen/arch/arm/arm64/head.S | 7 ++-
xen/arch/arm/mm.c | 11 +++-
xen/arch/arm/p2m.c | 132 ++++++++++++++++++++++++++++++++++-----
xen/include/asm-arm/p2m.h | 5 +-
xen/include/asm-arm/page.h | 117 ++++++++++++++++++++++++++++++----
xen/include/asm-arm/processor.h | 14 ++++-
6 files changed, 249 insertions(+), 37 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index d151724..c0e0362 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -224,13 +224,16 @@ skip_bss:
ldr x0, =MAIRVAL
msr mair_el2, x0
+ mrs x1, ID_AA64MMFR0_EL1
+
/* Set up the HTCR:
- * PASize -- 40 bits / 1TB
+ * PASize -- based on ID_AA64MMFR0_EL1.PARange value
* Top byte is used
* PT walks use Outer-Shareable accesses,
* PT walks are write-back, write-allocate in both cache levels,
* Full 64-bit address space goes through this table. */
- ldr x0, =0x80822500
+ ldr x0, =TCR_VAL_40PA
+ bfi x0, x1, #16, #3
msr tcr_el2, x0
/* Set up the SCTLR_EL2:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 305879f..d577b23 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -382,13 +382,18 @@ void __cpuinit setup_virt_paging(void)
/* SH0=00, ORGN0=IRGN0=01
* SL0=01 (Level-1)
* ARVv7: T0SZ=(1)1000 = -8 (32-(-8) = 40 bit physical addresses)
* ARMv8: T0SZ=01 1000 = 24 (64-24 = 40 bit physical addresses)
* PS=010 == 40 bits
*/
#ifdef CONFIG_ARM_32
- WRITE_SYSREG32(0x80002558, VTCR_EL2);
+ WRITE_SYSREG32(VTCR_VAL, VTCR_EL2);
#else
- WRITE_SYSREG32(0x80022558, VTCR_EL2);
+ /* Change PS to 48 and T0SZ = 16 SL0 - 2 to take VA 48 bit */
+ if ( current_cpu_data.mm64.pa_range == VTCR_PS_48BIT )
+ WRITE_SYSREG32(VTCR_VAL_48PA, VTCR_EL2);
+ else
+ /* Consider by default 40 PA support for ARM64 */
+ WRITE_SYSREG32(VTCR_VAL_40PA, VTCR_EL2);
#endif
isb();
}
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d00c882..bdaab46 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -10,29 +10,38 @@
#include <asm/hardirq.h>
#include <asm/page.h>
+#ifdef CONFIG_ARM_64
+/* Zeroeth level is of 1 page size */
+#define P2M_FIRST_ORDER 0
+#else
/* First level P2M is 2 consecutive pages */
#define P2M_FIRST_ORDER 1
+#endif
#define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
void dump_p2m_lookup(struct domain *d, paddr_t addr)
{
struct p2m_domain *p2m = &d->arch.p2m;
- lpae_t *first;
+ lpae_t *lookup;
printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
+#ifdef CONFIG_ARM_64
+ if ( zeroeth_linear_offset(addr) > LPAE_ENTRIES )
+#else
if ( first_linear_offset(addr) > LPAE_ENTRIES )
+#endif
{
- printk("Cannot dump addresses in second of first level pages...\n");
+ printk("Cannot dump addresses in second of first(ARM32)/zeroeth(ARM64) level pages...\n");
return;
}
printk("P2M @ %p mfn:0x%lx\n",
- p2m->first_level, page_to_mfn(p2m->first_level));
+ p2m->lookup_level, page_to_mfn(p2m->lookup_level));
- first = __map_domain_page(p2m->first_level);
- dump_pt_walk(first, addr);
- unmap_domain_page(first);
+ lookup = __map_domain_page(p2m->lookup_level);
+ dump_pt_walk(lookup, addr);
+ unmap_domain_page(lookup);
}
void p2m_load_VTTBR(struct domain *d)
@@ -44,6 +53,20 @@ void p2m_load_VTTBR(struct domain *d)
isb(); /* Ensure update is visible */
}
+#ifdef CONFIG_ARM_64
+/*
+ * Map zeroeth level page that addr contains.
+ */
+static lpae_t *p2m_map_zeroeth(struct p2m_domain *p2m, paddr_t addr)
+{
+ if ( zeroeth_linear_offset(addr) >= LPAE_ENTRIES )
+ return NULL;
+
+ return __map_domain_page(p2m->lookup_level);
+}
+
+#else
+
static int p2m_first_level_index(paddr_t addr)
{
/*
@@ -64,10 +87,11 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
if ( first_linear_offset(addr) >= P2M_FIRST_ENTRIES )
return NULL;
- page = p2m->first_level + p2m_first_level_index(addr);
+ page = p2m->lookup_level + p2m_first_level_index(addr);
return __map_domain_page(page);
}
+#endif
/*
* Lookup the MFN corresponding to a domain's PFN.
@@ -79,6 +103,9 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
{
struct p2m_domain *p2m = &d->arch.p2m;
lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
+#ifdef CONFIG_ARM_64
+ lpae_t *zeroeth = NULL;
+#endif
paddr_t maddr = INVALID_PADDR;
p2m_type_t _t;
@@ -89,9 +116,29 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
spin_lock(&p2m->lock);
+#ifdef CONFIG_ARM_64
+ zeroeth = p2m_map_zeroeth(p2m, paddr);
+ if ( !zeroeth )
+ goto err;
+
+ pte = zeroeth[zeroeth_table_offset(paddr)];
+ /* Zeroeth level does not support block translation
+ * so pte.p2m.table should be always set.
+ * Just check for valid bit
+ */
+ if ( !pte.p2m.valid )
+ goto done;
+
+#else
first = p2m_map_first(p2m, paddr);
if ( !first )
goto err;
+#endif
+
+#ifdef CONFIG_ARM_64
+ /* Map first level table */
+ first = map_domain_page(pte.p2m.base);
+#endif
pte = first[first_table_offset(paddr)];
if ( !pte.p2m.valid || !pte.p2m.table )
@@ -120,6 +167,9 @@ done:
if (third) unmap_domain_page(third);
if (second) unmap_domain_page(second);
if (first) unmap_domain_page(first);
+#ifdef CONFIG_ARM_64
+ if (zeroeth) unmap_domain_page(zeroeth);
+#endif
err:
spin_unlock(&p2m->lock);
@@ -244,8 +294,14 @@ static int apply_p2m_changes(struct domain *d,
struct p2m_domain *p2m = &d->arch.p2m;
lpae_t *first = NULL, *second = NULL, *third = NULL;
paddr_t addr;
- unsigned long cur_first_page = ~0,
- cur_first_offset = ~0,
+#ifdef CONFIG_ARM_64
+ lpae_t *zeroeth = NULL;
+ unsigned long cur_zeroeth_page = ~0,
+ cur_zeroeth_offset = ~0;
+#else
+ unsigned long cur_first_page = ~0;
+#endif
+ unsigned long cur_first_offset = ~0,
cur_second_offset = ~0;
unsigned long count = 0;
unsigned int flush = 0;
@@ -260,6 +316,37 @@ static int apply_p2m_changes(struct domain *d,
addr = start_gpaddr;
while ( addr < end_gpaddr )
{
+#ifdef CONFIG_ARM_64
+ /* Find zeoeth offset and Map zeroeth page */
+ if ( cur_zeroeth_page != zeroeth_table_offset(addr) )
+ {
+ if ( zeroeth ) unmap_domain_page(zeroeth);
+ zeroeth = p2m_map_zeroeth(p2m, addr);
+ if ( !zeroeth )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+ cur_zeroeth_page = zeroeth_table_offset(addr);
+ }
+
+ if ( !zeroeth[zeroeth_table_offset(addr)].p2m.valid )
+ {
+ if ( !populate )
+ {
+ addr = (addr + ZEROETH_SIZE) & ZEROETH_MASK;
+ continue;
+ }
+ rc = p2m_create_table(d, &zeroeth[zeroeth_table_offset(addr)]);
+ if ( rc < 0 )
+ {
+ printk("p2m_populate_ram: L0 failed\n");
+ goto out;
+ }
+ }
+
+ BUG_ON(!zeroeth[zeroeth_table_offset(addr)].p2m.valid);
+#else
if ( cur_first_page != p2m_first_level_index(addr) )
{
if ( first ) unmap_domain_page(first);
@@ -271,7 +358,16 @@ static int apply_p2m_changes(struct domain *d,
}
cur_first_page = p2m_first_level_index(addr);
}
+#endif
+#ifdef CONFIG_ARM_64
+ if ( cur_zeroeth_offset != zeroeth_table_offset(addr) )
+ {
+ if ( first ) unmap_domain_page(first);
+ first = map_domain_page(zeroeth[zeroeth_table_offset(addr)].p2m.base);
+ cur_zeroeth_offset = zeroeth_table_offset(addr);
+ }
+#endif
if ( !first[first_table_offset(addr)].p2m.valid )
{
if ( !populate )
@@ -279,7 +375,6 @@ static int apply_p2m_changes(struct domain *d,
addr = (addr + FIRST_SIZE) & FIRST_MASK;
continue;
}
-
rc = p2m_create_table(d, &first[first_table_offset(addr)]);
if ( rc < 0 )
{
@@ -287,7 +382,6 @@ static int apply_p2m_changes(struct domain *d,
goto out;
}
}
-
BUG_ON(!first[first_table_offset(addr)].p2m.valid);
if ( cur_first_offset != first_table_offset(addr) )
@@ -305,7 +399,6 @@ static int apply_p2m_changes(struct domain *d,
addr = (addr + SECOND_SIZE) & SECOND_MASK;
continue;
}
-
rc = p2m_create_table(d, &second[second_table_offset(addr)]);
if ( rc < 0 ) {
printk("p2m_populate_ram: L2 failed\n");
@@ -435,6 +528,9 @@ out:
if (third) unmap_domain_page(third);
if (second) unmap_domain_page(second);
if (first) unmap_domain_page(first);
+#ifdef CONFIG_ARM_64
+ if ( zeroeth ) unmap_domain_page(zeroeth);
+#endif
if ( d != current->domain )
p2m_load_VTTBR(current->domain);
@@ -500,13 +596,15 @@ int p2m_alloc_table(struct domain *d)
clear_page(p);
unmap_domain_page(p);
+#ifdef CONFIG_ARM_32
p = __map_domain_page(page + 1);
clear_page(p);
unmap_domain_page(p);
+#endif
- p2m->first_level = page;
+ p2m->lookup_level = page;
- d->arch.vttbr = page_to_maddr(p2m->first_level)
+ d->arch.vttbr = page_to_maddr(p2m->lookup_level)
| ((uint64_t)p2m->vmid&0xff)<<48;
p2m_load_VTTBR(d);
@@ -587,9 +685,9 @@ void p2m_teardown(struct domain *d)
while ( (pg = page_list_remove_head(&p2m->pages)) )
free_domheap_page(pg);
- free_domheap_pages(p2m->first_level, P2M_FIRST_ORDER);
+ free_domheap_pages(p2m->lookup_level, P2M_FIRST_ORDER);
- p2m->first_level = NULL;
+ p2m->lookup_level = NULL;
p2m_free_vmid(d);
@@ -613,7 +711,7 @@ int p2m_init(struct domain *d)
d->arch.vttbr = 0;
- p2m->first_level = NULL;
+ p2m->lookup_level = NULL;
p2m->max_mapped_gfn = 0;
p2m->lowest_mapped_gfn = ULONG_MAX;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3b39c45..3aa3623 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -13,8 +13,9 @@ struct p2m_domain {
/* Pages used to construct the p2m */
struct page_list_head pages;
- /* Root of p2m page tables, 2 contiguous pages */
- struct page_info *first_level;
+ /* ARMv7: Root of p2m page tables, 2 contiguous pages */
+ /* ARMv8: Look up table is zeroeth level */
+ struct page_info *lookup_level;
/* Current VMID in use */
uint8_t vmid;
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 905beb8..8477206 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -6,12 +6,13 @@
#include <public/xen.h>
#include <asm/processor.h>
+#ifdef CONFIG_ARM_64
+#define PADDR_BITS 48
+#else
#define PADDR_BITS 40
+#endif
#define PADDR_MASK ((1ULL << PADDR_BITS)-1)
-#define VADDR_BITS 32
-#define VADDR_MASK (~0UL)
-
/* Shareability values for the LPAE entries */
#define LPAE_SH_NON_SHAREABLE 0x0
#define LPAE_SH_UNPREDICTALE 0x1
@@ -40,6 +41,94 @@
#define MAIR1VAL 0xff000004
#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32)
+/*
+ * VTCR register configuration for stage 2 translation
+ */
+#define VTCR_TOSZ_40BIT 24
+#define VTCR_TOSZ_48BIT 16
+#define VTCR_SL0_0 0x2
+#define VTCR_SL0_1 0x1
+#define VTCR_SL0_2 0x0
+#define VTCR_SL0_SHIFT 6
+#define VTCR_IRGN0_NC 0x0
+#define VTCR_IRGN0_WBWA 0x1
+#define VTCR_IRGN0_WT 0x2
+#define VTCR_IRGN0_WB 0x3
+#define VTCR_IRGN0_SHIFT 8
+#define VTCR_ORGN0_NC 0x0
+#define VTCR_ORGN0_WBWA 0x1
+#define VTCR_ORGN0_WT 0x2
+#define VTCR_ORGN0_WB 0x3
+#define VTCR_ORGN0_SHIFT 10
+#define VTCR_SH0_NS 0x0
+#define VTCR_SH0_OS 0x2
+#define VTCR_SH0_IS 0x3
+#define VTCR_SH0_SHIFT 12
+#define VTCR_TG0_4K 0x0
+#define VTCR_TG0_64K 0x1
+#define VTCR_TG0_SHIFT 14
+#define VTCR_PS_32BIT 0x0
+#define VTCR_PS_40BIT 0x2
+#define VTCR_PS_48BIT 0x5
+#define VTCR_PS_SHIFT 16
+
+#ifdef CONFIG_ARM_64
+/* 48 bit VA to 48 bit PA */
+#define VTCR_VAL_48PA ((VTCR_TOSZ_48BIT) | \
+ (VTCR_SL0_0 << VTCR_SL0_SHIFT) | \
+ (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
+ (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
+ (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
+ (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
+ (VTCR_PS_48BIT << VTCR_PS_SHIFT))
+
+/* 40 bit VA to 40 bit PA */
+#define VTCR_VAL_40PA ((VTCR_TOSZ_40BIT) | \
+ (VTCR_SL0_1 << VTCR_SL0_SHIFT) | \
+ (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
+ (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
+ (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
+ (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
+ (VTCR_PS_40BIT << VTCR_PS_SHIFT))
+#else
+/* 40 bit VA to 32 bit PA */
+#define VTCR_VAL ((VTCR_TOSZ_40BIT) | \
+ (VTCR_SL0_1 << VTCR_SL0_SHIFT) | \
+ (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
+ (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
+ (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
+ (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
+ (VTCR_PS_32BIT << VTCR_PS_SHIFT))
+#endif
+
+/* TCR register configuration for Xen Stage 1 translation*/
+
+#define TCR_TBI_USE_TBYTE 0x0
+#define TCR_TBI_SHIFT 20
+
+#ifdef CONFIG_ARM_64
+/*
+ * 48 bit VA to 40 bit PA
+ * if platform supports 48 bit PA update runtime in head.S
+ */
+#define TCR_VAL_40PA ((VTCR_TOSZ_48BIT) | \
+ (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
+ (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
+ (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
+ (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
+ (VTCR_PS_40BIT << VTCR_PS_SHIFT) | \
+ (TCR_TBI_USE_TBYTE << TCR_TBI_SHIFT))
+#endif
+
/*
* Attribute Indexes.
*
@@ -109,9 +198,10 @@ typedef struct __packed {
unsigned long af:1; /* Access Flag */
unsigned long ng:1; /* Not-Global */
- /* The base address must be appropriately aligned for Block entries */
- unsigned long base:28; /* Base address of block or next table */
- unsigned long sbz:12; /* Must be zero */
+ /* The base address must be appropriately aligned for Block entries.
+ * base now can hold upto 36 bits to support 48 PA */
+ unsigned long base:36; /* Base address of block or next table */
+ unsigned long sbz:4; /* Must be zero */
/* These seven bits are only used in Block entries and are ignored
* in Table entries. */
@@ -144,9 +234,10 @@ typedef struct __packed {
unsigned long af:1; /* Access Flag */
unsigned long sbz4:1;
- /* The base address must be appropriately aligned for Block entries */
- unsigned long base:28; /* Base address of block or next table */
- unsigned long sbz3:12;
+ /* The base address must be appropriately aligned for Block entries.
+ * base now can hold upto 36 bits to support 48 PA */
+ unsigned long base:36; /* Base address of block or next table */
+ unsigned long sbz3:4;
/* These seven bits are only used in Block entries and are ignored
* in Table entries. */
@@ -169,10 +260,12 @@ typedef struct __packed {
unsigned long pad2:10;
- /* The base address must be appropriately aligned for Block entries */
- unsigned long base:28; /* Base address of block or next table */
+ /* The base address must be appropriately aligned for Block entries.
+ * base now can hold upto 36 bits to support 48 PA */
+ unsigned long base:36; /* Base address of block or next table */
- unsigned long pad1:24;
+ //unsigned long pad1:24;
+ unsigned long pad1:16;
} lpae_walk_t;
typedef union {
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 06e638f..1355d81 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -173,9 +173,21 @@ struct cpuinfo_arm {
uint64_t bits[2];
} aux64;
- struct {
+ union {
uint64_t bits[2];
+ struct {
+ unsigned long pa_range:4;
+ unsigned long asid_bits:4;
+ unsigned long bigend:4;
+ unsigned long secure_ns:4;
+ unsigned long bigend_el0:4;
+ unsigned long tgranule_16K:4;
+ unsigned long tgranule_64K:4;
+ unsigned long tgranule_4K:4;
+ unsigned long __res0:32;
+ };
} mm64;
+
struct {
uint64_t bits[2];
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation
2014-04-30 12:15 [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation vijay.kilari
@ 2014-04-30 12:39 ` Julien Grall
2014-05-02 16:12 ` Ian Campbell
2014-05-03 8:09 ` Vijay Kilari
2014-05-02 16:09 ` Ian Campbell
1 sibling, 2 replies; 6+ messages in thread
From: Julien Grall @ 2014-04-30 12:39 UTC (permalink / raw)
To: vijay.kilari
Cc: Ian.Campbell, stefano.stabellini, Prasun.Kapoor, vijaya.kumar,
xen-devel, stefano.stabellini
Hello Vijaya,
On 04/30/2014 01:15 PM, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d151724..c0e0362 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -224,13 +224,16 @@ skip_bss:
> ldr x0, =MAIRVAL
> msr mair_el2, x0
>
> + mrs x1, ID_AA64MMFR0_EL1
> +
> /* Set up the HTCR:
> - * PASize -- 40 bits / 1TB
> + * PASize -- based on ID_AA64MMFR0_EL1.PARange value
> * Top byte is used
> * PT walks use Outer-Shareable accesses,
> * PT walks are write-back, write-allocate in both cache levels,
> * Full 64-bit address space goes through this table. */
> - ldr x0, =0x80822500
> + ldr x0, =TCR_VAL_40PA
If you define TCR_VAL_40PA in another header, I would also move the
comments explaining the different bits in this header.
> + bfi x0, x1, #16, #3
> msr tcr_el2, x0
>
> /* Set up the SCTLR_EL2:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 305879f..d577b23 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -382,13 +382,18 @@ void __cpuinit setup_virt_paging(void)
> /* SH0=00, ORGN0=IRGN0=01
> * SL0=01 (Level-1)
> * ARVv7: T0SZ=(1)1000 = -8 (32-(-8) = 40 bit physical addresses)
> * ARMv8: T0SZ=01 1000 = 24 (64-24 = 40 bit physical addresses)
> * PS=010 == 40 bits
> */
> #ifdef CONFIG_ARM_32
> - WRITE_SYSREG32(0x80002558, VTCR_EL2);
> + WRITE_SYSREG32(VTCR_VAL, VTCR_EL2);
> #else
> - WRITE_SYSREG32(0x80022558, VTCR_EL2);
> + /* Change PS to 48 and T0SZ = 16 SL0 - 2 to take VA 48 bit */
> + if ( current_cpu_data.mm64.pa_range == VTCR_PS_48BIT )
> + WRITE_SYSREG32(VTCR_VAL_48PA, VTCR_EL2);
> + else
> + /* Consider by default 40 PA support for ARM64 */
> + WRITE_SYSREG32(VTCR_VAL_40PA, VTCR_EL2);
> #endif
Same remark here. Also, please update ARMv8 stuff for 48 bits PA in the
comment.
> isb();
> }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..bdaab46 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -10,29 +10,38 @@
> #include <asm/hardirq.h>
> #include <asm/page.h>
>
> +#ifdef CONFIG_ARM_64
> +/* Zeroeth level is of 1 page size */
> +#define P2M_FIRST_ORDER 0
>From your comment, should not it be P2M_ZEROETH_ORDER?
> +#else
> /* First level P2M is 2 consecutive pages */
> #define P2M_FIRST_ORDER 1
> +#endif
> #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
>
> void dump_p2m_lookup(struct domain *d, paddr_t addr)
> {
> struct p2m_domain *p2m = &d->arch.p2m;
> - lpae_t *first;
> + lpae_t *lookup;
>
> printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
>
> +#ifdef CONFIG_ARM_64
> + if ( zeroeth_linear_offset(addr) > LPAE_ENTRIES )
> +#else
> if ( first_linear_offset(addr) > LPAE_ENTRIES )
> +#endif
> {
> - printk("Cannot dump addresses in second of first level pages...\n");
> + printk("Cannot dump addresses in second of first(ARM32)/zeroeth(ARM64) level pages...\n");
> return;
> }
>
> printk("P2M @ %p mfn:0x%lx\n",
> - p2m->first_level, page_to_mfn(p2m->first_level));
> + p2m->lookup_level, page_to_mfn(p2m->lookup_level));
>
> - first = __map_domain_page(p2m->first_level);
> - dump_pt_walk(first, addr);
> - unmap_domain_page(first);
> + lookup = __map_domain_page(p2m->lookup_level);
> + dump_pt_walk(lookup, addr);
dump_pt_walk assume the table is the first level. I think you should
update dump_pt_walk to handle the zeroeth level.
> + unmap_domain_page(lookup);
> }
>
> void p2m_load_VTTBR(struct domain *d)
> @@ -44,6 +53,20 @@ void p2m_load_VTTBR(struct domain *d)
> isb(); /* Ensure update is visible */
> }
>
> +#ifdef CONFIG_ARM_64
> +/*
> + * Map zeroeth level page that addr contains.
> + */
> +static lpae_t *p2m_map_zeroeth(struct p2m_domain *p2m, paddr_t addr)
> +{
> + if ( zeroeth_linear_offset(addr) >= LPAE_ENTRIES )
> + return NULL;
> +
> + return __map_domain_page(p2m->lookup_level);
> +}
> +
> +#else
> +
> static int p2m_first_level_index(paddr_t addr)
> {
> /*
> @@ -64,10 +87,11 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
> if ( first_linear_offset(addr) >= P2M_FIRST_ENTRIES )
> return NULL;
>
> - page = p2m->first_level + p2m_first_level_index(addr);
> + page = p2m->lookup_level + p2m_first_level_index(addr);
>
> return __map_domain_page(page);
> }
> +#endif
>
> /*
> * Lookup the MFN corresponding to a domain's PFN.
> @@ -79,6 +103,9 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> {
> struct p2m_domain *p2m = &d->arch.p2m;
> lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
> +#ifdef CONFIG_ARM_64
> + lpae_t *zeroeth = NULL;
> +#endif
> paddr_t maddr = INVALID_PADDR;
> p2m_type_t _t;
>
> @@ -89,9 +116,29 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>
> spin_lock(&p2m->lock);
>
> +#ifdef CONFIG_ARM_64
> + zeroeth = p2m_map_zeroeth(p2m, paddr);
> + if ( !zeroeth )
> + goto err;
> +
> + pte = zeroeth[zeroeth_table_offset(paddr)];
> + /* Zeroeth level does not support block translation
> + * so pte.p2m.table should be always set.
> + * Just check for valid bit
> + */
> + if ( !pte.p2m.valid )
> + goto done;
> +
> +#else
> first = p2m_map_first(p2m, paddr);
> if ( !first )
> goto err;
> +#endif
> +
> +#ifdef CONFIG_ARM_64
> + /* Map first level table */
> + first = map_domain_page(pte.p2m.base);
> +#endif
Can you move the code in this ifdef into the first one?
[..]
> @@ -244,8 +294,14 @@ static int apply_p2m_changes(struct domain *d,
> struct p2m_domain *p2m = &d->arch.p2m;
> lpae_t *first = NULL, *second = NULL, *third = NULL;
> paddr_t addr;
> - unsigned long cur_first_page = ~0,
> - cur_first_offset = ~0,
> +#ifdef CONFIG_ARM_64
> + lpae_t *zeroeth = NULL;
> + unsigned long cur_zeroeth_page = ~0,
> + cur_zeroeth_offset = ~0;
> +#else
> + unsigned long cur_first_page = ~0;
> +#endif
> + unsigned long cur_first_offset = ~0,
> cur_second_offset = ~0;
> unsigned long count = 0;
> unsigned int flush = 0;
> @@ -260,6 +316,37 @@ static int apply_p2m_changes(struct domain *d,
> addr = start_gpaddr;
> while ( addr < end_gpaddr )
> {
> +#ifdef CONFIG_ARM_64
> + /* Find zeoeth offset and Map zeroeth page */
> + if ( cur_zeroeth_page != zeroeth_table_offset(addr) )
> + {
> + if ( zeroeth ) unmap_domain_page(zeroeth);
> + zeroeth = p2m_map_zeroeth(p2m, addr);
> + if ( !zeroeth )
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> + cur_zeroeth_page = zeroeth_table_offset(addr);
> + }
> +
> + if ( !zeroeth[zeroeth_table_offset(addr)].p2m.valid )
> + {
> + if ( !populate )
> + {
> + addr = (addr + ZEROETH_SIZE) & ZEROETH_MASK;
> + continue;
> + }
> + rc = p2m_create_table(d, &zeroeth[zeroeth_table_offset(addr)]);
> + if ( rc < 0 )
> + {
> + printk("p2m_populate_ram: L0 failed\n");
> + goto out;
> + }
> + }
> +
> + BUG_ON(!zeroeth[zeroeth_table_offset(addr)].p2m.valid);
> +#else
> if ( cur_first_page != p2m_first_level_index(addr) )
> {
> if ( first ) unmap_domain_page(first);
> @@ -271,7 +358,16 @@ static int apply_p2m_changes(struct domain *d,
> }
> cur_first_page = p2m_first_level_index(addr);
> }
> +#endif
>
> +#ifdef CONFIG_ARM_64
> + if ( cur_zeroeth_offset != zeroeth_table_offset(addr) )
> + {
> + if ( first ) unmap_domain_page(first);
> + first = map_domain_page(zeroeth[zeroeth_table_offset(addr)].p2m.base);
> + cur_zeroeth_offset = zeroeth_table_offset(addr);
> + }
> +#endif
Same remark here.
> if ( !first[first_table_offset(addr)].p2m.valid )
> {
> if ( !populate )
> @@ -279,7 +375,6 @@ static int apply_p2m_changes(struct domain *d,
> addr = (addr + FIRST_SIZE) & FIRST_MASK;
> continue;
> }
> -
Why did you remove this empty line?
> rc = p2m_create_table(d, &first[first_table_offset(addr)]);
> if ( rc < 0 )
> {
> @@ -287,7 +382,6 @@ static int apply_p2m_changes(struct domain *d,
> goto out;
> }
> }
> -
Same here.
> BUG_ON(!first[first_table_offset(addr)].p2m.valid);
>
> if ( cur_first_offset != first_table_offset(addr) )
> @@ -305,7 +399,6 @@ static int apply_p2m_changes(struct domain *d,
> addr = (addr + SECOND_SIZE) & SECOND_MASK;
> continue;
> }
> -
Same here.
[..]
> /* Current VMID in use */
> uint8_t vmid;
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 905beb8..8477206 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -6,12 +6,13 @@
> #include <public/xen.h>
> #include <asm/processor.h>
>
> +#ifdef CONFIG_ARM_64
> +#define PADDR_BITS 48
> +#else
> #define PADDR_BITS 40
> +#endif
> #define PADDR_MASK ((1ULL << PADDR_BITS)-1)
>
> -#define VADDR_BITS 32
> -#define VADDR_MASK (~0UL)
> -
Why did you remove VADDR_{BITS,MASK}?
> +#ifdef CONFIG_ARM_64
> +/* 48 bit VA to 48 bit PA */
[..]
> +/* 40 bit VA to 40 bit PA */
[..]
> +/* 40 bit VA to 32 bit PA */
Did you mean IPA instead of VA?
Also, on ARM, the PA is encoded with 40 bits.
[..]
>
> - unsigned long pad1:24;
> + //unsigned long pad1:24;
This line should be removed rather than comment.
> + unsigned long pad1:16;
> } lpae_walk_t;
>
> typedef union {
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 06e638f..1355d81 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -173,9 +173,21 @@ struct cpuinfo_arm {
> uint64_t bits[2];
> } aux64;
>
> - struct {
> + union {
> uint64_t bits[2];
> + struct {
> + unsigned long pa_range:4;
> + unsigned long asid_bits:4;
> + unsigned long bigend:4;
> + unsigned long secure_ns:4;
> + unsigned long bigend_el0:4;
> + unsigned long tgranule_16K:4;
> + unsigned long tgranule_64K:4;
> + unsigned long tgranule_4K:4;
> + unsigned long __res0:32;
> + };
> } mm64;
> +
Spurious line.
>
> struct {
> uint64_t bits[2];
>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation
2014-04-30 12:15 [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation vijay.kilari
2014-04-30 12:39 ` Julien Grall
@ 2014-05-02 16:09 ` Ian Campbell
1 sibling, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2014-05-02 16:09 UTC (permalink / raw)
To: vijay.kilari
Cc: stefano.stabellini, Prasun.Kapoor, vijaya.kumar, julien.grall,
xen-devel, stefano.stabellini
On Wed, 2014-04-30 at 17:45 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> To support 48-bit Physical Address support, add 4-level
> page tables for stage 2 translation.By default stage 1 and
> stage 2 translation at EL2 are with 4-levels
Not just by default, but "Now", default implies a config option.
> Configure TCR_EL2.IPS and VTCR_EL2.PS based on platform
> supported PA range at runtime
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> xen/arch/arm/arm64/head.S | 7 ++-
> xen/arch/arm/mm.c | 11 +++-
> xen/arch/arm/p2m.c | 132 ++++++++++++++++++++++++++++++++++-----
> xen/include/asm-arm/p2m.h | 5 +-
> xen/include/asm-arm/page.h | 117 ++++++++++++++++++++++++++++++----
> xen/include/asm-arm/processor.h | 14 ++++-
> 6 files changed, 249 insertions(+), 37 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d151724..c0e0362 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -224,13 +224,16 @@ skip_bss:
> ldr x0, =MAIRVAL
> msr mair_el2, x0
>
> + mrs x1, ID_AA64MMFR0_EL1
Please pay attention to the indentation and alignment of the rest of
this file.
> +
> /* Set up the HTCR:
> - * PASize -- 40 bits / 1TB
> + * PASize -- based on ID_AA64MMFR0_EL1.PARange value
> * Top byte is used
> * PT walks use Outer-Shareable accesses,
> * PT walks are write-back, write-allocate in both cache levels,
> * Full 64-bit address space goes through this table. */
> - ldr x0, =0x80822500
> + ldr x0, =TCR_VAL_40PA
> + bfi x0, x1, #16, #3
Notice that the rest of this file is extensively commented. This should
say something like /* Or in the PASize from x1 */
> msr tcr_el2, x0
>
> /* Set up the SCTLR_EL2:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 305879f..d577b23 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -382,13 +382,18 @@ void __cpuinit setup_virt_paging(void)
> /* SH0=00, ORGN0=IRGN0=01
> * SL0=01 (Level-1)
> * ARVv7: T0SZ=(1)1000 = -8 (32-(-8) = 40 bit physical addresses)
> * ARMv8: T0SZ=01 1000 = 24 (64-24 = 40 bit physical addresses)
This comment is no longer correct.
> * PS=010 == 40 bits
> */
> #ifdef CONFIG_ARM_32
> - WRITE_SYSREG32(0x80002558, VTCR_EL2);
> + WRITE_SYSREG32(VTCR_VAL, VTCR_EL2);
> #else
> - WRITE_SYSREG32(0x80022558, VTCR_EL2);
> + /* Change PS to 48 and T0SZ = 16 SL0 - 2 to take VA 48 bit */
SL0 should not be variable, it should just be unconditionally configured
for 4 level page tables. Otherwise all sorts of other places needs to
know dynamically how many level the page tables have.
TBH, I'm not sure why we don't just use 48bit IPAs unconditionally.
> + if ( current_cpu_data.mm64.pa_range == VTCR_PS_48BIT )
> + WRITE_SYSREG32(VTCR_VAL_48PA, VTCR_EL2);
> + else
> + /* Consider by default 40 PA support for ARM64 */
> + WRITE_SYSREG32(VTCR_VAL_40PA, VTCR_EL2);
> #endif
> isb();
> }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..bdaab46 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -10,29 +10,38 @@
> #include <asm/hardirq.h>
> #include <asm/page.h>
>
> +#ifdef CONFIG_ARM_64
> +/* Zeroeth level is of 1 page size */
> +#define P2M_FIRST_ORDER 0
> +#else
> /* First level P2M is 2 consecutive pages */
> #define P2M_FIRST_ORDER 1
> +#endif
> #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
I think P2M_FIRST_ENTRIES is now only used under ifdef CONFIG_ARM_32
(or !ARM64). If I'm right please move it inside the #else here.
>
> void dump_p2m_lookup(struct domain *d, paddr_t addr)
> {
> struct p2m_domain *p2m = &d->arch.p2m;
> - lpae_t *first;
> + lpae_t *lookup;
>
> printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
>
> +#ifdef CONFIG_ARM_64
> + if ( zeroeth_linear_offset(addr) > LPAE_ENTRIES )
> +#else
> if ( first_linear_offset(addr) > LPAE_ENTRIES )
> +#endif
> {
> - printk("Cannot dump addresses in second of first level pages...\n");
> + printk("Cannot dump addresses in second of first(ARM32)/zeroeth(ARM64) level pages...\n");
"Cannot dump addresses in second page of a concatenated initial paging
level" or something perhaps.
Is concatenation possible at level 0? Table D5-9 in the ARMv8 ARM
suggests not, which would simplify this I think, by putting the bulk of
the above in an arm32 only ifdef.
> return;
> }
>
> printk("P2M @ %p mfn:0x%lx\n",
> - p2m->first_level, page_to_mfn(p2m->first_level));
> + p2m->lookup_level, page_to_mfn(p2m->lookup_level));
>
> - first = __map_domain_page(p2m->first_level);
> - dump_pt_walk(first, addr);
> - unmap_domain_page(first);
> + lookup = __map_domain_page(p2m->lookup_level);
> + dump_pt_walk(lookup, addr);
> + unmap_domain_page(lookup);
> }
>
> void p2m_load_VTTBR(struct domain *d)
> @@ -44,6 +53,20 @@ void p2m_load_VTTBR(struct domain *d)
> isb(); /* Ensure update is visible */
> }
>
> +#ifdef CONFIG_ARM_64
> +/*
> + * Map zeroeth level page that addr contains.
> + */
> +static lpae_t *p2m_map_zeroeth(struct p2m_domain *p2m, paddr_t addr)
> +{
> + if ( zeroeth_linear_offset(addr) >= LPAE_ENTRIES )
> + return NULL;
> +
> + return __map_domain_page(p2m->lookup_level);
> +}
> +
> +#else
> +
> static int p2m_first_level_index(paddr_t addr)
> {
> /*
> @@ -64,10 +87,11 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
> if ( first_linear_offset(addr) >= P2M_FIRST_ENTRIES )
> return NULL;
>
> - page = p2m->first_level + p2m_first_level_index(addr);
> + page = p2m->lookup_level + p2m_first_level_index(addr);
>
> return __map_domain_page(page);
> }
> +#endif
>
> /*
> * Lookup the MFN corresponding to a domain's PFN.
> @@ -79,6 +103,9 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> {
> struct p2m_domain *p2m = &d->arch.p2m;
> lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
> +#ifdef CONFIG_ARM_64
> + lpae_t *zeroeth = NULL;
> +#endif
> paddr_t maddr = INVALID_PADDR;
> p2m_type_t _t;
>
> @@ -89,9 +116,29 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>
> spin_lock(&p2m->lock);
>
> +#ifdef CONFIG_ARM_64
> + zeroeth = p2m_map_zeroeth(p2m, paddr);
> + if ( !zeroeth )
> + goto err;
> +
> + pte = zeroeth[zeroeth_table_offset(paddr)];
> + /* Zeroeth level does not support block translation
> + * so pte.p2m.table should be always set.
> + * Just check for valid bit
> + */
> + if ( !pte.p2m.valid )
> + goto done;
> +
> +#else
> first = p2m_map_first(p2m, paddr);
> if ( !first )
> goto err;
> +#endif
> +
> +#ifdef CONFIG_ARM_64
> + /* Map first level table */
> + first = map_domain_page(pte.p2m.base);
> +#endif
You should merge this into the existing ifdef just above.
>
> pte = first[first_table_offset(paddr)];
> if ( !pte.p2m.valid || !pte.p2m.table )
> @@ -120,6 +167,9 @@ done:
> if (third) unmap_domain_page(third);
> if (second) unmap_domain_page(second);
> if (first) unmap_domain_page(first);
> +#ifdef CONFIG_ARM_64
> + if (zeroeth) unmap_domain_page(zeroeth);
> +#endif
>
> err:
> spin_unlock(&p2m->lock);
> @@ -244,8 +294,14 @@ static int apply_p2m_changes(struct domain *d,
> struct p2m_domain *p2m = &d->arch.p2m;
> lpae_t *first = NULL, *second = NULL, *third = NULL;
> paddr_t addr;
> - unsigned long cur_first_page = ~0,
> - cur_first_offset = ~0,
> +#ifdef CONFIG_ARM_64
> + lpae_t *zeroeth = NULL;
> + unsigned long cur_zeroeth_page = ~0,
> + cur_zeroeth_offset = ~0;
> +#else
> + unsigned long cur_first_page = ~0;
> +#endif
> + unsigned long cur_first_offset = ~0,
> cur_second_offset = ~0;
> unsigned long count = 0;
> unsigned int flush = 0;
> @@ -260,6 +316,37 @@ static int apply_p2m_changes(struct domain *d,
> addr = start_gpaddr;
> while ( addr < end_gpaddr )
> {
> +#ifdef CONFIG_ARM_64
> + /* Find zeoeth offset and Map zeroeth page */
^r
> + if ( cur_zeroeth_page != zeroeth_table_offset(addr) )
> + {
> + if ( zeroeth ) unmap_domain_page(zeroeth);
> + zeroeth = p2m_map_zeroeth(p2m, addr);
> + if ( !zeroeth )
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> + cur_zeroeth_page = zeroeth_table_offset(addr);
> + }
> +
> + if ( !zeroeth[zeroeth_table_offset(addr)].p2m.valid )
> + {
> + if ( !populate )
> + {
> + addr = (addr + ZEROETH_SIZE) & ZEROETH_MASK;
> + continue;
> + }
> + rc = p2m_create_table(d, &zeroeth[zeroeth_table_offset(addr)]);
> + if ( rc < 0 )
> + {
> + printk("p2m_populate_ram: L0 failed\n");
> + goto out;
> + }
> + }
> +
> + BUG_ON(!zeroeth[zeroeth_table_offset(addr)].p2m.valid);
> +#else
> if ( cur_first_page != p2m_first_level_index(addr) )
> {
> if ( first ) unmap_domain_page(first);
> @@ -271,7 +358,16 @@ static int apply_p2m_changes(struct domain *d,
> }
> cur_first_page = p2m_first_level_index(addr);
> }
> +#endif
>
> +#ifdef CONFIG_ARM_64
> + if ( cur_zeroeth_offset != zeroeth_table_offset(addr) )
> + {
> + if ( first ) unmap_domain_page(first);
> + first = map_domain_page(zeroeth[zeroeth_table_offset(addr)].p2m.base);
> + cur_zeroeth_offset = zeroeth_table_offset(addr);
> + }
> +#endif
Again, I think you can do this in the previous ifdef, or possibly use a
simpler ifdef within what is currently the #else clause above. Depending
on how much ends up looking the same.
> if ( !first[first_table_offset(addr)].p2m.valid )
> {
> if ( !populate )
> @@ -279,7 +375,6 @@ static int apply_p2m_changes(struct domain *d,
> addr = (addr + FIRST_SIZE) & FIRST_MASK;
> continue;
> }
> -
Please drop these spurious changes.
(two more followed)
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3b39c45..3aa3623 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -13,8 +13,9 @@ struct p2m_domain {
> /* Pages used to construct the p2m */
> struct page_list_head pages;
>
> - /* Root of p2m page tables, 2 contiguous pages */
> - struct page_info *first_level;
> + /* ARMv7: Root of p2m page tables, 2 contiguous pages */
> + /* ARMv8: Look up table is zeroeth level */
> + struct page_info *lookup_level;
"root" might be a better name.
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 905beb8..8477206 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -6,12 +6,13 @@
> #include <public/xen.h>
> #include <asm/processor.h>
>
> +#ifdef CONFIG_ARM_64
> +#define PADDR_BITS 48
> +#else
> #define PADDR_BITS 40
> +#endif
Move these to arm32/page.h and arm64/page.h I think, you'll needto move
the #include above mfn_to_xen_entry.
> @@ -40,6 +41,94 @@
> #define MAIR1VAL 0xff000004
> #define MAIRVAL (MAIR0VAL|MAIR1VAL<<32)
>
> +/*
> + * VTCR register configuration for stage 2 translation
> + */
> +#define VTCR_TOSZ_40BIT 24
> +#define VTCR_TOSZ_48BIT 16
Please use (0xN << M) form for all of these. Also processor.h is the
right place for these sorts of defines.
> +#define VTCR_SL0_0 0x2
> +#define VTCR_SL0_1 0x1
> +#define VTCR_SL0_2 0x0
> +#define VTCR_SL0_SHIFT 6
> +#define VTCR_IRGN0_NC 0x0
> +#define VTCR_IRGN0_WBWA 0x1
> +#define VTCR_IRGN0_WT 0x2
> +#define VTCR_IRGN0_WB 0x3
> +#define VTCR_IRGN0_SHIFT 8
> +#define VTCR_ORGN0_NC 0x0
> +#define VTCR_ORGN0_WBWA 0x1
> +#define VTCR_ORGN0_WT 0x2
> +#define VTCR_ORGN0_WB 0x3
> +#define VTCR_ORGN0_SHIFT 10
> +#define VTCR_SH0_NS 0x0
> +#define VTCR_SH0_OS 0x2
> +#define VTCR_SH0_IS 0x3
> +#define VTCR_SH0_SHIFT 12
> +#define VTCR_TG0_4K 0x0
> +#define VTCR_TG0_64K 0x1
> +#define VTCR_TG0_SHIFT 14
> +#define VTCR_PS_32BIT 0x0
> +#define VTCR_PS_40BIT 0x2
> +#define VTCR_PS_48BIT 0x5
> +#define VTCR_PS_SHIFT 16
> +
> +#ifdef CONFIG_ARM_64
> +/* 48 bit VA to 48 bit PA */
> +#define VTCR_VAL_48PA ((VTCR_TOSZ_48BIT) | \
> + (VTCR_SL0_0 << VTCR_SL0_SHIFT) | \
> + (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
> + (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
> + (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
> + (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
> + (VTCR_PS_48BIT << VTCR_PS_SHIFT))
> +
> +/* 40 bit VA to 40 bit PA */
> +#define VTCR_VAL_40PA ((VTCR_TOSZ_40BIT) | \
> + (VTCR_SL0_1 << VTCR_SL0_SHIFT) | \
> + (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
> + (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
> + (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
> + (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
> + (VTCR_PS_40BIT << VTCR_PS_SHIFT))
> +#else
> +/* 40 bit VA to 32 bit PA */
> +#define VTCR_VAL ((VTCR_TOSZ_40BIT) | \
> + (VTCR_SL0_1 << VTCR_SL0_SHIFT) | \
> + (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
> + (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
> + (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
> + (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
> + (VTCR_PS_32BIT << VTCR_PS_SHIFT))
> +#endif
IMHO this was fine as a suitably commented literal number in the
corresponding function and would continue to be if suitable commented.
If you feel you must go this route then please define a VTCR_VAL_BASE
with all the common bits. Also please consider or-ing in the variable
bits in explicitly during setup_virt_paging(). Whatever you do the
comment and the corresponding #defines should be in the same place (if
indeed the comment isn't then redundant).
> +
> +/* TCR register configuration for Xen Stage 1 translation*/
> +
> +#define TCR_TBI_USE_TBYTE 0x0
> +#define TCR_TBI_SHIFT 20
> +
> +#ifdef CONFIG_ARM_64
> +/*
> + * 48 bit VA to 40 bit PA
> + * if platform supports 48 bit PA update runtime in head.S
IMHO this was also fine as a (commented) 0xXXXX in head.S. It's no
clearer here like this.
> + */
> +#define TCR_VAL_40PA ((VTCR_TOSZ_48BIT) | \
> + (VTCR_IRGN0_WBWA << VTCR_IRGN0_SHIFT) | \
> + (VTCR_ORGN0_WBWA << VTCR_ORGN0_SHIFT) | \
> + (VTCR_SH0_OS << VTCR_SH0_SHIFT) | \
> + (VTCR_TG0_4K << VTCR_TG0_SHIFT) | \
> + (VTCR_PS_40BIT << VTCR_PS_SHIFT) | \
> + (TCR_TBI_USE_TBYTE << TCR_TBI_SHIFT))
> +#endif
> +
> /*
> * Attribute Indexes.
> *
> @@ -109,9 +198,10 @@ typedef struct __packed {
> unsigned long af:1; /* Access Flag */
> unsigned long ng:1; /* Not-Global */
>
> - /* The base address must be appropriately aligned for Block entries */
> - unsigned long base:28; /* Base address of block or next table */
> - unsigned long sbz:12; /* Must be zero */
> + /* The base address must be appropriately aligned for Block entries.
> + * base now can hold upto 36 bits to support 48 PA */
"up to" is two words, although I think the sentence you added is
unnecessary.
> + unsigned long base:36; /* Base address of block or next table */
> + unsigned long sbz:4; /* Must be zero */
This comment is no longer lined up.
>
> /* These seven bits are only used in Block entries and are ignored
> * in Table entries. */
> @@ -144,9 +234,10 @@ typedef struct __packed {
> unsigned long af:1; /* Access Flag */
> unsigned long sbz4:1;
>
> - /* The base address must be appropriately aligned for Block entries */
> - unsigned long base:28; /* Base address of block or next table */
> - unsigned long sbz3:12;
> + /* The base address must be appropriately aligned for Block entries.
> + * base now can hold upto 36 bits to support 48 PA */
As above.
> + unsigned long base:36; /* Base address of block or next table */
> + unsigned long sbz3:4;
>
> /* These seven bits are only used in Block entries and are ignored
> * in Table entries. */
> @@ -169,10 +260,12 @@ typedef struct __packed {
>
> unsigned long pad2:10;
>
> - /* The base address must be appropriately aligned for Block entries */
> - unsigned long base:28; /* Base address of block or next table */
> + /* The base address must be appropriately aligned for Block entries.
> + * base now can hold upto 36 bits to support 48 PA */
As above.
> + unsigned long base:36; /* Base address of block or next table */
>
> - unsigned long pad1:24;
> + //unsigned long pad1:24;
Please remove.
> + unsigned long pad1:16;
> } lpae_walk_t;
>
> typedef union {
Thanks,
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation
2014-04-30 12:39 ` Julien Grall
@ 2014-05-02 16:12 ` Ian Campbell
2014-05-03 8:09 ` Vijay Kilari
1 sibling, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2014-05-02 16:12 UTC (permalink / raw)
To: Julien Grall
Cc: vijay.kilari, stefano.stabellini, Prasun.Kapoor, vijaya.kumar,
xen-devel, stefano.stabellini
On Wed, 2014-04-30 at 13:39 +0100, Julien Grall wrote:
> Hello Vijaya,
>
> On 04/30/2014 01:15 PM, vijay.kilari@gmail.com wrote:
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index d151724..c0e0362 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -224,13 +224,16 @@ skip_bss:
> > ldr x0, =MAIRVAL
> > msr mair_el2, x0
> >
> > + mrs x1, ID_AA64MMFR0_EL1
> > +
> > /* Set up the HTCR:
> > - * PASize -- 40 bits / 1TB
> > + * PASize -- based on ID_AA64MMFR0_EL1.PARange value
> > * Top byte is used
> > * PT walks use Outer-Shareable accesses,
> > * PT walks are write-back, write-allocate in both cache levels,
> > * Full 64-bit address space goes through this table. */
> > - ldr x0, =0x80822500
> > + ldr x0, =TCR_VAL_40PA
>
> If you define TCR_VAL_40PA in another header, I would also move the
> comments explaining the different bits in this header.
Yeah, I'm increasingly not sure what was so wrong with this extensively
commented number TBH.
> > +#ifdef CONFIG_ARM_64
> > +/* Zeroeth level is of 1 page size */
> > +#define P2M_FIRST_ORDER 0
>
> From your comment, should not it be P2M_ZEROETH_ORDER?
Actually it should be P2M_ROOT_ORDER.
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index 905beb8..8477206 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -6,12 +6,13 @@
> > #include <public/xen.h>
> > #include <asm/processor.h>
> >
> > +#ifdef CONFIG_ARM_64
> > +#define PADDR_BITS 48
> > +#else
> > #define PADDR_BITS 40
> > +#endif
> > #define PADDR_MASK ((1ULL << PADDR_BITS)-1)
> >
> > -#define VADDR_BITS 32
> > -#define VADDR_MASK (~0UL)
> > -
>
> Why did you remove VADDR_{BITS,MASK}?
These were unused (and wrong on arm64) I think removing them is OK, if a
little bit tangential to this patch.
> > +/* 40 bit VA to 32 bit PA */
>
> Did you mean IPA instead of VA?
>
> Also, on ARM, the PA is encoded with 40 bits.
Do you mean arm32?
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation
2014-04-30 12:39 ` Julien Grall
2014-05-02 16:12 ` Ian Campbell
@ 2014-05-03 8:09 ` Vijay Kilari
2014-05-05 18:43 ` Julien Grall
1 sibling, 1 reply; 6+ messages in thread
From: Vijay Kilari @ 2014-05-03 8:09 UTC (permalink / raw)
To: Julien Grall
Cc: Ian Campbell, Stefano Stabellini, Prasun Kapoor, Vijaya Kumar K,
xen-devel, Stefano Stabellini
On Wed, Apr 30, 2014 at 6:09 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Vijaya,
>
> On 04/30/2014 01:15 PM, vijay.kilari@gmail.com wrote:
>> - first = __map_domain_page(p2m->first_level);
>> - dump_pt_walk(first, addr);
>> - unmap_domain_page(first);
>> + lookup = __map_domain_page(p2m->lookup_level);
>> + dump_pt_walk(lookup, addr);
>
> dump_pt_walk assume the table is the first level. I think you should
> update dump_pt_walk to handle the zeroeth level.
It was misguide by dump_hyp_walk(), which is passing zeroeth level
to dump_pt_walk() for ARM64. Will update it
>>
>> -#define VADDR_BITS 32
>> -#define VADDR_MASK (~0UL)
>> -
>
> Why did you remove VADDR_{BITS,MASK}?
It is not used at all. at least for ARM
- Vijay
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation
2014-05-03 8:09 ` Vijay Kilari
@ 2014-05-05 18:43 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2014-05-05 18:43 UTC (permalink / raw)
To: Vijay Kilari
Cc: Ian Campbell, Stefano Stabellini, Prasun Kapoor, Vijaya Kumar K,
xen-devel, Stefano Stabellini
Hi Vijay,
On 03/05/14 09:09, Vijay Kilari wrote:
> On Wed, Apr 30, 2014 at 6:09 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello Vijaya,
>>
>> On 04/30/2014 01:15 PM, vijay.kilari@gmail.com wrote:
>>> - first = __map_domain_page(p2m->first_level);
>>> - dump_pt_walk(first, addr);
>>> - unmap_domain_page(first);
>>> + lookup = __map_domain_page(p2m->lookup_level);
>>> + dump_pt_walk(lookup, addr);
>>
>> dump_pt_walk assume the table is the first level. I think you should
>> update dump_pt_walk to handle the zeroeth level.
>
> It was misguide by dump_hyp_walk(), which is passing zeroeth level
> to dump_pt_walk() for ARM64. Will update it
>
>>>
>>> -#define VADDR_BITS 32
>>> -#define VADDR_MASK (~0UL)
>>> -
>>
>> Why did you remove VADDR_{BITS,MASK}?
>
> It is not used at all. at least for ARM
From the commit message point of view, you are removing theses lines
without no reason.
IHMO, it should be part of another patch.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-05 18:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-30 12:15 [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation vijay.kilari
2014-04-30 12:39 ` Julien Grall
2014-05-02 16:12 ` Ian Campbell
2014-05-03 8:09 ` Vijay Kilari
2014-05-05 18:43 ` Julien Grall
2014-05-02 16:09 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.