Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
> >
> >
> > > 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@xxxxxxx>
> > > ---
> > >   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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel