[PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack

[PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack

[Andrew Cooper]

When resyncing the stack, the asm statement does not identify %rsi, %rdi and
%rcx as clobbered by the 'rep movsq'.

Luckily, there are no functional problems in the generated code.  GCC decides
not to save any of them before calling boostrap_map(), which clobbers them.

Correct the clobbers, by listing them as earlyclobber discarded outputs.

Reported-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---

I have done an audit of the other uses of `rep $STRINGOP` and this is the only
asm statement with incorrect clobbers.
---
 xen/arch/x86/setup.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8c8b91f..f470c4a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -836,6 +836,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             l2_pgentry_t *pl2e;
             uint64_t load_start;
             int i, j, k;
+            long _discard;
 
             /* Select relocation address. */
             e = end - reloc_size;
@@ -902,11 +903,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                 "movq %%cr4,%%rsi ; "
                 "andb $0x7f,%%sil ; "
                 "movq %%rsi,%%cr4 ; " /* CR4.PGE == 0 */
-                "movq %0,%%cr3    ; " /* CR3 == new pagetables */
+                "movq %6,%%cr3    ; " /* CR3 == new pagetables */
                 "orb $0x80,%%sil  ; "
                 "movq %%rsi,%%cr4   " /* CR4.PGE == 1 */
-                : : "r" (__pa(idle_pg_table)), "S" (cpu0_stack),
-                "D" (__va(__pa(cpu0_stack))), "c" (STACK_SIZE / 8) : "memory" );
+                : "=&S"(_discard), "=&D"(_discard), "=&c"(_discard)
+                : "0"(cpu0_stack), "1"(__va(__pa(cpu0_stack))),
+                  "2"(STACK_SIZE / 8), "r"(__pa(idle_pg_table))
+                : "memory" );
 
             bootstrap_map(NULL);
         }
-- 
1.7.10.4

Re: [PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack

[Daniel Kiper]

On Mon, Oct 20, 2014 at 06:30:07PM +0100, Andrew Cooper wrote:
> When resyncing the stack, the asm statement does not identify %rsi, %rdi and
> %rcx as clobbered by the 'rep movsq'.
>
> Luckily, there are no functional problems in the generated code.  GCC decides
> not to save any of them before calling boostrap_map(), which clobbers them.
>
> Correct the clobbers, by listing them as earlyclobber discarded outputs.
>
> Reported-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
>
> ---
>
> I have done an audit of the other uses of `rep $STRINGOP` and this is the only
> asm statement with incorrect clobbers.
> ---
>  xen/arch/x86/setup.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack

[Jan Beulich]

>>> On 20.10.14 at 19:30, <andrew.cooper3@citrix.com> wrote:
> When resyncing the stack, the asm statement does not identify %rsi, %rdi and
> %rcx as clobbered by the 'rep movsq'.
> 
> Luckily, there are no functional problems in the generated code.  GCC 
> decides
> not to save any of them before calling boostrap_map(), which clobbers them.
> 
> Correct the clobbers, by listing them as earlyclobber discarded outputs.
> 
> Reported-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> 
> ---
> 
> I have done an audit of the other uses of `rep $STRINGOP` and this is the 
> only
> asm statement with incorrect clobbers.
> ---
>  xen/arch/x86/setup.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 8c8b91f..f470c4a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -836,6 +836,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>              l2_pgentry_t *pl2e;
>              uint64_t load_start;
>              int i, j, k;
> +            long _discard;

I can't see why you couldn't use i, j, or k for the discarding purposes.

> @@ -902,11 +903,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                  "movq %%cr4,%%rsi ; "
>                  "andb $0x7f,%%sil ; "
>                  "movq %%rsi,%%cr4 ; " /* CR4.PGE == 0 */
> -                "movq %0,%%cr3    ; " /* CR3 == new pagetables */
> +                "movq %6,%%cr3    ; " /* CR3 == new pagetables */
>                  "orb $0x80,%%sil  ; "
>                  "movq %%rsi,%%cr4   " /* CR4.PGE == 1 */
> -                : : "r" (__pa(idle_pg_table)), "S" (cpu0_stack),
> -                "D" (__va(__pa(cpu0_stack))), "c" (STACK_SIZE / 8) : "memory" );
> +                : "=&S"(_discard), "=&D"(_discard), "=&c"(_discard)
> +                : "0"(cpu0_stack), "1"(__va(__pa(cpu0_stack))),
> +                  "2"(STACK_SIZE / 8), "r"(__pa(idle_pg_table))
> +                : "memory" );

Among the inputs, please put the one actively used by number first,
or convert to labeled operands. Also please don't corrupt the
formatting - there ought to be a blank between the closing constraint
quote and the opening parenthesis in all operands.

Jan

