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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julienm

> On Aug 24, 2023, at 18:19, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

Sounds good to me.

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

Got it :))

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

Thanks for sharing this info, I will drop the modification to 
update_identity_mapping()
from this patch.

Kind regards,
Henry


> 
> Cheers,
> 
> 
> [1] https://lore.kernel.org/all/20221216114853.8227-21-julien@xxxxxxx/
> 
> -- 
> Julien Grall