Re: [PATCH v2 07/40] xen/arm64: add .text.idmap for Xen identity map sections

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/01/2023 05:28, Penny Zheng wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> Only the first 4KB of Xen image will be mapped as identity
> (PA == VA). At the moment, Xen guarantees this by having
> everything that needs to be used in the identity mapping
> in head.S before _end_boot and checking at link time if this
> fits in 4KB.
>
> In previous patch, we have moved the MMU code outside of
> head.S. Although we have added .text.header to the new file
> to guarantee all identity map code still in the first 4KB.
> However, the order of these two files on this 4KB depends
> on the build tools. Currently, we use the build tools to
> process the order of objs in the Makefile to ensure that
> head.S must be at the top. But if you change to another build
> tools, it may not be the same result.
Right, so this is fixing a bug you introduced in the previous patch. We 
should really avoid introducing (latent) regression in a series. So 
please re-order the patches.
> In this patch we introduce .text.idmap to head_mmu.S, and
> add this section after .text.header. to ensure code of
> head_mmu.S after the code of header.S.
>
> After this, we will still include some code that does not
> belong to identity map before _end_boot. Because we have
> moved _end_boot to head_mmu.S. 
I dislike this approach because you are expecting that only head_mmu.S 
will be part of .text.idmap. If it is not, everything could blow up again.
That said, if you look at staging, you will notice that now _end_boot is 
defined in the linker script to avoid any issue.
> That means all code in head.S
> will be included before _end_boot. In this patch, we also
> added .text flag in the place of original _end_boot in head.S.
> All the code after .text in head.S will not be included in
> identity map section.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
> v1 -> v2:
> 1. New patch.
> ---
>   xen/arch/arm/arm64/head.S     | 6 ++++++
>   xen/arch/arm/arm64/head_mmu.S | 2 +-
>   xen/arch/arm/xen.lds.S        | 1 +
>   3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 5cfa47279b..782bd1f94c 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -466,6 +466,12 @@ fail:   PRINT("- Boot failed -\r\n")
>           b     1b
>   ENDPROC(fail)
>   
> +/*
> + * For the code that do not need in indentity map section,
> + * we put them back to normal .text section
> + */
> +.section .text, "ax", %progbits
> +
I would argue that puts wants to be part of the idmap.

>   #ifdef CONFIG_EARLY_PRINTK
>   /*
>    * Initialize the UART. Should only be called on the boot CPU.
> diff --git a/xen/arch/arm/arm64/head_mmu.S b/xen/arch/arm/arm64/head_mmu.S
> index e2c8f07140..6ff13c751c 100644
> --- a/xen/arch/arm/arm64/head_mmu.S
> +++ b/xen/arch/arm/arm64/head_mmu.S
> @@ -105,7 +105,7 @@
>           str   \tmp2, [\tmp3, \tmp1, lsl #3]
>   .endm
>   
> -.section .text.header, "ax", %progbits
> +.section .text.idmap, "ax", %progbits
>   /*.aarch64*/
>   
>   /*
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 92c2984052..bc45ea2c65 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -33,6 +33,7 @@ SECTIONS
>     .text : {
>           _stext = .;            /* Text section */
>          *(.text.header)
> +       *(.text.idmap)
>   
>          *(.text.cold)
>          *(.text.unlikely .text.*_unlikely .text.unlikely.*)
Cheers,

--
Julien Grall