| Message ID | 20170419171311.3243-3-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Properly map the FDT in the boot page table | expand |
On Wed, 19 Apr 2017, Julien Grall wrote: > The FDT will not be accessed before start_xen (begining of C code) is > called and it will be easier to maintain as the code could be common > between AArch32 and AArch64. > > A new function early_fdt_map is introduced to map the FDT in the boot > page table. > > Note that create_mapping will flush the TLBs for us so no need to add an > extra on in case the entry was previously used by the 1:1 mapping. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > I can move the function create_mappings at the beginning of the > function instead of adding a static declaration. But I thought it > was not Xen 4.9 material. Any opinions? > --- > xen/arch/arm/arm32/head.S | 14 -------------- > xen/arch/arm/arm64/head.S | 13 ------------- > xen/arch/arm/mm.c | 20 ++++++++++++++++++++ > xen/arch/arm/setup.c | 4 +--- > xen/include/asm-arm/mm.h | 2 ++ > 5 files changed, 23 insertions(+), 30 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index ec63ba4c04..4090f4a744 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -389,20 +389,6 @@ paging: > /* Use a virtual address to access the UART. */ > ldr r11, =EARLY_UART_VIRTUAL_ADDRESS > #endif > - /* Map the DTB in the boot misc slot */ > - teq r12, #0 /* Only on boot CPU */ > - bne 1f > - > - ldr r1, =boot_second > - mov r3, #0x0 > - lsr r2, r8, #SECOND_SHIFT > - lsl r2, r2, #SECOND_SHIFT /* r2: 2MB-aligned paddr of DTB */ > - orr r2, r2, #PT_UPPER(MEM) > - orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */ > - ldr r4, =BOOT_FDT_VIRT_START > - mov r4, r4, lsr #(SECOND_SHIFT - 3) /* Slot for BOOT_FDT_VIRT_START */ > - strd r2, r3, [r1, r4] /* Map it in the early fdt slot */ > -1: > > /* > * Flush the TLB in case the 1:1 mapping happens to clash with > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 72ea4e0233..78292f4396 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -550,19 +550,6 @@ paging: > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > #endif > > - /* Map the DTB in the boot misc slot */ > - cbnz x22, 1f /* Only on boot CPU */ > - > - ldr x4, =boot_second /* x4 := vaddr (boot_second) */ > - lsr x2, x21, #SECOND_SHIFT > - lsl x2, x2, #SECOND_SHIFT /* x2 := 2MB-aligned paddr of DTB */ > - mov x3, #PT_MEM /* x2 := 2MB RAM incl. DTB */ > - orr x2, x2, x3 > - ldr x1, =BOOT_FDT_VIRT_START > - lsr x1, x1, #(SECOND_SHIFT - 3) /* x4 := Slot for BOOT_FDT_VIRT_START */ > - str x2, [x4, x1] /* Map it in the early fdt slot */ > -1: > - > /* > * Flush the TLB in case the 1:1 mapping happens to clash with > * the virtual addresses used by the fixmap or DTB. > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index f0a2eddaaf..0d076489c6 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -38,6 +38,13 @@ > #include <xen/vmap.h> > #include <xsm/xsm.h> > #include <xen/pfn.h> > +#include <xen/sizes.h> > + > +static void __init create_mappings(lpae_t *second, > + unsigned long virt_offset, > + unsigned long base_mfn, > + unsigned long nr_mfns, > + unsigned int mapping_size); I would have added early_fdt_map to this file in a way to avoid the need for duplicating the declaration of create_mappings (because this version doesn't have the useful comment on top). In any case Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > struct domain *dom_xen, *dom_io, *dom_cow; > > @@ -434,6 +441,19 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va) > return mfn_to_xen_entry(mfn, WRITEALLOC); > } > > + > +/* Map the FDT in the early boot page table */ > +void * __init early_fdt_map(paddr_t fdt_paddr) > +{ > + /* We are using 2MB superpage for mapping the FDT */ > + paddr_t base_paddr = fdt_paddr & SECOND_MASK; > + > + create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr), > + SZ_2M >> PAGE_SHIFT, SZ_2M); > + > + return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE); > +} > + > void __init remove_early_mappings(void) > { > lpae_t pte = {0}; > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 92a2de6b70..986398970f 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -724,9 +724,7 @@ void __init start_xen(unsigned long boot_phys_offset, > > smp_clear_cpu_maps(); > > - /* This is mapped by head.S */ > - device_tree_flattened = (void *)BOOT_FDT_VIRT_START > - + (fdt_paddr & ((1 << SECOND_SHIFT) - 1)); > + device_tree_flattened = early_fdt_map(fdt_paddr); > fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr); > > cmdline = boot_fdt_cmdline(device_tree_flattened); > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index 0fef612f42..f6915ad882 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -160,6 +160,8 @@ extern unsigned long total_pages; > > /* Boot-time pagetable setup */ > extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr); > +/* Map FDT in boot pagetable */ > +extern void *early_fdt_map(paddr_t fdt_paddr); > /* Remove early mappings */ > extern void remove_early_mappings(void); > /* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the > -- > 2.11.0 >
Hi Stefano, On 19/04/2017 22:01, Stefano Stabellini wrote: > On Wed, 19 Apr 2017, Julien Grall wrote: >> The FDT will not be accessed before start_xen (begining of C code) is >> called and it will be easier to maintain as the code could be common >> between AArch32 and AArch64. >> >> A new function early_fdt_map is introduced to map the FDT in the boot >> page table. >> >> Note that create_mapping will flush the TLBs for us so no need to add an >> extra on in case the entry was previously used by the 1:1 mapping. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> >> I can move the function create_mappings at the beginning of the >> function instead of adding a static declaration. But I thought it >> was not Xen 4.9 material. Any opinions? >> --- >> xen/arch/arm/arm32/head.S | 14 -------------- >> xen/arch/arm/arm64/head.S | 13 ------------- >> xen/arch/arm/mm.c | 20 ++++++++++++++++++++ >> xen/arch/arm/setup.c | 4 +--- >> xen/include/asm-arm/mm.h | 2 ++ >> 5 files changed, 23 insertions(+), 30 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index ec63ba4c04..4090f4a744 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -389,20 +389,6 @@ paging: >> /* Use a virtual address to access the UART. */ >> ldr r11, =EARLY_UART_VIRTUAL_ADDRESS >> #endif >> - /* Map the DTB in the boot misc slot */ >> - teq r12, #0 /* Only on boot CPU */ >> - bne 1f >> - >> - ldr r1, =boot_second >> - mov r3, #0x0 >> - lsr r2, r8, #SECOND_SHIFT >> - lsl r2, r2, #SECOND_SHIFT /* r2: 2MB-aligned paddr of DTB */ >> - orr r2, r2, #PT_UPPER(MEM) >> - orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */ >> - ldr r4, =BOOT_FDT_VIRT_START >> - mov r4, r4, lsr #(SECOND_SHIFT - 3) /* Slot for BOOT_FDT_VIRT_START */ >> - strd r2, r3, [r1, r4] /* Map it in the early fdt slot */ >> -1: >> >> /* >> * Flush the TLB in case the 1:1 mapping happens to clash with >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 72ea4e0233..78292f4396 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -550,19 +550,6 @@ paging: >> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >> #endif >> >> - /* Map the DTB in the boot misc slot */ >> - cbnz x22, 1f /* Only on boot CPU */ >> - >> - ldr x4, =boot_second /* x4 := vaddr (boot_second) */ >> - lsr x2, x21, #SECOND_SHIFT >> - lsl x2, x2, #SECOND_SHIFT /* x2 := 2MB-aligned paddr of DTB */ >> - mov x3, #PT_MEM /* x2 := 2MB RAM incl. DTB */ >> - orr x2, x2, x3 >> - ldr x1, =BOOT_FDT_VIRT_START >> - lsr x1, x1, #(SECOND_SHIFT - 3) /* x4 := Slot for BOOT_FDT_VIRT_START */ >> - str x2, [x4, x1] /* Map it in the early fdt slot */ >> -1: >> - >> /* >> * Flush the TLB in case the 1:1 mapping happens to clash with >> * the virtual addresses used by the fixmap or DTB. >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index f0a2eddaaf..0d076489c6 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -38,6 +38,13 @@ >> #include <xen/vmap.h> >> #include <xsm/xsm.h> >> #include <xen/pfn.h> >> +#include <xen/sizes.h> >> + >> +static void __init create_mappings(lpae_t *second, >> + unsigned long virt_offset, >> + unsigned long base_mfn, >> + unsigned long nr_mfns, >> + unsigned int mapping_size); > > I would have added early_fdt_map to this file in a way to avoid the need > for duplicating the declaration of create_mappings (because this version > doesn't have the useful comment on top). I wanted to keep the function close to the counterpart remove_early_mappings rather than adding somewhere that make less sense. Hence why I suggested to move the create_mappings function. Would you be fine with code motion for Xen 4.9? Cheers,
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: > > > The FDT will not be accessed before start_xen (begining of C code) is > > > called and it will be easier to maintain as the code could be common > > > between AArch32 and AArch64. > > > > > > A new function early_fdt_map is introduced to map the FDT in the boot > > > page table. > > > > > > Note that create_mapping will flush the TLBs for us so no need to add an > > > extra on in case the entry was previously used by the 1:1 mapping. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > > --- > > > > > > I can move the function create_mappings at the beginning of the > > > function instead of adding a static declaration. But I thought it > > > was not Xen 4.9 material. Any opinions? > > > --- > > > xen/arch/arm/arm32/head.S | 14 -------------- > > > xen/arch/arm/arm64/head.S | 13 ------------- > > > xen/arch/arm/mm.c | 20 ++++++++++++++++++++ > > > xen/arch/arm/setup.c | 4 +--- > > > xen/include/asm-arm/mm.h | 2 ++ > > > 5 files changed, 23 insertions(+), 30 deletions(-) > > > > > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > > > index ec63ba4c04..4090f4a744 100644 > > > --- a/xen/arch/arm/arm32/head.S > > > +++ b/xen/arch/arm/arm32/head.S > > > @@ -389,20 +389,6 @@ paging: > > > /* Use a virtual address to access the UART. */ > > > ldr r11, =EARLY_UART_VIRTUAL_ADDRESS > > > #endif > > > - /* Map the DTB in the boot misc slot */ > > > - teq r12, #0 /* Only on boot CPU */ > > > - bne 1f > > > - > > > - ldr r1, =boot_second > > > - mov r3, #0x0 > > > - lsr r2, r8, #SECOND_SHIFT > > > - lsl r2, r2, #SECOND_SHIFT /* r2: 2MB-aligned paddr of DTB */ > > > - orr r2, r2, #PT_UPPER(MEM) > > > - orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */ > > > - ldr r4, =BOOT_FDT_VIRT_START > > > - mov r4, r4, lsr #(SECOND_SHIFT - 3) /* Slot for > > > BOOT_FDT_VIRT_START */ > > > - strd r2, r3, [r1, r4] /* Map it in the early fdt slot */ > > > -1: > > > > > > /* > > > * Flush the TLB in case the 1:1 mapping happens to clash with > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > > > index 72ea4e0233..78292f4396 100644 > > > --- a/xen/arch/arm/arm64/head.S > > > +++ b/xen/arch/arm/arm64/head.S > > > @@ -550,19 +550,6 @@ paging: > > > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > > > #endif > > > > > > - /* Map the DTB in the boot misc slot */ > > > - cbnz x22, 1f /* Only on boot CPU */ > > > - > > > - ldr x4, =boot_second /* x4 := vaddr (boot_second) */ > > > - lsr x2, x21, #SECOND_SHIFT > > > - lsl x2, x2, #SECOND_SHIFT /* x2 := 2MB-aligned paddr of DTB */ > > > - mov x3, #PT_MEM /* x2 := 2MB RAM incl. DTB */ > > > - orr x2, x2, x3 > > > - ldr x1, =BOOT_FDT_VIRT_START > > > - lsr x1, x1, #(SECOND_SHIFT - 3) /* x4 := Slot for > > > BOOT_FDT_VIRT_START */ > > > - str x2, [x4, x1] /* Map it in the early fdt slot */ > > > -1: > > > - > > > /* > > > * Flush the TLB in case the 1:1 mapping happens to clash with > > > * the virtual addresses used by the fixmap or DTB. > > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > > > index f0a2eddaaf..0d076489c6 100644 > > > --- a/xen/arch/arm/mm.c > > > +++ b/xen/arch/arm/mm.c > > > @@ -38,6 +38,13 @@ > > > #include <xen/vmap.h> > > > #include <xsm/xsm.h> > > > #include <xen/pfn.h> > > > +#include <xen/sizes.h> > > > + > > > +static void __init create_mappings(lpae_t *second, > > > + unsigned long virt_offset, > > > + unsigned long base_mfn, > > > + unsigned long nr_mfns, > > > + unsigned int mapping_size); > > > > I would have added early_fdt_map to this file in a way to avoid the need > > for duplicating the declaration of create_mappings (because this version > > doesn't have the useful comment on top). > > I wanted to keep the function close to the counterpart remove_early_mappings > rather than adding somewhere that make less sense. > > Hence why I suggested to move the create_mappings function. Would you be fine > with code motion for Xen 4.9? Sure. But please keep the code motion in its own separate patch.
On 19/04/2017 22:12, Julien Grall wrote:
> Would you be fine with code motion for Xen 4.9?
The only important question is whether the patch/series is suitable
material for 4.9. By the looks of this series, it most certainly is
(and at the end of the day, you have final say on this matter).
Other than that, the modification should conform to the normal
guidelines (break different logical changes apart into different
patches, don't skimp on style, etc).
What matters (moreso during a stabilisation period) is review-ability of
the changes, not the quantity of patches. I'd have no qualms submitting
a 25 patch series for 4.9, if I decided the bugfix was important enough
and 25 patches is what it took to do sensibly.
~Andrew
Hi Stefano, On 19/04/17 22:18, Stefano Stabellini wrote: > On Wed, 19 Apr 2017, Julien Grall wrote: >> On 19/04/2017 22:01, Stefano Stabellini wrote: >>> On Wed, 19 Apr 2017, Julien Grall wrote: >>> I would have added early_fdt_map to this file in a way to avoid the need >>> for duplicating the declaration of create_mappings (because this version >>> doesn't have the useful comment on top). >> >> I wanted to keep the function close to the counterpart remove_early_mappings >> rather than adding somewhere that make less sense. >> >> Hence why I suggested to move the create_mappings function. Would you be fine >> with code motion for Xen 4.9? > > Sure. But please keep the code motion in its own separate patch. I was planning to do the code motion in a separate patch :). My concern was to shuffle the code unnecessarily during code freeze and was planning to do it after the freeze. Anyway, I will resend this series with a patch to move the function. Cheers,
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index ec63ba4c04..4090f4a744 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -389,20 +389,6 @@ paging: /* Use a virtual address to access the UART. */ ldr r11, =EARLY_UART_VIRTUAL_ADDRESS #endif - /* Map the DTB in the boot misc slot */ - teq r12, #0 /* Only on boot CPU */ - bne 1f - - ldr r1, =boot_second - mov r3, #0x0 - lsr r2, r8, #SECOND_SHIFT - lsl r2, r2, #SECOND_SHIFT /* r2: 2MB-aligned paddr of DTB */ - orr r2, r2, #PT_UPPER(MEM) - orr r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */ - ldr r4, =BOOT_FDT_VIRT_START - mov r4, r4, lsr #(SECOND_SHIFT - 3) /* Slot for BOOT_FDT_VIRT_START */ - strd r2, r3, [r1, r4] /* Map it in the early fdt slot */ -1: /* * Flush the TLB in case the 1:1 mapping happens to clash with diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 72ea4e0233..78292f4396 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -550,19 +550,6 @@ paging: ldr x23, =EARLY_UART_VIRTUAL_ADDRESS #endif - /* Map the DTB in the boot misc slot */ - cbnz x22, 1f /* Only on boot CPU */ - - ldr x4, =boot_second /* x4 := vaddr (boot_second) */ - lsr x2, x21, #SECOND_SHIFT - lsl x2, x2, #SECOND_SHIFT /* x2 := 2MB-aligned paddr of DTB */ - mov x3, #PT_MEM /* x2 := 2MB RAM incl. DTB */ - orr x2, x2, x3 - ldr x1, =BOOT_FDT_VIRT_START - lsr x1, x1, #(SECOND_SHIFT - 3) /* x4 := Slot for BOOT_FDT_VIRT_START */ - str x2, [x4, x1] /* Map it in the early fdt slot */ -1: - /* * Flush the TLB in case the 1:1 mapping happens to clash with * the virtual addresses used by the fixmap or DTB. diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index f0a2eddaaf..0d076489c6 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -38,6 +38,13 @@ #include <xen/vmap.h> #include <xsm/xsm.h> #include <xen/pfn.h> +#include <xen/sizes.h> + +static void __init create_mappings(lpae_t *second, + unsigned long virt_offset, + unsigned long base_mfn, + unsigned long nr_mfns, + unsigned int mapping_size); struct domain *dom_xen, *dom_io, *dom_cow; @@ -434,6 +441,19 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va) return mfn_to_xen_entry(mfn, WRITEALLOC); } + +/* Map the FDT in the early boot page table */ +void * __init early_fdt_map(paddr_t fdt_paddr) +{ + /* We are using 2MB superpage for mapping the FDT */ + paddr_t base_paddr = fdt_paddr & SECOND_MASK; + + create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr), + SZ_2M >> PAGE_SHIFT, SZ_2M); + + return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE); +} + void __init remove_early_mappings(void) { lpae_t pte = {0}; diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 92a2de6b70..986398970f 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -724,9 +724,7 @@ void __init start_xen(unsigned long boot_phys_offset, smp_clear_cpu_maps(); - /* This is mapped by head.S */ - device_tree_flattened = (void *)BOOT_FDT_VIRT_START - + (fdt_paddr & ((1 << SECOND_SHIFT) - 1)); + device_tree_flattened = early_fdt_map(fdt_paddr); fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr); cmdline = boot_fdt_cmdline(device_tree_flattened); diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index 0fef612f42..f6915ad882 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -160,6 +160,8 @@ extern unsigned long total_pages; /* Boot-time pagetable setup */ extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr); +/* Map FDT in boot pagetable */ +extern void *early_fdt_map(paddr_t fdt_paddr); /* Remove early mappings */ extern void remove_early_mappings(void); /* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the
The FDT will not be accessed before start_xen (begining of C code) is called and it will be easier to maintain as the code could be common between AArch32 and AArch64. A new function early_fdt_map is introduced to map the FDT in the boot page table. Note that create_mapping will flush the TLBs for us so no need to add an extra on in case the entry was previously used by the 1:1 mapping. Signed-off-by: Julien Grall <julien.grall@arm.com> --- I can move the function create_mappings at the beginning of the function instead of adding a static declaration. But I thought it was not Xen 4.9 material. Any opinions? --- xen/arch/arm/arm32/head.S | 14 -------------- xen/arch/arm/arm64/head.S | 13 ------------- xen/arch/arm/mm.c | 20 ++++++++++++++++++++ xen/arch/arm/setup.c | 4 +--- xen/include/asm-arm/mm.h | 2 ++ 5 files changed, 23 insertions(+), 30 deletions(-)