| Message ID | 20231025170304.2331922-1-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN] xen/arm: arm32: Use adr_l instead of load_paddr for getting address of symbols | expand |
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. 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. ~Michal
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. Cheers,
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
Hi, On 25/10/2023 18: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. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Refer https://lists.archive.carbon60.com/xen/devel/682900 for details. > > xen/arch/arm/arm32/head.S | 35 ++++++++++++++++++++--------------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 33b038e7e0..bf442b0434 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -171,7 +171,7 @@ past_zImage: > > /* Using the DTB in the .dtb section? */ > .ifnes CONFIG_DTB_FILE,"" > - load_paddr r8, _sdtb > + adr_l r8, _sdtb > .endif > > /* Initialize the UART if earlyprintk has been enabled. */ > @@ -213,7 +213,7 @@ GLOBAL(init_secondary) > mrc CP32(r1, MPIDR) > bic r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */ > > - load_paddr r0, smp_up_cpu > + adr_l r0, smp_up_cpu > dsb > 2: ldr r1, [r0] > cmp r1, r7 > @@ -406,6 +406,7 @@ ENDPROC(cpu_init) > * tbl: table symbol to point to > * virt: virtual address > * lvl: page-table level > + * mmu_on: is mmu on > * > * Preserves \virt > * Clobbers r1 - r4 > @@ -414,10 +415,14 @@ ENDPROC(cpu_init) > * > * Note that \virt should be in a register other than r1 - r4 > */ > -.macro create_table_entry, ptbl, tbl, virt, lvl > +.macro create_table_entry, ptbl, tbl, virt, lvl, mmu_on > + .if \mmu_on == 1 > load_paddr r4, \tbl > - create_table_entry_from_paddr \ptbl, r4, \virt, \lvl > - .endm > + .else > + adr_l r4, \tbl > + .endif > + create_table_entry_from_paddr \ptbl, r4, \virt, \lvl > +.endm > > /* > * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd > @@ -479,7 +484,7 @@ create_page_tables: > * create_table_entry_paddr() will clobber the register storing > * the physical address of the table to point to. > */ > - load_paddr r5, boot_third > + adr_l r5, boot_third > mov_w r4, XEN_VIRT_START > .rept XEN_NR_ENTRIES(2) > mov r0, r5 /* r0 := paddr(l3 table) */ > @@ -522,8 +527,8 @@ create_page_tables: > * Setup the 1:1 mapping so we can turn the MMU on. Note that > * only the first page of Xen will be part of the 1:1 mapping. > */ > - create_table_entry boot_pgtable, boot_second_id, r9, 1 > - create_table_entry boot_second_id, boot_third_id, r9, 2 > + create_table_entry boot_pgtable, boot_second_id, r9, 1, 0 > + create_table_entry boot_second_id, boot_third_id, r9, 2, 0 > create_mapping_entry boot_third_id, r9, r9 > > /* > @@ -537,7 +542,7 @@ create_page_tables: > bne use_temporary_mapping > > mov_w r0, XEN_VIRT_START > - create_table_entry boot_pgtable, boot_second, r0, 1 > + create_table_entry boot_pgtable, boot_second, r0, 1, 0 > mov r12, #0 /* r12 := temporary mapping not created */ > mov pc, lr > > @@ -551,7 +556,7 @@ use_temporary_mapping: > > /* Map boot_second (cover Xen mappings) to the temporary 1st slot */ > mov_w r0, TEMPORARY_XEN_VIRT_START > - create_table_entry boot_pgtable, boot_second, r0, 1 > + create_table_entry boot_pgtable, boot_second, r0, 1, 0 > > mov r12, #1 /* r12 := temporary mapping created */ > mov pc, lr > @@ -578,7 +583,7 @@ enable_mmu: > flush_xen_tlb_local r0 > > /* Write Xen's PT's paddr into the HTTBR */ > - load_paddr r0, boot_pgtable > + adr_l r0, boot_pgtable > mov r1, #0 /* r0:r1 is paddr (boot_pagetable) */ > mcrr CP64(r0, r1, HTTBR) > isb > @@ -658,7 +663,7 @@ switch_to_runtime_mapping: > > /* Map boot_second into boot_pgtable */ > mov_w r0, XEN_VIRT_START > - create_table_entry boot_pgtable, boot_second, r0, 1 > + create_table_entry boot_pgtable, boot_second, r0, 1, 1 > > /* Ensure any page table updates are visible before continuing */ > dsb nsh > @@ -739,7 +744,7 @@ setup_fixmap: > #endif > /* Map fixmap into boot_second */ > mov_w r0, FIXMAP_ADDR(0) > - create_table_entry boot_second, xen_fixmap, r0, 2 > + create_table_entry boot_second, xen_fixmap, r0, 2, 0 > /* Ensure any page table updates made above have occurred. */ > dsb nshst > /* > @@ -897,8 +902,8 @@ ENTRY(lookup_processor_type) > */ > __lookup_processor_type: > mrc CP32(r0, MIDR) /* r0 := our cpu id */ > - load_paddr r1, __proc_info_start > - load_paddr r2, __proc_info_end > + adr_l r1, __proc_info_start > + adr_l r2, __proc_info_end On top of what's Michal already mentioned, you can remove the line in lookup_process_type() which set r10. It was only present so load_paddr() do nothing when called from the C world. It is a good hack to remove :). Cheers,
On 26/10/2023 10:40, Julien Grall wrote: > Hi, Hi Julien/Michal, > > On 25/10/2023 18: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. >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> Refer https://lists.archive.carbon60.com/xen/devel/682900 for details. >> >> xen/arch/arm/arm32/head.S | 35 ++++++++++++++++++++--------------- >> 1 file changed, 20 insertions(+), 15 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index 33b038e7e0..bf442b0434 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -171,7 +171,7 @@ past_zImage: >> /* Using the DTB in the .dtb section? */ >> .ifnes CONFIG_DTB_FILE,"" >> - load_paddr r8, _sdtb >> + adr_l r8, _sdtb >> .endif >> /* Initialize the UART if earlyprintk has been enabled. */ >> @@ -213,7 +213,7 @@ GLOBAL(init_secondary) >> mrc CP32(r1, MPIDR) >> bic r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get >> CPU ID */ >> - load_paddr r0, smp_up_cpu >> + adr_l r0, smp_up_cpu >> dsb >> 2: ldr r1, [r0] >> cmp r1, r7 >> @@ -406,6 +406,7 @@ ENDPROC(cpu_init) >> * tbl: table symbol to point to >> * virt: virtual address >> * lvl: page-table level >> + * mmu_on: is mmu on >> * >> * Preserves \virt >> * Clobbers r1 - r4 >> @@ -414,10 +415,14 @@ ENDPROC(cpu_init) >> * >> * Note that \virt should be in a register other than r1 - r4 >> */ >> -.macro create_table_entry, ptbl, tbl, virt, lvl >> +.macro create_table_entry, ptbl, tbl, virt, lvl, mmu_on >> + .if \mmu_on == 1 >> load_paddr r4, \tbl >> - create_table_entry_from_paddr \ptbl, r4, \virt, \lvl >> - .endm >> + .else >> + adr_l r4, \tbl >> + .endif >> + create_table_entry_from_paddr \ptbl, r4, \virt, \lvl >> +.endm >> /* >> * Macro to create a mapping entry in \tbl to \paddr. Only mapping >> in 3rd >> @@ -479,7 +484,7 @@ create_page_tables: >> * create_table_entry_paddr() will clobber the register >> storing >> * the physical address of the table to point to. >> */ >> - load_paddr r5, boot_third >> + adr_l r5, boot_third >> mov_w r4, XEN_VIRT_START >> .rept XEN_NR_ENTRIES(2) >> mov r0, r5 /* r0 := paddr(l3 >> table) */ >> @@ -522,8 +527,8 @@ create_page_tables: >> * Setup the 1:1 mapping so we can turn the MMU on. Note that >> * only the first page of Xen will be part of the 1:1 mapping. >> */ >> - create_table_entry boot_pgtable, boot_second_id, r9, 1 >> - create_table_entry boot_second_id, boot_third_id, r9, 2 >> + create_table_entry boot_pgtable, boot_second_id, r9, 1, 0 >> + create_table_entry boot_second_id, boot_third_id, r9, 2, 0 >> create_mapping_entry boot_third_id, r9, r9 >> /* >> @@ -537,7 +542,7 @@ create_page_tables: >> bne use_temporary_mapping >> mov_w r0, XEN_VIRT_START >> - create_table_entry boot_pgtable, boot_second, r0, 1 >> + create_table_entry boot_pgtable, boot_second, r0, 1, 0 >> mov r12, #0 /* r12 := temporary mapping >> not created */ >> mov pc, lr >> @@ -551,7 +556,7 @@ use_temporary_mapping: >> /* Map boot_second (cover Xen mappings) to the temporary >> 1st slot */ >> mov_w r0, TEMPORARY_XEN_VIRT_START >> - create_table_entry boot_pgtable, boot_second, r0, 1 >> + create_table_entry boot_pgtable, boot_second, r0, 1, 0 >> mov r12, #1 /* r12 := temporary mapping >> created */ >> mov pc, lr >> @@ -578,7 +583,7 @@ enable_mmu: >> flush_xen_tlb_local r0 >> /* Write Xen's PT's paddr into the HTTBR */ >> - load_paddr r0, boot_pgtable >> + adr_l r0, boot_pgtable >> mov r1, #0 /* r0:r1 is paddr >> (boot_pagetable) */ >> mcrr CP64(r0, r1, HTTBR) >> isb >> @@ -658,7 +663,7 @@ switch_to_runtime_mapping: >> /* Map boot_second into boot_pgtable */ >> mov_w r0, XEN_VIRT_START >> - create_table_entry boot_pgtable, boot_second, r0, 1 >> + create_table_entry boot_pgtable, boot_second, r0, 1, 1 >> /* Ensure any page table updates are visible before >> continuing */ >> dsb nsh >> @@ -739,7 +744,7 @@ setup_fixmap: >> #endif >> /* Map fixmap into boot_second */ >> mov_w r0, FIXMAP_ADDR(0) >> - create_table_entry boot_second, xen_fixmap, r0, 2 >> + create_table_entry boot_second, xen_fixmap, r0, 2, 0 >> /* Ensure any page table updates made above have occurred. */ >> dsb nshst >> /* >> @@ -897,8 +902,8 @@ ENTRY(lookup_processor_type) >> */ >> __lookup_processor_type: >> mrc CP32(r0, MIDR) /* r0 := our cpu id */ >> - load_paddr r1, __proc_info_start >> - load_paddr r2, __proc_info_end >> + adr_l r1, __proc_info_start >> + adr_l r2, __proc_info_end > > On top of what's Michal already mentioned, you can remove the line in > lookup_process_type() which set r10. It was only present so > load_paddr() do nothing when called from the C world. > > It is a good hack to remove :). I have addressed this and the other comments. I have sent out "[XEN v2] xen/arm: arm32: Use adr_l instead of load_paddr for getting address of symbols". - Ayan > > Cheers, >
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 33b038e7e0..bf442b0434 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -171,7 +171,7 @@ past_zImage: /* Using the DTB in the .dtb section? */ .ifnes CONFIG_DTB_FILE,"" - load_paddr r8, _sdtb + adr_l r8, _sdtb .endif /* Initialize the UART if earlyprintk has been enabled. */ @@ -213,7 +213,7 @@ GLOBAL(init_secondary) mrc CP32(r1, MPIDR) bic r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */ - load_paddr r0, smp_up_cpu + adr_l r0, smp_up_cpu dsb 2: ldr r1, [r0] cmp r1, r7 @@ -406,6 +406,7 @@ ENDPROC(cpu_init) * tbl: table symbol to point to * virt: virtual address * lvl: page-table level + * mmu_on: is mmu on * * Preserves \virt * Clobbers r1 - r4 @@ -414,10 +415,14 @@ ENDPROC(cpu_init) * * Note that \virt should be in a register other than r1 - r4 */ -.macro create_table_entry, ptbl, tbl, virt, lvl +.macro create_table_entry, ptbl, tbl, virt, lvl, mmu_on + .if \mmu_on == 1 load_paddr r4, \tbl - create_table_entry_from_paddr \ptbl, r4, \virt, \lvl - .endm + .else + adr_l r4, \tbl + .endif + create_table_entry_from_paddr \ptbl, r4, \virt, \lvl +.endm /* * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd @@ -479,7 +484,7 @@ create_page_tables: * create_table_entry_paddr() will clobber the register storing * the physical address of the table to point to. */ - load_paddr r5, boot_third + adr_l r5, boot_third mov_w r4, XEN_VIRT_START .rept XEN_NR_ENTRIES(2) mov r0, r5 /* r0 := paddr(l3 table) */ @@ -522,8 +527,8 @@ create_page_tables: * Setup the 1:1 mapping so we can turn the MMU on. Note that * only the first page of Xen will be part of the 1:1 mapping. */ - create_table_entry boot_pgtable, boot_second_id, r9, 1 - create_table_entry boot_second_id, boot_third_id, r9, 2 + create_table_entry boot_pgtable, boot_second_id, r9, 1, 0 + create_table_entry boot_second_id, boot_third_id, r9, 2, 0 create_mapping_entry boot_third_id, r9, r9 /* @@ -537,7 +542,7 @@ create_page_tables: bne use_temporary_mapping mov_w r0, XEN_VIRT_START - create_table_entry boot_pgtable, boot_second, r0, 1 + create_table_entry boot_pgtable, boot_second, r0, 1, 0 mov r12, #0 /* r12 := temporary mapping not created */ mov pc, lr @@ -551,7 +556,7 @@ use_temporary_mapping: /* Map boot_second (cover Xen mappings) to the temporary 1st slot */ mov_w r0, TEMPORARY_XEN_VIRT_START - create_table_entry boot_pgtable, boot_second, r0, 1 + create_table_entry boot_pgtable, boot_second, r0, 1, 0 mov r12, #1 /* r12 := temporary mapping created */ mov pc, lr @@ -578,7 +583,7 @@ enable_mmu: flush_xen_tlb_local r0 /* Write Xen's PT's paddr into the HTTBR */ - load_paddr r0, boot_pgtable + adr_l r0, boot_pgtable mov r1, #0 /* r0:r1 is paddr (boot_pagetable) */ mcrr CP64(r0, r1, HTTBR) isb @@ -658,7 +663,7 @@ switch_to_runtime_mapping: /* Map boot_second into boot_pgtable */ mov_w r0, XEN_VIRT_START - create_table_entry boot_pgtable, boot_second, r0, 1 + create_table_entry boot_pgtable, boot_second, r0, 1, 1 /* Ensure any page table updates are visible before continuing */ dsb nsh @@ -739,7 +744,7 @@ setup_fixmap: #endif /* Map fixmap into boot_second */ mov_w r0, FIXMAP_ADDR(0) - create_table_entry boot_second, xen_fixmap, r0, 2 + create_table_entry boot_second, xen_fixmap, r0, 2, 0 /* Ensure any page table updates made above have occurred. */ dsb nshst /* @@ -897,8 +902,8 @@ ENTRY(lookup_processor_type) */ __lookup_processor_type: mrc CP32(r0, MIDR) /* r0 := our cpu id */ - load_paddr r1, __proc_info_start - load_paddr r2, __proc_info_end + adr_l r1, __proc_info_start + adr_l r2, __proc_info_end 1: ldr r3, [r1, #PROCINFO_cpu_mask] and r4, r0, r3 /* r4 := our cpu id with mask */ ldr r3, [r1, #PROCINFO_cpu_val] /* r3 := cpu val in current proc info */
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. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Refer https://lists.archive.carbon60.com/xen/devel/682900 for details. xen/arch/arm/arm32/head.S | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)