Re: [PATCH v6 06/13] xen/arm: Split page table related code to mmu/pt.c

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Sep 26, 2023, at 02:57, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> I haven't checked that the code movement is just a code movement. For now, I 
> am mainly looking at the split.

No problem, thank you. I will rebase v6 and use text comparison tools
to check if the code movement is just code movement before sending v7
to make sure I won’t make the same mistake again. You can double check
the final version. 

> 
> On 28/08/2023 02:32, Henry Wang wrote:
>> The extraction of MMU related code is the basis of MPU support.
>> This commit starts this work by firstly splitting the page table
>> related code to mmu/pt.c, so that we will not end up with again
>> massive mm.c files.
>> Introduce a mmu specific directory and setup the Makefiles for it.
>> Move the page table related functions and macros from arch/arm/mm.c
>> to arch/arm/mmu/pt.c. Expose the previously static global variable
>> "phys_offset".
> 
> I don't much like the idea to expose phys_offset everywhere. Looking at the 
> code there are two users:
>  * pte_of_xenaddr(): I realize you suggested to add it in pt.c and I agreed. 
> However, looking at the split, this function is exclusively used for boot 
> (and you confirmed below). So I think it would be preferable to move it in 
> mmu/setup.c.

Sounds good, I will fix this in v7.

>  * prepare_secondary_mm(): I think we could switch to virt_to_mfn().

Actually I found the third use of phys_offset in setup_pagetables(), but it
looks like the usage is similar as the usage here, so probably these two
are the same case.

Also, please correct me if I am wrong, by suggesting the “switch”, do you
mean using virt_to_mfn() on xen_pgtable to change it to min, then change
the mfn to PA and delete the phys_offset? If so, why not use virt_to_maddr()? 

> 
> I can understand if you don't want to make the second change. So I would at 
> least request to ...
> 
>> While moving, mark pte_of_xenaddr() as __init to make clear that
>> this helper is only intended to be used during early boot.
>> Take the opportunity to fix the in-code comment coding styles when
>> possible, and drop the unnecessary #include headers in the original
>> arch/arm/mm.c.
>> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
>> ---
>> v6:
>> - Rework the original patch "[v5,07/13] xen/arm: Extract MMU-specific
>>   code", only split the page table related code out in this patch.
>> ---
>>  xen/arch/arm/Makefile         |   1 +
>>  xen/arch/arm/include/asm/mm.h |   2 +
> 
> ... declare phys_offset in asm/mmu/mm.h because this variable doesn't make a 
> lot of sense when the MPU is used (it will always be equal to 0).
> 
> The rest of the split looks good to me.

Thanks for confirming, I will see what the above discussion ends and then
do the change accordingly.

> 
> 
> [...]
> 
>> -lpae_t pte_of_xenaddr(vaddr_t va)
>> -{
>> -    paddr_t ma = va + phys_offset;
>> -
>> -    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
>> -}
>> -
> 
> See above. I think this should stay here for now and the be moved to setup.c 
> in a follow-up patch.

Sure.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall