| Message ID | 20230801034419.2047541-7-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Split MMU code as the prepration of MPU work | expand |
Hi Henry, Title: NIT: It is more a fold rather than move. On 01/08/2023 04:44, Henry Wang wrote: > For the > future MPU support work, the early UART mapping could then be moved > in prepare_early_mappings(). I would drop this sentence as this is more related to the future implementation of MPU rather than this patch itself. > > No functional change intended. > > [1] https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998ed80@xen.org/ > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > v4: > - Rework "[v3,12/52] xen/mmu: extract early uart mapping from setup_fixmap" > --- > xen/arch/arm/arm64/head.S | 1 - > xen/arch/arm/arm64/mmu/head.S | 50 ++++++++++++----------------------- > 2 files changed, 17 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index e4f579a48e..56f68a8e37 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -275,7 +275,6 @@ real_start_efi: > b enable_boot_cpu_mm > > primary_switched: > - bl setup_fixmap > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S > index b7c3dd423a..6bd94c3a45 100644 > --- a/xen/arch/arm/arm64/mmu/head.S > +++ b/xen/arch/arm/arm64/mmu/head.S The second paragraph in create_page_tables() now needs to be dropped. > @@ -231,6 +231,23 @@ link_from_second_id: > create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2 > link_from_third_id: > create_mapping_entry boot_third_id, x19, x19, x0, x1, x2 > + > +#ifdef CONFIG_EARLY_PRINTK > + /* Add UART to the fixmap table */ > + ldr x0, =EARLY_UART_VIRTUAL_ADDRESS > + /* x23: Early UART base physical address */ > + create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3 > +#endif > + /* Map fixmap into boot_second */ > + ldr x0, =FIXMAP_ADDR(0) > + create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 > + /* Ensure any page table updates made above have occurred. */ > + dsb nshst > + /* > + * The fixmap area will be used soon after. So ensure no hardware > + * translation happens before the dsb completes. > + */ > + isb The DSB/ISB is not necessary anymore as you are not modifying live page-tables. So I would prefer if this is removed. Cheers,
Hi Julien, > On Aug 9, 2023, at 20:28, Julien Grall <julien@xen.org> wrote: > > Hi Henry, > > Title: NIT: It is more a fold rather than move. You are correct, using “fold” here is more appropriate. > > On 01/08/2023 04:44, Henry Wang wrote: >> For the >> future MPU support work, the early UART mapping could then be moved >> in prepare_early_mappings(). > > I would drop this sentence as this is more related to the future implementation of MPU rather than this patch itself. Sure, v5 will drop this. > >> No functional change intended. >> [1] https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998ed80@xen.org/ >> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> --- >> v4: >> - Rework "[v3,12/52] xen/mmu: extract early uart mapping from setup_fixmap" >> --- >> xen/arch/arm/arm64/head.S | 1 - >> xen/arch/arm/arm64/mmu/head.S | 50 ++++++++++++----------------------- >> 2 files changed, 17 insertions(+), 34 deletions(-) >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index e4f579a48e..56f68a8e37 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -275,7 +275,6 @@ real_start_efi: >> b enable_boot_cpu_mm >> primary_switched: >> - bl setup_fixmap >> #ifdef CONFIG_EARLY_PRINTK >> /* Use a virtual address to access the UART. */ >> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >> diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S >> index b7c3dd423a..6bd94c3a45 100644 >> --- a/xen/arch/arm/arm64/mmu/head.S >> +++ b/xen/arch/arm/arm64/mmu/head.S > > The second paragraph in create_page_tables() now needs to be dropped. Thanks for finding this! Yes this should be dropped. > >> @@ -231,6 +231,23 @@ link_from_second_id: >> create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2 >> link_from_third_id: >> create_mapping_entry boot_third_id, x19, x19, x0, x1, x2 >> + >> +#ifdef CONFIG_EARLY_PRINTK >> + /* Add UART to the fixmap table */ >> + ldr x0, =EARLY_UART_VIRTUAL_ADDRESS >> + /* x23: Early UART base physical address */ >> + create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3 >> +#endif >> + /* Map fixmap into boot_second */ >> + ldr x0, =FIXMAP_ADDR(0) >> + create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 >> + /* Ensure any page table updates made above have occurred. */ >> + dsb nshst >> + /* >> + * The fixmap area will be used soon after. So ensure no hardware >> + * translation happens before the dsb completes. >> + */ >> + isb > > The DSB/ISB is not necessary anymore as you are not modifying live page-tables. So I would prefer if this is removed. Sure. Kind regards, Henry > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index e4f579a48e..56f68a8e37 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -275,7 +275,6 @@ real_start_efi: b enable_boot_cpu_mm primary_switched: - bl setup_fixmap #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ ldr x23, =EARLY_UART_VIRTUAL_ADDRESS diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S index b7c3dd423a..6bd94c3a45 100644 --- a/xen/arch/arm/arm64/mmu/head.S +++ b/xen/arch/arm/arm64/mmu/head.S @@ -231,6 +231,23 @@ link_from_second_id: create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2 link_from_third_id: create_mapping_entry boot_third_id, x19, x19, x0, x1, x2 + +#ifdef CONFIG_EARLY_PRINTK + /* Add UART to the fixmap table */ + ldr x0, =EARLY_UART_VIRTUAL_ADDRESS + /* x23: Early UART base physical address */ + create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3 +#endif + /* Map fixmap into boot_second */ + ldr x0, =FIXMAP_ADDR(0) + create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 + /* Ensure any page table updates made above have occurred. */ + dsb nshst + /* + * The fixmap area will be used soon after. So ensure no hardware + * translation happens before the dsb completes. + */ + isb ret virtphys_clash: @@ -395,39 +412,6 @@ identity_mapping_removed: ret ENDPROC(remove_identity_mapping) -/* - * Map the UART in the fixmap (when earlyprintk is used) and hook the - * fixmap table in the page tables. - * - * The fixmap cannot be mapped in create_page_tables because it may - * clash with the 1:1 mapping. - * - * Inputs: - * x20: Physical offset - * x23: Early UART base physical address - * - * Clobbers x0 - x3 - */ -ENTRY(setup_fixmap) -#ifdef CONFIG_EARLY_PRINTK - /* Add UART to the fixmap table */ - 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 x0, =FIXMAP_ADDR(0) - create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 - /* Ensure any page table updates made above have occurred. */ - dsb nshst - /* - * The fixmap area will be used soon after. So ensure no hardware - * translation happens before the dsb completes. - */ - isb - - ret -ENDPROC(setup_fixmap) - /* Fail-stop */ fail: PRINT("- Boot failed -\r\n") 1: wfe
The original assembly setup_fixmap() is actually doing two seperate tasks, one is enabling the early UART when earlyprintk on, and the other is to set up the fixmap (even when earlyprintk is off). Per discussion in [1], since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), there is no chance that the fixmap and the mapping of early UART will clash with the 1:1 mapping. Therefore the mapping of both the fixmap and the early UART can be moved to the end of create_pagetables(). For the future MPU support work, the early UART mapping could then be moved in prepare_early_mappings(). No functional change intended. [1] https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998ed80@xen.org/ Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- v4: - Rework "[v3,12/52] xen/mmu: extract early uart mapping from setup_fixmap" --- xen/arch/arm/arm64/head.S | 1 - xen/arch/arm/arm64/mmu/head.S | 50 ++++++++++++----------------------- 2 files changed, 17 insertions(+), 34 deletions(-)