Re: [PATCH v5 09/13] xen/arm: mm: Use generic variable/function names for extendability

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 24/08/2023 10:46, Henry Wang wrote:
> > On Aug 22, 2023, at 02:32, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On 14/08/2023 05:25, Henry Wang wrote:
> > > From: Penny Zheng <penny.zheng@xxxxxxx>
> > > As preparation for MPU support, which will use some variables/functions
> > > for both MMU and MPU system, We rename the affected variable/function
> > > to more generic names:
> > > - init_ttbr -> init_mm,
> > You moved init_ttbr to mm/mmu.c. So why does this need to be renamed?
> >
> > And if you really planned to use it for the MPU code. Then init_ttbr should not 
> > have been moved.
> You are correct. I think we need to use the “init_mm” for MPU SMP support,
> so I would not move this variable in v6.
Your branch mpu_v5 doesn't seem to contain any use. But I would expect 
that the common is never going to use the variable. Also, at the moment 
it is 64-bit but I don't see why it would be necessary to be bigger than 
32-bit on 32-bit.
So I think it would be preferable if init_ttbr is move in mm/mmu.c. You
can then introduce an MPU specific variable.

In general, only variables that will be used by common code should be 
defined in common. All the rest should be defined in their specific 
directory.
> > > - mmu_init_secondary_cpu() -> mm_init_secondary_cpu()
> > > - init_secondary_pagetables() -> init_secondary_mm()
> > The original naming were not great but the new one are a lot more confusing as 
> > they seem to just be a reshuffle of word.
> >
> > mm_init_secondary_cpu() is only setting the WxN bit. For the MMU, I think it 
> > can be done much earlier. Do you have anything to add in it? If not, then I 
> > would consider to get rid of it.
> I’ve got rid of mmu_init_secondary_cpu() function in my local v6 as it is now
> folded to the assembly code.
>
> > For init_secondary_mm(), I would renamed it to prepare_secondary_mm().
> Sure, thanks for the name suggestion.
>
> > >   -void update_identity_mapping(bool enable)
> > > +static void update_identity_mapping(bool enable)
> > Why not simply renaming this function to update_mm_mapping()? But...
> >
> > >   {
> > >       paddr_t id_addr = virt_to_maddr(_start);
> > >       int rc;
> > > @@ -120,6 +120,11 @@ void update_identity_mapping(bool enable)
> > >       BUG_ON(rc);
> > >   }
> > >   +void update_mm_mapping(bool enable)
> > ... the new name it quite confusing. What is the mapping it is referring to?
> So I checked the MPU SMP support code and now I think I understand the reason
> why update_mm_mapping() was introduced:
>
> In the future we eventually need to support SMP for MMU systems, which means
> we need to call arch_cpu_up() and arch_cpu_up_finish(). These two functions call
> update_identity_mapping(). Since we believe "identity mapping" is a MMU concept,
> we changed this to generic name "mm mapping” as arch_cpu_up() and
> arch_cpu_up_finish() is a shared path between MMU and MPU.
The function is today called "update_identity_mapping()" because this is 
what the implementation does on arm64. But the goal of this function is 
to make sure that any mapping necessary for bring-up secondary CPUs are 
present.
So if you don't need similar work for the MPU then I would go with...

> But I think MPU won’t use update_mm_mapping() function at all, so I wonder do
> you prefer creating an empty stub update_identity_mapping() for MPU? Or use 
> #ifdef
> as suggested in your previous email...

... #ifdef. I have some preliminary work where the call to 
update_identity_mapping() may end up to be moved somewhere else as the 
page-tables would not be shared between pCPU anymore. So the logic will 
not some rework (see [1]).
Cheers,


[1] https://lore.kernel.org/all/20221216114853.8227-21-julien@xxxxxxx/

--
Julien Grall