Hi Julienm
> On Aug 24, 2023, at 18:19, Julien Grall <jul...@xen.org> wrote:
>
> Hi Henry,
>
> On 24/08/2023 10:46, Henry Wang wrote:
>>> On Aug 22, 2023, at 02:32, Julien Grall <jul...@xen.org> wrote:
>>>
>>> Hi,
>>>
>>> On 14/08/2023 05:25, Henry Wang wrote:
>>>> From: Penny Zheng <penny.zh...@arm.com>
>>>> 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-jul...@xen.org/
>
> --
> Julien Grall
