Re: [PATCH v3 10/52] xen/arm: Move MMU related definitions from config.h to mmu/layout.h

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Julien

On 2023/7/5 18:30, Julien Grall wrote:
> Hi Penny,
>
> On 05/07/2023 07:51, Penny Zheng wrote:
> > On 2023/7/5 05:54, Julien Grall wrote:
> > > Hi Penny,
> > >
> > > On 26/06/2023 04:34, 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 MMU systems, but there are some address
> > > Did you mean MPU?
> > yes, typo
> >
> > > > layout related definitions that are defined for MMU systems only.
> > > > These definitions could not be used by MPU systems, but adding
> > > > ifdefery with CONFIG_HAS_MPU to gate these definitions will result
> > > > in a messy and hard-to-read/maintain code.
> > > >
> > > > So we keep some common definitions still in config.h, but move MMU
> > > > related definitions to a new file - mmu/layout.h to avoid spreading
> > > > "#ifdef" everywhere.
> > > Just to ease the review, can you add some details which one are 
> > > considered common?
> > Sure,
> > IMO， the only part left in common is as follows:
> > ```
> > #ifdef CONFIG_ARM_64
> >
> > #define FRAMETABLE_SIZE        GB(32)
> > #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
> >
> > #endif
> > ```
> Hmmm... Looking at the result of the patch, you moved FRAMETABLE_SIZE 
> and FRAMETABLE_NR in layout.h. Also, I can't find any layout specific 
> define in config.h. So I think the paragraph could be dropped.
That's because I define this same snippet in both mmu/layout.h and
mpu/layout.h, see [PATCH v3 23/52] xen/arm: create mpu/layout.h for MPU 
related address definitions.
So it is common for both mmu/layout.h and mpu/layout.h.
We could leave it later for discussion.

> > I couldn't figure a proper way to remove the limit for MPU system.
> >
> > 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, it may bring a lot of changes in pdx 
> > common codes, like, variable "pdx_group_valid" needs to be allocated 
> > in runtime, according actual frametable size, at least for MPU case.
> The main problem is that, at least on Arm, the PDX is initialized before 
> you can allocate any memory. You should be able to re-order the code so 
> we populate the boot allocator first.
> But I don't think this is worth it right now as, if I am not mistaken, 
> the pdx_group_valid is only 256 bytes on Arm64.
> Cheers,