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

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 06/07/2023 08:36, Penny Zheng wrote:
> 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.
This is different from what you wrote in the commit message:

"So we keep some common definitions still in config.h"

From the perspective of this patch you moved everything related to the 
layout in mmu/layout.h. Whether there might be duplication in the future 
is a different subject.
In this case, I would prefer the duplication because the size of the 
frametable really depend on how you configure the layout. So it should 
stay close the rest of the defines.
Cheers,

--
Julien Grall