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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

I haven't checked that the code movement is just a code movement. For 
now, I am mainly looking at the split.
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.
  * prepare_secondary_mm(): I think we could switch to virt_to_mfn().

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.


[...]

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

--
Julien Grall