| Message ID | 20190610193215.23704-3-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm64: Rework head.S to make it more compliant with the Arm Arm | expand |
On Mon, 10 Jun 2019, Julien Grall wrote: >> The current implementation of the macro PRINT will clobber x30/lr. This > means the user should save lr if it cares about it. By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer expression. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > Follow-up patches will introduce more use of PRINT in place where lr > should be preserved. Rather than requiring all the users to preserve lr, > the macro PRINT is modified to save and restore it. > > While the comment state x3 will be clobbered, this is not the case. So > PRINT will use x3 to preserve lr. > > Lastly, take the opportunity to move the comment on top of PRINT and use > PRINT in init_uart. Both changes will be helpful in a follow-up patch. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/arm64/head.S | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index c8bbdf05a6..a5147c8d80 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -78,12 +78,17 @@ > * x30 - lr > */ > > -/* Macro to print a string to the UART, if there is one. > - * Clobbers x0-x3. */ > #ifdef CONFIG_EARLY_PRINTK > +/* > + * Macro to print a string to the UART, if there is one. > + * > + * Clobbers x0 - x3 > + */ > #define PRINT(_s) \ > + mov x3, lr ; \ > adr x0, 98f ; \ > bl puts ; \ > + mov lr, x3 ; \ > RODATA_STR(98, _s) > #else /* CONFIG_EARLY_PRINTK */ > #define PRINT(s) > @@ -630,9 +635,8 @@ init_uart: > #ifdef EARLY_PRINTK_INIT_UART > early_uart_init x23, 0 > #endif > - adr x0, 1f > - b puts > -RODATA_STR(1, "- UART enabled -\r\n") > + PRINT("- UART enabled -\r\n") > + ret > > /* Print early debug messages. > * x0: Nul-terminated string to print. > -- > 2.11.0 >
On Tue, 25 Jun 2019, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: > >> The current implementation of the macro PRINT will clobber x30/lr. This > > means the user should save lr if it cares about it. > > By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer > expression. No of course not! You meant x30 which is a synonym of lr! It is just that in this case it is also supposed to clobber x0-x3 -- I got confused! The commit message is also fine as is then. More below. > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > > Follow-up patches will introduce more use of PRINT in place where lr > > should be preserved. Rather than requiring all the users to preserve lr, > > the macro PRINT is modified to save and restore it. > > > > While the comment state x3 will be clobbered, this is not the case. So > > PRINT will use x3 to preserve lr. > > > > Lastly, take the opportunity to move the comment on top of PRINT and use > > PRINT in init_uart. Both changes will be helpful in a follow-up patch. > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > xen/arch/arm/arm64/head.S | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > > index c8bbdf05a6..a5147c8d80 100644 > > --- a/xen/arch/arm/arm64/head.S > > +++ b/xen/arch/arm/arm64/head.S > > @@ -78,12 +78,17 @@ > > * x30 - lr > > */ > > > > -/* Macro to print a string to the UART, if there is one. > > - * Clobbers x0-x3. */ > > #ifdef CONFIG_EARLY_PRINTK > > +/* > > + * Macro to print a string to the UART, if there is one. > > + * > > + * Clobbers x0 - x3 > > + */ > > #define PRINT(_s) \ > > + mov x3, lr ; \ > > adr x0, 98f ; \ > > bl puts ; \ > > + mov lr, x3 ; \ > > RODATA_STR(98, _s) Strangely enough I get a build error with gcc 7.3.1, but if I use x30 instead of lr, it builds fine. Have you seen this before? The error is: arm64/head.S: Assembler messages: arm64/head.S:272: Error: operand 1 must be an integer register -- `mov lr,x3' [...] arm64/head.S:272: Error: undefined symbol lr used as an immediate value [...]
Hi Stefano, On 26/06/2019 00:59, Stefano Stabellini wrote: > On Tue, 25 Jun 2019, Stefano Stabellini wrote: >> On Mon, 10 Jun 2019, Julien Grall wrote: >>>> The current implementation of the macro PRINT will clobber x30/lr. This >>> means the user should save lr if it cares about it. >> >> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer >> expression. > > No of course not! You meant x30 which is a synonym of lr! It is just > that in this case it is also supposed to clobber x0-x3 -- I got > confused! The commit message is also fine as is then. More below. > > >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >> >>> Follow-up patches will introduce more use of PRINT in place where lr >>> should be preserved. Rather than requiring all the users to preserve lr, >>> the macro PRINT is modified to save and restore it. >>> >>> While the comment state x3 will be clobbered, this is not the case. So >>> PRINT will use x3 to preserve lr. >>> >>> Lastly, take the opportunity to move the comment on top of PRINT and use >>> PRINT in init_uart. Both changes will be helpful in a follow-up patch. >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> --- >>> xen/arch/arm/arm64/head.S | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>> index c8bbdf05a6..a5147c8d80 100644 >>> --- a/xen/arch/arm/arm64/head.S >>> +++ b/xen/arch/arm/arm64/head.S >>> @@ -78,12 +78,17 @@ >>> * x30 - lr >>> */ >>> >>> -/* Macro to print a string to the UART, if there is one. >>> - * Clobbers x0-x3. */ >>> #ifdef CONFIG_EARLY_PRINTK >>> +/* >>> + * Macro to print a string to the UART, if there is one. >>> + * >>> + * Clobbers x0 - x3 >>> + */ >>> #define PRINT(_s) \ >>> + mov x3, lr ; \ >>> adr x0, 98f ; \ >>> bl puts ; \ >>> + mov lr, x3 ; \ >>> RODATA_STR(98, _s) > > Strangely enough I get a build error with gcc 7.3.1, but if I use x30 > instead of lr, it builds fine. Have you seen this before? Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is not all the assembler is able to understand "lr". In the file entry.S we have the following line: lr .req x30 // link register Could you give a try to add the line in head.S? > The error is: > > arm64/head.S: Assembler messages: > arm64/head.S:272: Error: operand 1 must be an integer register -- `mov lr,x3' > [...] > arm64/head.S:272: Error: undefined symbol lr used as an immediate value > [...] > Cheers,
On Wed, 26 Jun 2019, Julien Grall wrote: > Hi Stefano, > > On 26/06/2019 00:59, Stefano Stabellini wrote: > > On Tue, 25 Jun 2019, Stefano Stabellini wrote: > > > On Mon, 10 Jun 2019, Julien Grall wrote: > > > > > The current implementation of the macro PRINT will clobber x30/lr. > > > > > This > > > > means the user should save lr if it cares about it. > > > > > > By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer > > > expression. > > > > No of course not! You meant x30 which is a synonym of lr! It is just > > that in this case it is also supposed to clobber x0-x3 -- I got > > confused! The commit message is also fine as is then. More below. > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > > > > > > > > Follow-up patches will introduce more use of PRINT in place where lr > > > > should be preserved. Rather than requiring all the users to preserve lr, > > > > the macro PRINT is modified to save and restore it. > > > > > > > > While the comment state x3 will be clobbered, this is not the case. So > > > > PRINT will use x3 to preserve lr. > > > > > > > > Lastly, take the opportunity to move the comment on top of PRINT and use > > > > PRINT in init_uart. Both changes will be helpful in a follow-up patch. > > > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > --- > > > > xen/arch/arm/arm64/head.S | 14 +++++++++----- > > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > > > > index c8bbdf05a6..a5147c8d80 100644 > > > > --- a/xen/arch/arm/arm64/head.S > > > > +++ b/xen/arch/arm/arm64/head.S > > > > @@ -78,12 +78,17 @@ > > > > * x30 - lr > > > > */ > > > > -/* Macro to print a string to the UART, if there is one. > > > > - * Clobbers x0-x3. */ > > > > #ifdef CONFIG_EARLY_PRINTK > > > > +/* > > > > + * Macro to print a string to the UART, if there is one. > > > > + * > > > > + * Clobbers x0 - x3 > > > > + */ > > > > #define PRINT(_s) \ > > > > + mov x3, lr ; \ > > > > adr x0, 98f ; \ > > > > bl puts ; \ > > > > + mov lr, x3 ; \ > > > > RODATA_STR(98, _s) > > > > Strangely enough I get a build error with gcc 7.3.1, but if I use x30 > > instead of lr, it builds fine. Have you seen this before? > > Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is not > all the assembler is able to understand "lr". > > In the file entry.S we have the following line: > > lr .req x30 // link register > > > Could you give a try to add the line in head.S? That works
Hi Stefano, On 26/06/2019 16:27, Stefano Stabellini wrote: > On Wed, 26 Jun 2019, Julien Grall wrote: >> Hi Stefano, >> >> On 26/06/2019 00:59, Stefano Stabellini wrote: >>> On Tue, 25 Jun 2019, Stefano Stabellini wrote: >>>> On Mon, 10 Jun 2019, Julien Grall wrote: >>>>>> The current implementation of the macro PRINT will clobber x30/lr. >>>>>> This >>>>> means the user should save lr if it cares about it. >>>> >>>> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer >>>> expression. >>> >>> No of course not! You meant x30 which is a synonym of lr! It is just >>> that in this case it is also supposed to clobber x0-x3 -- I got >>> confused! The commit message is also fine as is then. More below. >>> >>> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>> >>>> >>>>> Follow-up patches will introduce more use of PRINT in place where lr >>>>> should be preserved. Rather than requiring all the users to preserve lr, >>>>> the macro PRINT is modified to save and restore it. >>>>> >>>>> While the comment state x3 will be clobbered, this is not the case. So >>>>> PRINT will use x3 to preserve lr. >>>>> >>>>> Lastly, take the opportunity to move the comment on top of PRINT and use >>>>> PRINT in init_uart. Both changes will be helpful in a follow-up patch. >>>>> >>>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>>> --- >>>>> xen/arch/arm/arm64/head.S | 14 +++++++++----- >>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>>>> index c8bbdf05a6..a5147c8d80 100644 >>>>> --- a/xen/arch/arm/arm64/head.S >>>>> +++ b/xen/arch/arm/arm64/head.S >>>>> @@ -78,12 +78,17 @@ >>>>> * x30 - lr >>>>> */ >>>>> -/* Macro to print a string to the UART, if there is one. >>>>> - * Clobbers x0-x3. */ >>>>> #ifdef CONFIG_EARLY_PRINTK >>>>> +/* >>>>> + * Macro to print a string to the UART, if there is one. >>>>> + * >>>>> + * Clobbers x0 - x3 >>>>> + */ >>>>> #define PRINT(_s) \ >>>>> + mov x3, lr ; \ >>>>> adr x0, 98f ; \ >>>>> bl puts ; \ >>>>> + mov lr, x3 ; \ >>>>> RODATA_STR(98, _s) >>> >>> Strangely enough I get a build error with gcc 7.3.1, but if I use x30 >>> instead of lr, it builds fine. Have you seen this before? >> >> Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is not >> all the assembler is able to understand "lr". >> >> In the file entry.S we have the following line: >> >> lr .req x30 // link register >> >> >> Could you give a try to add the line in head.S? > > That works Thank you. I thought a bit more during the day and decided to use "x30" directly rather than lr. We can decide to revisit it if we think the code is too difficult to read. Cheers,
On Wed, 26 Jun 2019, Julien Grall wrote: > Hi Stefano, > > On 26/06/2019 16:27, Stefano Stabellini wrote: > > On Wed, 26 Jun 2019, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 26/06/2019 00:59, Stefano Stabellini wrote: > > > > On Tue, 25 Jun 2019, Stefano Stabellini wrote: > > > > > On Mon, 10 Jun 2019, Julien Grall wrote: > > > > > > > The current implementation of the macro PRINT will clobber > > > > > > > x30/lr. > > > > > > > This > > > > > > means the user should save lr if it cares about it. > > > > > > > > > > By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer > > > > > expression. > > > > > > > > No of course not! You meant x30 which is a synonym of lr! It is just > > > > that in this case it is also supposed to clobber x0-x3 -- I got > > > > confused! The commit message is also fine as is then. More below. > > > > > > > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > > > > > > > > > > > > > > Follow-up patches will introduce more use of PRINT in place where lr > > > > > > should be preserved. Rather than requiring all the users to preserve > > > > > > lr, > > > > > > the macro PRINT is modified to save and restore it. > > > > > > > > > > > > While the comment state x3 will be clobbered, this is not the case. > > > > > > So > > > > > > PRINT will use x3 to preserve lr. > > > > > > > > > > > > Lastly, take the opportunity to move the comment on top of PRINT and > > > > > > use > > > > > > PRINT in init_uart. Both changes will be helpful in a follow-up > > > > > > patch. > > > > > > > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > > --- > > > > > > xen/arch/arm/arm64/head.S | 14 +++++++++----- > > > > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > > > > > > index c8bbdf05a6..a5147c8d80 100644 > > > > > > --- a/xen/arch/arm/arm64/head.S > > > > > > +++ b/xen/arch/arm/arm64/head.S > > > > > > @@ -78,12 +78,17 @@ > > > > > > * x30 - lr > > > > > > */ > > > > > > -/* Macro to print a string to the UART, if there is one. > > > > > > - * Clobbers x0-x3. */ > > > > > > #ifdef CONFIG_EARLY_PRINTK > > > > > > +/* > > > > > > + * Macro to print a string to the UART, if there is one. > > > > > > + * > > > > > > + * Clobbers x0 - x3 > > > > > > + */ > > > > > > #define PRINT(_s) \ > > > > > > + mov x3, lr ; \ > > > > > > adr x0, 98f ; \ > > > > > > bl puts ; \ > > > > > > + mov lr, x3 ; \ > > > > > > RODATA_STR(98, _s) > > > > > > > > Strangely enough I get a build error with gcc 7.3.1, but if I use x30 > > > > instead of lr, it builds fine. Have you seen this before? > > > > > > Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is > > > not > > > all the assembler is able to understand "lr". > > > > > > In the file entry.S we have the following line: > > > > > > lr .req x30 // link register > > > > > > > > > Could you give a try to add the line in head.S? > > > > That works > > Thank you. > > I thought a bit more during the day and decided to use "x30" directly rather > than lr. We can decide to revisit it if we think the code is too difficult to > read. I was going to suggest x30 too yesterday, but if we can make `lr' work then that would be my preference because it makes it more immediately obvious what the code is doing.
Hi Stefano, On 6/26/19 7:32 PM, Stefano Stabellini wrote: > On Wed, 26 Jun 2019, Julien Grall wrote: >> Hi Stefano, >> >> On 26/06/2019 16:27, Stefano Stabellini wrote: >>> On Wed, 26 Jun 2019, Julien Grall wrote: >>>> Hi Stefano, >>>> >>>> On 26/06/2019 00:59, Stefano Stabellini wrote: >>>>> On Tue, 25 Jun 2019, Stefano Stabellini wrote: >>>>>> On Mon, 10 Jun 2019, Julien Grall wrote: >>>>>>>> The current implementation of the macro PRINT will clobber >>>>>>>> x30/lr. >>>>>>>> This >>>>>>> means the user should save lr if it cares about it. >>>>>> >>>>>> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer >>>>>> expression. >>>>> >>>>> No of course not! You meant x30 which is a synonym of lr! It is just >>>>> that in this case it is also supposed to clobber x0-x3 -- I got >>>>> confused! The commit message is also fine as is then. More below. >>>>> >>>>> >>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>>>> >>>>>> >>>>>>> Follow-up patches will introduce more use of PRINT in place where lr >>>>>>> should be preserved. Rather than requiring all the users to preserve >>>>>>> lr, >>>>>>> the macro PRINT is modified to save and restore it. >>>>>>> >>>>>>> While the comment state x3 will be clobbered, this is not the case. >>>>>>> So >>>>>>> PRINT will use x3 to preserve lr. >>>>>>> >>>>>>> Lastly, take the opportunity to move the comment on top of PRINT and >>>>>>> use >>>>>>> PRINT in init_uart. Both changes will be helpful in a follow-up >>>>>>> patch. >>>>>>> >>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>>>>> --- >>>>>>> xen/arch/arm/arm64/head.S | 14 +++++++++----- >>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>>>>>> index c8bbdf05a6..a5147c8d80 100644 >>>>>>> --- a/xen/arch/arm/arm64/head.S >>>>>>> +++ b/xen/arch/arm/arm64/head.S >>>>>>> @@ -78,12 +78,17 @@ >>>>>>> * x30 - lr >>>>>>> */ >>>>>>> -/* Macro to print a string to the UART, if there is one. >>>>>>> - * Clobbers x0-x3. */ >>>>>>> #ifdef CONFIG_EARLY_PRINTK >>>>>>> +/* >>>>>>> + * Macro to print a string to the UART, if there is one. >>>>>>> + * >>>>>>> + * Clobbers x0 - x3 >>>>>>> + */ >>>>>>> #define PRINT(_s) \ >>>>>>> + mov x3, lr ; \ >>>>>>> adr x0, 98f ; \ >>>>>>> bl puts ; \ >>>>>>> + mov lr, x3 ; \ >>>>>>> RODATA_STR(98, _s) >>>>> >>>>> Strangely enough I get a build error with gcc 7.3.1, but if I use x30 >>>>> instead of lr, it builds fine. Have you seen this before? >>>> >>>> Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is >>>> not >>>> all the assembler is able to understand "lr". >>>> >>>> In the file entry.S we have the following line: >>>> >>>> lr .req x30 // link register >>>> >>>> >>>> Could you give a try to add the line in head.S? >>> >>> That works >> >> Thank you. >> >> I thought a bit more during the day and decided to use "x30" directly rather >> than lr. We can decide to revisit it if we think the code is too difficult to >> read. > > I was going to suggest x30 too yesterday, but if we can make `lr' work > then that would be my preference because it makes it more immediately > obvious what the code is doing. I will have a look to move the line in an header. Cheers,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index c8bbdf05a6..a5147c8d80 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -78,12 +78,17 @@ * x30 - lr */ -/* Macro to print a string to the UART, if there is one. - * Clobbers x0-x3. */ #ifdef CONFIG_EARLY_PRINTK +/* + * Macro to print a string to the UART, if there is one. + * + * Clobbers x0 - x3 + */ #define PRINT(_s) \ + mov x3, lr ; \ adr x0, 98f ; \ bl puts ; \ + mov lr, x3 ; \ RODATA_STR(98, _s) #else /* CONFIG_EARLY_PRINTK */ #define PRINT(s) @@ -630,9 +635,8 @@ init_uart: #ifdef EARLY_PRINTK_INIT_UART early_uart_init x23, 0 #endif - adr x0, 1f - b puts -RODATA_STR(1, "- UART enabled -\r\n") + PRINT("- UART enabled -\r\n") + ret /* Print early debug messages. * x0: Nul-terminated string to print.
The current implementation of the macro PRINT will clobber x30/lr. This means the user should save lr if it cares about it. Follow-up patches will introduce more use of PRINT in place where lr should be preserved. Rather than requiring all the users to preserve lr, the macro PRINT is modified to save and restore it. While the comment state x3 will be clobbered, this is not the case. So PRINT will use x3 to preserve lr. Lastly, take the opportunity to move the comment on top of PRINT and use PRINT in init_uart. Both changes will be helpful in a follow-up patch. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/arm64/head.S | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)