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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2023年1月18日 7:46
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> 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.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.
> 

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.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.
> 

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