Hi Julien, > On Sep 26, 2023, at 02:57, Julien Grall <[email protected]> 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 <[email protected]> >> Signed-off-by: Penny Zheng <[email protected]> >> --- >> 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
