| Message ID | 20231121094516.24714-2-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm64: head: misc cleanup | expand |
> On 21 Nov 2023, at 09:45, Michal Orzel <michal.orzel@amd.com> wrote: > > At the moment, the 'hex' string is placed right after the 'putn' > function in the .text section. This is because of the limited range > (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use > 'adr_l' instead (range extended to +/- 4GB) and move the string to > .rodata.str. This way all the earlyprintk messages will be part of .rodata > and the behavior will be consistent with what we already do on arm32. > > 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 Michal, On 21/11/2023 09:45, Michal Orzel wrote: > At the moment, the 'hex' string is placed right after the 'putn' > function in the .text section. This is because of the limited range > (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use > 'adr_l' instead (range extended to +/- 4GB) and move the string to > .rodata.str. This way all the earlyprintk messages will be part of .rodata > and the behavior will be consistent with what we already do on arm32. This will be correct today, but I am actually thinking to move 'hex' to .rodata.idmap so it can be used while we are on the 1:1 mapping and/or temporary mapping. So I would consider to drop this patch for now. Cheers, > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > xen/arch/arm/arm64/head.S | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 8dbd3300d89f..b6111399e766 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -498,7 +498,7 @@ ENDPROC(asm_puts) > * Clobbers x0-x3 > */ > putn: > - adr x1, hex > + adr_l x1, hex > mov x3, #16 > 1: > early_uart_ready x23, 2 > @@ -512,8 +512,7 @@ putn: > ret > ENDPROC(putn) > > -hex: .ascii "0123456789abcdef" > - .align 2 > +RODATA_STR(hex, "0123456789abcdef") > > #else /* CONFIG_EARLY_PRINTK */ > [1] https://lore.kernel.org/20231101233011.83098-3-julien@xen.org
Hi Julien, On 21/11/2023 17:09, Julien Grall wrote: > > > Hi Michal, > > On 21/11/2023 09:45, Michal Orzel wrote: >> At the moment, the 'hex' string is placed right after the 'putn' >> function in the .text section. This is because of the limited range >> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use >> 'adr_l' instead (range extended to +/- 4GB) and move the string to >> .rodata.str. This way all the earlyprintk messages will be part of .rodata >> and the behavior will be consistent with what we already do on arm32. > > This will be correct today, but I am actually thinking to move 'hex' to > .rodata.idmap so it can be used while we are on the 1:1 mapping and/or > temporary mapping. So you are planning on extending your series to also cover arm64? If that is the case, then I'm ok on postponing/dropping this patch. ~Michal
On 21/11/2023 17:00, Michal Orzel wrote: > Hi Julien, Hi, > On 21/11/2023 17:09, Julien Grall wrote: >> >> >> Hi Michal, >> >> On 21/11/2023 09:45, Michal Orzel wrote: >>> At the moment, the 'hex' string is placed right after the 'putn' >>> function in the .text section. This is because of the limited range >>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use >>> 'adr_l' instead (range extended to +/- 4GB) and move the string to >>> .rodata.str. This way all the earlyprintk messages will be part of .rodata >>> and the behavior will be consistent with what we already do on arm32. >> >> This will be correct today, but I am actually thinking to move 'hex' to >> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or >> temporary mapping. > So you are planning on extending your series to also cover arm64? It is not in my soonish plan. But you are arguing that this patch is for consistency with arm32. This will not be after my series. And I would not change arm64 just for consistency because I don't view it as necessary. The boot code is already not the same. Cheers,
On 21/11/2023 18:04, Julien Grall wrote: > > > On 21/11/2023 17:00, Michal Orzel wrote: >> Hi Julien, > > Hi, > >> On 21/11/2023 17:09, Julien Grall wrote: >>> >>> >>> Hi Michal, >>> >>> On 21/11/2023 09:45, Michal Orzel wrote: >>>> At the moment, the 'hex' string is placed right after the 'putn' >>>> function in the .text section. This is because of the limited range >>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use >>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to >>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata >>>> and the behavior will be consistent with what we already do on arm32. >>> >>> This will be correct today, but I am actually thinking to move 'hex' to >>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or >>> temporary mapping. >> So you are planning on extending your series to also cover arm64? > > It is not in my soonish plan. But you are arguing that this patch is for > consistency with arm32. This will not be after my series. Hmm, consistency was not the only reason for sending this patch. It's a beneficial side effect given that putn implementations are very similar on arm32/arm64. Are you saying that moving a string that is const from .text to .rodata is not a good change? > > And I would not change arm64 just for consistency because I don't view > it as necessary. The boot code is already not the same. I don't recall trying to make the entire boot code consistent. But if there are parts that are almost identical, I think symmetry is welcomed. It helps reading the code, at least for me. ~Michal
Hi Michal, On 21/11/2023 17:18, Michal Orzel wrote: > > > On 21/11/2023 18:04, Julien Grall wrote: >> >> >> On 21/11/2023 17:00, Michal Orzel wrote: >>> Hi Julien, >> >> Hi, >> >>> On 21/11/2023 17:09, Julien Grall wrote: >>>> >>>> >>>> Hi Michal, >>>> >>>> On 21/11/2023 09:45, Michal Orzel wrote: >>>>> At the moment, the 'hex' string is placed right after the 'putn' >>>>> function in the .text section. This is because of the limited range >>>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use >>>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to >>>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata >>>>> and the behavior will be consistent with what we already do on arm32. >>>> >>>> This will be correct today, but I am actually thinking to move 'hex' to >>>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or >>>> temporary mapping. >>> So you are planning on extending your series to also cover arm64? >> >> It is not in my soonish plan. But you are arguing that this patch is for >> consistency with arm32. This will not be after my series. > Hmm, consistency was not the only reason for sending this patch. It's a beneficial side effect > given that putn implementations are very similar on arm32/arm64. > Are you saying that > moving a string that is const from .text to .rodata is not a good change? .rodata is better but I would rather prefer if this is moved to .rodata.idmap directly. > >> >> And I would not change arm64 just for consistency because I don't view >> it as necessary. The boot code is already not the same. > I don't recall trying to make the entire boot code consistent. But if there are parts that > are almost identical, I think symmetry is welcomed. It helps reading the code, at least for me. If you want symmetry, then I am happy to switch to .rodata.idmap. But I would rather not first move to .rodata and then in a month time switch to .rodata.idmap. Cheers,
Hi, On 21/11/2023 19:31, Julien Grall wrote: > > > Hi Michal, > > On 21/11/2023 17:18, Michal Orzel wrote: >> >> >> On 21/11/2023 18:04, Julien Grall wrote: >>> >>> >>> On 21/11/2023 17:00, Michal Orzel wrote: >>>> Hi Julien, >>> >>> Hi, >>> >>>> On 21/11/2023 17:09, Julien Grall wrote: >>>>> >>>>> >>>>> Hi Michal, >>>>> >>>>> On 21/11/2023 09:45, Michal Orzel wrote: >>>>>> At the moment, the 'hex' string is placed right after the 'putn' >>>>>> function in the .text section. This is because of the limited range >>>>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use >>>>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to >>>>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata >>>>>> and the behavior will be consistent with what we already do on arm32. >>>>> >>>>> This will be correct today, but I am actually thinking to move 'hex' to >>>>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or >>>>> temporary mapping. >>>> So you are planning on extending your series to also cover arm64? >>> >>> It is not in my soonish plan. But you are arguing that this patch is for >>> consistency with arm32. This will not be after my series. >> Hmm, consistency was not the only reason for sending this patch. It's a beneficial side effect >> given that putn implementations are very similar on arm32/arm64. >> Are you saying that >> moving a string that is const from .text to .rodata is not a good change? > > .rodata is better but I would rather prefer if this is moved to > .rodata.idmap directly. > >> >>> >>> And I would not change arm64 just for consistency because I don't view >>> it as necessary. The boot code is already not the same. >> I don't recall trying to make the entire boot code consistent. But if there are parts that >> are almost identical, I think symmetry is welcomed. It helps reading the code, at least for me. > > If you want symmetry, then I am happy to switch to .rodata.idmap. But I > would rather not first move to .rodata and then in a month time switch > to .rodata.idmap. Ok, fair enough. ~Michal
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 8dbd3300d89f..b6111399e766 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -498,7 +498,7 @@ ENDPROC(asm_puts) * Clobbers x0-x3 */ putn: - adr x1, hex + adr_l x1, hex mov x3, #16 1: early_uart_ready x23, 2 @@ -512,8 +512,7 @@ putn: ret ENDPROC(putn) -hex: .ascii "0123456789abcdef" - .align 2 +RODATA_STR(hex, "0123456789abcdef") #else /* CONFIG_EARLY_PRINTK */
At the moment, the 'hex' string is placed right after the 'putn' function in the .text section. This is because of the limited range (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use 'adr_l' instead (range extended to +/- 4GB) and move the string to .rodata.str. This way all the earlyprintk messages will be part of .rodata and the behavior will be consistent with what we already do on arm32. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/arch/arm/arm64/head.S | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)