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

[Julien Grall <julien@xxxxxxx>]

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