| Message ID | 20230801034419.2047541-2-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Split MMU code as the prepration of MPU work | expand |
Hi Henry, On 01/08/2023 04:44, Henry Wang 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 on the boot CPU 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_cpu_mm() for boot CPU and > enable_secondary_cpu_mm() for secondary CPUs in this patch. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> With two comments Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> Tested-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > v4: > - Clarify remove_identity_mapping() is called on boot CPU and keep > the function/proc format consistent in commit msg. > - Drop inaccurate (due to the refactor) in-code comment. > - Rename enable_{boot,runtime}_mmu to enable_{boot,secondary}_cpu_mm. > - Reword the in-code comment on top of enable_{boot,secondary}_cpu_mm. > - Call "fail" for unreachable code. > v3: > - new patch > --- > xen/arch/arm/arm64/head.S | 89 ++++++++++++++++++++++++++++++--------- > 1 file changed, 70 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 31cdb54d74..2af9f974d5 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -313,21 +313,11 @@ 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_cpu_mm > + > 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. */ > @@ -372,13 +362,10 @@ 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_secondary_cpu_mm > + > secondary_switched: > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -737,6 +724,70 @@ enable_mmu: > ret > ENDPROC(enable_mmu) > > +/* > + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. > + * 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_secondary_cpu_mm: I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And ... > + mov x5, lr > + > + load_paddr x0, init_ttbr > + ldr x0, [x0] > + > + bl enable_mmu > + mov lr, x5 > + > + /* return to secondary_switched */ > + ret > +ENDPROC(enable_secondary_cpu_mm) > + > +/* > + * Enable mm (turn on the data cache and the MMU) for the boot CPU. > + * 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_cpu_mm: prefer "enable_boot_cpu_mmu" as it is MMU specific as well. - Ayan > + 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 > + > + /* > + * Below is supposed to be unreachable code, as "ret" in > + * remove_identity_mapping will use the return address in LR in advance. > + */ > + b fail > +ENDPROC(enable_boot_cpu_mm) > + > /* > * 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 Ayan, > -----Original Message----- > Hi Henry, > > > 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 on the boot CPU 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_cpu_mm() for boot CPU and > > enable_secondary_cpu_mm() for secondary CPUs in this patch. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > With two comments > > Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > > Tested-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> Thanks, and... > > > +/* > > + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. > > + * 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_secondary_cpu_mm: > I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And ... ...actually this as well as... > > +/* > > + * Enable mm (turn on the data cache and the MMU) for the boot CPU. > > + * 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_cpu_mm: > > prefer "enable_boot_cpu_mmu" as it is MMU specific as well. ...this, are the name suggested by Julien in [1], so probably I will stick to these names, unless he would prefer the proposed names. I would personally prefer the names that Julien suggested too, because from the comments above these two functions, these functions not only enable the MMU, but also turn on the d-cache, hence a more generic name (using "mm"), is more appropriate here I guess. [1] https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb-9ee619934ffb@xen.org/ Kind regards, Henry > > - Ayan
On 07/08/2023 12:35, Henry Wang wrote: > Hi Ayan, > >> -----Original Message----- >> Hi Henry, >> >>> 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 on the boot CPU 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_cpu_mm() for boot CPU and >>> enable_secondary_cpu_mm() for secondary CPUs in this patch. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> With two comments >> >> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> >> Tested-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > Thanks, and... > >>> +/* >>> + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. >>> + * 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_secondary_cpu_mm: >> I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And ... > ...actually this as well as... > >>> +/* >>> + * Enable mm (turn on the data cache and the MMU) for the boot CPU. >>> + * 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_cpu_mm: >> prefer "enable_boot_cpu_mmu" as it is MMU specific as well. > ...this, are the name suggested by Julien in [1], so probably I will stick > to these names, unless he would prefer the proposed names. I would > personally prefer the names that Julien suggested too, because from > the comments above these two functions, these functions not only > enable the MMU, but also turn on the d-cache, hence a more generic > name (using "mm"), is more appropriate here I guess. > > [1] https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb-9ee619934ffb@xen.org/ This is fine. My concern is minor. If this file is going to contain MPU specific logic as well in future, then suffixing a *_mmu might help to distinguish the two. - Ayan > > Kind regards, > Henry > >> - Ayan
On 07/08/2023 12:35, Henry Wang wrote: > Hi Ayan, Hi Henry, >> -----Original Message----- >> Hi Henry, >> >>> 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 on the boot CPU 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_cpu_mm() for boot CPU and >>> enable_secondary_cpu_mm() for secondary CPUs in this patch. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> >> With two comments >> >> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> >> Tested-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > > Thanks, and... > >> >>> +/* >>> + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. >>> + * 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_secondary_cpu_mm: >> I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And ... > > ...actually this as well as... > >>> +/* >>> + * Enable mm (turn on the data cache and the MMU) for the boot CPU. >>> + * 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_cpu_mm: >> >> prefer "enable_boot_cpu_mmu" as it is MMU specific as well. > > ...this, are the name suggested by Julien in [1], so probably I will stick > to these names, unless he would prefer the proposed names. I would > personally prefer the names that Julien suggested too, because from > the comments above these two functions, these functions not only > enable the MMU, but also turn on the d-cache, hence a more generic > name (using "mm"), is more appropriate here I guess. I have suggested those name because the two functions are meant to abstract the implementation between MPU and MMU (see [2] for the MPU). If we prefix them with *_mmu now, they will have to be renamed later on and will just introduce unnecessary churn. > > [1] https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb-9ee619934ffb@xen.org/ [2] https://gitlab.com/xen-project/people/henryw/xen/-/blob/mpu_v4/xen/arch/arm/arm64/mpu/head.S?ref_type=heads#L205
On 07/08/2023 12:43, Ayan Kumar Halder wrote: > > On 07/08/2023 12:35, Henry Wang wrote: >> Hi Ayan, >> >>> -----Original Message----- >>> Hi Henry, >>> >>>> 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 on the boot CPU 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_cpu_mm() for boot CPU and >>>> enable_secondary_cpu_mm() for secondary CPUs in this patch. >>>> >>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>> With two comments >>> >>> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> >>> Tested-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> Thanks, and... >> >>>> +/* >>>> + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. >>>> + * 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_secondary_cpu_mm: >>> I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And ... >> ...actually this as well as... >> >>>> +/* >>>> + * Enable mm (turn on the data cache and the MMU) for the boot CPU. >>>> + * 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_cpu_mm: >>> prefer "enable_boot_cpu_mmu" as it is MMU specific as well. >> ...this, are the name suggested by Julien in [1], so probably I will >> stick >> to these names, unless he would prefer the proposed names. I would >> personally prefer the names that Julien suggested too, because from >> the comments above these two functions, these functions not only >> enable the MMU, but also turn on the d-cache, hence a more generic >> name (using "mm"), is more appropriate here I guess. >> >> [1] >> https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb-9ee619934ffb@xen.org/ > > This is fine. My concern is minor. > > If this file is going to contain MPU specific logic as well in future, > then suffixing a *_mmu might help to distinguish the two. For this series, it is quite important to look at the end result. In this case, the MMU logic will be moved in its own file. enable_boot_cpu_mm() and enable_second_cpu_mm() will be implemented differently between the MMU and MPU to avoid #ifdeferay in head.S. Cheers,
Hi Julien, > -----Original Message----- > Subject: Re: [PATCH v4 01/13] xen/arm64: head.S: Introduce > enable_{boot,secondary}_cpu_mm > >> prefer "enable_boot_cpu_mmu" as it is MMU specific as well. > > > > ...this, are the name suggested by Julien in [1], so probably I will stick > > to these names, unless he would prefer the proposed names. I would > > personally prefer the names that Julien suggested too, because from > > the comments above these two functions, these functions not only > > enable the MMU, but also turn on the d-cache, hence a more generic > > name (using "mm"), is more appropriate here I guess. > > I have suggested those name because the two functions are meant to > abstract the implementation between MPU and MMU (see [2] for the MPU). > > If we prefix them with *_mmu now, they will have to be renamed later on > and will just introduce unnecessary churn. Exactly, I fully agree with you. If we can do all the renaming in one shot to fit both MMU and MPU, we should definitely go for it. Kind regards, Henry > > > > > [1] https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb- > 9ee619934ffb@xen.org/ > > [2] > https://gitlab.com/xen-project/people/henryw/xen/- > /blob/mpu_v4/xen/arch/arm/arm64/mpu/head.S?ref_type=heads#L205 > > -- > Julien Grall
Hi Julien/Henry, Thanks for the explanation. On 07/08/2023 12:47, Julien Grall wrote: > > > On 07/08/2023 12:43, Ayan Kumar Halder wrote: >> >> On 07/08/2023 12:35, Henry Wang wrote: >>> Hi Ayan, >>> >>>> -----Original Message----- >>>> Hi Henry, >>>> >>>>> 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 on the boot CPU 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_cpu_mm() for boot CPU and >>>>> enable_secondary_cpu_mm() for secondary CPUs in this patch. >>>>> >>>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>>> With two comments >>>> >>>> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>> >>>> Tested-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> Thanks, and... >>> >>>>> +/* >>>>> + * Enable mm (turn on the data cache and the MMU) for secondary >>>>> CPUs. >>>>> + * 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_secondary_cpu_mm: >>>> I will prefer "enable_secondary_cpu_mmu" as it is MMU specific. And >>>> ... >>> ...actually this as well as... >>> >>>>> +/* >>>>> + * Enable mm (turn on the data cache and the MMU) for the boot CPU. >>>>> + * 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_cpu_mm: >>>> prefer "enable_boot_cpu_mmu" as it is MMU specific as well. >>> ...this, are the name suggested by Julien in [1], so probably I will >>> stick >>> to these names, unless he would prefer the proposed names. I would >>> personally prefer the names that Julien suggested too, because from >>> the comments above these two functions, these functions not only >>> enable the MMU, but also turn on the d-cache, hence a more generic >>> name (using "mm"), is more appropriate here I guess. >>> >>> [1] >>> https://lore.kernel.org/xen-devel/c10bc254-ad79-dada-d5fb-9ee619934ffb@xen.org/ >> >> This is fine. My concern is minor. >> >> If this file is going to contain MPU specific logic as well in >> future, then suffixing a *_mmu might help to distinguish the two. > > For this series, it is quite important to look at the end result. In > this case, the MMU logic will be moved in its own file. > enable_boot_cpu_mm() and enable_second_cpu_mm() will be implemented > differently between the MMU and MPU to avoid #ifdeferay in head.S. Makes sense. I am happy with it. - Ayan > > Cheers, >
Hi Henry, Title: NIT: Add () after _mm to stay consistent with the rest. On 01/08/2023 04:44, Henry Wang 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 on the boot CPU 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_cpu_mm() for boot CPU and > enable_secondary_cpu_mm() for secondary CPUs in this patch. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > v4: > - Clarify remove_identity_mapping() is called on boot CPU and keep > the function/proc format consistent in commit msg. > - Drop inaccurate (due to the refactor) in-code comment. > - Rename enable_{boot,runtime}_mmu to enable_{boot,secondary}_cpu_mm. > - Reword the in-code comment on top of enable_{boot,secondary}_cpu_mm. > - Call "fail" for unreachable code. > v3: > - new patch > --- > xen/arch/arm/arm64/head.S | 89 ++++++++++++++++++++++++++++++--------- > 1 file changed, 70 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 31cdb54d74..2af9f974d5 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -313,21 +313,11 @@ 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_cpu_mm > + > 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. */ > @@ -372,13 +362,10 @@ 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_secondary_cpu_mm > + > secondary_switched: > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > @@ -737,6 +724,70 @@ enable_mmu: > ret > ENDPROC(enable_mmu) > > +/* > + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. > + * 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_secondary_cpu_mm: > + mov x5, lr > + > + load_paddr x0, init_ttbr > + ldr x0, [x0] > + > + bl enable_mmu > + mov lr, x5 > + > + /* return to secondary_switched */ Technically, you will return to the virtual address set in 'lr'. This is 'secondary_switched' today but this could change. So it would be better to have a more generic comment like: Return to the virtual address requested by the caller. > + ret > +ENDPROC(enable_secondary_cpu_mm) > + > +/* > + * Enable mm (turn on the data cache and the MMU) for the boot CPU. > + * 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_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. Same remark here. > + */ > + b remove_identity_mapping > + > + /* > + * Below is supposed to be unreachable code, as "ret" in > + * remove_identity_mapping will use the return address in LR in advance. > + */ > + b fail Looking at this again, I am not entirely sure how this could reached if remove_identity_mapping use 'ret' and you call it with 'b'. So I would suggest to drop it and move 'mov lr, x5' closer to 'b remove_identity_mapping'. So it is clearer that it will return. > +ENDPROC(enable_boot_cpu_mm) > + > /* > * 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 Aug 9, 2023, at 19:59, Julien Grall <julien@xen.org> wrote: > > Hi Henry, > > Title: NIT: Add () after _mm to stay consistent with the rest. Yes sure, I will add “()” in v5. > > On 01/08/2023 04:44, Henry Wang wrote: >> From: Wei Chen <wei.chen@arm.com> >> >> +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 */ > > Technically, you will return to the virtual address set in 'lr'. This is 'secondary_switched' today but this could change. > > So it would be better to have a more generic comment like: > > Return to the virtual address requested by the caller. Sure, and... > >> + ret >> +ENDPROC(enable_secondary_cpu_mm) >> + >> >> +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. > > Same remark here. …same here. > >> + */ >> + b remove_identity_mapping >> + >> + /* >> + * Below is supposed to be unreachable code, as "ret" in >> + * remove_identity_mapping will use the return address in LR in advance. >> + */ >> + b fail > > Looking at this again, I am not entirely sure how this could reached if remove_identity_mapping use 'ret' and you call it with 'b'. So I would suggest to drop it and move 'mov lr, x5' closer to 'b remove_identity_mapping'. So it is clearer that it will return. Ok, I’ve addressed this locally and tested the change, Xen and Dom0 boot fine with the changes that you suggested. Will send v5 soon after fixing all your comments. Thanks! Kind regards, Henry > >> +ENDPROC(enable_boot_cpu_mm) >> + >> /* >> * 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, > > -- > Julien Grall
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 31cdb54d74..2af9f974d5 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -313,21 +313,11 @@ 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_cpu_mm + 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. */ @@ -372,13 +362,10 @@ 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_secondary_cpu_mm + secondary_switched: #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ @@ -737,6 +724,70 @@ enable_mmu: ret ENDPROC(enable_mmu) +/* + * Enable mm (turn on the data cache and the MMU) for secondary CPUs. + * 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_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_secondary_cpu_mm) + +/* + * Enable mm (turn on the data cache and the MMU) for the boot CPU. + * 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_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 + + /* + * Below is supposed to be unreachable code, as "ret" in + * remove_identity_mapping will use the return address in LR in advance. + */ + b fail +ENDPROC(enable_boot_cpu_mm) + /* * 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