Re: [PATCH v6 05/11] xen/arm: define Xen start address for FVP BaseR platform

[Julien Grall <julien@xxxxxxx>]

On 04/11/2022 10:07, Wei Chen wrote:
> 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 FVP BaseR platform's parameters. So we define default
> `XEN_START_ADDRESS` for FVP BaseR in its platform file.
>
> 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.
>
> And as we introduced Armv8-R platforms to Xen, that means the
> existed Arm64 platforms should not be listed in Armv8-R platform
> list, so we add !ARM_V8R dependency for these platforms.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Jiamei.Xie <jiamei.xie@xxxxxxx>
> ---
>   xen/arch/arm/Kconfig                           | 11 +++++++++++
>   xen/arch/arm/include/asm/platforms/fvp_baser.h | 14 ++++++++++++++
I looked at the content of fvp_baser.h after this series is applied. 
There are a bit of boiler plate that I expect to be part for other 
platforms. In particular...
>   xen/arch/arm/platforms/Kconfig                 | 16 +++++++++++++---
>   3 files changed, 38 insertions(+), 3 deletions(-)
>   create mode 100644 xen/arch/arm/include/asm/platforms/fvp_baser.h
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ad592367bd..ac276307d6 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -138,6 +138,17 @@ config TEE
>           This option enables generic TEE mediators support. It allows guests
>           to access real TEE via one of TEE mediators implemented in XEN.
>   
> +config XEN_START_ADDRESS
> +       hex "Xen start address: keep default to use platform defined address"
> +       default 0xFFFFFFFF
... this default value will need to be tested everywhere. At least for 
now, I think you can avoid the per platform header by using the Kconfig 
to select the proper address (see the config for selecting early printk 
address).
This will also avoids to use an invalid value here.

> +       depends on HAS_MPU
> +       help
> +         This option allows to set the customized address at which Xen will be
> +         linked on MPU systems. 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.
> +
>   source "arch/arm/tee/Kconfig"
>   
>   config STATIC_SHM
> diff --git a/xen/arch/arm/include/asm/platforms/fvp_baser.h 
> b/xen/arch/arm/include/asm/platforms/fvp_baser.h
> new file mode 100644
> index 0000000000..9450a411a9
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/platforms/fvp_baser.h
> @@ -0,0 +1,14 @@
> +#ifndef __ASM_ARM_PLATFORMS_FVP_BASER_H__
> +#define __ASM_ARM_PLATFORMS_FVP_BASER_H__
> +
> +/*
> + * 0xFFFFFFFF indicates users haven't customized XEN_START_ADDRESS,
> + * we will use platform defined default address.
> + */
> +#if CONFIG_XEN_START_ADDRESS == 0xFFFFFFFF
> +#define XEN_START_ADDRESS 0x200000
> +#else
> +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
> +#endif
> +
> +#endif /* __ASM_ARM_PLATFORMS_FVP_BASER_H__ */
> diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
> index c93a6b2756..0904793a0b 100644
> --- a/xen/arch/arm/platforms/Kconfig
> +++ b/xen/arch/arm/platforms/Kconfig
> @@ -1,6 +1,7 @@
>   choice
>         prompt "Platform Support"
>         default ALL_PLAT
> +       default FVP_BASER if ARM_V8R
Is there any reason to create a new Kconfig rather than using MPU?

>         ---help---
>         Choose which hardware platform to enable in Xen.
>   
> @@ -8,13 +9,14 @@ choice
>   
>   config ALL_PLAT
>         bool "All Platforms"
> +       depends on !ARM_V8R
>         ---help---
>         Enable support for all available hardware platforms. It doesn't
>         automatically select any of the related drivers.
>   
>   config QEMU
>         bool "QEMU aarch virt machine support"
> -       depends on ARM_64
> +       depends on ARM_64 && !ARM_V8R
>         select GICV3
>         select HAS_PL011
>         ---help---
> @@ -23,7 +25,7 @@ config QEMU
>   
>   config RCAR3
>         bool "Renesas RCar3 support"
> -       depends on ARM_64
> +       depends on ARM_64 && !ARM_V8R
>         select HAS_SCIF
>         select IPMMU_VMSA
>         ---help---
> @@ -31,14 +33,22 @@ config RCAR3
>   
>   config MPSOC
>         bool "Xilinx Ultrascale+ MPSoC support"
> -       depends on ARM_64
> +       depends on ARM_64 && !ARM_V8R
>         select HAS_CADENCE_UART
>         select ARM_SMMU
>         ---help---
>         Enable all the required drivers for Xilinx Ultrascale+ MPSoC
>   
> +config FVP_BASER
> +       bool "Fixed Virtual Platform BaseR support"
> +       depends on ARM_V8R
> +       help
> +         Enable platform specific configurations for Fixed Virtual
> +         Platform BaseR
> +
>   config NO_PLAT
>         bool "No Platforms"
> +       depends on !ARM_V8R
>         ---help---
>         Do not enable specific support for any platform.
>   
Cheers,

--
Julien Grall