Hi, On 21/11/2023 19:43, Julien Grall wrote: > > > Hi, > > On 21/11/2023 18:13, Michal Orzel wrote: >> On 21/11/2023 17:30, Julien Grall wrote: >>> >>> >>> Hi Michal, >>> >>> On 21/11/2023 09:45, Michal Orzel wrote: >>>> Macros load_paddr and adr_l are equivalent when used before the MMU is >>>> enabled, resulting in obtaining physical address of a symbol. The former >>>> requires to know the physical offset (PA - VA) and can be used both before >>>> and after the MMU is enabled. In the spirit of using something only when >>>> truly necessary, replace all instances of load_paddr with adr_l, except >>> >>> I don't buy this argument. The advantage with using "load_paddr" is that >>> it is pretty clear what you get from the call. With "adr_l" you will >>> need to check whether the MMU is on or off. >>> >>>> in create_table_entry macro. Even though there is currently no use of >>>> load_paddr after MMU is enabled, this macro used to be call in such a >>>> context and we can't rule out that it won't happen again. >>>> >>>> This way, the logic behind using load_paddr/adr_l is consistent between >>>> arm32 and arm64, making it easier for developers to determine which one >>>> to use and when. >>> >>> Not really. See above. But there is also no documentation stating that >>> "load_paddr" should not be used before the MMU is on. And as I said >>> above, I find it easier to work with compare to "adr_l". >> I guess it is a matter of taste. load_paddr requires adding a physical >> offset to > > I agree this is a matter of taste. > >> calculate an address, where in fact we have no places in the code where this >> is truly needed. > > I added adr_l only recently (2019). Before hand, we were using > open-coded adrp and load_paddr (which was introduced in 2017). > >> Seeing an instance of this macro makes me think that this piece of code runs >> with MMU enabled. > > Fair enough. But how do you know when 'adr_l' effectively returns a > physical address or virtual address? You could go through the functions > call but that's fairly cumbersome. I see your point but we already use adr_l in MMU code. Also, recently we accepted a patch from Ayan for arm32 that does exactly the same - load_paddr is used only in one place in MMU head.S where it is required (I thought we are aligned on this subject + I shared my plan some weeks ago). Ayan's change together with my patch would make it obvious that we use load_paddr only in MMU enabled context. That is why I struggle to understand why nacking this patch if you let the other one go. IMO this can create confusion for a future developer \wrt which one to use.
~Michal
