Hi Julien,
> 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.
>
>> - 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.
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...
>
> If you don't want to keep update_identity_mapping(), then I would consider to
> simply wrap...
…here and ...
>
>> +{
>> + update_identity_mapping(enable);
>> +}
>> +
>> extern void switch_ttbr_id(uint64_t ttbr);
>> typedef void (switch_ttbr_fn)(uint64_t ttbr);
>> @@ -131,7 +136,7 @@ void __init switch_ttbr(uint64_t ttbr)
>> lpae_t pte;
>> /* Enable the identity mapping in the boot page tables */
>> - update_identity_mapping(true);
>> + update_mm_mapping(true);
>> /* Enable the identity mapping in the runtime page tables */
>> pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id);
>> @@ -148,7 +153,7 @@ void __init switch_ttbr(uint64_t ttbr)
>> * Note it is not necessary to disable it in the boot page tables
>> * because they are not going to be used by this CPU anymore.
>> */
>> - update_identity_mapping(false);
>> + update_mm_mapping(false);
>> }
>> /*
>> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
>> index 9637f42469..2b1d086a1e 100644
>> --- a/xen/arch/arm/arm64/smpboot.c
>> +++ b/xen/arch/arm/arm64/smpboot.c
>> @@ -111,18 +111,18 @@ int arch_cpu_up(int cpu)
>> if ( !smp_enable_ops[cpu].prepare_cpu )
>> return -ENODEV;
>> - update_identity_mapping(true);
>> + update_mm_mapping(true);
>
> ... with #ifdef CONFIG_MMU here...
>
>> rc = smp_enable_ops[cpu].prepare_cpu(cpu);
>> if ( rc )
>> - update_identity_mapping(false);
>> + update_mm_mapping(false);
>
> ... here and ...
>
>
>> return rc;
>> }
>> void arch_cpu_up_finish(void)
>> {
>> - update_identity_mapping(false);
>> + update_mm_mapping(false);
>
> ... here.
…all here?
Kind regards,
Henry
