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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 23/08/2024 17:31, 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 in this patch, we reuse the existing arm/platforms to store
> Armv8-R platforms' parameters. And `XEN_START_ADDRESS` is one
> kind of MPU based platform's parameters. So we define default
> `XEN_START_ADDRESS` in the MPU specific header file.
This seems a left-over from as previous work?

> We also introduce one Kconfig option for users to override the
> default Xen start address of selected platform, if they think
> the default address doesn't suit their scenarios. For this
> Kconfig option, we use an unaligned address "0xffffffff" as the
> default value to indicate that users haven't used a customized
> Xen start address.
>
> 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>
> ---
>   xen/arch/arm/Kconfig                  | 11 +++++++++++
>   xen/arch/arm/include/asm/mpu/layout.h | 25 +++++++++++++++++++++++++
>   2 files changed, 36 insertions(+)
>   create mode 100644 xen/arch/arm/include/asm/mpu/layout.h
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 686948cefd..d722ca690c 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -23,6 +23,17 @@ 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
> +         This option is allowed to set customized address at which Xen will be
> +         linked on MPU systems.
I don't quite understand what you mean by "allowed".

>  This address must be aligned to a page size.> +	  Use 0xFFFFFFFF as 
the default value to indicate that user hasn't
> +         customized this address, and Xen use use the default value that has
> +         been defined in platform files.
At least in this patch, there is no default set. Instead you will throw 
an error.
> +
>   menu "Architecture Features"
>   
>   choice
> 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..82a74b8b5b
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/layout.h
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Coding style: /* SPDX-... */

> +/*
> + * config_mpu.h: A Linux-style configuration list for Arm MPU systems,
> + *               only can be included by config.h
> + */
I don't understand how this description match the content below. Also, 
shouldn't you include it from xen/config.h?
> +
> +#ifndef __ARM_CONFIG_MPU_H__
> +#define __ARM_CONFIG_MPU_H__
This doesn't match the name of the file.

> +
> +
> +#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.
> + */
> +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0
> +#error "XEN_START_ADDRESS must be aligned to PAGE_SIZE"
> +#endif
> +
> +#define XEN_VIRT_START         _AT(paddr_t, XEN_START_ADDRESS)
> +
> +#endif /* __ARM_CONFIG_MPU_H__ */
> +
Cheers,

--
Julien Grall