Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 2023年1月18日 7:46
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>
> Subject: Re: [PATCH v2 07/40] xen/arm64: add .text.idmap for Xen identity
> map sections
>
> Hi,
>
> On 13/01/2023 05:28, Penny Zheng wrote:
> > From: Wei Chen <wei.c...@arm.com>
> >
> > 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.
>
Ok.
> >
> > 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.
>
I agree.
> That said, if you look at staging, you will notice that now _end_boot is
> defined in the linker script to avoid any issue.
>
Sorry, I am not quite clear about this comment. The _end_boot of original
staging branch is defined in head.S. And I am not quite sure how this
_end_boot solve multiple files contain idmap code.
Cheers,
Wei Chen
> > 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.c...@arm.com>
> > ---
> > 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.
>
I am ok to move puts to idmap. But from the original head.S, puts is
placed after _end_boot, and from the xen.ld.S, we can see idmap is
area is the section of "_end_boot - start". The reason of moving puts
to idmap is because we're using it in idmap?
Cheers,
Wei Chen
> > #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
