Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate

[Michal Orzel <michal.orzel@xxxxxxx>]

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