Re: [PATCH v5 11/13] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START"

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Fri, Jan 5, 2024 at 7:20 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Carlo,
>
> On 02/01/2024 09:51, Carlo Nonato wrote:
> > This reverts commit 0c18fb76323bfb13615b6f13c98767face2d8097 (not clean).
> >
> > This is not a clean revert since the rework of the memory layout, but it is
> > sufficiently similar to a clean one.
> > The only difference is that the BOOT_RELOC_VIRT_START must match the new
> > layout.
> >
> > Cache coloring support for Xen needs to relocate Xen code and data in a new
> > colored physical space. The BOOT_RELOC_VIRT_START will be used as the 
> > virtual
> > base address for a temporary mapping to this new space.
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > ---
> >   xen/arch/arm/include/asm/mmu/layout.h | 2 ++
> >   xen/arch/arm/mmu/setup.c              | 1 +
> >   2 files changed, 3 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/mmu/layout.h 
> > b/xen/arch/arm/include/asm/mmu/layout.h
> > index eac7eef885..30031f74d9 100644
> > --- a/xen/arch/arm/include/asm/mmu/layout.h
> > +++ b/xen/arch/arm/include/asm/mmu/layout.h
> > @@ -74,6 +74,8 @@
> >   #define BOOT_FDT_VIRT_START     (FIXMAP_VIRT_START + FIXMAP_VIRT_SIZE)
> >   #define BOOT_FDT_VIRT_SIZE      _AT(vaddr_t, MB(4))
> >
> > +#define BOOT_RELOC_VIRT_START   (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE)
>
> This new addition wants to be documented in the layout comment in a few
> lines above. Also, the area you are using is 2MB whereas Xen can now be
> up to 8MB.
>
> Secondly, you want to add a BOOTRELOC_VIRT_SIZE with a check in
> build_assertions() making sure that this is at least as big as
> XEN_VIRT_SIZE.
>
> Overall, I am not sure this is really a revert at this point. The idea
> is the same, but you are defining BOOT_FDT_VIRT_START differently.
>
> To me it feels like it belong to the first patch where you will use it.
> And that would be ok to mention in the commit message that the idea was
> borrowed from a previously reverted commit.

Ok, nice.

> > +
> >   #ifdef CONFIG_LIVEPATCH
> >   #define LIVEPATCH_VMAP_START    (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE)
> >   #define LIVEPATCH_VMAP_SIZE    _AT(vaddr_t, MB(2))
> > diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> > index d5264e51bc..37b6d230ad 100644
> > --- a/xen/arch/arm/mmu/setup.c
> > +++ b/xen/arch/arm/mmu/setup.c
> > @@ -69,6 +69,7 @@ static void __init __maybe_unused build_assertions(void)
> >       /* 2MB aligned regions */
> >       BUILD_BUG_ON(XEN_VIRT_START & ~SECOND_MASK);
> >       BUILD_BUG_ON(FIXMAP_ADDR(0) & ~SECOND_MASK);
> > +    BUILD_BUG_ON(BOOT_RELOC_VIRT_START & ~SECOND_MASK);
> >       /* 1GB aligned regions */
> >   #ifdef CONFIG_ARM_32
> >       BUILD_BUG_ON(XENHEAP_VIRT_START & ~FIRST_MASK);
>
> --
> Julien Grall

Thanks.