| Message ID | 20231121094516.24714-4-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm64: head: misc cleanup | expand |
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". Cheers,
> On 21 Nov 2023, at 09:45, Michal Orzel <michal.orzel@amd.com> 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 > 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. > > Take the opportunity to fix a comment with incorrect function name. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- Hi Michal, I’ve also tested on FVP Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
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@xen.org/ ~Michal
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,
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
diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S index 10774f30e40e..41779020eb4d 100644 --- a/xen/arch/arm/arm64/mmu/head.S +++ b/xen/arch/arm/arm64/mmu/head.S @@ -146,10 +146,10 @@ create_page_tables: /* * We need to use a stash register because - * create_table_entry_paddr() will clobber the register storing + * create_table_entry_from_paddr() will clobber the register storing * the physical address of the table to point to. */ - load_paddr x4, boot_third + adr_l x4, boot_third ldr x1, =XEN_VIRT_START .rept XEN_NR_ENTRIES(2) mov x0, x4 /* x0 := paddr(l3 table) */ @@ -311,7 +311,7 @@ ENDPROC(enable_mmu) ENTRY(enable_secondary_cpu_mm) mov x6, lr - load_paddr x0, init_ttbr + adr_l x0, init_ttbr ldr x0, [x0] mov x1, #SCTLR_Axx_ELx_WXN /* Enable WxN from the start */ @@ -336,7 +336,7 @@ ENTRY(enable_boot_cpu_mm) mov x6, lr bl create_page_tables - load_paddr x0, boot_pgtable + adr_l x0, boot_pgtable mov x1, #0 /* No extra SCTLR flags */ bl enable_mmu
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 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. Take the opportunity to fix a comment with incorrect function name. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/arch/arm/arm64/mmu/head.S | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)