Re: [PATCH v2 10/40] xen/arm: split MMU and MPU config files from config.h

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/01/2023 05:28, Penny Zheng wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> Xen defines some global configuration macros for Arm in
> config.h. We still want to use it for Armv8-R systems, but
> there are some address related macros that are defined for
> MMU systems. These macros will not be used by MPU systems,
> Adding ifdefery with CONFIG_HAS_MPU to gate these macros
> will result in a messy and hard-to-read/maintain code.
>
> So we keep some common definitions still in config.h, but
> move virtual address related definitions to a new file -
> config_mmu.h. And use a new file config_mpu.h to store
> definitions for MPU systems. To avoid spreading #ifdef
> everywhere, we keep the same definition names for MPU
> systems, like XEN_VIRT_START and HYPERVISOR_VIRT_START,
> but the definition contents are MPU specific.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
> v1 -> v2:
> 1. Remove duplicated FIXMAP definitions from config_mmu.h
> ---
>   xen/arch/arm/include/asm/config.h     | 103 +++--------------------
>   xen/arch/arm/include/asm/config_mmu.h | 112 ++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/config_mpu.h |  25 ++++++
I think this patch wants to be split in two. So we keep code movement 
separate from the introduction of new feature (e.g. MPU).
Furthermore, I think it would be better to name the new header layout_* 
(or similar).
Lastly, you are going to introduce several file with _mmu or _mpu. I 
would rather prefer if we create directory instead.

>   3 files changed, 147 insertions(+), 93 deletions(-)
>   create mode 100644 xen/arch/arm/include/asm/config_mmu.h
>   create mode 100644 xen/arch/arm/include/asm/config_mpu.h
>
> diff --git a/xen/arch/arm/include/asm/config.h 
> b/xen/arch/arm/include/asm/config.h
> index 25a625ff08..86d8142959 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -48,6 +48,12 @@
>   
>   #define INVALID_VCPU_ID MAX_VIRT_CPUS
>   
> +/* Used for calculating PDX */
I am not entirely sure to understand the purpose of this comment.

> +#ifdef CONFIG_ARM_64
> +#define FRAMETABLE_SIZE        GB(32)
> +#define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
> +#endif
> +
Why do you only keep the 64-bit version in config.h?

However... the frametable size is limited by the space we reserve in the 
virtual address space. This would not be the case for the MPU.
So having the limit in common seems a bit odd. In fact, I think we 
should look at getting rid of the limit for the MPU.
[...]

> diff --git a/xen/arch/arm/include/asm/config_mmu.h 
> b/xen/arch/arm/include/asm/config_mmu.h
> new file mode 100644
> index 0000000000..c12ff25cf4
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/config_mmu.h
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/******************************************************************************
> + * config_mmu.h
> + *
> + * A Linux-style configuration list, only can be included by config.h
Why do you need to restrict where this is included? And if you really 
need it, then you should check it.
Cheers,

--
Julien Grall