* [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes
@ 2018-11-29 11:37 Julien Grall
2018-11-29 11:37 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Julien Grall @ 2018-11-29 11:37 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, andre.przywara, Matthew Daley, Jan-Peter Larsson,
Shameerali Kolothum Thodi, Julien Grall
Hi all,
This patch series fixes 2 bug in the boot code for the memory management.
The first patch should resolve Xen stall when setting SCTLR.XN on some
platforms.
The second patch should allow to boot Xen again the Hikey board.
Cheers,
Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Cc: Jan-Peter Larsson <Jan-Peter.Larsson@arm.com>
Cc: Matthew Daley <mattd@bugfuzz.com>
Julien Grall (2):
xen/arm: mm: Set-up page permission for Xen mappings earlier on
xen/arm: Stop relocating Xen
xen/arch/arm/arm32/head.S | 54 +++-----------------------------------
xen/arch/arm/arm64/head.S | 50 +++++------------------------------
xen/arch/arm/mm.c | 67 ++++++++++++++++++-----------------------------
xen/arch/arm/setup.c | 65 +++------------------------------------------
xen/include/asm-arm/mm.h | 2 +-
5 files changed, 39 insertions(+), 199 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on
2018-11-29 11:37 [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Julien Grall
@ 2018-11-29 11:37 ` Julien Grall
2018-12-12 23:54 ` Stefano Stabellini
2018-11-29 11:37 ` [PATCH 2/2] xen/arm: Stop relocating Xen Julien Grall
2018-12-01 1:27 ` [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Matthew Daley
2 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2018-11-29 11:37 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
Xen mapping is first create using a 2MB page and then shatterred in 4KB
page for fine-graine permission. However, it is not safe to break-down
superpage page without going to an intermediate step invalidating
the entry.
As we are changing Xen mappings, we cannot go through the intermediate
step. The only solution is to create Xen mapping using 4KB entries
directly. As the Xen should always access the mappings according with
the runtime permission, it is then possible to set-up the permissions
while create the mapping.
We are still playing with the fire as there are still some
break-before-make issue in setup_pagetables (i.e switch between 2 sets of
page-tables). But it should slightly be better than the current state.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Reported-by: Jan-Peter Larsson <Jan-Peter.Larsson@arm.com>
---
I had few reports on new platforms where Xen reliably stale as soon as
SCTLR.WXN is turned on. This likely happens because of not complying
with Break-Before-Make when setting-up the permission as we
break-down a superpage to 4KB mappings.
---
xen/arch/arm/mm.c | 49 ++++++++++++++++++++++---------------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 987fcb9162..2556e57a99 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -649,11 +649,31 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
}
#endif
+ /* Break up the Xen mapping into 4k pages and protect them separately. */
+ for ( i = 0; i < LPAE_ENTRIES; i++ )
+ {
+ mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
+ unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
+
+ if ( !is_kernel(va) )
+ break;
+ pte = mfn_to_xen_entry(mfn, MT_NORMAL);
+ pte.pt.table = 1; /* 4k mappings always have this bit set */
+ if ( is_kernel_text(va) || is_kernel_inittext(va) )
+ {
+ pte.pt.xn = 0;
+ pte.pt.ro = 1;
+ }
+ if ( is_kernel_rodata(va) )
+ pte.pt.ro = 1;
+ xen_xenmap[i] = pte;
+ }
+
/* Initialise xen second level entries ... */
/* ... Xen's text etc */
- pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
- pte.pt.xn = 0;/* Contains our text mapping! */
+ pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
+ pte.pt.table = 1;
xen_second[second_table_offset(XEN_VIRT_START)] = pte;
/* ... Fixmap */
@@ -693,31 +713,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
clear_table(boot_second);
clear_table(boot_third);
- /* Break up the Xen mapping into 4k pages and protect them separately. */
- for ( i = 0; i < LPAE_ENTRIES; i++ )
- {
- mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
- unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
- if ( !is_kernel(va) )
- break;
- pte = mfn_to_xen_entry(mfn, MT_NORMAL);
- pte.pt.table = 1; /* 4k mappings always have this bit set */
- if ( is_kernel_text(va) || is_kernel_inittext(va) )
- {
- pte.pt.xn = 0;
- pte.pt.ro = 1;
- }
- if ( is_kernel_rodata(va) )
- pte.pt.ro = 1;
- write_pte(xen_xenmap + i, pte);
- /* No flush required here as page table is not hooked in yet. */
- }
-
- pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
- pte.pt.table = 1;
- write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
- /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */
-
/* From now on, no mapping may be both writable and executable. */
WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
/* Flush everything after setting WXN bit. */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] xen/arm: Stop relocating Xen
2018-11-29 11:37 [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Julien Grall
2018-11-29 11:37 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
@ 2018-11-29 11:37 ` Julien Grall
2018-12-12 23:53 ` Stefano Stabellini
2018-12-01 1:27 ` [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Matthew Daley
2 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2018-11-29 11:37 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
At the moment, Xen is relocated towards the end of the memory. While
this has the advantage to free space in low memory, the code is not
compliant with the break-before-make because it requires to switch
between two sets of page-table. This is not entirely trivial to fix as
it would require us to go through an identity mapping and disabling MMU.
Furthermore, it looks like that some platform (such as the Hikey960)
may not be able to bring-up secondary CPUs if the entry is too high.
I don't believe the low memory is an issue because Xen is quite tiny
(< 2MB). So the best solution is to stop relocating Xen. This has the
advantage to simplify the code and should speed-up the boot as relocation
is not necessary anymore.
Note that the break-before-make issue is not fixed by this patch.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reported-by: Matthew Daley <mattd@bugfuzz.com>
---
xen/arch/arm/arm32/head.S | 54 +++------------------------------------
xen/arch/arm/arm64/head.S | 50 +++++-------------------------------
xen/arch/arm/mm.c | 20 ++++-----------
xen/arch/arm/setup.c | 65 +++--------------------------------------------
xen/include/asm-arm/mm.h | 2 +-
5 files changed, 18 insertions(+), 173 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 93b51e9ef2..390a505e05 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -469,58 +469,12 @@ fail: PRINT("- Boot failed -\r\n")
GLOBAL(_end_boot)
/*
- * Copy Xen to new location and switch TTBR
+ * Switch TTBR
* r1:r0 ttbr
- * r2 source address
- * r3 destination address
- * [sp]=>r4 length
*
- * Source and destination must be word aligned, length is rounded up
- * to a 16 byte boundary.
- *
- * MUST BE VERY CAREFUL when saving things to RAM over the copy
+ * TODO: This code does not comply with break-before-make.
*/
-ENTRY(relocate_xen)
- push {r4,r5,r6,r7,r8,r9,r10,r11}
-
- ldr r4, [sp, #8*4] /* Get 4th argument from stack */
-
- /* Copy 16 bytes at a time using:
- * r5: counter
- * r6: data
- * r7: data
- * r8: data
- * r9: data
- * r10: source
- * r11: destination
- */
- mov r5, r4
- mov r10, r2
- mov r11, r3
-1: ldmia r10!, {r6, r7, r8, r9}
- stmia r11!, {r6, r7, r8, r9}
-
- subs r5, r5, #16
- bgt 1b
-
- /* Flush destination from dcache using:
- * r5: counter
- * r6: step
- * r7: vaddr
- */
- dsb /* So the CPU issues all writes to the range */
-
- mov r5, r4
- ldr r6, =dcache_line_bytes /* r6 := step */
- ldr r6, [r6]
- mov r7, r3
-
-1: mcr CP32(r7, DCCMVAC)
-
- add r7, r7, r6
- subs r5, r5, r6
- bgt 1b
-
+ENTRY(switch_ttbr)
dsb /* Ensure the flushes happen before
* continuing */
isb /* Ensure synchronization with previous
@@ -543,8 +497,6 @@ ENTRY(relocate_xen)
dsb /* Ensure completion of TLB+BP flush */
isb
- pop {r4, r5,r6,r7,r8,r9,r10,r11}
-
mov pc, lr
#ifdef CONFIG_EARLY_PRINTK
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 9428c3f5a2..4589a37874 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -605,52 +605,14 @@ fail: PRINT("- Boot failed -\r\n")
GLOBAL(_end_boot)
-/* Copy Xen to new location and switch TTBR
- * x0 ttbr
- * x1 source address
- * x2 destination address
- * x3 length
+/*
+ * Switch TTBR
*
- * Source and destination must be word aligned, length is rounded up
- * to a 16 byte boundary.
+ * x0 ttbr
*
- * MUST BE VERY CAREFUL when saving things to RAM over the copy */
-ENTRY(relocate_xen)
- /* Copy 16 bytes at a time using:
- * x9: counter
- * x10: data
- * x11: data
- * x12: source
- * x13: destination
- */
- mov x9, x3
- mov x12, x1
- mov x13, x2
-
-1: ldp x10, x11, [x12], #16
- stp x10, x11, [x13], #16
-
- subs x9, x9, #16
- bgt 1b
-
- /* Flush destination from dcache using:
- * x9: counter
- * x10: step
- * x11: vaddr
- */
- dsb sy /* So the CPU issues all writes to the range */
-
- mov x9, x3
- ldr x10, =dcache_line_bytes /* x10 := step */
- ldr x10, [x10]
- mov x11, x2
-
-1: dc cvac, x11
-
- add x11, x11, x10
- subs x9, x9, x10
- bgt 1b
-
+ * TODO: This code does not comply with break-before-make.
+ */
+ENTRY(switch_ttbr)
dsb sy /* Ensure the flushes happen before
* continuing */
isb /* Ensure synchronization with previous
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 2556e57a99..f6931e007f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -601,7 +601,7 @@ void __init remove_early_mappings(void)
flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
}
-extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
+extern void switch_ttbr(uint64_t ttbr);
/* Clear a translation table and clean & invalidate the cache */
static void clear_table(void *table)
@@ -612,15 +612,13 @@ static void clear_table(void *table)
/* Boot-time pagetable setup.
* Changes here may need matching changes in head.S */
-void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
+void __init setup_pagetables(unsigned long boot_phys_offset)
{
uint64_t ttbr;
- unsigned long dest_va;
lpae_t pte, *p;
int i;
- /* Calculate virt-to-phys offset for the new location */
- phys_offset = xen_paddr - (unsigned long) _start;
+ phys_offset = boot_phys_offset;
#ifdef CONFIG_ARM_64
p = (void *) xen_pgtable;
@@ -652,7 +650,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
/* Break up the Xen mapping into 4k pages and protect them separately. */
for ( i = 0; i < LPAE_ENTRIES; i++ )
{
- mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
+ mfn_t mfn = mfn_add(maddr_to_mfn((vaddr_t)_start + phys_offset), i);
unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
if ( !is_kernel(va) )
@@ -687,21 +685,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
- /* ... Boot Misc area for xen relocation */
- dest_va = BOOT_RELOC_VIRT_START;
- pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
- /* Map the destination in xen_second. */
- xen_second[second_table_offset(dest_va)] = pte;
- /* Map the destination in boot_second. */
- write_pte(boot_second + second_table_offset(dest_va), pte);
- flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE);
#ifdef CONFIG_ARM_64
ttbr = (uintptr_t) xen_pgtable + phys_offset;
#else
ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
#endif
- relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
+ switch_ttbr(ttbr);
/* Clear the copy of the boot pagetables. Each secondary CPU
* rebuilds these itself (see head.S) */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index aec53f30d3..e84172fbc3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -374,6 +374,7 @@ void __init discard_initial_modules(void)
remove_early_mappings();
}
+#ifdef CONFIG_ARM_32
/*
* Returns the end address of the highest region in the range s..e
* with required size and alignment that does not conflict with the
@@ -440,6 +441,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
}
return e;
}
+#endif
/*
* Return the end of the non-module region starting at s. In other
@@ -475,59 +477,6 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
return lowest;
}
-
-/**
- * get_xen_paddr - get physical address to relocate Xen to
- *
- * Xen is relocated to as near to the top of RAM as possible and
- * aligned to a XEN_PADDR_ALIGN boundary.
- */
-static paddr_t __init get_xen_paddr(void)
-{
- struct meminfo *mi = &bootinfo.mem;
- paddr_t min_size;
- paddr_t paddr = 0;
- int i;
-
- min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
-
- /* Find the highest bank with enough space. */
- for ( i = 0; i < mi->nr_banks; i++ )
- {
- const struct membank *bank = &mi->bank[i];
- paddr_t s, e;
-
- if ( bank->size >= min_size )
- {
- e = consider_modules(bank->start, bank->start + bank->size,
- min_size, XEN_PADDR_ALIGN, 0);
- if ( !e )
- continue;
-
-#ifdef CONFIG_ARM_32
- /* Xen must be under 4GB */
- if ( e > 0x100000000ULL )
- e = 0x100000000ULL;
- if ( e < bank->start )
- continue;
-#endif
-
- s = e - min_size;
-
- if ( s > paddr )
- paddr = s;
- }
- }
-
- if ( !paddr )
- panic("Not enough memory to relocate Xen\n");
-
- printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
- paddr, paddr + min_size);
-
- return paddr;
-}
-
static void __init init_pdx(void)
{
paddr_t bank_start, bank_size, bank_end;
@@ -783,7 +732,6 @@ void __init start_xen(unsigned long boot_phys_offset,
{
size_t fdt_size;
int cpus, i;
- paddr_t xen_paddr;
const char *cmdline;
struct bootmodule *xen_bootmodule;
struct domain *dom0;
@@ -827,14 +775,7 @@ void __init start_xen(unsigned long boot_phys_offset,
(paddr_t)(uintptr_t)(_end - _start + 1), false);
BUG_ON(!xen_bootmodule);
- xen_paddr = get_xen_paddr();
- setup_pagetables(boot_phys_offset, xen_paddr);
-
- /* Update Xen's address now that we have relocated. */
- printk("Update BOOTMOD_XEN from %"PRIpaddr"-%"PRIpaddr" => %"PRIpaddr"-%"PRIpaddr"\n",
- xen_bootmodule->start, xen_bootmodule->start + xen_bootmodule->size,
- xen_paddr, xen_paddr + xen_bootmodule->size);
- xen_bootmodule->start = xen_paddr;
+ setup_pagetables(boot_phys_offset);
setup_mm(fdt_paddr, fdt_size);
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index b2f6104a7f..eafa26f56e 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -169,7 +169,7 @@ extern unsigned long total_pages;
#define PDX_GROUP_SHIFT SECOND_SHIFT
/* Boot-time pagetable setup */
-extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
+extern void setup_pagetables(unsigned long boot_phys_offset);
/* Map FDT in boot pagetable */
extern void *early_fdt_map(paddr_t fdt_paddr);
/* Remove early mappings */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes
2018-11-29 11:37 [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Julien Grall
2018-11-29 11:37 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
2018-11-29 11:37 ` [PATCH 2/2] xen/arm: Stop relocating Xen Julien Grall
@ 2018-12-01 1:27 ` Matthew Daley
2 siblings, 0 replies; 9+ messages in thread
From: Matthew Daley @ 2018-12-01 1:27 UTC (permalink / raw)
To: julien.grall
Cc: xen-devel, Jan-Peter.Larsson, Stefano Stabellini,
shameerali.kolothum.thodi, andre.przywara
Hi Julien,
These patches fix my Hikey960 problems - I'm able to boot all expected
CPUs without trouble now.
Feel free to add
Tested-by: Matthew Daley <mattd@bugfuzz.com>
if you want.
Thanks for the good work!
- Matthew
On Fri, 30 Nov 2018 at 00:37, Julien Grall <julien.grall@arm.com> wrote:
>
> Hi all,
>
> This patch series fixes 2 bug in the boot code for the memory management.
>
> The first patch should resolve Xen stall when setting SCTLR.XN on some
> platforms.
>
> The second patch should allow to boot Xen again the Hikey board.
>
> Cheers,
>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Jan-Peter Larsson <Jan-Peter.Larsson@arm.com>
> Cc: Matthew Daley <mattd@bugfuzz.com>
>
> Julien Grall (2):
> xen/arm: mm: Set-up page permission for Xen mappings earlier on
> xen/arm: Stop relocating Xen
>
> xen/arch/arm/arm32/head.S | 54 +++-----------------------------------
> xen/arch/arm/arm64/head.S | 50 +++++------------------------------
> xen/arch/arm/mm.c | 67 ++++++++++++++++++-----------------------------
> xen/arch/arm/setup.c | 65 +++------------------------------------------
> xen/include/asm-arm/mm.h | 2 +-
> 5 files changed, 39 insertions(+), 199 deletions(-)
>
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xen/arm: Stop relocating Xen
2018-11-29 11:37 ` [PATCH 2/2] xen/arm: Stop relocating Xen Julien Grall
@ 2018-12-12 23:53 ` Stefano Stabellini
2018-12-14 10:41 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2018-12-12 23:53 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, sstabellini, andre.przywara
On Thu, 29 Nov 2018, Julien Grall wrote:
> At the moment, Xen is relocated towards the end of the memory. While
> this has the advantage to free space in low memory, the code is not
> compliant with the break-before-make because it requires to switch
> between two sets of page-table. This is not entirely trivial to fix as
> it would require us to go through an identity mapping and disabling MMU.
>
> Furthermore, it looks like that some platform (such as the Hikey960)
> may not be able to bring-up secondary CPUs if the entry is too high.
>
> I don't believe the low memory is an issue because Xen is quite tiny
> (< 2MB). So the best solution is to stop relocating Xen. This has the
> advantage to simplify the code and should speed-up the boot as relocation
> is not necessary anymore.
>
> Note that the break-before-make issue is not fixed by this patch.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reported-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> xen/arch/arm/arm32/head.S | 54 +++------------------------------------
> xen/arch/arm/arm64/head.S | 50 +++++-------------------------------
> xen/arch/arm/mm.c | 20 ++++-----------
> xen/arch/arm/setup.c | 65 +++--------------------------------------------
> xen/include/asm-arm/mm.h | 2 +-
> 5 files changed, 18 insertions(+), 173 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 93b51e9ef2..390a505e05 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -469,58 +469,12 @@ fail: PRINT("- Boot failed -\r\n")
> GLOBAL(_end_boot)
>
> /*
> - * Copy Xen to new location and switch TTBR
> + * Switch TTBR
> * r1:r0 ttbr
> - * r2 source address
> - * r3 destination address
> - * [sp]=>r4 length
> *
> - * Source and destination must be word aligned, length is rounded up
> - * to a 16 byte boundary.
> - *
> - * MUST BE VERY CAREFUL when saving things to RAM over the copy
> + * TODO: This code does not comply with break-before-make.
> */
> -ENTRY(relocate_xen)
> - push {r4,r5,r6,r7,r8,r9,r10,r11}
> -
> - ldr r4, [sp, #8*4] /* Get 4th argument from stack */
> -
> - /* Copy 16 bytes at a time using:
> - * r5: counter
> - * r6: data
> - * r7: data
> - * r8: data
> - * r9: data
> - * r10: source
> - * r11: destination
> - */
> - mov r5, r4
> - mov r10, r2
> - mov r11, r3
> -1: ldmia r10!, {r6, r7, r8, r9}
> - stmia r11!, {r6, r7, r8, r9}
> -
> - subs r5, r5, #16
> - bgt 1b
> -
> - /* Flush destination from dcache using:
> - * r5: counter
> - * r6: step
> - * r7: vaddr
> - */
> - dsb /* So the CPU issues all writes to the range */
> -
> - mov r5, r4
> - ldr r6, =dcache_line_bytes /* r6 := step */
> - ldr r6, [r6]
> - mov r7, r3
> -
> -1: mcr CP32(r7, DCCMVAC)
> -
> - add r7, r7, r6
> - subs r5, r5, r6
> - bgt 1b
> -
> +ENTRY(switch_ttbr)
> dsb /* Ensure the flushes happen before
> * continuing */
> isb /* Ensure synchronization with previous
> @@ -543,8 +497,6 @@ ENTRY(relocate_xen)
> dsb /* Ensure completion of TLB+BP flush */
> isb
>
> - pop {r4, r5,r6,r7,r8,r9,r10,r11}
> -
> mov pc, lr
>
> #ifdef CONFIG_EARLY_PRINTK
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 9428c3f5a2..4589a37874 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -605,52 +605,14 @@ fail: PRINT("- Boot failed -\r\n")
>
> GLOBAL(_end_boot)
>
> -/* Copy Xen to new location and switch TTBR
> - * x0 ttbr
> - * x1 source address
> - * x2 destination address
> - * x3 length
> +/*
> + * Switch TTBR
> *
> - * Source and destination must be word aligned, length is rounded up
> - * to a 16 byte boundary.
> + * x0 ttbr
> *
> - * MUST BE VERY CAREFUL when saving things to RAM over the copy */
> -ENTRY(relocate_xen)
> - /* Copy 16 bytes at a time using:
> - * x9: counter
> - * x10: data
> - * x11: data
> - * x12: source
> - * x13: destination
> - */
> - mov x9, x3
> - mov x12, x1
> - mov x13, x2
> -
> -1: ldp x10, x11, [x12], #16
> - stp x10, x11, [x13], #16
> -
> - subs x9, x9, #16
> - bgt 1b
> -
> - /* Flush destination from dcache using:
> - * x9: counter
> - * x10: step
> - * x11: vaddr
> - */
> - dsb sy /* So the CPU issues all writes to the range */
> -
> - mov x9, x3
> - ldr x10, =dcache_line_bytes /* x10 := step */
> - ldr x10, [x10]
> - mov x11, x2
> -
> -1: dc cvac, x11
> -
> - add x11, x11, x10
> - subs x9, x9, x10
> - bgt 1b
> -
> + * TODO: This code does not comply with break-before-make.
> + */
> +ENTRY(switch_ttbr)
> dsb sy /* Ensure the flushes happen before
> * continuing */
> isb /* Ensure synchronization with previous
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 2556e57a99..f6931e007f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -601,7 +601,7 @@ void __init remove_early_mappings(void)
> flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
> }
>
> -extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
> +extern void switch_ttbr(uint64_t ttbr);
>
> /* Clear a translation table and clean & invalidate the cache */
> static void clear_table(void *table)
> @@ -612,15 +612,13 @@ static void clear_table(void *table)
>
> /* Boot-time pagetable setup.
> * Changes here may need matching changes in head.S */
> -void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
> +void __init setup_pagetables(unsigned long boot_phys_offset)
> {
> uint64_t ttbr;
> - unsigned long dest_va;
> lpae_t pte, *p;
> int i;
>
> - /* Calculate virt-to-phys offset for the new location */
> - phys_offset = xen_paddr - (unsigned long) _start;
> + phys_offset = boot_phys_offset;
>
> #ifdef CONFIG_ARM_64
> p = (void *) xen_pgtable;
> @@ -652,7 +650,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
> /* Break up the Xen mapping into 4k pages and protect them separately. */
> for ( i = 0; i < LPAE_ENTRIES; i++ )
> {
> - mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
> + mfn_t mfn = mfn_add(maddr_to_mfn((vaddr_t)_start + phys_offset), i);
Why (vaddr_t) instead of (paddr_t) or (unsigned long)? I understand that
vaddr_t is sufficient but paddr_t or unsigned long would be more
consistent and appropriate?
> unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
>
> if ( !is_kernel(va) )
> @@ -687,21 +685,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
> pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
> xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
>
> - /* ... Boot Misc area for xen relocation */
> - dest_va = BOOT_RELOC_VIRT_START;
let's take this opportunity to get rid of BOOT_RELOC_VIRT_START
completely
These are two minor issues -- I tested this patch on the ZynqMP and it
works fine.
> - pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
> - /* Map the destination in xen_second. */
> - xen_second[second_table_offset(dest_va)] = pte;
> - /* Map the destination in boot_second. */
> - write_pte(boot_second + second_table_offset(dest_va), pte);
> - flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE);
> #ifdef CONFIG_ARM_64
> ttbr = (uintptr_t) xen_pgtable + phys_offset;
> #else
> ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
> #endif
>
> - relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
> + switch_ttbr(ttbr);
>
> /* Clear the copy of the boot pagetables. Each secondary CPU
> * rebuilds these itself (see head.S) */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index aec53f30d3..e84172fbc3 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -374,6 +374,7 @@ void __init discard_initial_modules(void)
> remove_early_mappings();
> }
>
> +#ifdef CONFIG_ARM_32
> /*
> * Returns the end address of the highest region in the range s..e
> * with required size and alignment that does not conflict with the
> @@ -440,6 +441,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
> }
> return e;
> }
> +#endif
>
> /*
> * Return the end of the non-module region starting at s. In other
> @@ -475,59 +477,6 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
> return lowest;
> }
>
> -
> -/**
> - * get_xen_paddr - get physical address to relocate Xen to
> - *
> - * Xen is relocated to as near to the top of RAM as possible and
> - * aligned to a XEN_PADDR_ALIGN boundary.
> - */
> -static paddr_t __init get_xen_paddr(void)
> -{
> - struct meminfo *mi = &bootinfo.mem;
> - paddr_t min_size;
> - paddr_t paddr = 0;
> - int i;
> -
> - min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
> -
> - /* Find the highest bank with enough space. */
> - for ( i = 0; i < mi->nr_banks; i++ )
> - {
> - const struct membank *bank = &mi->bank[i];
> - paddr_t s, e;
> -
> - if ( bank->size >= min_size )
> - {
> - e = consider_modules(bank->start, bank->start + bank->size,
> - min_size, XEN_PADDR_ALIGN, 0);
> - if ( !e )
> - continue;
> -
> -#ifdef CONFIG_ARM_32
> - /* Xen must be under 4GB */
> - if ( e > 0x100000000ULL )
> - e = 0x100000000ULL;
> - if ( e < bank->start )
> - continue;
> -#endif
> -
> - s = e - min_size;
> -
> - if ( s > paddr )
> - paddr = s;
> - }
> - }
> -
> - if ( !paddr )
> - panic("Not enough memory to relocate Xen\n");
> -
> - printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
> - paddr, paddr + min_size);
> -
> - return paddr;
> -}
> -
> static void __init init_pdx(void)
> {
> paddr_t bank_start, bank_size, bank_end;
> @@ -783,7 +732,6 @@ void __init start_xen(unsigned long boot_phys_offset,
> {
> size_t fdt_size;
> int cpus, i;
> - paddr_t xen_paddr;
> const char *cmdline;
> struct bootmodule *xen_bootmodule;
> struct domain *dom0;
> @@ -827,14 +775,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> (paddr_t)(uintptr_t)(_end - _start + 1), false);
> BUG_ON(!xen_bootmodule);
>
> - xen_paddr = get_xen_paddr();
> - setup_pagetables(boot_phys_offset, xen_paddr);
> -
> - /* Update Xen's address now that we have relocated. */
> - printk("Update BOOTMOD_XEN from %"PRIpaddr"-%"PRIpaddr" => %"PRIpaddr"-%"PRIpaddr"\n",
> - xen_bootmodule->start, xen_bootmodule->start + xen_bootmodule->size,
> - xen_paddr, xen_paddr + xen_bootmodule->size);
> - xen_bootmodule->start = xen_paddr;
> + setup_pagetables(boot_phys_offset);
>
> setup_mm(fdt_paddr, fdt_size);
>
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index b2f6104a7f..eafa26f56e 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -169,7 +169,7 @@ extern unsigned long total_pages;
> #define PDX_GROUP_SHIFT SECOND_SHIFT
>
> /* Boot-time pagetable setup */
> -extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
> +extern void setup_pagetables(unsigned long boot_phys_offset);
> /* Map FDT in boot pagetable */
> extern void *early_fdt_map(paddr_t fdt_paddr);
> /* Remove early mappings */
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on
2018-11-29 11:37 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
@ 2018-12-12 23:54 ` Stefano Stabellini
0 siblings, 0 replies; 9+ messages in thread
From: Stefano Stabellini @ 2018-12-12 23:54 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, sstabellini, andre.przywara
On Thu, 29 Nov 2018, Julien Grall wrote:
> Xen mapping is first create using a 2MB page and then shatterred in 4KB
> page for fine-graine permission. However, it is not safe to break-down
> superpage page without going to an intermediate step invalidating
> the entry.
>
> As we are changing Xen mappings, we cannot go through the intermediate
> step. The only solution is to create Xen mapping using 4KB entries
> directly. As the Xen should always access the mappings according with
> the runtime permission, it is then possible to set-up the permissions
> while create the mapping.
>
> We are still playing with the fire as there are still some
> break-before-make issue in setup_pagetables (i.e switch between 2 sets of
> page-tables). But it should slightly be better than the current state.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Reported-by: Jan-Peter Larsson <Jan-Peter.Larsson@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
and committed
> ---
> I had few reports on new platforms where Xen reliably stale as soon as
> SCTLR.WXN is turned on. This likely happens because of not complying
> with Break-Before-Make when setting-up the permission as we
> break-down a superpage to 4KB mappings.
> ---
> xen/arch/arm/mm.c | 49 ++++++++++++++++++++++---------------------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 987fcb9162..2556e57a99 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -649,11 +649,31 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
> }
> #endif
>
> + /* Break up the Xen mapping into 4k pages and protect them separately. */
> + for ( i = 0; i < LPAE_ENTRIES; i++ )
> + {
> + mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
> + unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
> +
> + if ( !is_kernel(va) )
> + break;
> + pte = mfn_to_xen_entry(mfn, MT_NORMAL);
> + pte.pt.table = 1; /* 4k mappings always have this bit set */
> + if ( is_kernel_text(va) || is_kernel_inittext(va) )
> + {
> + pte.pt.xn = 0;
> + pte.pt.ro = 1;
> + }
> + if ( is_kernel_rodata(va) )
> + pte.pt.ro = 1;
> + xen_xenmap[i] = pte;
> + }
> +
> /* Initialise xen second level entries ... */
> /* ... Xen's text etc */
>
> - pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
> - pte.pt.xn = 0;/* Contains our text mapping! */
> + pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
> + pte.pt.table = 1;
> xen_second[second_table_offset(XEN_VIRT_START)] = pte;
>
> /* ... Fixmap */
> @@ -693,31 +713,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
> clear_table(boot_second);
> clear_table(boot_third);
>
> - /* Break up the Xen mapping into 4k pages and protect them separately. */
> - for ( i = 0; i < LPAE_ENTRIES; i++ )
> - {
> - mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
> - unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
> - if ( !is_kernel(va) )
> - break;
> - pte = mfn_to_xen_entry(mfn, MT_NORMAL);
> - pte.pt.table = 1; /* 4k mappings always have this bit set */
> - if ( is_kernel_text(va) || is_kernel_inittext(va) )
> - {
> - pte.pt.xn = 0;
> - pte.pt.ro = 1;
> - }
> - if ( is_kernel_rodata(va) )
> - pte.pt.ro = 1;
> - write_pte(xen_xenmap + i, pte);
> - /* No flush required here as page table is not hooked in yet. */
> - }
> -
> - pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
> - pte.pt.table = 1;
> - write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
> - /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */
> -
> /* From now on, no mapping may be both writable and executable. */
> WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
> /* Flush everything after setting WXN bit. */
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xen/arm: Stop relocating Xen
2018-12-12 23:53 ` Stefano Stabellini
@ 2018-12-14 10:41 ` Julien Grall
0 siblings, 0 replies; 9+ messages in thread
From: Julien Grall @ 2018-12-14 10:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, andre.przywara
Hi Stefano,
On 12/12/2018 23:53, Stefano Stabellini wrote:
> On Thu, 29 Nov 2018, Julien Grall wrote:
>> At the moment, Xen is relocated towards the end of the memory. While
>> this has the advantage to free space in low memory, the code is not
>> compliant with the break-before-make because it requires to switch
>> between two sets of page-table. This is not entirely trivial to fix as
>> it would require us to go through an identity mapping and disabling MMU.
>>
>> Furthermore, it looks like that some platform (such as the Hikey960)
>> may not be able to bring-up secondary CPUs if the entry is too high.
>>
>> I don't believe the low memory is an issue because Xen is quite tiny
>> (< 2MB). So the best solution is to stop relocating Xen. This has the
>> advantage to simplify the code and should speed-up the boot as relocation
>> is not necessary anymore.
>>
>> Note that the break-before-make issue is not fixed by this patch.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reported-by: Matthew Daley <mattd@bugfuzz.com>
>> ---
>> xen/arch/arm/arm32/head.S | 54 +++------------------------------------
>> xen/arch/arm/arm64/head.S | 50 +++++-------------------------------
>> xen/arch/arm/mm.c | 20 ++++-----------
>> xen/arch/arm/setup.c | 65 +++--------------------------------------------
>> xen/include/asm-arm/mm.h | 2 +-
>> 5 files changed, 18 insertions(+), 173 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 93b51e9ef2..390a505e05 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -469,58 +469,12 @@ fail: PRINT("- Boot failed -\r\n")
>> GLOBAL(_end_boot)
>>
>> /*
>> - * Copy Xen to new location and switch TTBR
>> + * Switch TTBR
>> * r1:r0 ttbr
>> - * r2 source address
>> - * r3 destination address
>> - * [sp]=>r4 length
>> *
>> - * Source and destination must be word aligned, length is rounded up
>> - * to a 16 byte boundary.
>> - *
>> - * MUST BE VERY CAREFUL when saving things to RAM over the copy
>> + * TODO: This code does not comply with break-before-make.
>> */
>> -ENTRY(relocate_xen)
>> - push {r4,r5,r6,r7,r8,r9,r10,r11}
>> -
>> - ldr r4, [sp, #8*4] /* Get 4th argument from stack */
>> -
>> - /* Copy 16 bytes at a time using:
>> - * r5: counter
>> - * r6: data
>> - * r7: data
>> - * r8: data
>> - * r9: data
>> - * r10: source
>> - * r11: destination
>> - */
>> - mov r5, r4
>> - mov r10, r2
>> - mov r11, r3
>> -1: ldmia r10!, {r6, r7, r8, r9}
>> - stmia r11!, {r6, r7, r8, r9}
>> -
>> - subs r5, r5, #16
>> - bgt 1b
>> -
>> - /* Flush destination from dcache using:
>> - * r5: counter
>> - * r6: step
>> - * r7: vaddr
>> - */
>> - dsb /* So the CPU issues all writes to the range */
>> -
>> - mov r5, r4
>> - ldr r6, =dcache_line_bytes /* r6 := step */
>> - ldr r6, [r6]
>> - mov r7, r3
>> -
>> -1: mcr CP32(r7, DCCMVAC)
>> -
>> - add r7, r7, r6
>> - subs r5, r5, r6
>> - bgt 1b
>> -
>> +ENTRY(switch_ttbr)
>> dsb /* Ensure the flushes happen before
>> * continuing */
>> isb /* Ensure synchronization with previous
>> @@ -543,8 +497,6 @@ ENTRY(relocate_xen)
>> dsb /* Ensure completion of TLB+BP flush */
>> isb
>>
>> - pop {r4, r5,r6,r7,r8,r9,r10,r11}
>> -
>> mov pc, lr
>>
>> #ifdef CONFIG_EARLY_PRINTK
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 9428c3f5a2..4589a37874 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -605,52 +605,14 @@ fail: PRINT("- Boot failed -\r\n")
>>
>> GLOBAL(_end_boot)
>>
>> -/* Copy Xen to new location and switch TTBR
>> - * x0 ttbr
>> - * x1 source address
>> - * x2 destination address
>> - * x3 length
>> +/*
>> + * Switch TTBR
>> *
>> - * Source and destination must be word aligned, length is rounded up
>> - * to a 16 byte boundary.
>> + * x0 ttbr
>> *
>> - * MUST BE VERY CAREFUL when saving things to RAM over the copy */
>> -ENTRY(relocate_xen)
>> - /* Copy 16 bytes at a time using:
>> - * x9: counter
>> - * x10: data
>> - * x11: data
>> - * x12: source
>> - * x13: destination
>> - */
>> - mov x9, x3
>> - mov x12, x1
>> - mov x13, x2
>> -
>> -1: ldp x10, x11, [x12], #16
>> - stp x10, x11, [x13], #16
>> -
>> - subs x9, x9, #16
>> - bgt 1b
>> -
>> - /* Flush destination from dcache using:
>> - * x9: counter
>> - * x10: step
>> - * x11: vaddr
>> - */
>> - dsb sy /* So the CPU issues all writes to the range */
>> -
>> - mov x9, x3
>> - ldr x10, =dcache_line_bytes /* x10 := step */
>> - ldr x10, [x10]
>> - mov x11, x2
>> -
>> -1: dc cvac, x11
>> -
>> - add x11, x11, x10
>> - subs x9, x9, x10
>> - bgt 1b
>> -
>> + * TODO: This code does not comply with break-before-make.
>> + */
>> +ENTRY(switch_ttbr)
>> dsb sy /* Ensure the flushes happen before
>> * continuing */
>> isb /* Ensure synchronization with previous
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 2556e57a99..f6931e007f 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -601,7 +601,7 @@ void __init remove_early_mappings(void)
>> flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
>> }
>>
>> -extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
>> +extern void switch_ttbr(uint64_t ttbr);
>>
>> /* Clear a translation table and clean & invalidate the cache */
>> static void clear_table(void *table)
>> @@ -612,15 +612,13 @@ static void clear_table(void *table)
>>
>> /* Boot-time pagetable setup.
>> * Changes here may need matching changes in head.S */
>> -void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>> +void __init setup_pagetables(unsigned long boot_phys_offset)
>> {
>> uint64_t ttbr;
>> - unsigned long dest_va;
>> lpae_t pte, *p;
>> int i;
>>
>> - /* Calculate virt-to-phys offset for the new location */
>> - phys_offset = xen_paddr - (unsigned long) _start;
>> + phys_offset = boot_phys_offset;
>>
>> #ifdef CONFIG_ARM_64
>> p = (void *) xen_pgtable;
>> @@ -652,7 +650,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>> /* Break up the Xen mapping into 4k pages and protect them separately. */
>> for ( i = 0; i < LPAE_ENTRIES; i++ )
>> {
>> - mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
>> + mfn_t mfn = mfn_add(maddr_to_mfn((vaddr_t)_start + phys_offset), i);
>
> Why (vaddr_t) instead of (paddr_t) or (unsigned long)? I understand that
> vaddr_t is sufficient but paddr_t or unsigned long would be more
> consistent and appropriate?
Consistent with what? You can't use (paddr_t) because on arm32 we are casting a
pointer from an integer of different size. Using "unsigned long" is arguable. If
you look at the code we use (vaddr_t) when we deal with virtual pointer.
Actually, some of the logic can actually be hidden using pte_of_xenaddr(...). I
will prepend a patch to clean-up that code.
>
>
>> unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
>>
>> if ( !is_kernel(va) )
>> @@ -687,21 +685,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>> pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
>> xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
>>
>> - /* ... Boot Misc area for xen relocation */
>> - dest_va = BOOT_RELOC_VIRT_START;
>
> let's take this opportunity to get rid of BOOT_RELOC_VIRT_START
> completely
Good idea. I will remove it.
>
>
> These are two minor issues -- I tested this patch on the ZynqMP and it
> works fine.
>
>
>> - pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
>> - /* Map the destination in xen_second. */
>> - xen_second[second_table_offset(dest_va)] = pte;
>> - /* Map the destination in boot_second. */
>> - write_pte(boot_second + second_table_offset(dest_va), pte);
>> - flush_xen_data_tlb_range_va_local(dest_va, SECOND_SIZE);
>> #ifdef CONFIG_ARM_64
>> ttbr = (uintptr_t) xen_pgtable + phys_offset;
>> #else
>> ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
>> #endif
>>
>> - relocate_xen(ttbr, _start, (void*)dest_va, _end - _start);
>> + switch_ttbr(ttbr);
>>
>> /* Clear the copy of the boot pagetables. Each secondary CPU
>> * rebuilds these itself (see head.S) */
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index aec53f30d3..e84172fbc3 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -374,6 +374,7 @@ void __init discard_initial_modules(void)
>> remove_early_mappings();
>> }
>>
>> +#ifdef CONFIG_ARM_32
>> /*
>> * Returns the end address of the highest region in the range s..e
>> * with required size and alignment that does not conflict with the
>> @@ -440,6 +441,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>> }
>> return e;
>> }
>> +#endif
>>
>> /*
>> * Return the end of the non-module region starting at s. In other
>> @@ -475,59 +477,6 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>> return lowest;
>> }
>>
>> -
>> -/**
>> - * get_xen_paddr - get physical address to relocate Xen to
>> - *
>> - * Xen is relocated to as near to the top of RAM as possible and
>> - * aligned to a XEN_PADDR_ALIGN boundary.
>> - */
>> -static paddr_t __init get_xen_paddr(void)
>> -{
>> - struct meminfo *mi = &bootinfo.mem;
>> - paddr_t min_size;
>> - paddr_t paddr = 0;
>> - int i;
>> -
>> - min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
>> -
>> - /* Find the highest bank with enough space. */
>> - for ( i = 0; i < mi->nr_banks; i++ )
>> - {
>> - const struct membank *bank = &mi->bank[i];
>> - paddr_t s, e;
>> -
>> - if ( bank->size >= min_size )
>> - {
>> - e = consider_modules(bank->start, bank->start + bank->size,
>> - min_size, XEN_PADDR_ALIGN, 0);
>> - if ( !e )
>> - continue;
>> -
>> -#ifdef CONFIG_ARM_32
>> - /* Xen must be under 4GB */
>> - if ( e > 0x100000000ULL )
>> - e = 0x100000000ULL;
>> - if ( e < bank->start )
>> - continue;
>> -#endif
>> -
>> - s = e - min_size;
>> -
>> - if ( s > paddr )
>> - paddr = s;
>> - }
>> - }
>> -
>> - if ( !paddr )
>> - panic("Not enough memory to relocate Xen\n");
>> -
>> - printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
>> - paddr, paddr + min_size);
>> -
>> - return paddr;
>> -}
>> -
>> static void __init init_pdx(void)
>> {
>> paddr_t bank_start, bank_size, bank_end;
>> @@ -783,7 +732,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>> {
>> size_t fdt_size;
>> int cpus, i;
>> - paddr_t xen_paddr;
>> const char *cmdline;
>> struct bootmodule *xen_bootmodule;
>> struct domain *dom0;
>> @@ -827,14 +775,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>> (paddr_t)(uintptr_t)(_end - _start + 1), false);
>> BUG_ON(!xen_bootmodule);
>>
>> - xen_paddr = get_xen_paddr();
>> - setup_pagetables(boot_phys_offset, xen_paddr);
>> -
>> - /* Update Xen's address now that we have relocated. */
>> - printk("Update BOOTMOD_XEN from %"PRIpaddr"-%"PRIpaddr" => %"PRIpaddr"-%"PRIpaddr"\n",
>> - xen_bootmodule->start, xen_bootmodule->start + xen_bootmodule->size,
>> - xen_paddr, xen_paddr + xen_bootmodule->size);
>> - xen_bootmodule->start = xen_paddr;
>> + setup_pagetables(boot_phys_offset);
>>
>> setup_mm(fdt_paddr, fdt_size);
>>
>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>> index b2f6104a7f..eafa26f56e 100644
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -169,7 +169,7 @@ extern unsigned long total_pages;
>> #define PDX_GROUP_SHIFT SECOND_SHIFT
>>
>> /* Boot-time pagetable setup */
>> -extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
>> +extern void setup_pagetables(unsigned long boot_phys_offset);
>> /* Map FDT in boot pagetable */
>> extern void *early_fdt_map(paddr_t fdt_paddr);
>> /* Remove early mappings */
>> --
>> 2.11.0
>>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on
2018-11-29 11:38 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
@ 2018-11-29 16:51 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 9+ messages in thread
From: Shameerali Kolothum Thodi @ 2018-11-29 16:51 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Stefano Stabellini, xuwei (O)
Hi Julien,
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of
> Julien Grall
> Sent: 29 November 2018 11:39
> To: julien.grall@arm.com; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Subject: [Xen-devel] [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen
> mappings earlier on
>
> Xen mapping is first create using a 2MB page and then shatterred in 4KB
> page for fine-graine permission. However, it is not safe to break-down
> superpage page without going to an intermediate step invalidating
> the entry.
>
> As we are changing Xen mappings, we cannot go through the intermediate
> step. The only solution is to create Xen mapping using 4KB entries
> directly. As the Xen should always access the mappings according with
> the runtime permission, it is then possible to set-up the permissions
> while create the mapping.
>
> We are still playing with the fire as there are still some
> break-before-make issue in setup_pagetables (i.e switch between 2 sets of
> page-tables). But it should slightly be better than the current state.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reported-by: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Reported-by: Jan-Peter Larsson <Jan-Peter.Larsson@arm.com>
>
> ---
> I had few reports on new platforms where Xen reliably stale as soon as
> SCTLR.WXN is turned on. This likely happens because of not complying
> with Break-Before-Make when setting-up the permission as we
> break-down a superpage to 4KB mappings.
Thanks for this. I can confirm that one of our platform on which we observed
the boot stall issue just after SCTLR.WXN is turned on, can now boot with
this patch.
FWIW:
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Cheers,
Shameer
> ---
> xen/arch/arm/mm.c | 49 ++++++++++++++++++++++---------------------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 987fcb9162..2556e57a99 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -649,11 +649,31 @@ void __init setup_pagetables(unsigned long
> boot_phys_offset, paddr_t xen_paddr)
> }
> #endif
>
> + /* Break up the Xen mapping into 4k pages and protect them separately. */
> + for ( i = 0; i < LPAE_ENTRIES; i++ )
> + {
> + mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
> + unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
> +
> + if ( !is_kernel(va) )
> + break;
> + pte = mfn_to_xen_entry(mfn, MT_NORMAL);
> + pte.pt.table = 1; /* 4k mappings always have this bit set */
> + if ( is_kernel_text(va) || is_kernel_inittext(va) )
> + {
> + pte.pt.xn = 0;
> + pte.pt.ro = 1;
> + }
> + if ( is_kernel_rodata(va) )
> + pte.pt.ro = 1;
> + xen_xenmap[i] = pte;
> + }
> +
> /* Initialise xen second level entries ... */
> /* ... Xen's text etc */
>
> - pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
> - pte.pt.xn = 0;/* Contains our text mapping! */
> + pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
> + pte.pt.table = 1;
> xen_second[second_table_offset(XEN_VIRT_START)] = pte;
>
> /* ... Fixmap */
> @@ -693,31 +713,6 @@ void __init setup_pagetables(unsigned long
> boot_phys_offset, paddr_t xen_paddr)
> clear_table(boot_second);
> clear_table(boot_third);
>
> - /* Break up the Xen mapping into 4k pages and protect them separately. */
> - for ( i = 0; i < LPAE_ENTRIES; i++ )
> - {
> - mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
> - unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
> - if ( !is_kernel(va) )
> - break;
> - pte = mfn_to_xen_entry(mfn, MT_NORMAL);
> - pte.pt.table = 1; /* 4k mappings always have this bit set */
> - if ( is_kernel_text(va) || is_kernel_inittext(va) )
> - {
> - pte.pt.xn = 0;
> - pte.pt.ro = 1;
> - }
> - if ( is_kernel_rodata(va) )
> - pte.pt.ro = 1;
> - write_pte(xen_xenmap + i, pte);
> - /* No flush required here as page table is not hooked in yet. */
> - }
> -
> - pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
> - pte.pt.table = 1;
> - write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
> - /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */
> -
> /* From now on, no mapping may be both writable and executable. */
> WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
> /* Flush everything after setting WXN bit. */
> --
> 2.11.0
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on
[not found] <20181129113836.2853-1-julien.grall@arm.com>
@ 2018-11-29 11:38 ` Julien Grall
2018-11-29 16:51 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2018-11-29 11:38 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: Stefano Stabellini
Xen mapping is first create using a 2MB page and then shatterred in 4KB
page for fine-graine permission. However, it is not safe to break-down
superpage page without going to an intermediate step invalidating
the entry.
As we are changing Xen mappings, we cannot go through the intermediate
step. The only solution is to create Xen mapping using 4KB entries
directly. As the Xen should always access the mappings according with
the runtime permission, it is then possible to set-up the permissions
while create the mapping.
We are still playing with the fire as there are still some
break-before-make issue in setup_pagetables (i.e switch between 2 sets of
page-tables). But it should slightly be better than the current state.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Reported-by: Jan-Peter Larsson <Jan-Peter.Larsson@arm.com>
---
I had few reports on new platforms where Xen reliably stale as soon as
SCTLR.WXN is turned on. This likely happens because of not complying
with Break-Before-Make when setting-up the permission as we
break-down a superpage to 4KB mappings.
---
xen/arch/arm/mm.c | 49 ++++++++++++++++++++++---------------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 987fcb9162..2556e57a99 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -649,11 +649,31 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
}
#endif
+ /* Break up the Xen mapping into 4k pages and protect them separately. */
+ for ( i = 0; i < LPAE_ENTRIES; i++ )
+ {
+ mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
+ unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
+
+ if ( !is_kernel(va) )
+ break;
+ pte = mfn_to_xen_entry(mfn, MT_NORMAL);
+ pte.pt.table = 1; /* 4k mappings always have this bit set */
+ if ( is_kernel_text(va) || is_kernel_inittext(va) )
+ {
+ pte.pt.xn = 0;
+ pte.pt.ro = 1;
+ }
+ if ( is_kernel_rodata(va) )
+ pte.pt.ro = 1;
+ xen_xenmap[i] = pte;
+ }
+
/* Initialise xen second level entries ... */
/* ... Xen's text etc */
- pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
- pte.pt.xn = 0;/* Contains our text mapping! */
+ pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
+ pte.pt.table = 1;
xen_second[second_table_offset(XEN_VIRT_START)] = pte;
/* ... Fixmap */
@@ -693,31 +713,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
clear_table(boot_second);
clear_table(boot_third);
- /* Break up the Xen mapping into 4k pages and protect them separately. */
- for ( i = 0; i < LPAE_ENTRIES; i++ )
- {
- mfn_t mfn = mfn_add(maddr_to_mfn(xen_paddr), i);
- unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
- if ( !is_kernel(va) )
- break;
- pte = mfn_to_xen_entry(mfn, MT_NORMAL);
- pte.pt.table = 1; /* 4k mappings always have this bit set */
- if ( is_kernel_text(va) || is_kernel_inittext(va) )
- {
- pte.pt.xn = 0;
- pte.pt.ro = 1;
- }
- if ( is_kernel_rodata(va) )
- pte.pt.ro = 1;
- write_pte(xen_xenmap + i, pte);
- /* No flush required here as page table is not hooked in yet. */
- }
-
- pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
- pte.pt.table = 1;
- write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
- /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */
-
/* From now on, no mapping may be both writable and executable. */
WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
/* Flush everything after setting WXN bit. */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-14 10:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-29 11:37 [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Julien Grall
2018-11-29 11:37 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
2018-12-12 23:54 ` Stefano Stabellini
2018-11-29 11:37 ` [PATCH 2/2] xen/arm: Stop relocating Xen Julien Grall
2018-12-12 23:53 ` Stefano Stabellini
2018-12-14 10:41 ` Julien Grall
2018-12-01 1:27 ` [PATCH for-4.12 0/2] xen/arm: mm: Boot fixes Matthew Daley
[not found] <20181129113836.2853-1-julien.grall@arm.com>
2018-11-29 11:38 ` [PATCH 1/2] xen/arm: mm: Set-up page permission for Xen mappings earlier on Julien Grall
2018-11-29 16:51 ` Shameerali Kolothum Thodi
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.