Re: [PATCH 2/9] xen/arm64: head: Don't map too much in boot_third

[Julien Grall <julien@xxxxxxx>]

Hi,

On 29/06/2023 08:26, Michal Orzel wrote:
> On 28/06/2023 20:21, Julien Grall wrote:
> > Hi,
> >
> > On 26/06/2023 12:28, Michal Orzel wrote:
> > > On 25/06/2023 22:49, Julien Grall wrote:
> > > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > >
> > > > At the moment, we are mapping the size of the reserved area for Xen
> > > > (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
> > > > after Xen, so it is not a good idea to map more than necessary for a
> > > > couple of reasons:
> > > >       * We would need to use break-before-make if the extra PTE needs to
> > > >         be updated to point to another region
> > > >       * The extra area mapped may be mapped again by Xen with different
> > > >         memory attribute. This would result to attribue mismatch.
> > > s/attribue/attribute
> > >
> > > > Therefore, rework the logic in create_page_tables() to map only what's
> > > > necessary. To simplify the logic, we also want to make sure _end
> > > > is page-aligned. So align the symbol in the linker and add an assert
> > > > to catch any change.
> > > >
> > > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > > ---
> > > >    xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
> > > >    xen/arch/arm/xen.lds.S    |  3 +++
> > > >    2 files changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > > index f37133cf7ccd..66bc85d4c39e 100644
> > > > --- a/xen/arch/arm/arm64/head.S
> > > > +++ b/xen/arch/arm/arm64/head.S
> > > > @@ -572,6 +572,19 @@ create_page_tables:
> > > >            create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
> > > >            create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
> > > >
> > > > +        /*
> > > > +         * Find the size of Xen in pages and multiply by the size of a
> > > > +         * PTE. This will then be compared in the mapping loop below.
> > > > +         *
> > > > +         * Note the multiplication is just to avoid using an extra
> > > > +         * register/instruction per iteration.
> > > > +         */
> > > > +        ldr   x0, =_start            /* x0 := vaddr(_start) */
> > > x0 is already set to vaddr of _start by the first instruction of 
> > > create_page_tables
> > > and is preserved by create_table_entry. You could just reuse it instead of 
> > > re-loading.
> > I agree that the load is technically redundant. However, I find this
> > approach easier to read (you don't have to remember that _start equals
> > XEN_VIRT_START).
> Ok. As a side note not related to this patch, would it make sense to add an assert in linke > script to make sure _start equals XEN_VIRT_START, since we use them 
interchangeably?

Good point. I can add it in this patch.

Cheers,

--
Julien Grall