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 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@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?
> > > 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,

--
Julien Grall

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