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

[Julien Grall <julien.grall@arm.com>]

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,

-- 
Julien Grall

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