Re: [PATCH v2 2/4] xen/arm: mpu: Define Xen start address for MPU systems

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 18/09/2024 19:51, Ayan Kumar Halder wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> On Armv8-A, Xen has a fixed virtual start address (link address too) for all
> Armv8-A platforms. In an MMU based system, Xen can map its loaded address to
> this virtual start address. So, on Armv8-A platforms, the Xen start address does
> not need to be configurable. But on Armv8-R platforms, there is no MMU to map
> loaded address to a fixed virtual address and different platforms will have very
> different address space layout. So Xen cannot use a fixed physical address on
> MPU based system and need to have it configurable.
>
> So, we introduce a Kconfig option for users to set the start address. The start
> address needs to be aligned to 4KB. We have a check for this alignment.
It would suggest to add some explanation why you want the start address 
to be 4KB aligned.
> In case if the user forgets to set the start address, then 0xffffffff is used
> as default. This is to trigger the error (on alignment check) and thereby prompt
> user to set the start address.
>
> Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is
> defined.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Jiamei.Xie <jiamei.xie@xxxxxxx>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> Changes from :-
>
> v1 - 1. Fixed some of the coding style issues.
> 2. Reworded the help message.
> 3. Updated the commit message.
>
>   xen/arch/arm/Kconfig                  | 10 ++++++++++
>   xen/arch/arm/include/asm/config.h     |  4 +++-
>   xen/arch/arm/include/asm/mpu/layout.h | 27 +++++++++++++++++++++++++++
Looking at this patch again, I don't see any modification in xen.lds.S. 
Is it intended?
>   3 files changed, 40 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/arm/include/asm/mpu/layout.h
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index e881f5ba57..ab3ef005a6 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -23,6 +23,16 @@ config ARCH_DEFCONFIG
>         default "arch/arm/configs/arm32_defconfig" if ARM_32
>         default "arch/arm/configs/arm64_defconfig" if ARM_64
>   
> +config XEN_START_ADDRESS
> +       hex "Xen start address: keep default to use platform defined address"
> +       default 0xFFFFFFFF
> +       depends on MPU
> +       help
> +         Used to set customized address at which which Xen will be linked on 
> MPU
> +         systems. Must be aligned to 4KB.
> +         0xFFFFFFFF is used as default value to indicate that user has not
> +         customized this address.
> +
>   menu "Architecture Features"
>   
>   choice
> diff --git a/xen/arch/arm/include/asm/config.h 
> b/xen/arch/arm/include/asm/config.h
> index a2e22b659d..0a51142efd 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -69,8 +69,10 @@
>   #include <xen/const.h>
>   #include <xen/page-size.h>
>   
> -#ifdef CONFIG_MMU
> +#if defined(CONFIG_MMU)
>   #include <asm/mmu/layout.h>
> +#elif defined(CONFIG_MPU)
> +#include <asm/mpu/layout.h>
>   #else
>   # error "Unknown memory management layout"
>   #endif
> diff --git a/xen/arch/arm/include/asm/mpu/layout.h 
> b/xen/arch/arm/include/asm/mpu/layout.h
> new file mode 100644
> index 0000000000..f9a5be2d6b
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/layout.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_MPU_LAYOUT_H__
> +#define __ARM_MPU_LAYOUT_H__
> +
> +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
> +
> +/*
> + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. This
> + * address indicates where Xen image will be loaded and run from. This
> + * address must be aligned to a PAGE_SIZE.
Strictly speaking, this doesn't match the Kconfig. AFAIU, we still said 
the internal code may continue to rely on PAGE_SIZE for the time being. 
But I think we would want to have a BUILD_BUG_ON(PAGE_SIZE != SZ_4K) to 
catch any value change.
> + */
> +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0
> +#error "XEN_START_ADDRESS must be aligned to PAGE_SIZE"
In the error message, you want to mention 4KB.

Cheers,

--
Julien Grall