Hi Julien, On 26/10/2023 10:40, Julien Grall wrote: > > > Hi, > > On 25/10/2023 18:59, Michal Orzel wrote: >> Hi Ayan, >> >> On 25/10/2023 19:03, Ayan Kumar Halder wrote: >>> Before the MMU is turned on, the address returned for any symbol will >>> always be >>> physical address. Thus, one can use adr_l instead of load_paddr. >>> >>> create_table_entry() now takes an extra argument to denote if it is called >>> after >>> the mmu is turned on or not. If it is called with mmu on, then it uses >>> load_paddr to get the physical address of the page table symbol. >> I wonder if we need this extra complexity. > > +1. We used to request the caller to tell whether the MMU is on. But > this made the code more complex. So I decided to remove it completely. > >> Can we just remove load_paddr macro completely (I have a plan to do this for >> arm64 mmu head.S) >> and open code it in create_table_entry? I don't think there is any benefit in >> having the if/else there to use either load_paddr or adr_l. This function >> will also go >> into arm32 mmu head.S. > > While I was ok (I am not 100% happy) with open-coding load_paddr in > arm64, I would rather not do it on Arm32 because I still haven't figured > out whether I would need other use (which could not be replaced with > adr_l) when fixing the secondary CPU boot code (it is still not > compliant with the Arm Arm). > > So the question here is what do we gain by removing load_paddr > completely? We still need a register allocate for the offset, and the > macro makes it clearer what's the code is about. I agree that it might not be super beneficial. My opinion was based on the fact that why bother having a macro if it is only used in one place and consists of 2 instructions only. That said, I'm fully ok to keep the macro if it improves readability and the only use would be in create_table_entry. Then, on an arm32 head.S split, the macro together with function would moved to mmu specific head.S
~Michal
