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

From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org, Oleksandr_Tyshchenko@epam.com,
	andrii_anisov@epam.com, andre.przywara@arm.com
Subject: Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT
Date: Wed, 26 Jun 2019 16:28:33 +0100	[thread overview]
Message-ID: <d8c94588-8f7f-84a5-aed1-fe7bfe7bd178@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1906260827080.5851@sstabellini-ThinkPad-T480s>

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,
-- 
Julien Grall

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