| Message ID | 20190610193215.23704-15-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: Rework head.S to make it more compliant with the Arm Arm | expand |
On Mon, 10 Jun 2019, Julien Grall wrote: > The ID map may clash with other parts of the Xen virtual memory layout. > At the moment, Xen is handling the clash by only creating a mapping to > the runtime virtual address before enabling the MMU. > > The rest of the mappings (such as the fixmap) will be mapped after the > MMU is enabled. However, the code doing the mapping is not safe as it > replace mapping without using the Break-Before-Make sequence. > > As the ID map can be anywhere in the memory, it is easier to remove all > the entries added as soon as the ID map is not used rather than adding > the Break-Before-Make sequence everywhere. I think it is a good idea, but I would ask you to mention 1:1 map instead of "ID map" in comments and commit messages because that is the wording we used in all comments so far (see the one in create_page_tables and mm.c). It is easier to grep and refer to if we use the same nomenclature. Note that I don't care about which nomenclature we decide to use, I am only asking for consistency. Otherwise, it would also work if you say it both way at least once: The ID map (1:1 map) may clash [...] > It is difficult to track where exactly the ID map was created without a > full rework of create_page_tables(). Instead, introduce a new function > remove_id_map() will look where is the top-level entry for the ID map > and remove it. Do you think it would be worth simplifying this code below by preserving where/how the ID map was created? We could repurpose x25 for that, carrying for instance the address of the ID map section slot or a code number to specify which case we are dealing with. We should be able to turn remove_id_map into only ~5 lines? > The new function is only called for the boot CPU. Secondary CPUs will > switch directly to the runtime page-tables so there are no need to > remove the ID mapping. Note that this still doesn't make the Secondary > CPUs path safe but it is not making it worst. > > --- > Note that the comment refers to the patch "xen/arm: tlbflush: Rework > TLB helpers" under review (see [1]). > > Furthermore, it is very likely we will need to re-introduce the ID > map to cater secondary CPUs boot and suspend/resume. For now, the > attempt is to make boot CPU path fully Arm Arm compliant. > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01134.html > --- > xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 71 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 192af3e8a2..96e85f8834 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -300,6 +300,13 @@ real_start_efi: > ldr x0, =primary_switched > br x0 > primary_switched: > + /* > + * The ID map may clash with other parts of the Xen virtual memory > + * layout. As it is not used anymore, remove it completely to > + * avoid having to worry about replacing existing mapping > + * afterwards. > + */ > + bl remove_id_map > bl setup_fixmap > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -632,10 +639,68 @@ enable_mmu: > ret > ENDPROC(enable_mmu) > > +/* > + * Remove the ID map for the page-tables. It is not easy to keep track > + * where the ID map was mapped, so we will look for the top-level entry > + * exclusive to the ID Map and remove it. > + * > + * Inputs: > + * x19: paddr(start) > + * > + * Clobbers x0 - x1 > + */ > +remove_id_map: > + /* > + * Find the zeroeth slot used. Remove the entry from zeroeth > + * table if the slot is not 0. For slot 0, the ID map was either > + * done in first or second table. > + */ > + lsr x1, x19, #ZEROETH_SHIFT /* x1 := zeroeth slot */ > + cbz x1, 1f > + /* It is not in slot 0, remove the entry */ > + ldr x0, =boot_pgtable /* x0 := root table */ > + str xzr, [x0, x1, lsl #3] > + b id_map_removed > + > +1: > + /* > + * Find the first slot used. Remove the entry for the first > + * table if the slot is not 0. For slot 0, the ID map was done > + * in the second table. > + */ > + lsr x1, x19, #FIRST_SHIFT > + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ > + cbz x1, 1f > + /* It is not in slot 0, remove the entry */ > + ldr x0, =boot_first /* x0 := first table */ > + str xzr, [x0, x1, lsl #3] > + b id_map_removed > + > +1: > + /* > + * Find the second slot used. Remove the entry for the first > + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). > + * For slot 1, it means the ID map was not created. > + */ > + lsr x1, x19, #SECOND_SHIFT > + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ > + cmp x1, #1 > + beq id_map_removed > + /* It is not in slot 1, remove the entry */ > + ldr x0, =boot_second /* x0 := second table */ > + str xzr, [x0, x1, lsl #3] > + > +id_map_removed: > + /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */ > + dsb nshst > + tlbi alle2 > + dsb nsh > + isb > + > + ret > +ENDPROC(remove_id_map) > + > setup_fixmap: > - /* Now we can install the fixmap and dtb mappings, since we > - * don't need the 1:1 map any more */ > - dsb sy > #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ > /* Add UART to the fixmap table */ > ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ > @@ -653,19 +718,10 @@ setup_fixmap: > ldr x1, =FIXMAP_ADDR(0) > lsr x1, x1, #(SECOND_SHIFT - 3) /* x1 := Slot for FIXMAP(0) */ > str x2, [x4, x1] /* Map it in the fixmap's slot */ > -#endif > > - /* > - * Flush the TLB in case the 1:1 mapping happens to clash with > - * the virtual addresses used by the fixmap or DTB. > - */ > - dsb sy /* Ensure any page table updates made above > - * have occurred. */ > - > - isb > - tlbi alle2 > - dsb sy /* Ensure completion of TLB flush */ > - isb > + /* Ensure any page table updates made above have occurred */ > + dsb nshst > +#endif > ret > ENDPROC(setup_fixmap) > > -- > 2.11.0 >
Hi Stefano, On 6/26/19 9:25 PM, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: >> The ID map may clash with other parts of the Xen virtual memory layout. >> At the moment, Xen is handling the clash by only creating a mapping to >> the runtime virtual address before enabling the MMU. >> >> The rest of the mappings (such as the fixmap) will be mapped after the >> MMU is enabled. However, the code doing the mapping is not safe as it >> replace mapping without using the Break-Before-Make sequence. >> >> As the ID map can be anywhere in the memory, it is easier to remove all >> the entries added as soon as the ID map is not used rather than adding >> the Break-Before-Make sequence everywhere. > > I think it is a good idea, but I would ask you to mention 1:1 map > instead of "ID map" in comments and commit messages because that is the > wording we used in all comments so far (see the one in > create_page_tables and mm.c). It is easier to grep and refer to if we > use the same nomenclature. Note that I don't care about which > nomenclature we decide to use, I am only asking for consistency. > Otherwise, it would also work if you say it both way at least once: > > The ID map (1:1 map) may clash [...] I would rather drop the wording 1:1 as this is confusing. It is also not trivial to find anything on google typing "1:1". > > >> It is difficult to track where exactly the ID map was created without a >> full rework of create_page_tables(). Instead, introduce a new function >> remove_id_map() will look where is the top-level entry for the ID map >> and remove it. > > Do you think it would be worth simplifying this code below by preserving > where/how the ID map was created? We could repurpose x25 for that, > carrying for instance the address of the ID map section slot or a code > number to specify which case we are dealing with. We should be able to > turn remove_id_map into only ~5 lines? I thought about it but the current implementation of create_map_tables() is quite awful to read. So the less I touch this function, the better I feel :). I have some rework for the create_page_tables() which simplify it a lot. Yet, I am not entirely sure it is worth to spend time trying to simplify remove_id_map. This is unlikely to make the boot significantly faster and I don't expect the function to survive more than a release as the ID map as to be kept in place (for secondary boot and suspend/resume). The only reason it is removed now is because it clashes with other mapping we may do. Cheers,
On 26/06/2019 21:39, Julien Grall wrote: > On 6/26/19 9:25 PM, Stefano Stabellini wrote: >> On Mon, 10 Jun 2019, Julien Grall wrote: >>> The ID map may clash with other parts of the Xen virtual memory layout. >>> At the moment, Xen is handling the clash by only creating a mapping to >>> the runtime virtual address before enabling the MMU. >>> >>> The rest of the mappings (such as the fixmap) will be mapped after the >>> MMU is enabled. However, the code doing the mapping is not safe as it >>> replace mapping without using the Break-Before-Make sequence. >>> >>> As the ID map can be anywhere in the memory, it is easier to remove all >>> the entries added as soon as the ID map is not used rather than adding >>> the Break-Before-Make sequence everywhere. >> >> I think it is a good idea, but I would ask you to mention 1:1 map >> instead of "ID map" in comments and commit messages because that is the >> wording we used in all comments so far (see the one in >> create_page_tables and mm.c). It is easier to grep and refer to if we >> use the same nomenclature. Note that I don't care about which >> nomenclature we decide to use, I am only asking for consistency. >> Otherwise, it would also work if you say it both way at least once: >> >> The ID map (1:1 map) may clash [...] > > I would rather drop the wording 1:1 as this is confusing. It is also > not trivial to find anything on google typing "1:1". "one-to-one mapping", or "identity map" are both common terminology. 1:1 is a common representation for the former, whereas ID is not a abbreviation of "Identity". If you don't want to use 1:1, then you need to say "The identity map" to retain clarity. ~Andrew
On Mon, 10 Jun 2019, Julien Grall wrote: > The ID map may clash with other parts of the Xen virtual memory layout. > At the moment, Xen is handling the clash by only creating a mapping to > the runtime virtual address before enabling the MMU. > > The rest of the mappings (such as the fixmap) will be mapped after the > MMU is enabled. However, the code doing the mapping is not safe as it > replace mapping without using the Break-Before-Make sequence. > > As the ID map can be anywhere in the memory, it is easier to remove all > the entries added as soon as the ID map is not used rather than adding > the Break-Before-Make sequence everywhere. > > It is difficult to track where exactly the ID map was created without a > full rework of create_page_tables(). Instead, introduce a new function > remove_id_map() will look where is the top-level entry for the ID map > and remove it. > > The new function is only called for the boot CPU. Secondary CPUs will > switch directly to the runtime page-tables so there are no need to > remove the ID mapping. Note that this still doesn't make the Secondary > CPUs path safe but it is not making it worst. > > --- > Note that the comment refers to the patch "xen/arm: tlbflush: Rework > TLB helpers" under review (see [1]). > > Furthermore, it is very likely we will need to re-introduce the ID > map to cater secondary CPUs boot and suspend/resume. For now, the > attempt is to make boot CPU path fully Arm Arm compliant. > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01134.html > --- > xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 71 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 192af3e8a2..96e85f8834 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -300,6 +300,13 @@ real_start_efi: > ldr x0, =primary_switched > br x0 > primary_switched: > + /* > + * The ID map may clash with other parts of the Xen virtual memory > + * layout. As it is not used anymore, remove it completely to > + * avoid having to worry about replacing existing mapping > + * afterwards. > + */ > + bl remove_id_map > bl setup_fixmap > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -632,10 +639,68 @@ enable_mmu: > ret > ENDPROC(enable_mmu) > > +/* > + * Remove the ID map for the page-tables. It is not easy to keep track > + * where the ID map was mapped, so we will look for the top-level entry > + * exclusive to the ID Map and remove it. > + * > + * Inputs: > + * x19: paddr(start) > + * > + * Clobbers x0 - x1 > + */ > +remove_id_map: > + /* > + * Find the zeroeth slot used. Remove the entry from zeroeth > + * table if the slot is not 0. For slot 0, the ID map was either > + * done in first or second table. > + */ > + lsr x1, x19, #ZEROETH_SHIFT /* x1 := zeroeth slot */ > + cbz x1, 1f > + /* It is not in slot 0, remove the entry */ > + ldr x0, =boot_pgtable /* x0 := root table */ > + str xzr, [x0, x1, lsl #3] > + b id_map_removed > + > +1: > + /* > + * Find the first slot used. Remove the entry for the first > + * table if the slot is not 0. For slot 0, the ID map was done > + * in the second table. > + */ > + lsr x1, x19, #FIRST_SHIFT > + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ > + cbz x1, 1f > + /* It is not in slot 0, remove the entry */ > + ldr x0, =boot_first /* x0 := first table */ > + str xzr, [x0, x1, lsl #3] > + b id_map_removed > + > +1: > + /* > + * Find the second slot used. Remove the entry for the first > + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). > + * For slot 1, it means the ID map was not created. > + */ > + lsr x1, x19, #SECOND_SHIFT > + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ > + cmp x1, #1 > + beq id_map_removed > + /* It is not in slot 1, remove the entry */ > + ldr x0, =boot_second /* x0 := second table */ > + str xzr, [x0, x1, lsl #3] Wouldn't it be a bit more reliable if we checked whether the slot in question for x19 (whether zero, first, second) is a pagetable pointer or section map, then zero it if it is a section map, otherwise go down one level? If we did it this way it would be independent from the way create_page_tables is written. With the current code, we are somewhat reliant on the behavior of create_page_tables, because we rely on the position of the slot for the ID map? Where the assumption for instance is that at level one, if the slot is zero, then we need to go down a level, etc. Instead, if we checked if the slot is a section map, we could remove it immediately, if it is a pagetable pointer, we proceed. The code should be similar in complexity and LOC, but it would be more robust. Something like the following, in pseudo-uncompiled assembly: lsr x1, x19, #FIRST_SHIFT ldr x0, =boot_first /* x0 := first table */ ldr x2, [x0, x1, lsl #3] # check x2 against #PT_MEM cbz x2, 1f str xzr, [x0, x1, lsl #3] b id_map_removed > +id_map_removed: > + /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */ Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the explanation you are referring to. > + dsb nshst > + tlbi alle2 > + dsb nsh > + isb > + > + ret > +ENDPROC(remove_id_map) > + > setup_fixmap: > - /* Now we can install the fixmap and dtb mappings, since we > - * don't need the 1:1 map any more */ > - dsb sy > #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ > /* Add UART to the fixmap table */ > ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ > @@ -653,19 +718,10 @@ setup_fixmap: > ldr x1, =FIXMAP_ADDR(0) > lsr x1, x1, #(SECOND_SHIFT - 3) /* x1 := Slot for FIXMAP(0) */ > str x2, [x4, x1] /* Map it in the fixmap's slot */ > -#endif > > - /* > - * Flush the TLB in case the 1:1 mapping happens to clash with > - * the virtual addresses used by the fixmap or DTB. > - */ > - dsb sy /* Ensure any page table updates made above > - * have occurred. */ > - > - isb > - tlbi alle2 > - dsb sy /* Ensure completion of TLB flush */ > - isb > + /* Ensure any page table updates made above have occurred */ > + dsb nshst > +#endif > ret > ENDPROC(setup_fixmap) > > -- > 2.11.0 >
Hi Stefano, On 6/27/19 7:55 PM, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: >> +1: >> + /* >> + * Find the second slot used. Remove the entry for the first >> + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). >> + * For slot 1, it means the ID map was not created. >> + */ >> + lsr x1, x19, #SECOND_SHIFT >> + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ >> + cmp x1, #1 >> + beq id_map_removed >> + /* It is not in slot 1, remove the entry */ >> + ldr x0, =boot_second /* x0 := second table */ >> + str xzr, [x0, x1, lsl #3] > > Wouldn't it be a bit more reliable if we checked whether the slot in > question for x19 (whether zero, first, second) is a pagetable pointer or > section map, then zero it if it is a section map, otherwise go down one > level? If we did it this way it would be independent from the way > create_page_tables is written. Your suggestion will not comply with the architecture compliance and how Xen is/will be working after the full rework. We want to remove everything (mapping + table) added specifically for the 1:1 mapping. Otherwise, you may end up in a position where boot_first_id is still in place. We would need to use the break-before-make sequence in subsequent code if we were about to insert 1GB mapping at the same place. After my rework, we would have virtually no place where break-before-make will be necessary as it will enforce all the mappings to be destroyed before hand. So I would rather avoid to make a specific case for the 1:1 mapping. As a side note, the current code for the 1:1 mapping is completely wrong as using 1GB (or even 2MB) mapping may result to map MMIO region (or reserved-region). This may result to cache problem. I have this partially fixed on for the next version of series (see [1]). > > With the current code, we are somewhat reliant on the behavior of > create_page_tables, because we rely on the position of the slot for > the ID map? Where the assumption for instance is that at level one, if > the slot is zero, then we need to go down a level, etc. Instead, if we > checked if the slot is a section map, we could remove it immediately, if > it is a pagetable pointer, we proceed. The code should be similar in > complexity and LOC, but it would be more robust. See above :). > > Something like the following, in pseudo-uncompiled assembly: > > lsr x1, x19, #FIRST_SHIFT > ldr x0, =boot_first /* x0 := first table */ > ldr x2, [x0, x1, lsl #3] > # check x2 against #PT_MEM > cbz x2, 1f > str xzr, [x0, x1, lsl #3] > b id_map_removed > > >> +id_map_removed: >> + /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */ > > Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the > explanation you are referring to. The big comment at the top of the header: /* * Every invalidation operation use the following patterns: * * DSB ISHST // Ensure prior page-tables updates have completed * TLBI... // Invalidate the TLB * DSB ISH // Ensure the TLB invalidation has completed * ISB // See explanation below * * For Xen page-tables the ISB will discard any instructions fetched * from the old mappings. * * For the Stage-2 page-tables the ISB ensures the completion of the DSB * (and therefore the TLB invalidation) before continuing. So we know * the TLBs cannot contain an entry for a mapping we may have removed. */ Note that we are using nsh (and not ish) because we are using local TLB flush (see page D5-230 ARM DDI 0487D.a). For convenience here is the text: "In all cases in this section where a DMB or DSB is referred to, it refers to a DMB or DSB whose required access type is both loads and stores. A DSB NSH is sufficient to ensure completion of TLB maintenance instructions that apply to a single PE. A DSB ISH is sufficient to ensure completion of TLB maintenance instructions that apply to PEs in the same Inner Shareable domain." I discovered this section after the changes in flushtlb.h has been merged. But I am thinking to do a follow-up the local TLB flush code. > > >> + dsb nshst >> + tlbi alle2 >> + dsb nsh >> + isb >> + >> + ret >> +ENDPROC(remove_id_map) [...] [1] Rework for create_page_tables diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index a79ae54822..c019dd3e04 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -483,6 +483,60 @@ cpu_init: ENDPROC(cpu_init) /* + * Macro to create a page table entry in \ptbl to \tbl + * + * ptbl: table symbol where the entry will be created + * tbl: table symbol to point to + * virt: virtual address + * shift: #imm page table shift + * tmp1: scratch register + * tmp2: scratch register + * tmp3: scratch register + * + * Preserves \virt + * Clobbers \tmp1, \tmp2, \tmp3 + * + * Also use x20 for the phys offset. + * + * Note that all parameters using registers should be distinct. + */ +.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3 + lsr \tmp1, \virt, #\shift + and \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */ + load_paddr \tmp2, \tbl + mov \tmp3, #PT_PT /* \tmp3 := right for linear PT */ + orr \tmp3, \tmp3, \tmp2 /* + \tlb paddr */ + adr_l \tmp2, \ptbl + str \tmp3, [\tmp2, \tmp1, lsl #3] +.endm + +/* + * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd + * level table is supported. + * + * tbl: table symbol where the entry will be created + * virt: virtual address + * paddr: physical address (should be page aligned) + * tmp1: scratch register + * tmp2: scratch register + * tmp3: scratch register + * type: mapping type. If not specified it will be normal memory (PT_MEM_L3) + * + * Preserves \virt, \paddr + * Clobbers \tmp1, \tmp2, \tmp3 + * + * Note that all parameters using registers should be distinct. + */ +.macro create_mapping_entry, tbl, virt, paddr, tmp1, tmp2, tmp3, type=PT_MEM_L3 + lsr \tmp1, \virt, #THIRD_SHIFT + and \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */ + mov \tmp2, #\type /* \tmp2 := right for section PT */ + orr \tmp2, \tmp2, \paddr /* + paddr */ + adr_l \tmp3, \tbl + str \tmp2, [\tmp3, \tmp1, lsl #3] +.endm + +/* * Rebuild the boot pagetable's first-level entries. The structure * is described in mm.c. * @@ -495,100 +549,17 @@ ENDPROC(cpu_init) * x19: paddr(start) * x20: phys offset * - * Clobbers x0 - x4, x25 - * - * Register usage within this function: - * x25: Identity map in place + * Clobbers x0 - x4 */ create_page_tables: - /* - * If Xen is loaded at exactly XEN_VIRT_START then we don't - * need an additional 1:1 mapping, the virtual mapping will - * suffice. - */ - cmp x19, #XEN_VIRT_START - cset x25, eq /* x25 := identity map in place, or not */ - - load_paddr x4, boot_pgtable - - /* Setup boot_pgtable: */ - load_paddr x1, boot_first - - /* ... map boot_first in boot_pgtable[0] */ - mov x3, #PT_PT /* x2 := table map of boot_first */ - orr x2, x1, x3 /* + rights for linear PT */ - str x2, [x4, #0] /* Map it in slot 0 */ - - /* ... map of paddr(start) in boot_pgtable+boot_first_id */ - lsr x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in boot_pgtable */ - cbz x1, 1f /* It's in slot 0, map in boot_first - * or boot_second later on */ - - /* - * Level zero does not support superpage mappings, so we have - * to use an extra first level page in which we create a 1GB mapping. - */ - load_paddr x2, boot_first_id - - mov x3, #PT_PT /* x2 := table map of boot_first_id */ - orr x2, x2, x3 /* + rights for linear PT */ - str x2, [x4, x1, lsl #3] - - load_paddr x4, boot_first_id - - lsr x1, x19, #FIRST_SHIFT /* x1 := Offset of base paddr in boot_first_id */ - lsl x2, x1, #FIRST_SHIFT /* x2 := Base address for 1GB mapping */ - mov x3, #PT_MEM /* x2 := Section map */ - orr x2, x2, x3 - and x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */ - str x2, [x4, x1, lsl #3] /* Mapping of paddr(start) */ - mov x25, #1 /* x25 := identity map now in place */ - -1: /* Setup boot_first: */ - load_paddr x4, boot_first /* Next level into boot_first */ - - /* ... map boot_second in boot_first[0] */ - load_paddr x1, boot_second - mov x3, #PT_PT /* x2 := table map of boot_second */ - orr x2, x1, x3 /* + rights for linear PT */ - str x2, [x4, #0] /* Map it in slot 0 */ - - /* ... map of paddr(start) in boot_first */ - cbnz x25, 1f /* x25 is set if already created */ - lsr x2, x19, #FIRST_SHIFT /* x2 := Offset of base paddr in boot_first */ - and x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */ - cbz x1, 1f /* It's in slot 0, map in boot_second */ - - lsl x2, x2, #FIRST_SHIFT /* Base address for 1GB mapping */ - mov x3, #PT_MEM /* x2 := Section map */ - orr x2, x2, x3 - str x2, [x4, x1, lsl #3] /* Create mapping of paddr(start)*/ - mov x25, #1 /* x25 := identity map now in place */ - -1: /* Setup boot_second: */ - load_paddr x4, boot_second - - /* ... map boot_third in boot_second[1] */ - load_paddr x1, boot_third - mov x3, #PT_PT /* x2 := table map of boot_third */ - orr x2, x1, x3 /* + rights for linear PT */ - str x2, [x4, #8] /* Map it in slot 1 */ - - /* ... map of paddr(start) in boot_second */ - cbnz x25, 1f /* x25 is set if already created */ - lsr x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in boot_second */ - and x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */ - cmp x1, #1 - b.eq virtphys_clash /* It's in slot 1, which we cannot handle */ + /* Prepare the page-tables for mapping Xen */ + ldr x0, =XEN_VIRT_START + create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, x1, x2, x3 + create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, x1, x2, x3 + create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, x1, x2, x3 - lsl x2, x2, #SECOND_SHIFT /* Base address for 2MB mapping */ - mov x3, #PT_MEM /* x2 := Section map */ - orr x2, x2, x3 - str x2, [x4, x1, lsl #3] /* Create mapping of paddr(start)*/ - mov x25, #1 /* x25 := identity map now in place */ - -1: /* Setup boot_third: */ - load_paddr x4, boot_third + /* Map Xen */ + adr_l x4, boot_third lsr x2, x19, #THIRD_SHIFT /* Base address for 4K mapping */ lsl x2, x2, #THIRD_SHIFT @@ -603,21 +574,68 @@ create_page_tables: cmp x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */ b.lt 1b - /* Defer fixmap and dtb mapping until after paging enabled, to - * avoid them clashing with the 1:1 mapping. */ + /* + * If Xen is loaded at exactly XEN_VIRT_START then we don't + * need an additional 1:1 mapping, the virtual mapping will + * suffice. + */ + cmp x19, #XEN_VIRT_START + bne 1f + ret +1: + /* + * Only the first page of Xen will be part of the 1:1 mapping. + * All the boot_*_id tables are linked together even if they may + * not be all used. They will then be linked to the boot page + * tables at the correct level. + */ + create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2 + create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2 + create_mapping_entry boot_third_id, x19, x19, x0, x1, x2 + + /* + * Find the zeroeth slot used. Link boot_first_id into + * boot_pgtable if the slot is not 0. For slot 0, the tables + * associated with the 1:1 mapping will need to be linked in + * boot_first or boot_second. + */ + lsr x0, x19, #ZEROETH_SHIFT /* x0 := zeroeth slot */ + cbz x0, 1f + /* It is not in slot 0, Link boot_first_id into boot_pgtable */ + create_table_entry boot_pgtable, boot_first_id, x19, ZEROETH_SHIFT, x0, x1, x2 + ret + +1: + /* + * Find the first slot used. Link boot_second_id into boot_first + * if the slot is not 0. For slot 0, the tables associated with + * the 1:1 mapping will need to be linked in boot_second. + */ + lsr x0, x19, #FIRST_SHIFT + and x0, x0, #LPAE_ENTRY_MASK /* x0 := first slot */ + cbz x0, 1f + /* It is not in slot 0, Link boot_second_id into boot_first */ + create_table_entry boot_first, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2 + ret - /* boot pagetable setup complete */ +1: + /* + * Find the second slot used. Link boot_third_id into boot_second + * if the slot is not 1 (runtime Xen mapping is 2M - 4M). + * For slot 1, Xen is not yet able to handle it. + */ + lsr x0, x19, #SECOND_SHIFT + and x0, x0, #LPAE_ENTRY_MASK /* x0 := first slot */ + cmp x0, #1 + beq virtphys_clash + /* It is not in slot 1, link boot_third_id into boot_second */ + create_table_entry boot_second, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2 + ret - cbnz x25, 1f /* Did we manage to create an identity mapping ? */ - PRINT("Unable to build boot page tables - Failed to identity map Xen.\r\n") - b fail virtphys_clash: /* Identity map clashes with boot_third, which we cannot handle yet */ PRINT("- Unable to build boot page tables - virt and phys addresses clash. -\r\n") b fail - -1: - ret ENDPROC(create_page_tables) /* @@ -719,28 +737,15 @@ ENDPROC(remove_identity_mapping) * The fixmap cannot be mapped in create_page_tables because it may * clash with the 1:1 mapping. * - * Clobbers x1 - x4 + * Clobbers x0 - x3 */ setup_fixmap: #ifdef CONFIG_EARLY_PRINTK - /* Add UART to the fixmap table */ - ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ - lsr x2, x23, #THIRD_SHIFT - lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */ - mov x3, #PT_DEV_L3 - orr x2, x2, x3 /* x2 := 4K dev map including UART */ - str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */ + ldr x0, =EARLY_UART_VIRTUAL_ADDRESS + create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3 #endif - - /* Map fixmap into boot_second */ - ldr x4, =boot_second /* x4 := vaddr (boot_second) */ - load_paddr x2, xen_fixmap - mov x3, #PT_PT - orr x2, x2, x3 /* x2 := table map of xen_fixmap */ - ldr x1, =FIXMAP_ADDR(0) - lsr x1, x1, #(SECOND_SHIFT - 3) /* x1 := Slot for FIXMAP(0) */ - str x2, [x4, x1] /* Map it in the fixmap's slot */ - + ldr x0, =FIXMAP_ADDR(0) + create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, x1, x2, x3 /* Ensure any page table updates made above have occurred */ dsb nshst ret diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index c2f1795a71..bc1824d3ca 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -107,6 +107,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable); DEFINE_BOOT_PAGE_TABLE(boot_first); DEFINE_BOOT_PAGE_TABLE(boot_first_id); #endif +DEFINE_BOOT_PAGE_TABLE(boot_second_id); +DEFINE_BOOT_PAGE_TABLE(boot_third_id); DEFINE_BOOT_PAGE_TABLE(boot_second); DEFINE_BOOT_PAGE_TABLE(boot_third);
On Wed, 26 Jun 2019, Julien Grall wrote: > Hi Stefano, > > On 6/26/19 9:25 PM, Stefano Stabellini wrote: > > On Mon, 10 Jun 2019, Julien Grall wrote: > > > The ID map may clash with other parts of the Xen virtual memory layout. > > > At the moment, Xen is handling the clash by only creating a mapping to > > > the runtime virtual address before enabling the MMU. > > > > > > The rest of the mappings (such as the fixmap) will be mapped after the > > > MMU is enabled. However, the code doing the mapping is not safe as it > > > replace mapping without using the Break-Before-Make sequence. > > > > > > As the ID map can be anywhere in the memory, it is easier to remove all > > > the entries added as soon as the ID map is not used rather than adding > > > the Break-Before-Make sequence everywhere. > > > > I think it is a good idea, but I would ask you to mention 1:1 map > > instead of "ID map" in comments and commit messages because that is the > > wording we used in all comments so far (see the one in > > create_page_tables and mm.c). It is easier to grep and refer to if we > > use the same nomenclature. Note that I don't care about which > > nomenclature we decide to use, I am only asking for consistency. > > Otherwise, it would also work if you say it both way at least once: > > > > The ID map (1:1 map) may clash [...] > > I would rather drop the wording 1:1 as this is confusing. It is also not > trivial to find anything on google typing "1:1". That's fine too
Hi, @Stefano, I am going through the series and noticed you didn't give any update. Could you confirm if my reply makes sense? Cheers, On 6/27/19 8:30 PM, Julien Grall wrote: > Hi Stefano, > > On 6/27/19 7:55 PM, Stefano Stabellini wrote: >> On Mon, 10 Jun 2019, Julien Grall wrote: >>> +1: >>> + /* >>> + * Find the second slot used. Remove the entry for the first >>> + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). >>> + * For slot 1, it means the ID map was not created. >>> + */ >>> + lsr x1, x19, #SECOND_SHIFT >>> + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ >>> + cmp x1, #1 >>> + beq id_map_removed >>> + /* It is not in slot 1, remove the entry */ >>> + ldr x0, =boot_second /* x0 := second table */ >>> + str xzr, [x0, x1, lsl #3] >> >> Wouldn't it be a bit more reliable if we checked whether the slot in >> question for x19 (whether zero, first, second) is a pagetable pointer or >> section map, then zero it if it is a section map, otherwise go down one >> level? If we did it this way it would be independent from the way >> create_page_tables is written. > > Your suggestion will not comply with the architecture compliance and how > Xen is/will be working after the full rework. We want to remove > everything (mapping + table) added specifically for the 1:1 mapping. > > Otherwise, you may end up in a position where boot_first_id is still in > place. We would need to use the break-before-make sequence in subsequent > code if we were about to insert 1GB mapping at the same place. > > After my rework, we would have virtually no place where > break-before-make will be necessary as it will enforce all the mappings > to be destroyed before hand. So I would rather avoid to make a specific > case for the 1:1 mapping. > > As a side note, the current code for the 1:1 mapping is completely wrong > as using 1GB (or even 2MB) mapping may result to map MMIO region (or > reserved-region). This may result to cache problem. I have this > partially fixed on for the next version of series (see [1]). > >> >> With the current code, we are somewhat reliant on the behavior of >> create_page_tables, because we rely on the position of the slot for >> the ID map? Where the assumption for instance is that at level one, if >> the slot is zero, then we need to go down a level, etc. Instead, if we >> checked if the slot is a section map, we could remove it immediately, if >> it is a pagetable pointer, we proceed. The code should be similar in >> complexity and LOC, but it would be more robust. > > See above :). > >> >> Something like the following, in pseudo-uncompiled assembly: >> >> lsr x1, x19, #FIRST_SHIFT >> ldr x0, =boot_first /* x0 := first table */ >> ldr x2, [x0, x1, lsl #3] >> # check x2 against #PT_MEM >> cbz x2, 1f >> str xzr, [x0, x1, lsl #3] >> b id_map_removed >> >> >>> +id_map_removed: >>> + /* See asm-arm/arm64/flushtlb.h for the explanation of the >>> sequence. */ >> >> Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the >> explanation you are referring to. > > The big comment at the top of the header: > > /* > * Every invalidation operation use the following patterns: > * > * DSB ISHST // Ensure prior page-tables updates have completed > * TLBI... // Invalidate the TLB > * DSB ISH // Ensure the TLB invalidation has completed > * ISB // See explanation below > * > * For Xen page-tables the ISB will discard any instructions fetched > * from the old mappings. > * > * For the Stage-2 page-tables the ISB ensures the completion of the DSB > * (and therefore the TLB invalidation) before continuing. So we know > * the TLBs cannot contain an entry for a mapping we may have removed. > */ > > Note that we are using nsh (and not ish) because we are using local TLB > flush (see page D5-230 ARM DDI 0487D.a). For convenience here is the text: > > "In all cases in this section where a DMB or DSB is referred to, it > refers to a DMB or DSB whose required access type is > both loads and stores. A DSB NSH is sufficient to ensure completion of > TLB maintenance instructions that apply to a > single PE. A DSB ISH is sufficient to ensure completion of TLB > maintenance instructions that apply to PEs in the > same Inner Shareable domain." > > I discovered this section after the changes in flushtlb.h has been > merged. But I am thinking to do a follow-up the local TLB flush code. > >> >> >>> + dsb nshst >>> + tlbi alle2 >>> + dsb nsh >>> + isb >>> + >>> + ret >>> +ENDPROC(remove_id_map) > > [...] > > [1] Rework for create_page_tables > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index a79ae54822..c019dd3e04 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -483,6 +483,60 @@ cpu_init: > ENDPROC(cpu_init) > > /* > + * Macro to create a page table entry in \ptbl to \tbl > + * > + * ptbl: table symbol where the entry will be created > + * tbl: table symbol to point to > + * virt: virtual address > + * shift: #imm page table shift > + * tmp1: scratch register > + * tmp2: scratch register > + * tmp3: scratch register > + * > + * Preserves \virt > + * Clobbers \tmp1, \tmp2, \tmp3 > + * > + * Also use x20 for the phys offset. > + * > + * Note that all parameters using registers should be distinct. > + */ > +.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3 > + lsr \tmp1, \virt, #\shift > + and \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */ > + load_paddr \tmp2, \tbl > + mov \tmp3, #PT_PT /* \tmp3 := right for > linear PT */ > + orr \tmp3, \tmp3, \tmp2 /* + \tlb paddr */ > + adr_l \tmp2, \ptbl > + str \tmp3, [\tmp2, \tmp1, lsl #3] > +.endm > + > +/* > + * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd > + * level table is supported. > + * > + * tbl: table symbol where the entry will be created > + * virt: virtual address > + * paddr: physical address (should be page aligned) > + * tmp1: scratch register > + * tmp2: scratch register > + * tmp3: scratch register > + * type: mapping type. If not specified it will be normal memory > (PT_MEM_L3) > + * > + * Preserves \virt, \paddr > + * Clobbers \tmp1, \tmp2, \tmp3 > + * > + * Note that all parameters using registers should be distinct. > + */ > +.macro create_mapping_entry, tbl, virt, paddr, tmp1, tmp2, tmp3, > type=PT_MEM_L3 > + lsr \tmp1, \virt, #THIRD_SHIFT > + and \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */ > + mov \tmp2, #\type /* \tmp2 := right for > section PT */ > + orr \tmp2, \tmp2, \paddr /* + paddr */ > + adr_l \tmp3, \tbl > + str \tmp2, [\tmp3, \tmp1, lsl #3] > +.endm > + > +/* > * Rebuild the boot pagetable's first-level entries. The structure > * is described in mm.c. > * > @@ -495,100 +549,17 @@ ENDPROC(cpu_init) > * x19: paddr(start) > * x20: phys offset > * > - * Clobbers x0 - x4, x25 > - * > - * Register usage within this function: > - * x25: Identity map in place > + * Clobbers x0 - x4 > */ > create_page_tables: > - /* > - * If Xen is loaded at exactly XEN_VIRT_START then we don't > - * need an additional 1:1 mapping, the virtual mapping will > - * suffice. > - */ > - cmp x19, #XEN_VIRT_START > - cset x25, eq /* x25 := identity map in place, > or not */ > - > - load_paddr x4, boot_pgtable > - > - /* Setup boot_pgtable: */ > - load_paddr x1, boot_first > - > - /* ... map boot_first in boot_pgtable[0] */ > - mov x3, #PT_PT /* x2 := table map of boot_first */ > - orr x2, x1, x3 /* + rights for linear PT */ > - str x2, [x4, #0] /* Map it in slot 0 */ > - > - /* ... map of paddr(start) in boot_pgtable+boot_first_id */ > - lsr x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in > boot_pgtable */ > - cbz x1, 1f /* It's in slot 0, map in boot_first > - * or boot_second later on */ > - > - /* > - * Level zero does not support superpage mappings, so we have > - * to use an extra first level page in which we create a 1GB > mapping. > - */ > - load_paddr x2, boot_first_id > - > - mov x3, #PT_PT /* x2 := table map of > boot_first_id */ > - orr x2, x2, x3 /* + rights for linear PT */ > - str x2, [x4, x1, lsl #3] > - > - load_paddr x4, boot_first_id > - > - lsr x1, x19, #FIRST_SHIFT /* x1 := Offset of base paddr in > boot_first_id */ > - lsl x2, x1, #FIRST_SHIFT /* x2 := Base address for 1GB > mapping */ > - mov x3, #PT_MEM /* x2 := Section map */ > - orr x2, x2, x3 > - and x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */ > - str x2, [x4, x1, lsl #3] /* Mapping of paddr(start) */ > - mov x25, #1 /* x25 := identity map now in > place */ > - > -1: /* Setup boot_first: */ > - load_paddr x4, boot_first /* Next level into boot_first */ > - > - /* ... map boot_second in boot_first[0] */ > - load_paddr x1, boot_second > - mov x3, #PT_PT /* x2 := table map of boot_second */ > - orr x2, x1, x3 /* + rights for linear PT */ > - str x2, [x4, #0] /* Map it in slot 0 */ > - > - /* ... map of paddr(start) in boot_first */ > - cbnz x25, 1f /* x25 is set if already created */ > - lsr x2, x19, #FIRST_SHIFT /* x2 := Offset of base paddr in > boot_first */ > - and x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */ > - cbz x1, 1f /* It's in slot 0, map in > boot_second */ > - > - lsl x2, x2, #FIRST_SHIFT /* Base address for 1GB mapping */ > - mov x3, #PT_MEM /* x2 := Section map */ > - orr x2, x2, x3 > - str x2, [x4, x1, lsl #3] /* Create mapping of paddr(start)*/ > - mov x25, #1 /* x25 := identity map now in > place */ > - > -1: /* Setup boot_second: */ > - load_paddr x4, boot_second > - > - /* ... map boot_third in boot_second[1] */ > - load_paddr x1, boot_third > - mov x3, #PT_PT /* x2 := table map of boot_third */ > - orr x2, x1, x3 /* + rights for linear PT */ > - str x2, [x4, #8] /* Map it in slot 1 */ > - > - /* ... map of paddr(start) in boot_second */ > - cbnz x25, 1f /* x25 is set if already created */ > - lsr x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in > boot_second */ > - and x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */ > - cmp x1, #1 > - b.eq virtphys_clash /* It's in slot 1, which we cannot > handle */ > + /* Prepare the page-tables for mapping Xen */ > + ldr x0, =XEN_VIRT_START > + create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, > x1, x2, x3 > + create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, > x1, x2, x3 > + create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, > x1, x2, x3 > > - lsl x2, x2, #SECOND_SHIFT /* Base address for 2MB mapping */ > - mov x3, #PT_MEM /* x2 := Section map */ > - orr x2, x2, x3 > - str x2, [x4, x1, lsl #3] /* Create mapping of paddr(start)*/ > - mov x25, #1 /* x25 := identity map now in > place */ > - > -1: /* Setup boot_third: */ > - load_paddr x4, boot_third > + /* Map Xen */ > + adr_l x4, boot_third > > lsr x2, x19, #THIRD_SHIFT /* Base address for 4K mapping */ > lsl x2, x2, #THIRD_SHIFT > @@ -603,21 +574,68 @@ create_page_tables: > cmp x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */ > b.lt 1b > > - /* Defer fixmap and dtb mapping until after paging enabled, to > - * avoid them clashing with the 1:1 mapping. */ > + /* > + * If Xen is loaded at exactly XEN_VIRT_START then we don't > + * need an additional 1:1 mapping, the virtual mapping will > + * suffice. > + */ > + cmp x19, #XEN_VIRT_START > + bne 1f > + ret > +1: > + /* > + * Only the first page of Xen will be part of the 1:1 mapping. > + * All the boot_*_id tables are linked together even if they may > + * not be all used. They will then be linked to the boot page > + * tables at the correct level. > + */ > + create_table_entry boot_first_id, boot_second_id, x19, > FIRST_SHIFT, x0, x1, x2 > + create_table_entry boot_second_id, boot_third_id, x19, > SECOND_SHIFT, x0, x1, x2 > + create_mapping_entry boot_third_id, x19, x19, x0, x1, x2 > + > + /* > + * Find the zeroeth slot used. Link boot_first_id into > + * boot_pgtable if the slot is not 0. For slot 0, the tables > + * associated with the 1:1 mapping will need to be linked in > + * boot_first or boot_second. > + */ > + lsr x0, x19, #ZEROETH_SHIFT /* x0 := zeroeth slot */ > + cbz x0, 1f > + /* It is not in slot 0, Link boot_first_id into boot_pgtable */ > + create_table_entry boot_pgtable, boot_first_id, x19, > ZEROETH_SHIFT, x0, x1, x2 > + ret > + > +1: > + /* > + * Find the first slot used. Link boot_second_id into boot_first > + * if the slot is not 0. For slot 0, the tables associated with > + * the 1:1 mapping will need to be linked in boot_second. > + */ > + lsr x0, x19, #FIRST_SHIFT > + and x0, x0, #LPAE_ENTRY_MASK /* x0 := first slot */ > + cbz x0, 1f > + /* It is not in slot 0, Link boot_second_id into boot_first */ > + create_table_entry boot_first, boot_second_id, x19, > FIRST_SHIFT, x0, x1, x2 > + ret > > - /* boot pagetable setup complete */ > +1: > + /* > + * Find the second slot used. Link boot_third_id into boot_second > + * if the slot is not 1 (runtime Xen mapping is 2M - 4M). > + * For slot 1, Xen is not yet able to handle it. > + */ > + lsr x0, x19, #SECOND_SHIFT > + and x0, x0, #LPAE_ENTRY_MASK /* x0 := first slot */ > + cmp x0, #1 > + beq virtphys_clash > + /* It is not in slot 1, link boot_third_id into boot_second */ > + create_table_entry boot_second, boot_third_id, x19, > SECOND_SHIFT, x0, x1, x2 > + ret > > - cbnz x25, 1f /* Did we manage to create an > identity mapping ? */ > - PRINT("Unable to build boot page tables - Failed to identity > map Xen.\r\n") > - b fail > virtphys_clash: > /* Identity map clashes with boot_third, which we cannot > handle yet */ > PRINT("- Unable to build boot page tables - virt and phys > addresses clash. -\r\n") > b fail > - > -1: > - ret > ENDPROC(create_page_tables) > > /* > @@ -719,28 +737,15 @@ ENDPROC(remove_identity_mapping) > * The fixmap cannot be mapped in create_page_tables because it may > * clash with the 1:1 mapping. > * > - * Clobbers x1 - x4 > + * Clobbers x0 - x3 > */ > setup_fixmap: > #ifdef CONFIG_EARLY_PRINTK > - /* Add UART to the fixmap table */ > - ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ > - lsr x2, x23, #THIRD_SHIFT > - lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */ > - mov x3, #PT_DEV_L3 > - orr x2, x2, x3 /* x2 := 4K dev map including UART */ > - str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first > fixmap's slot */ > + ldr x0, =EARLY_UART_VIRTUAL_ADDRESS > + create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, > type=PT_DEV_L3 > #endif > - > - /* Map fixmap into boot_second */ > - ldr x4, =boot_second /* x4 := vaddr (boot_second) */ > - load_paddr x2, xen_fixmap > - mov x3, #PT_PT > - orr x2, x2, x3 /* x2 := table map of xen_fixmap */ > - ldr x1, =FIXMAP_ADDR(0) > - lsr x1, x1, #(SECOND_SHIFT - 3) /* x1 := Slot for FIXMAP(0) */ > - str x2, [x4, x1] /* Map it in the fixmap's slot */ > - > + ldr x0, =FIXMAP_ADDR(0) > + create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, > x1, x2, x3 > /* Ensure any page table updates made above have occurred */ > dsb nshst > ret > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index c2f1795a71..bc1824d3ca 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -107,6 +107,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable); > DEFINE_BOOT_PAGE_TABLE(boot_first); > DEFINE_BOOT_PAGE_TABLE(boot_first_id); > #endif > +DEFINE_BOOT_PAGE_TABLE(boot_second_id); > +DEFINE_BOOT_PAGE_TABLE(boot_third_id); > DEFINE_BOOT_PAGE_TABLE(boot_second); > DEFINE_BOOT_PAGE_TABLE(boot_third); > >
On Thu, 27 Jun 2019, Julien Grall wrote: > On 6/27/19 7:55 PM, Stefano Stabellini wrote: > > On Mon, 10 Jun 2019, Julien Grall wrote: > > > +1: > > > + /* > > > + * Find the second slot used. Remove the entry for the first > > > + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). > > > + * For slot 1, it means the ID map was not created. > > > + */ > > > + lsr x1, x19, #SECOND_SHIFT > > > + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ > > > + cmp x1, #1 > > > + beq id_map_removed > > > + /* It is not in slot 1, remove the entry */ > > > + ldr x0, =boot_second /* x0 := second table */ > > > + str xzr, [x0, x1, lsl #3] > > > > Wouldn't it be a bit more reliable if we checked whether the slot in > > question for x19 (whether zero, first, second) is a pagetable pointer or > > section map, then zero it if it is a section map, otherwise go down one > > level? If we did it this way it would be independent from the way > > create_page_tables is written. > > Your suggestion will not comply with the architecture compliance and how Xen > is/will be working after the full rework. We want to remove everything > (mapping + table) added specifically for the 1:1 mapping. > > Otherwise, you may end up in a position where boot_first_id is still in place. > We would need to use the break-before-make sequence in subsequent code if we > were about to insert 1GB mapping at the same place. > > After my rework, we would have virtually no place where break-before-make will > be necessary as it will enforce all the mappings to be destroyed before hand. > So I would rather avoid to make a specific case for the 1:1 mapping. I don't fully understand your explanation. I understand the final goal of "removing everything (mapping + table) added specifically for the 1:1 mapping". I don't understand why my suggestion would be a hindrance toward that goal, compared to what it is done in this patch.
Hi Stefano, On 7/30/19 6:33 PM, Stefano Stabellini wrote: > On Thu, 27 Jun 2019, Julien Grall wrote: >> On 6/27/19 7:55 PM, Stefano Stabellini wrote: >>> On Mon, 10 Jun 2019, Julien Grall wrote: >>>> +1: >>>> + /* >>>> + * Find the second slot used. Remove the entry for the first >>>> + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). >>>> + * For slot 1, it means the ID map was not created. >>>> + */ >>>> + lsr x1, x19, #SECOND_SHIFT >>>> + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ >>>> + cmp x1, #1 >>>> + beq id_map_removed >>>> + /* It is not in slot 1, remove the entry */ >>>> + ldr x0, =boot_second /* x0 := second table */ >>>> + str xzr, [x0, x1, lsl #3] >>> >>> Wouldn't it be a bit more reliable if we checked whether the slot in >>> question for x19 (whether zero, first, second) is a pagetable pointer or >>> section map, then zero it if it is a section map, otherwise go down one >>> level? If we did it this way it would be independent from the way >>> create_page_tables is written. >> >> Your suggestion will not comply with the architecture compliance and how Xen >> is/will be working after the full rework. We want to remove everything >> (mapping + table) added specifically for the 1:1 mapping. >> >> Otherwise, you may end up in a position where boot_first_id is still in place. >> We would need to use the break-before-make sequence in subsequent code if we >> were about to insert 1GB mapping at the same place. >> >> After my rework, we would have virtually no place where break-before-make will >> be necessary as it will enforce all the mappings to be destroyed before hand. >> So I would rather avoid to make a specific case for the 1:1 mapping. > > I don't fully understand your explanation. I understand the final goal > of "removing everything (mapping + table) added specifically for the 1:1 > mapping". I don't understand why my suggestion would be a hindrance > toward that goal, compared to what it is done in this patch. Because, AFAICT, your suggestion will only remove the mapping and not the tables (such as boot_first_id). This is different from this patch where both mapping and tables are removed. So yes, my suggestion is not generic, but at least it does the job that is expected by this function. I.e removing anything that was specifically created for the identity mapping. Cheers,
On Tue, 30 Jul 2019, Julien Grall wrote: > Hi Stefano, > > On 7/30/19 6:33 PM, Stefano Stabellini wrote: > > On Thu, 27 Jun 2019, Julien Grall wrote: > > > On 6/27/19 7:55 PM, Stefano Stabellini wrote: > > > > On Mon, 10 Jun 2019, Julien Grall wrote: > > > > > +1: > > > > > + /* > > > > > + * Find the second slot used. Remove the entry for the first > > > > > + * table if the slot is not 1 (runtime Xen mapping is 2M - > > > > > 4M). > > > > > + * For slot 1, it means the ID map was not created. > > > > > + */ > > > > > + lsr x1, x19, #SECOND_SHIFT > > > > > + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ > > > > > + cmp x1, #1 > > > > > + beq id_map_removed > > > > > + /* It is not in slot 1, remove the entry */ > > > > > + ldr x0, =boot_second /* x0 := second table */ > > > > > + str xzr, [x0, x1, lsl #3] > > > > > > > > Wouldn't it be a bit more reliable if we checked whether the slot in > > > > question for x19 (whether zero, first, second) is a pagetable pointer or > > > > section map, then zero it if it is a section map, otherwise go down one > > > > level? If we did it this way it would be independent from the way > > > > create_page_tables is written. > > > > > > Your suggestion will not comply with the architecture compliance and how > > > Xen > > > is/will be working after the full rework. We want to remove everything > > > (mapping + table) added specifically for the 1:1 mapping. > > > > > > Otherwise, you may end up in a position where boot_first_id is still in > > > place. > > > We would need to use the break-before-make sequence in subsequent code if > > > we > > > were about to insert 1GB mapping at the same place. > > > > > > After my rework, we would have virtually no place where break-before-make > > > will > > > be necessary as it will enforce all the mappings to be destroyed before > > > hand. > > > So I would rather avoid to make a specific case for the 1:1 mapping. > > > > I don't fully understand your explanation. I understand the final goal > > of "removing everything (mapping + table) added specifically for the 1:1 > > mapping". I don't understand why my suggestion would be a hindrance > > toward that goal, compared to what it is done in this patch. > > Because, AFAICT, your suggestion will only remove the mapping and not the > tables (such as boot_first_id). This is different from this patch where both > mapping and tables are removed. > > So yes, my suggestion is not generic, but at least it does the job that is > expected by this function. I.e removing anything that was specifically created > for the identity mapping. I understand your comment now, and of course I agree that both mapping and tables need to be removed. I am careful making suggestions for assembly coding because I don't really want to suggest something that doesn't work, or even if it works that it's worse than the original. It should be possible to remove both the table and the mapping in a generic way. Instead of hardcoding the assemply equivalent of "It is not in slot 0, remove the entry", we could check whether the table offset matches the table offset of the mapping that we want to preserve. That way, "slot 0" would be calculate instead of hardcoded, and the code would be pretty generic. What do you think? It should only be a small addition.
Hi Stefano, On 7/31/19 9:40 PM, Stefano Stabellini wrote: > On Tue, 30 Jul 2019, Julien Grall wrote: >> Hi Stefano, >> >> On 7/30/19 6:33 PM, Stefano Stabellini wrote: >>> On Thu, 27 Jun 2019, Julien Grall wrote: >>>> On 6/27/19 7:55 PM, Stefano Stabellini wrote: >>>>> On Mon, 10 Jun 2019, Julien Grall wrote: >>>>>> +1: >>>>>> + /* >>>>>> + * Find the second slot used. Remove the entry for the first >>>>>> + * table if the slot is not 1 (runtime Xen mapping is 2M - >>>>>> 4M). >>>>>> + * For slot 1, it means the ID map was not created. >>>>>> + */ >>>>>> + lsr x1, x19, #SECOND_SHIFT >>>>>> + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ >>>>>> + cmp x1, #1 >>>>>> + beq id_map_removed >>>>>> + /* It is not in slot 1, remove the entry */ >>>>>> + ldr x0, =boot_second /* x0 := second table */ >>>>>> + str xzr, [x0, x1, lsl #3] >>>>> >>>>> Wouldn't it be a bit more reliable if we checked whether the slot in >>>>> question for x19 (whether zero, first, second) is a pagetable pointer or >>>>> section map, then zero it if it is a section map, otherwise go down one >>>>> level? If we did it this way it would be independent from the way >>>>> create_page_tables is written. >>>> >>>> Your suggestion will not comply with the architecture compliance and how >>>> Xen >>>> is/will be working after the full rework. We want to remove everything >>>> (mapping + table) added specifically for the 1:1 mapping. >>>> >>>> Otherwise, you may end up in a position where boot_first_id is still in >>>> place. >>>> We would need to use the break-before-make sequence in subsequent code if >>>> we >>>> were about to insert 1GB mapping at the same place. >>>> >>>> After my rework, we would have virtually no place where break-before-make >>>> will >>>> be necessary as it will enforce all the mappings to be destroyed before >>>> hand. >>>> So I would rather avoid to make a specific case for the 1:1 mapping. >>> >>> I don't fully understand your explanation. I understand the final goal >>> of "removing everything (mapping + table) added specifically for the 1:1 >>> mapping". I don't understand why my suggestion would be a hindrance >>> toward that goal, compared to what it is done in this patch. >> >> Because, AFAICT, your suggestion will only remove the mapping and not the >> tables (such as boot_first_id). This is different from this patch where both >> mapping and tables are removed. >> >> So yes, my suggestion is not generic, but at least it does the job that is >> expected by this function. I.e removing anything that was specifically created >> for the identity mapping. > > I understand your comment now, and of course I agree that both mapping > and tables need to be removed. > > I am careful making suggestions for assembly coding because I don't > really want to suggest something that doesn't work, or even if it works > that it's worse than the original. > > It should be possible to remove both the table and the mapping in a > generic way. Instead of hardcoding the assemply equivalent of "It is not > in slot 0, remove the entry", we could check whether the table offset > matches the table offset of the mapping that we want to preserve. That > way, "slot 0" would be calculate instead of hardcoded, and the code > would be pretty generic. What do you think? It should only be a small > addition. It should be feasible and may actually help the next step in my plan where I need to make Xen relocatable. I will have a look for both the arm32 and arm64 code. Cheers,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 192af3e8a2..96e85f8834 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -300,6 +300,13 @@ real_start_efi: ldr x0, =primary_switched br x0 primary_switched: + /* + * The ID map may clash with other parts of the Xen virtual memory + * layout. As it is not used anymore, remove it completely to + * avoid having to worry about replacing existing mapping + * afterwards. + */ + bl remove_id_map bl setup_fixmap #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ @@ -632,10 +639,68 @@ enable_mmu: ret ENDPROC(enable_mmu) +/* + * Remove the ID map for the page-tables. It is not easy to keep track + * where the ID map was mapped, so we will look for the top-level entry + * exclusive to the ID Map and remove it. + * + * Inputs: + * x19: paddr(start) + * + * Clobbers x0 - x1 + */ +remove_id_map: + /* + * Find the zeroeth slot used. Remove the entry from zeroeth + * table if the slot is not 0. For slot 0, the ID map was either + * done in first or second table. + */ + lsr x1, x19, #ZEROETH_SHIFT /* x1 := zeroeth slot */ + cbz x1, 1f + /* It is not in slot 0, remove the entry */ + ldr x0, =boot_pgtable /* x0 := root table */ + str xzr, [x0, x1, lsl #3] + b id_map_removed + +1: + /* + * Find the first slot used. Remove the entry for the first + * table if the slot is not 0. For slot 0, the ID map was done + * in the second table. + */ + lsr x1, x19, #FIRST_SHIFT + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ + cbz x1, 1f + /* It is not in slot 0, remove the entry */ + ldr x0, =boot_first /* x0 := first table */ + str xzr, [x0, x1, lsl #3] + b id_map_removed + +1: + /* + * Find the second slot used. Remove the entry for the first + * table if the slot is not 1 (runtime Xen mapping is 2M - 4M). + * For slot 1, it means the ID map was not created. + */ + lsr x1, x19, #SECOND_SHIFT + and x1, x1, #LPAE_ENTRY_MASK /* x1 := first slot */ + cmp x1, #1 + beq id_map_removed + /* It is not in slot 1, remove the entry */ + ldr x0, =boot_second /* x0 := second table */ + str xzr, [x0, x1, lsl #3] + +id_map_removed: + /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */ + dsb nshst + tlbi alle2 + dsb nsh + isb + + ret +ENDPROC(remove_id_map) + setup_fixmap: - /* Now we can install the fixmap and dtb mappings, since we - * don't need the 1:1 map any more */ - dsb sy #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ /* Add UART to the fixmap table */ ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ @@ -653,19 +718,10 @@ setup_fixmap: ldr x1, =FIXMAP_ADDR(0) lsr x1, x1, #(SECOND_SHIFT - 3) /* x1 := Slot for FIXMAP(0) */ str x2, [x4, x1] /* Map it in the fixmap's slot */ -#endif - /* - * Flush the TLB in case the 1:1 mapping happens to clash with - * the virtual addresses used by the fixmap or DTB. - */ - dsb sy /* Ensure any page table updates made above - * have occurred. */ - - isb - tlbi alle2 - dsb sy /* Ensure completion of TLB flush */ - isb + /* Ensure any page table updates made above have occurred */ + dsb nshst +#endif ret ENDPROC(setup_fixmap)
The ID map may clash with other parts of the Xen virtual memory layout. At the moment, Xen is handling the clash by only creating a mapping to the runtime virtual address before enabling the MMU. The rest of the mappings (such as the fixmap) will be mapped after the MMU is enabled. However, the code doing the mapping is not safe as it replace mapping without using the Break-Before-Make sequence. As the ID map can be anywhere in the memory, it is easier to remove all the entries added as soon as the ID map is not used rather than adding the Break-Before-Make sequence everywhere. It is difficult to track where exactly the ID map was created without a full rework of create_page_tables(). Instead, introduce a new function remove_id_map() will look where is the top-level entry for the ID map and remove it. The new function is only called for the boot CPU. Secondary CPUs will switch directly to the runtime page-tables so there are no need to remove the ID mapping. Note that this still doesn't make the Secondary CPUs path safe but it is not making it worst. --- Note that the comment refers to the patch "xen/arm: tlbflush: Rework TLB helpers" under review (see [1]). Furthermore, it is very likely we will need to re-introduce the ID map to cater secondary CPUs boot and suspend/resume. For now, the attempt is to make boot CPU path fully Arm Arm compliant. [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01134.html --- xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 71 insertions(+), 15 deletions(-)