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

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi,

Sorry for the late reply. Got sidetracked by other tasks...

On 2023/1/19 22:20, Julien Grall wrote:

> 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 FfaRAMETABLE_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.
>       
>       
>

Yes, but when calculating variable pdx_group_valid, which is defined as '''

unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
                      (FRAMETABLE_NR + PDX_GROUP_COUNT - 1) / PDX_GROUP_COUNT)] = { [0] = 1 } ''',

it relies on FRAMETABLE_NR to limit array length. If we are trying to get rid of the limit for the MPU, hmmm,

it may bring a lot of changes in pdx common codes, for example, maybe variable pdx_group_valid needs to

be allocated in runtime, according actual frametable size, at least for MPU case.

So, here, I intend to keep the same limit as MMU has for MPU too, or any suggestion from you?

> 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.
>       
>       
>       [...]
>       
>

[...]

> Cheers,
>       
>       
>

Cheers,

Penny Zheng