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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> >>>
> >>> 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.
> 
> If you look at the latest staging, there is a commit (229ebd517b9d) that
> now define _end_boot in the linker script.
> 
> The .text.idmap section can be added before the definition of _end_boot.
> 

Oh, my branch was a little old, I have seen this new definition in xen.ld.S
after I update the branch. I understand now.

> >
> > 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 original position of _end_boot is wrong. It didn't take into account
> the literal pool (there are at the end of the unit). So they would be
> past _end_boot.
> 

Ok.

> > The reason of moving puts
> > to idmap is because we're using it in idmap?
> 
> I guess it depends of what idmap really mean here. If you only interpret
> as the MMU is on and VA == PA. Then not yet (I was thinking to introduce
> a few calls).
> 
> If you also include the MMU off. Then yes.
> 
> Also, in the context of cache coloring, we will need to have a
> trampoline for cache coloring. So it would be better to keep everything
> close together as it is easier to copy.
> 

Understand, thanks!

Cheers,
Wei Chen

> Cheers,
> 
> --
> Julien Grall