|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.9 4/4] xen/arm: Properly map the FDT in the boot page table
On Wed, 19 Apr 2017, Julien Grall wrote:
> Hi Stefano,
>
> On 19/04/2017 22:01, Stefano Stabellini wrote:
> > On Wed, 19 Apr 2017, Julien Grall wrote:
> > > Currently, Xen is assuming the FDT will always fit in a 2MB section.
> > > Recently, I noticed an early crash on Xen when using GRUB with the
> > > following call trace:
> > >
> > > (XEN) Hypervisor Trap. HSR=0x96000006 EC=0x25 IL=1 Syndrome=0x6
> > > (XEN) CPU0: Unexpected Trap: Hypervisor
> > > (XEN) ----[ Xen-4.9-unstable arm64 debug=y Not tainted ]----
> > > (XEN) CPU: 0
> > > (XEN) PC: 0000000000264140 strlen+0x10/0x84
> > > (XEN) LR: 00000000002401c0
> > > (XEN) SP: 00000000002cfc20
> > > (XEN) CPSR: 400003c9 MODE:64-bit EL2h (Hypervisor, handler)
> > > (XEN) X0: 0000000000801230 X1: 0000000000801230 X2:
> > > 0000000000005230
> > > (XEN) X3: 0000000000000030 X4: 0000000000000030 X5:
> > > 0000000000000038
> > > (XEN) X6: 0000000000000034 X7: 0000000000000000 X8:
> > > 7f7f7f7f7f7f7f7f
> > > (XEN) X9: 64622c6479687222 X10: 7f7f7f7f7f7f7f7f X11:
> > > 0101010101010101
> > > (XEN) X12: 0000000000000030 X13: ffffff00ff000000 X14:
> > > 0800000003000000
> > > (XEN) X15: ffffffffffffffff X16: 00000000fefff610 X17:
> > > 00000000000000f0
> > > (XEN) X18: 0000000000000004 X19: 0000000000000008 X20:
> > > 00000000007fc040
> > > (XEN) X21: 00000000007fc000 X22: 000000000000000e X23:
> > > 0000000000000000
> > > (XEN) X24: 00000000002a9f58 X25: 0000000000801230 X26:
> > > 00000000002a9f68
> > > (XEN) X27: 00000000002a9f58 X28: 0000000000298910 FP:
> > > 00000000002cfc20
> > > (XEN)
> > > (XEN) VTCR_EL2: 80010c40
> > > (XEN) VTTBR_EL2: 0000082800203000
> > > (XEN)
> > > (XEN) SCTLR_EL2: 30c5183d
> > > (XEN) HCR_EL2: 000000000038663f
> > > (XEN) TTBR0_EL2: 00000000f4912000
> > > (XEN)
> > > (XEN) ESR_EL2: 96000006
> > > (XEN) HPFAR_EL2: 00000000e8071000
> > > (XEN) FAR_EL2: 0000000000801230
> > > (XEN)
> > > (XEN) Xen stack trace from sp=00000000002cfc20:
> > > (XEN) 00000000002cfc70 0000000000240254 00000000002a9f58
> > > 00000000007fc000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 00000000007fc03c
> > > (XEN) 00000000002cfd78 0000000000000000 00000000002cfca0
> > > 00000000002986fc
> > > (XEN) 0000000000000000 00000000007fc000 0000000000000000
> > > 0000000000000000
> > > (XEN) 00000000002cfcc0 0000000000298f1c 0000000000000000
> > > 00000000007fc000
> > > (XEN) 00000000002cfdc0 000000000029904c 00000000f47fc000
> > > 00000000f4604000
> > > (XEN) 00000000f47fc000 00000000007fc000 0000000000400000
> > > 0000000000000100
> > > (XEN) 00000000f4604000 0000000000000001 0000000000000001
> > > 8000000000000002
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 00000000002cfdc0
> > > 0000000000299038
> > > (XEN) 00000000f47fc000 00000000f4604000 00000000f47fc000
> > > 0000000000000000
> > > (XEN) 00000000002cfe20 000000000029c420 00000000002d8000
> > > 00000000f4604000
> > > (XEN) 00000000f47fc000 0000000000000000 0000000000400000
> > > 0000000000000100
> > > (XEN) 00000000f4604000 0000000000000001 00000000f47fc000
> > > 000000000029c404
> > > (XEN) 00000000fefff510 0000000000200624 00000000f4804000
> > > 00000000f4604000
> > > (XEN) 00000000f47fc000 0000000000000000 0000000000400000
> > > 0000000000000100
> > > (XEN) 0000000000000001 0000000000000001 0000000000000001
> > > 8000000000000002
> > > (XEN) 00000000f47fc000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) 0000000000000000 0000000000000000 0000000000000000
> > > 0000000000000000
> > > (XEN) Xen call trace:
> > > (XEN) [<0000000000264140>] strlen+0x10/0x84 (PC)
> > > (XEN) [<00000000002401c0>] fdt_get_property_namelen+0x9c/0xf0 (LR)
> > > (XEN) [<0000000000240254>] fdt_get_property+0x40/0x50
> > > (XEN) [<00000000002986fc>] bootfdt.c#device_tree_get_u32+0x18/0x5c
> > > (XEN) [<0000000000298f1c>] device_tree_for_each_node+0x84/0x144
> > > (XEN) [<000000000029904c>] boot_fdt_info+0x70/0x23c
> > > (XEN) [<000000000029c420>] start_xen+0x9c/0xd30
> > > (XEN) [<0000000000200624>] arm64/head.o#paging+0x84/0xbc
> > > (XEN)
> > > (XEN)
> > > (XEN) ****************************************
> > > (XEN) Panic on CPU 0:
> > > (XEN) CPU0: Unexpected Trap: Hypervisor
> > > (XEN)
> > > (XEN) ****************************************
> > >
> > > Indeed, the booting documentation for AArch32 and AArch64 only requires
> > > the FDT to be placed on a 8-byte boundary. This means the Device-Tree can
> > > cross a 2MB boundary.
> > >
> > > Given that Xen limits the size of the FDT to 2MB, it will always fit in
> > > a 4MB slot. So extend the fixmap slot for FDT from 2MB to 4MB.
> > >
> > > The second 2MB superpage will only be mapped if the FDT is cross the 2MB
> > > boundary.
> > >
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > > xen/arch/arm/mm.c | 16 ++++++++++++++--
> > > xen/include/asm-arm/config.h | 14 +++++++-------
> > > 2 files changed, 21 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 53d36e2ce2..9cff1619c6 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -451,6 +451,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> > > paddr_t base_paddr = fdt_paddr & SECOND_MASK;
> > > paddr_t offset;
> > > void *fdt_virt;
> > > + uint32_t size;
> > >
> > > /*
> > > * Check whether the physical FDT address is set and meets the
> > > minimum
> > > @@ -475,9 +476,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> > > if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> > > return NULL;
> > >
> > > - if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
> > > + size = fdt_totalsize(fdt_virt);
> > > + if ( size > MAX_FDT_SIZE )
> > > return NULL;
> > >
> > > + if ( (offset + size) > SZ_2M )
> > > + {
> > > + create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
> > > + paddr_to_pfn(base_paddr + SZ_2M),
> > > + SZ_2M >> PAGE_SHIFT, SZ_2M);
> > > + }
> > > +
> > > return fdt_virt;
> > > }
> > >
> > > @@ -485,7 +494,8 @@ void __init remove_early_mappings(void)
> > > {
> > > lpae_t pte = {0};
> > > write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START),
> > > pte);
> > > - flush_xen_data_tlb_range_va(BOOT_FDT_VIRT_START, SECOND_SIZE);
> > > + write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START),
> > > pte);
> >
> > Aren't you writing the same pte twice at the same address?
>
> It was meant to be BOOT_FDT_VIRT_START + SZ_2M as to remove the mapping from
> the table. I will send a new version with that fixed.
>
> >
> >
> > > + 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);
> > > @@ -544,6 +554,8 @@ void __init setup_pagetables(unsigned long
> > > boot_phys_offset, paddr_t xen_paddr)
> > > /* ... DTB */
> > > pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
> > > xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
> > > + 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;
> > > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> > > index 9c14a385e7..5b6f3c985d 100644
> > > --- a/xen/include/asm-arm/config.h
> > > +++ b/xen/include/asm-arm/config.h
> > > @@ -77,12 +77,12 @@
> > > * 0 - 2M Unmapped
> > > * 2M - 4M Xen text, data, bss
> > > * 4M - 6M Fixmap: special-purpose 4K mapping slots
> > > - * 6M - 8M Early boot mapping of FDT
> > > - * 8M - 10M Early relocation address (used when relocating Xen)
> > > + * 6M - 10M Early boot mapping of FDT
> > > + * 10M - 12M Early relocation address (used when relocating Xen)
> > > * and later for livepatch vmap (if compiled in)
> > > *
> > > * ARM32 layout:
> > > - * 0 - 10M <COMMON>
> > > + * 0 - 12M <COMMON>
> > > *
> > > * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM
> > > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address
> > > @@ -93,7 +93,7 @@
> > > *
> > > * ARM64 layout:
> > > * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> > > - * 0 - 10M <COMMON>
> > > + * 0 - 12M <COMMON>
> > > *
> > > * 1G - 2G VMAP: ioremap and early_ioremap
> > > *
> > > @@ -113,12 +113,12 @@
> > > #define FIXMAP_ADDR(n) (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> > >
> > > #define BOOT_FDT_VIRT_START _AT(vaddr_t,0x00600000)
> > > -#define BOOT_FDT_SLOT_SIZE MB(2)
> > > +#define BOOT_FDT_SLOT_SIZE MB(4)
> > > #define BOOT_FDT_VIRT_END (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
> > >
> > > -#define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00800000)
> > > +#define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00a00000)
> >
> > Wouldn't it be better to define it as BOOT_FDT_VIRT_END?
>
> It is kind of not really related to this patch and how we handle all the
> region in config.h today. So if we define in term of *_END this one, then we
> need to do it for everyone.
Fair enough
> Also, BOOT_RELOC_VIRT_END is not defined. IHMO, these changes are post Xen 4.9
> materials.
>
>> >
> > > #ifdef CONFIG_LIVEPATCH
> > > -#define LIVEPATCH_VMAP_START _AT(vaddr_t,0x00800000)
> > > +#define LIVEPATCH_VMAP_START _AT(vaddr_t,0x00a00000)
> >
> > same here
> >
> >
> > > #define LIVEPATCH_VMAP_END (LIVEPATCH_VMAP_START + MB(2))
> > > #endif
> > >
> > > --
> > > 2.11.0
> > >
>
> Cheers,
>
> --
> Julien Grall
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |