| Message ID | 20230113052914.3845596-8-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, On 13/01/2023 05:28, Penny Zheng wrote: > From: Wei Chen <wei.chen@arm.com> > > Only the first 4KB of Xen image will be mapped as identity > (PA == VA). At the moment, Xen guarantees this by having > everything that needs to be used in the identity mapping > in head.S before _end_boot and checking at link time if this > fits in 4KB. > > In previous patch, we have moved the MMU code outside of > head.S. Although we have added .text.header to the new file > to guarantee all identity map code still in the first 4KB. > However, the order of these two files on this 4KB depends > on the build tools. Currently, we use the build tools to > process the order of objs in the Makefile to ensure that > head.S must be at the top. But if you change to another build > tools, it may not be the same result. Right, so this is fixing a bug you introduced in the previous patch. We should really avoid introducing (latent) regression in a series. So please re-order the patches. > > In this patch we introduce .text.idmap to head_mmu.S, and > add this section after .text.header. to ensure code of > head_mmu.S after the code of header.S. > > After this, we will still include some code that does not > belong to identity map before _end_boot. Because we have > moved _end_boot to head_mmu.S. I dislike this approach because you are expecting that only head_mmu.S will be part of .text.idmap. If it is not, everything could blow up again. That said, if you look at staging, you will notice that now _end_boot is defined in the linker script to avoid any issue. > That means all code in head.S > will be included before _end_boot. In this patch, we also > added .text flag in the place of original _end_boot in head.S. > All the code after .text in head.S will not be included in > identity map section. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > v1 -> v2: > 1. New patch. > --- > xen/arch/arm/arm64/head.S | 6 ++++++ > xen/arch/arm/arm64/head_mmu.S | 2 +- > xen/arch/arm/xen.lds.S | 1 + > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 5cfa47279b..782bd1f94c 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -466,6 +466,12 @@ fail: PRINT("- Boot failed -\r\n") > b 1b > ENDPROC(fail) > > +/* > + * For the code that do not need in indentity map section, > + * we put them back to normal .text section > + */ > +.section .text, "ax", %progbits > + I would argue that puts wants to be part of the idmap. > #ifdef CONFIG_EARLY_PRINTK > /* > * Initialize the UART. Should only be called on the boot CPU. > diff --git a/xen/arch/arm/arm64/head_mmu.S b/xen/arch/arm/arm64/head_mmu.S > index e2c8f07140..6ff13c751c 100644 > --- a/xen/arch/arm/arm64/head_mmu.S > +++ b/xen/arch/arm/arm64/head_mmu.S > @@ -105,7 +105,7 @@ > str \tmp2, [\tmp3, \tmp1, lsl #3] > .endm > > -.section .text.header, "ax", %progbits > +.section .text.idmap, "ax", %progbits > /*.aarch64*/ > > /* > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 92c2984052..bc45ea2c65 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -33,6 +33,7 @@ SECTIONS > .text : { > _stext = .; /* Text section */ > *(.text.header) > + *(.text.idmap) > > *(.text.cold) > *(.text.unlikely .text.*_unlikely .text.unlikely.*) Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2023年1月18日 7:46 > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v2 07/40] xen/arm64: add .text.idmap for Xen identity > map sections > > Hi, > > On 13/01/2023 05:28, Penny Zheng wrote: > > From: Wei Chen <wei.chen@arm.com> > > > > Only the first 4KB of Xen image will be mapped as identity > > (PA == VA). At the moment, Xen guarantees this by having > > everything that needs to be used in the identity mapping > > in head.S before _end_boot and checking at link time if this > > fits in 4KB. > > > > In previous patch, we have moved the MMU code outside of > > head.S. Although we have added .text.header to the new file > > to guarantee all identity map code still in the first 4KB. > > However, the order of these two files on this 4KB depends > > on the build tools. Currently, we use the build tools to > > process the order of objs in the Makefile to ensure that > > head.S must be at the top. But if you change to another build > > tools, it may not be the same result. > > Right, so this is fixing a bug you introduced in the previous patch. We > should really avoid introducing (latent) regression in a series. So > please re-order the patches. > Ok. > > > > In this patch we introduce .text.idmap to head_mmu.S, and > > add this section after .text.header. to ensure code of > > head_mmu.S after the code of header.S. > > > > After this, we will still include some code that does not > > belong to identity map before _end_boot. Because we have > > moved _end_boot to head_mmu.S. > > I dislike this approach because you are expecting that only head_mmu.S > will be part of .text.idmap. If it is not, everything could blow up again. > I agree. > That said, if you look at staging, you will notice that now _end_boot is > defined in the linker script to avoid any issue. > Sorry, I am not quite clear about this comment. The _end_boot of original staging branch is defined in head.S. And I am not quite sure how this _end_boot solve multiple files contain idmap code. Cheers, Wei Chen > > That means all code in head.S > > will be included before _end_boot. In this patch, we also > > added .text flag in the place of original _end_boot in head.S. > > All the code after .text in head.S will not be included in > > identity map section. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > v1 -> v2: > > 1. New patch. > > --- > > xen/arch/arm/arm64/head.S | 6 ++++++ > > xen/arch/arm/arm64/head_mmu.S | 2 +- > > xen/arch/arm/xen.lds.S | 1 + > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > > index 5cfa47279b..782bd1f94c 100644 > > --- a/xen/arch/arm/arm64/head.S > > +++ b/xen/arch/arm/arm64/head.S > > @@ -466,6 +466,12 @@ fail: PRINT("- Boot failed -\r\n") > > b 1b > > ENDPROC(fail) > > > > +/* > > + * For the code that do not need in indentity map section, > > + * we put them back to normal .text section > > + */ > > +.section .text, "ax", %progbits > > + > > I would argue that puts wants to be part of the idmap. > I am ok to move puts to idmap. But from the original head.S, puts is placed after _end_boot, and from the xen.ld.S, we can see idmap is area is the section of "_end_boot - start". The reason of moving puts to idmap is because we're using it in idmap? Cheers, Wei Chen > > #ifdef CONFIG_EARLY_PRINTK > > /* > > * Initialize the UART. Should only be called on the boot CPU. > > diff --git a/xen/arch/arm/arm64/head_mmu.S > b/xen/arch/arm/arm64/head_mmu.S > > index e2c8f07140..6ff13c751c 100644 > > --- a/xen/arch/arm/arm64/head_mmu.S > > +++ b/xen/arch/arm/arm64/head_mmu.S > > @@ -105,7 +105,7 @@ > > str \tmp2, [\tmp3, \tmp1, lsl #3] > > .endm > > > > -.section .text.header, "ax", %progbits > > +.section .text.idmap, "ax", %progbits > > /*.aarch64*/ > > > > /* > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > > index 92c2984052..bc45ea2c65 100644 > > --- a/xen/arch/arm/xen.lds.S > > +++ b/xen/arch/arm/xen.lds.S > > @@ -33,6 +33,7 @@ SECTIONS > > .text : { > > _stext = .; /* Text section */ > > *(.text.header) > > + *(.text.idmap) > > > > *(.text.cold) > > *(.text.unlikely .text.*_unlikely .text.unlikely.*) > > Cheers, > > -- > Julien Grall
On 18/01/2023 02:18, Wei Chen wrote: > Hi Julien, Hi Wei, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: 2023年1月18日 7:46 >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH v2 07/40] xen/arm64: add .text.idmap for Xen identity >> map sections >> >> Hi, >> >> On 13/01/2023 05:28, Penny Zheng wrote: >>> From: Wei Chen <wei.chen@arm.com> >>> >>> Only the first 4KB of Xen image will be mapped as identity >>> (PA == VA). At the moment, Xen guarantees this by having >>> everything that needs to be used in the identity mapping >>> in head.S before _end_boot and checking at link time if this >>> fits in 4KB. >>> >>> In previous patch, we have moved the MMU code outside of >>> head.S. Although we have added .text.header to the new file >>> to guarantee all identity map code still in the first 4KB. >>> However, the order of these two files on this 4KB depends >>> on the build tools. Currently, we use the build tools to >>> process the order of objs in the Makefile to ensure that >>> head.S must be at the top. But if you change to another build >>> tools, it may not be the same result. >> >> Right, so this is fixing a bug you introduced in the previous patch. We >> should really avoid introducing (latent) regression in a series. So >> please re-order the patches. >> > > Ok. > >>> >>> In this patch we introduce .text.idmap to head_mmu.S, and >>> add this section after .text.header. to ensure code of >>> head_mmu.S after the code of header.S. >>> >>> After this, we will still include some code that does not >>> belong to identity map before _end_boot. Because we have >>> moved _end_boot to head_mmu.S. >> >> I dislike this approach because you are expecting that only head_mmu.S >> will be part of .text.idmap. If it is not, everything could blow up again. >> > > I agree. > >> That said, if you look at staging, you will notice that now _end_boot is >> defined in the linker script to avoid any issue. >> > > Sorry, I am not quite clear about this comment. The _end_boot of original > staging branch is defined in head.S. And I am not quite sure how this > _end_boot solve multiple files contain idmap code. If you look at the latest staging, there is a commit (229ebd517b9d) that now define _end_boot in the linker script. The .text.idmap section can be added before the definition of _end_boot. > > Cheers, > Wei Chen > >>> That means all code in head.S >>> will be included before _end_boot. In this patch, we also >>> added .text flag in the place of original _end_boot in head.S. >>> All the code after .text in head.S will not be included in >>> identity map section. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> --- >>> v1 -> v2: >>> 1. New patch. >>> --- >>> xen/arch/arm/arm64/head.S | 6 ++++++ >>> xen/arch/arm/arm64/head_mmu.S | 2 +- >>> xen/arch/arm/xen.lds.S | 1 + >>> 3 files changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>> index 5cfa47279b..782bd1f94c 100644 >>> --- a/xen/arch/arm/arm64/head.S >>> +++ b/xen/arch/arm/arm64/head.S >>> @@ -466,6 +466,12 @@ fail: PRINT("- Boot failed -\r\n") >>> b 1b >>> ENDPROC(fail) >>> >>> +/* >>> + * For the code that do not need in indentity map section, >>> + * we put them back to normal .text section >>> + */ >>> +.section .text, "ax", %progbits >>> + >> >> I would argue that puts wants to be part of the idmap. >> > > I am ok to move puts to idmap. But from the original head.S, puts is > placed after _end_boot, and from the xen.ld.S, we can see idmap is > area is the section of "_end_boot - start". The original position of _end_boot is wrong. It didn't take into account the literal pool (there are at the end of the unit). So they would be past _end_boot. > The reason of moving puts > to idmap is because we're using it in idmap? I guess it depends of what idmap really mean here. If you only interpret as the MMU is on and VA == PA. Then not yet (I was thinking to introduce a few calls). If you also include the MMU off. Then yes. Also, in the context of cache coloring, we will need to have a trampoline for cache coloring. So it would be better to keep everything close together as it is easier to copy. Cheers,
Hi Julien, > -----Original Message----- > >>> > >>> In this patch we introduce .text.idmap to head_mmu.S, and > >>> add this section after .text.header. to ensure code of > >>> head_mmu.S after the code of header.S. > >>> > >>> After this, we will still include some code that does not > >>> belong to identity map before _end_boot. Because we have > >>> moved _end_boot to head_mmu.S. > >> > >> I dislike this approach because you are expecting that only head_mmu.S > >> will be part of .text.idmap. If it is not, everything could blow up > again. > >> > > > > I agree. > > > >> That said, if you look at staging, you will notice that now _end_boot > is > >> defined in the linker script to avoid any issue. > >> > > > > Sorry, I am not quite clear about this comment. The _end_boot of > original > > staging branch is defined in head.S. And I am not quite sure how this > > _end_boot solve multiple files contain idmap code. > > If you look at the latest staging, there is a commit (229ebd517b9d) that > now define _end_boot in the linker script. > > The .text.idmap section can be added before the definition of _end_boot. > Oh, my branch was a little old, I have seen this new definition in xen.ld.S after I update the branch. I understand now. > > > > Cheers, > > Wei Chen > > > >>> That means all code in head.S > >>> will be included before _end_boot. In this patch, we also > >>> added .text flag in the place of original _end_boot in head.S. > >>> All the code after .text in head.S will not be included in > >>> identity map section. > >>> > >>> Signed-off-by: Wei Chen <wei.chen@arm.com> > >>> --- > >>> v1 -> v2: > >>> 1. New patch. > >>> --- > >>> xen/arch/arm/arm64/head.S | 6 ++++++ > >>> xen/arch/arm/arm64/head_mmu.S | 2 +- > >>> xen/arch/arm/xen.lds.S | 1 + > >>> 3 files changed, 8 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > >>> index 5cfa47279b..782bd1f94c 100644 > >>> --- a/xen/arch/arm/arm64/head.S > >>> +++ b/xen/arch/arm/arm64/head.S > >>> @@ -466,6 +466,12 @@ fail: PRINT("- Boot failed -\r\n") > >>> b 1b > >>> ENDPROC(fail) > >>> > >>> +/* > >>> + * For the code that do not need in indentity map section, > >>> + * we put them back to normal .text section > >>> + */ > >>> +.section .text, "ax", %progbits > >>> + > >> > >> I would argue that puts wants to be part of the idmap. > >> > > > > I am ok to move puts to idmap. But from the original head.S, puts is > > placed after _end_boot, and from the xen.ld.S, we can see idmap is > > area is the section of "_end_boot - start". > > The original position of _end_boot is wrong. It didn't take into account > the literal pool (there are at the end of the unit). So they would be > past _end_boot. > Ok. > > The reason of moving puts > > to idmap is because we're using it in idmap? > > I guess it depends of what idmap really mean here. If you only interpret > as the MMU is on and VA == PA. Then not yet (I was thinking to introduce > a few calls). > > If you also include the MMU off. Then yes. > > Also, in the context of cache coloring, we will need to have a > trampoline for cache coloring. So it would be better to keep everything > close together as it is easier to copy. > Understand, thanks! Cheers, Wei Chen > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 5cfa47279b..782bd1f94c 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -466,6 +466,12 @@ fail: PRINT("- Boot failed -\r\n") b 1b ENDPROC(fail) +/* + * For the code that do not need in indentity map section, + * we put them back to normal .text section + */ +.section .text, "ax", %progbits + #ifdef CONFIG_EARLY_PRINTK /* * Initialize the UART. Should only be called on the boot CPU. diff --git a/xen/arch/arm/arm64/head_mmu.S b/xen/arch/arm/arm64/head_mmu.S index e2c8f07140..6ff13c751c 100644 --- a/xen/arch/arm/arm64/head_mmu.S +++ b/xen/arch/arm/arm64/head_mmu.S @@ -105,7 +105,7 @@ str \tmp2, [\tmp3, \tmp1, lsl #3] .endm -.section .text.header, "ax", %progbits +.section .text.idmap, "ax", %progbits /*.aarch64*/ /* diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 92c2984052..bc45ea2c65 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -33,6 +33,7 @@ SECTIONS .text : { _stext = .; /* Text section */ *(.text.header) + *(.text.idmap) *(.text.cold) *(.text.unlikely .text.*_unlikely .text.unlikely.*)