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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

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
calculate an address, where in fact we have no places in the code where this is 
truly needed.
Seeing an instance of this macro makes me think that this piece of code runs 
with MMU enabled.
We could in fact remove it completely and the only reason I did not is because 
you told me [1] that
one day we might want to use it.

[1] 
https://lore.kernel.org/xen-devel/2b10267a-514c-4c9b-b715-f65c26d7f757@xxxxxxx/

~Michal