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

[Julien Grall <julien@xxxxxxx>]

On 18/01/2023 02:18, Wei Chen wrote:
> Hi Julien,
Hi Wei,

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

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

--
Julien Grall