Re: [PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack

[Andrew Cooper]

On 21/10/14 09:44, Jan Beulich wrote:
>>>> On 20.10.14 at 19:30, <andrew.cooper3@citrix.com> wrote:
>> When resyncing the stack, the asm statement does not identify %rsi, %rdi and
>> %rcx as clobbered by the 'rep movsq'.
>>
>> Luckily, there are no functional problems in the generated code.  GCC 
>> decides
>> not to save any of them before calling boostrap_map(), which clobbers them.
>>
>> Correct the clobbers, by listing them as earlyclobber discarded outputs.
>>
>> Reported-by: Daniel Kiper <daniel.kiper@oracle.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>>
>> ---
>>
>> I have done an audit of the other uses of `rep $STRINGOP` and this is the 
>> only
>> asm statement with incorrect clobbers.
>> ---
>>  xen/arch/x86/setup.c |    9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 8c8b91f..f470c4a 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -836,6 +836,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>              l2_pgentry_t *pl2e;
>>              uint64_t load_start;
>>              int i, j, k;
>> +            long _discard;
> I can't see why you couldn't use i, j, or k for the discarding purposes.

A parameter with the name "_discard" is far more explicit about its
purpose when read as part of the output parameter list.

I could switch to i and leave a comment by the parameters if you insist.

>
>> @@ -902,11 +903,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>                  "movq %%cr4,%%rsi ; "
>>                  "andb $0x7f,%%sil ; "
>>                  "movq %%rsi,%%cr4 ; " /* CR4.PGE == 0 */
>> -                "movq %0,%%cr3    ; " /* CR3 == new pagetables */
>> +                "movq %6,%%cr3    ; " /* CR3 == new pagetables */
>>                  "orb $0x80,%%sil  ; "
>>                  "movq %%rsi,%%cr4   " /* CR4.PGE == 1 */
>> -                : : "r" (__pa(idle_pg_table)), "S" (cpu0_stack),
>> -                "D" (__va(__pa(cpu0_stack))), "c" (STACK_SIZE / 8) : "memory" );
>> +                : "=&S"(_discard), "=&D"(_discard), "=&c"(_discard)
>> +                : "0"(cpu0_stack), "1"(__va(__pa(cpu0_stack))),
>> +                  "2"(STACK_SIZE / 8), "r"(__pa(idle_pg_table))
>> +                : "memory" );
> Among the inputs, please put the one actively used by number first,
> or convert to labeled operands.

As far as I can tell, it is impossible to get the idle pagetables to
parameter %0 as it is not present in the output list.  I have switched
to a named parameter.

>  Also please don't corrupt the
> formatting - there ought to be a blank between the closing constraint
> quote and the opening parenthesis in all operands.

Done

~Andrew

Re: [PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack

[Jan Beulich]

>>> On 21.10.14 at 12:03, <andrew.cooper3@citrix.com> wrote:
> On 21/10/14 09:44, Jan Beulich wrote:
>>>>> On 20.10.14 at 19:30, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -836,6 +836,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>              l2_pgentry_t *pl2e;
>>>              uint64_t load_start;
>>>              int i, j, k;
>>> +            long _discard;
>> I can't see why you couldn't use i, j, or k for the discarding purposes.
> 
> A parameter with the name "_discard" is far more explicit about its
> purpose when read as part of the output parameter list.
> 
> I could switch to i and leave a comment by the parameters if you insist.

I don't strictly insist, but I dislike variables to be added without real
need. At the very least the leading underscore needs to be dropped
as needlessly being in conflict with C library standard naming rules.

>>> @@ -902,11 +903,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>                  "movq %%cr4,%%rsi ; "
>>>                  "andb $0x7f,%%sil ; "
>>>                  "movq %%rsi,%%cr4 ; " /* CR4.PGE == 0 */
>>> -                "movq %0,%%cr3    ; " /* CR3 == new pagetables */
>>> +                "movq %6,%%cr3    ; " /* CR3 == new pagetables */
>>>                  "orb $0x80,%%sil  ; "
>>>                  "movq %%rsi,%%cr4   " /* CR4.PGE == 1 */
>>> -                : : "r" (__pa(idle_pg_table)), "S" (cpu0_stack),
>>> -                "D" (__va(__pa(cpu0_stack))), "c" (STACK_SIZE / 8) : 
> "memory" );
>>> +                : "=&S"(_discard), "=&D"(_discard), "=&c"(_discard)
>>> +                : "0"(cpu0_stack), "1"(__va(__pa(cpu0_stack))),
>>> +                  "2"(STACK_SIZE / 8), "r"(__pa(idle_pg_table))
>>> +                : "memory" );
>> Among the inputs, please put the one actively used by number first,
>> or convert to labeled operands.
> 
> As far as I can tell, it is impossible to get the idle pagetables to
> parameter %0 as it is not present in the output list.  I have switched
> to a named parameter.

I didn't say make it %0 again (and specifically said "among the
inputs"), but it could have been moved ahead of all the other
inputs.

Jan