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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

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

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