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

[Julien Grall <julien@xxxxxxx>]

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.
This is why I don't particularly like this change and I am afraid, I 
will not ack it.
> 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.
Yes I still have plan to use it.

Cheers,

--
Julien Grall