| Message ID | 20230626033443.2943270-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
Hi Penny, On 26/06/2023 04:33, Penny Zheng wrote: > CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email. > > > From: Wei Chen <wei.chen@arm.com> > > At the moment, on MMU system, enable_mmu() will return to an > address in the 1:1 mapping, then each path is responsible to > switch to virtual runtime mapping. Then remove_identity_mapping() > is called to remove all 1:1 mapping. > > Since remove_identity_mapping() is not necessary on Non-MMU system, > and we also avoid creating empty function for Non-MMU system, trying > to keep only one codeflow in arm64/head.S, we move path switch and > remove_identity_mapping() in enable_mmu() on MMU system. > > As the remove_identity_mapping should only be called for the boot > CPU only, so we introduce enable_boot_mmu for boot CPU and > enable_runtime_mmu for secondary CPUs in this patch. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > v3: > - new patch > --- > xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++-------- > 1 file changed, 70 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 10a07db428..4dfbe0bc6f 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -314,21 +314,12 @@ real_start_efi: > > bl check_cpu_mode > bl cpu_init > - bl create_page_tables > - load_paddr x0, boot_pgtable > - bl enable_mmu > > /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > - ldr x0, =primary_switched > - br x0 > + ldr lr, =primary_switched > + b enable_boot_mmu > + > primary_switched: > - /* > - * The 1:1 map may clash with other parts of the Xen virtual memory > - * layout. As it is not used anymore, remove it completely to > - * avoid having to worry about replacing existing mapping > - * afterwards. > - */ > - bl remove_identity_mapping > bl setup_fixmap > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -373,13 +364,11 @@ GLOBAL(init_secondary) > #endif > bl check_cpu_mode > bl cpu_init > - load_paddr x0, init_ttbr > - ldr x0, [x0] > - bl enable_mmu > > /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > - ldr x0, =secondary_switched > - br x0 > + ldr lr, =secondary_switched > + b enable_runtime_mmu > + > secondary_switched: > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -694,6 +683,70 @@ enable_mmu: > ret > ENDPROC(enable_mmu) > > +/* > + * Turn on the Data Cache and the MMU. The function will return > + * to the virtual address provided in LR (e.g. the runtime mapping). > + * > + * Inputs: > + * lr : Virtual address to return to. > + * > + * Clobbers x0 - x5 > + */ > +enable_runtime_mmu: > + mov x5, lr > + > + load_paddr x0, init_ttbr > + ldr x0, [x0] > + > + bl enable_mmu > + mov lr, x5 > + > + /* return to secondary_switched */ > + ret > +ENDPROC(enable_runtime_mmu) You are renaming this in 08/52. > + > +/* > + * Turn on the Data Cache and the MMU. The function will return > + * to the virtual address provided in LR (e.g. the runtime mapping). > + * > + * Inputs: > + * lr : Virtual address to return to. > + * > + * Clobbers x0 - x5 > + */ > +enable_boot_mmu: > + mov x5, lr > + > + bl create_page_tables > + load_paddr x0, boot_pgtable > + > + bl enable_mmu > + mov lr, x5 > + > + /* > + * The MMU is turned on and we are in the 1:1 mapping. Switch > + * to the runtime mapping. > + */ > + ldr x0, =1f > + br x0 Where are you switching to ? > +1: > + /* > + * The 1:1 map may clash with other parts of the Xen virtual memory > + * layout. As it is not used anymore, remove it completely to > + * avoid having to worry about replacing existing mapping > + * afterwards. Function will return to primary_switched. > + */ > + b remove_identity_mapping > + > + /* > + * Here might not be reached, as "ret" in remove_identity_mapping > + * will use the return address in LR in advance. But keep ret here > + * might be more safe if "ret" in remove_identity_mapping is removed > + * in future. > + */ > + ret > +ENDPROC(enable_boot_mmu) You are renaming this function in 08/52. May be you should rename and move the fuctions to the correct place, in this patch itself. - Ayan > + > /* > * Remove the 1:1 map from the page-tables. It is not easy to keep track > * where the 1:1 map was mapped, so we will look for the top-level entry > -- > 2.25.1 > >
Hi Penny, On 26/06/2023 04:33, Penny Zheng wrote: > From: Wei Chen <wei.chen@arm.com> > > At the moment, on MMU system, enable_mmu() will return to an > address in the 1:1 mapping, then each path is responsible to > switch to virtual runtime mapping. Then remove_identity_mapping() > is called to remove all 1:1 mapping. The identity mapping is only removed for the boot CPU. You mention it correctly below but here it lead to think it may be called on the secondary CPU. So I would add 'on the boot CPU'. > > Since remove_identity_mapping() is not necessary on Non-MMU system, > and we also avoid creating empty function for Non-MMU system, trying > to keep only one codeflow in arm64/head.S, we move path switch and > remove_identity_mapping() in enable_mmu() on MMU system. > > As the remove_identity_mapping should only be called for the boot > CPU only, so we introduce enable_boot_mmu for boot CPU and > enable_runtime_mmu for secondary CPUs in this patch. NIT: Add () after enable_runtime_mmu to be consistent with the rest of commit message. Same for the title. Also, I would prefer if we name the functions properly from the start. So we don't have to rename them when they are moved in patch 7. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > v3: > - new patch > --- > xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++-------- > 1 file changed, 70 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 10a07db428..4dfbe0bc6f 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -314,21 +314,12 @@ real_start_efi: > > bl check_cpu_mode > bl cpu_init > - bl create_page_tables > - load_paddr x0, boot_pgtable > - bl enable_mmu > > /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ This comment is not accurate anymore given that the MMU is off. > - ldr x0, =primary_switched > - br x0 > + ldr lr, =primary_switched > + b enable_boot_mmu > + > primary_switched: > - /* > - * The 1:1 map may clash with other parts of the Xen virtual memory > - * layout. As it is not used anymore, remove it completely to > - * avoid having to worry about replacing existing mapping > - * afterwards. > - */ > - bl remove_identity_mapping > bl setup_fixmap > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -373,13 +364,11 @@ GLOBAL(init_secondary) > #endif > bl check_cpu_mode > bl cpu_init > - load_paddr x0, init_ttbr > - ldr x0, [x0] > - bl enable_mmu > > /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ > - ldr x0, =secondary_switched > - br x0 > + ldr lr, =secondary_switched > + b enable_runtime_mmu > + > secondary_switched: > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -694,6 +683,70 @@ enable_mmu: > ret > ENDPROC(enable_mmu) > > +/* > + * Turn on the Data Cache and the MMU. The function will return > + * to the virtual address provided in LR (e.g. the runtime mapping). The description here is exactly the same as below. However, there is a different between the two functions. One is to deal with the secondary CPUs whilst the second is for the boot CPUs. > + * > + * Inputs: > + * lr : Virtual address to return to. > + * > + * Clobbers x0 - x5 > + */ > +enable_runtime_mmu: I find "runtime" confusing in this case. How about "enable_secondary_cpu_mm"? > + mov x5, lr > + > + load_paddr x0, init_ttbr > + ldr x0, [x0] > + > + bl enable_mmu > + mov lr, x5 > + > + /* return to secondary_switched */ > + ret > +ENDPROC(enable_runtime_mmu) > + > +/* > + * Turn on the Data Cache and the MMU. The function will return > + * to the virtual address provided in LR (e.g. the runtime mapping). Similar remark as for the comment above. > + * > + * Inputs: > + * lr : Virtual address to return to. > + * > + * Clobbers x0 - x5 > + */ > +enable_boot_mmu: Based on my comment above, I would sugesst to call it "enable_boot_cpu_mm" > + mov x5, lr > + > + bl create_page_tables > + load_paddr x0, boot_pgtable > + > + bl enable_mmu > + mov lr, x5 > + > + /* > + * The MMU is turned on and we are in the 1:1 mapping. Switch > + * to the runtime mapping. > + */ > + ldr x0, =1f > + br x0 > +1: > + /* > + * The 1:1 map may clash with other parts of the Xen virtual memory > + * layout. As it is not used anymore, remove it completely to > + * avoid having to worry about replacing existing mapping > + * afterwards. Function will return to primary_switched. > + */ > + b remove_identity_mapping > + > + /* > + * Here might not be reached, as "ret" in remove_identity_mapping > + * will use the return address in LR in advance. But keep ret here > + * might be more safe if "ret" in remove_identity_mapping is removed > + * in future. > + */ > + ret Given this path is meant to be unreachable, I would prefer if we call "fail". > +ENDPROC(enable_boot_mmu) > + > /* > * Remove the 1:1 map from the page-tables. It is not easy to keep track > * where the 1:1 map was mapped, so we will look for the top-level entry Cheers,
Hi Julien On 2023/7/5 05:24, Julien Grall wrote: > Hi Penny, > > On 26/06/2023 04:33, Penny Zheng wrote: >> From: Wei Chen <wei.chen@arm.com> >> >> At the moment, on MMU system, enable_mmu() will return to an >> address in the 1:1 mapping, then each path is responsible to >> switch to virtual runtime mapping. Then remove_identity_mapping() >> is called to remove all 1:1 mapping. > > The identity mapping is only removed for the boot CPU. You mention it > correctly below but here it lead to think it may be called on the > secondary CPU. So I would add 'on the boot CPU'. > >> >> Since remove_identity_mapping() is not necessary on Non-MMU system, >> and we also avoid creating empty function for Non-MMU system, trying >> to keep only one codeflow in arm64/head.S, we move path switch and >> remove_identity_mapping() in enable_mmu() on MMU system. >> >> As the remove_identity_mapping should only be called for the boot >> CPU only, so we introduce enable_boot_mmu for boot CPU and >> enable_runtime_mmu for secondary CPUs in this patch. > > NIT: Add () after enable_runtime_mmu to be consistent with the rest of > commit message. Same for the title. > sure. > Also, I would prefer if we name the functions properly from the start. > So we don't have to rename them when they are moved in patch 7. > ok. change them to enable_runtime_mm() and enable_boot_mm() at the beginning >> >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> --- >> v3: >> - new patch >> --- >> xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++-------- >> 1 file changed, 70 insertions(+), 17 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 10a07db428..4dfbe0bc6f 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -314,21 +314,12 @@ real_start_efi: >> bl check_cpu_mode >> bl cpu_init >> - bl create_page_tables >> - load_paddr x0, boot_pgtable >> - bl enable_mmu >> /* We are still in the 1:1 mapping. Jump to the runtime >> Virtual Address. */ > > This comment is not accurate anymore given that the MMU is off. > I'll remove. >> - ldr x0, =primary_switched >> - br x0 >> + ldr lr, =primary_switched >> + b enable_boot_mmu >> + >> primary_switched: >> - /* >> - * The 1:1 map may clash with other parts of the Xen virtual >> memory >> - * layout. As it is not used anymore, remove it completely to >> - * avoid having to worry about replacing existing mapping >> - * afterwards. >> - */ >> - bl remove_identity_mapping >> bl setup_fixmap >> #ifdef CONFIG_EARLY_PRINTK >> /* Use a virtual address to access the UART. */ >> @@ -373,13 +364,11 @@ GLOBAL(init_secondary) >> #endif >> bl check_cpu_mode >> bl cpu_init >> - load_paddr x0, init_ttbr >> - ldr x0, [x0] >> - bl enable_mmu >> /* We are still in the 1:1 mapping. Jump to the runtime >> Virtual Address. */ Note: Remove this too. >> - ldr x0, =secondary_switched >> - br x0 >> + ldr lr, =secondary_switched >> + b enable_runtime_mmu >> + >> secondary_switched: >> #ifdef CONFIG_EARLY_PRINTK >> /* Use a virtual address to access the UART. */ >> @@ -694,6 +683,70 @@ enable_mmu: >> ret >> ENDPROC(enable_mmu) >> +/* >> + * Turn on the Data Cache and the MMU. The function will return >> + * to the virtual address provided in LR (e.g. the runtime mapping). > > The description here is exactly the same as below. However, there is a > different between the two functions. One is to deal with the secondary > CPUs whilst the second is for the boot CPUs. > I'll add-on: This function deals with the secondary CPUs. >> + * >> + * Inputs: >> + * lr : Virtual address to return to. >> + * >> + * Clobbers x0 - x5 >> + */ >> +enable_runtime_mmu: > > I find "runtime" confusing in this case. How about > "enable_secondary_cpu_mm"? > Sure, will do. >> + mov x5, lr >> + >> + load_paddr x0, init_ttbr >> + ldr x0, [x0] >> + >> + bl enable_mmu >> + mov lr, x5 >> + >> + /* return to secondary_switched */ >> + ret >> +ENDPROC(enable_runtime_mmu) >> + >> +/* >> + * Turn on the Data Cache and the MMU. The function will return >> + * to the virtual address provided in LR (e.g. the runtime mapping). > > Similar remark as for the comment above. I'll add-on: This function deals with the boot CPUs. > >> + * >> + * Inputs: >> + * lr : Virtual address to return to. >> + * >> + * Clobbers x0 - x5 >> + */ >> +enable_boot_mmu: > > Based on my comment above, I would sugesst to call it "enable_boot_cpu_mm" sure, > >> + mov x5, lr >> + >> + bl create_page_tables >> + load_paddr x0, boot_pgtable >> + >> + bl enable_mmu >> + mov lr, x5 >> + >> + /* >> + * The MMU is turned on and we are in the 1:1 mapping. Switch >> + * to the runtime mapping. >> + */ >> + ldr x0, =1f >> + br x0 >> +1: >> + /* >> + * The 1:1 map may clash with other parts of the Xen virtual >> memory >> + * layout. As it is not used anymore, remove it completely to >> + * avoid having to worry about replacing existing mapping >> + * afterwards. Function will return to primary_switched. >> + */ >> + b remove_identity_mapping >> + >> + /* >> + * Here might not be reached, as "ret" in >> remove_identity_mapping >> + * will use the return address in LR in advance. But keep ret >> here >> + * might be more safe if "ret" in remove_identity_mapping is >> removed >> + * in future. >> + */ >> + ret > > Given this path is meant to be unreachable, I would prefer if we call > "fail". sure, > >> +ENDPROC(enable_boot_mmu) >> + >> /* >> * Remove the 1:1 map from the page-tables. It is not easy to keep >> track >> * where the 1:1 map was mapped, so we will look for the top-level >> entry > > Cheers, >
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 10a07db428..4dfbe0bc6f 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -314,21 +314,12 @@ real_start_efi: bl check_cpu_mode bl cpu_init - bl create_page_tables - load_paddr x0, boot_pgtable - bl enable_mmu /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ - ldr x0, =primary_switched - br x0 + ldr lr, =primary_switched + b enable_boot_mmu + primary_switched: - /* - * The 1:1 map may clash with other parts of the Xen virtual memory - * layout. As it is not used anymore, remove it completely to - * avoid having to worry about replacing existing mapping - * afterwards. - */ - bl remove_identity_mapping bl setup_fixmap #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ @@ -373,13 +364,11 @@ GLOBAL(init_secondary) #endif bl check_cpu_mode bl cpu_init - load_paddr x0, init_ttbr - ldr x0, [x0] - bl enable_mmu /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */ - ldr x0, =secondary_switched - br x0 + ldr lr, =secondary_switched + b enable_runtime_mmu + secondary_switched: #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ @@ -694,6 +683,70 @@ enable_mmu: ret ENDPROC(enable_mmu) +/* + * Turn on the Data Cache and the MMU. The function will return + * to the virtual address provided in LR (e.g. the runtime mapping). + * + * Inputs: + * lr : Virtual address to return to. + * + * Clobbers x0 - x5 + */ +enable_runtime_mmu: + mov x5, lr + + load_paddr x0, init_ttbr + ldr x0, [x0] + + bl enable_mmu + mov lr, x5 + + /* return to secondary_switched */ + ret +ENDPROC(enable_runtime_mmu) + +/* + * Turn on the Data Cache and the MMU. The function will return + * to the virtual address provided in LR (e.g. the runtime mapping). + * + * Inputs: + * lr : Virtual address to return to. + * + * Clobbers x0 - x5 + */ +enable_boot_mmu: + mov x5, lr + + bl create_page_tables + load_paddr x0, boot_pgtable + + bl enable_mmu + mov lr, x5 + + /* + * The MMU is turned on and we are in the 1:1 mapping. Switch + * to the runtime mapping. + */ + ldr x0, =1f + br x0 +1: + /* + * The 1:1 map may clash with other parts of the Xen virtual memory + * layout. As it is not used anymore, remove it completely to + * avoid having to worry about replacing existing mapping + * afterwards. Function will return to primary_switched. + */ + b remove_identity_mapping + + /* + * Here might not be reached, as "ret" in remove_identity_mapping + * will use the return address in LR in advance. But keep ret here + * might be more safe if "ret" in remove_identity_mapping is removed + * in future. + */ + ret +ENDPROC(enable_boot_mmu) + /* * Remove the 1:1 map from the page-tables. It is not easy to keep track * where the 1:1 map was mapped, so we will look for the top-level entry