Re: [XEN] xen/arm: arm32: Use adr_l instead of load_paddr for getting address of symbols

[Michal Orzel <michal.orzel@xxxxxxx>]

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