[PATCH] x86: fix wait code asm() constraints

[PATCH] x86: fix wait code asm() constraints

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

In __prepare_to_wait(), properly mark early clobbered registers. By
doing so, we at once eliminate the need to save/restore rCX and rDI.

In check_wakeup_from_wait(), make the current constraints match by
removing the code that actuall alters registers. By adjusting the
resume address in __prepare_to_wait(), we can simply re-use the copying
operation there (rather than doing a second pointless copy in the
opposite direction after branching to the resume point), which at once
eliminates the need for re-loading rCX and rDI inside the asm().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -126,6 +126,7 @@ static void __prepare_to_wait(struct wai
 {
     char *cpu_info = (char *)get_cpu_info();
     struct vcpu *curr = current;
+    unsigned long dummy;
 
     ASSERT(wqv->esp == 0);
 
@@ -140,27 +141,27 @@ static void __prepare_to_wait(struct wai
 
     asm volatile (
 #ifdef CONFIG_X86_64
-        "push %%rax; push %%rbx; push %%rcx; push %%rdx; push %%rdi; "
+        "push %%rax; push %%rbx; push %%rdx; "
         "push %%rbp; push %%r8; push %%r9; push %%r10; push %%r11; "
         "push %%r12; push %%r13; push %%r14; push %%r15; call 1f; "
-        "1: mov 80(%%rsp),%%rdi; mov 96(%%rsp),%%rcx; mov %%rsp,%%rsi; "
+        "1: mov %%rsp,%%rsi; addq $2f-1b,(%%rsp); "
         "sub %%rsi,%%rcx; cmp %3,%%rcx; jbe 2f; "
         "xor %%esi,%%esi; jmp 3f; "
         "2: rep movsb; mov %%rsp,%%rsi; 3: pop %%rax; "
         "pop %%r15; pop %%r14; pop %%r13; pop %%r12; "
         "pop %%r11; pop %%r10; pop %%r9; pop %%r8; "
-        "pop %%rbp; pop %%rdi; pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax"
+        "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
 #else
-        "push %%eax; push %%ebx; push %%ecx; push %%edx; push %%edi; "
+        "push %%eax; push %%ebx; push %%edx; "
         "push %%ebp; call 1f; "
-        "1: mov 8(%%esp),%%edi; mov 16(%%esp),%%ecx; mov %%esp,%%esi; "
+        "1: mov %%esp,%%esi; addl $2f-1b,(%%esp); "
         "sub %%esi,%%ecx; cmp %3,%%ecx; jbe 2f; "
         "xor %%esi,%%esi; jmp 3f; "
         "2: rep movsb; mov %%esp,%%esi; 3: pop %%eax; "
-        "pop %%ebp; pop %%edi; pop %%edx; pop %%ecx; pop %%ebx; pop %%eax"
+        "pop %%ebp; pop %%edx; pop %%ebx; pop %%eax"
 #endif
-        : "=S" (wqv->esp)
-        : "c" (cpu_info), "D" (wqv->stack), "i" (PAGE_SIZE)
+        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
+        : "i" (PAGE_SIZE), "1" (cpu_info), "2" (wqv->stack)
         : "memory" );
 
     if ( unlikely(wqv->esp == 0) )
@@ -200,7 +201,7 @@ void check_wakeup_from_wait(void)
     }
 
     asm volatile (
-        "mov %1,%%"__OP"sp; rep movsb; jmp *(%%"__OP"sp)"
+        "mov %1,%%"__OP"sp; jmp *(%0)"
         : : "S" (wqv->stack), "D" (wqv->esp),
         "c" ((char *)get_cpu_info() - (char *)wqv->esp)
         : "memory" );




[-- Attachment #2: x86-wait-constraints.patch --]
[-- Type: text/plain, Size: 2975 bytes --]

x86: fix wait code asm() constraints

In __prepare_to_wait(), properly mark early clobbered registers. By
doing so, we at once eliminate the need to save/restore rCX and rDI.

In check_wakeup_from_wait(), make the current constraints match by
removing the code that actuall alters registers. By adjusting the
resume address in __prepare_to_wait(), we can simply re-use the copying
operation there (rather than doing a second pointless copy in the
opposite direction after branching to the resume point), which at once
eliminates the need for re-loading rCX and rDI inside the asm().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -126,6 +126,7 @@ static void __prepare_to_wait(struct wai
 {
     char *cpu_info = (char *)get_cpu_info();
     struct vcpu *curr = current;
+    unsigned long dummy;
 
     ASSERT(wqv->esp == 0);
 
@@ -140,27 +141,27 @@ static void __prepare_to_wait(struct wai
 
     asm volatile (
 #ifdef CONFIG_X86_64
-        "push %%rax; push %%rbx; push %%rcx; push %%rdx; push %%rdi; "
+        "push %%rax; push %%rbx; push %%rdx; "
         "push %%rbp; push %%r8; push %%r9; push %%r10; push %%r11; "
         "push %%r12; push %%r13; push %%r14; push %%r15; call 1f; "
-        "1: mov 80(%%rsp),%%rdi; mov 96(%%rsp),%%rcx; mov %%rsp,%%rsi; "
+        "1: mov %%rsp,%%rsi; addq $2f-1b,(%%rsp); "
         "sub %%rsi,%%rcx; cmp %3,%%rcx; jbe 2f; "
         "xor %%esi,%%esi; jmp 3f; "
         "2: rep movsb; mov %%rsp,%%rsi; 3: pop %%rax; "
         "pop %%r15; pop %%r14; pop %%r13; pop %%r12; "
         "pop %%r11; pop %%r10; pop %%r9; pop %%r8; "
-        "pop %%rbp; pop %%rdi; pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax"
+        "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
 #else
-        "push %%eax; push %%ebx; push %%ecx; push %%edx; push %%edi; "
+        "push %%eax; push %%ebx; push %%edx; "
         "push %%ebp; call 1f; "
-        "1: mov 8(%%esp),%%edi; mov 16(%%esp),%%ecx; mov %%esp,%%esi; "
+        "1: mov %%esp,%%esi; addl $2f-1b,(%%esp); "
         "sub %%esi,%%ecx; cmp %3,%%ecx; jbe 2f; "
         "xor %%esi,%%esi; jmp 3f; "
         "2: rep movsb; mov %%esp,%%esi; 3: pop %%eax; "
-        "pop %%ebp; pop %%edi; pop %%edx; pop %%ecx; pop %%ebx; pop %%eax"
+        "pop %%ebp; pop %%edx; pop %%ebx; pop %%eax"
 #endif
-        : "=S" (wqv->esp)
-        : "c" (cpu_info), "D" (wqv->stack), "i" (PAGE_SIZE)
+        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
+        : "i" (PAGE_SIZE), "1" (cpu_info), "2" (wqv->stack)
         : "memory" );
 
     if ( unlikely(wqv->esp == 0) )
@@ -200,7 +201,7 @@ void check_wakeup_from_wait(void)
     }
 
     asm volatile (
-        "mov %1,%%"__OP"sp; rep movsb; jmp *(%%"__OP"sp)"
+        "mov %1,%%"__OP"sp; jmp *(%0)"
         : : "S" (wqv->stack), "D" (wqv->esp),
         "c" ((char *)get_cpu_info() - (char *)wqv->esp)
         : "memory" );

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: fix wait code asm() constraints

[Keir Fraser]

On 03/08/2012 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:

> In __prepare_to_wait(), properly mark early clobbered registers. By
> doing so, we at once eliminate the need to save/restore rCX and rDI.
> 
> In check_wakeup_from_wait(), make the current constraints match by
> removing the code that actuall alters registers. By adjusting the
> resume address in __prepare_to_wait(), we can simply re-use the copying
> operation there (rather than doing a second pointless copy in the
> opposite direction after branching to the resume point), which at once
> eliminates the need for re-loading rCX and rDI inside the asm().

First of all, this is code improvement, rather than a bug fix, right? The
asm constraints are correct for the code as it is, I believe.

It also seems the patch splits into two independent parts:

 A. Not sure whether the trade-off of the rCX/rDI save/restore versus more
complex asm constraints makes sense.

 B. Separately, the adjustment of the restore return address, and avoiding
needing to reload rCX/rDI after label 1, as well as avoiding the copy in
check_wakeup_from_wait(), is very nice.

I'm inclined to take the second part only, and make it clearer in the
changeset comment that it is not a bug fix.

What do you think?

 -- Keir

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -126,6 +126,7 @@ static void __prepare_to_wait(struct wai
>  {
>      char *cpu_info = (char *)get_cpu_info();
>      struct vcpu *curr = current;
> +    unsigned long dummy;
>  
>      ASSERT(wqv->esp == 0);
>  
> @@ -140,27 +141,27 @@ static void __prepare_to_wait(struct wai
>  
>      asm volatile (
>  #ifdef CONFIG_X86_64
> -        "push %%rax; push %%rbx; push %%rcx; push %%rdx; push %%rdi; "
> +        "push %%rax; push %%rbx; push %%rdx; "
>          "push %%rbp; push %%r8; push %%r9; push %%r10; push %%r11; "
>          "push %%r12; push %%r13; push %%r14; push %%r15; call 1f; "
> -        "1: mov 80(%%rsp),%%rdi; mov 96(%%rsp),%%rcx; mov %%rsp,%%rsi; "
> +        "1: mov %%rsp,%%rsi; addq $2f-1b,(%%rsp); "
>          "sub %%rsi,%%rcx; cmp %3,%%rcx; jbe 2f; "
>          "xor %%esi,%%esi; jmp 3f; "
>          "2: rep movsb; mov %%rsp,%%rsi; 3: pop %%rax; "
>          "pop %%r15; pop %%r14; pop %%r13; pop %%r12; "
>          "pop %%r11; pop %%r10; pop %%r9; pop %%r8; "
> -        "pop %%rbp; pop %%rdi; pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax"
> +        "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
>  #else
> -        "push %%eax; push %%ebx; push %%ecx; push %%edx; push %%edi; "
> +        "push %%eax; push %%ebx; push %%edx; "
>          "push %%ebp; call 1f; "
> -        "1: mov 8(%%esp),%%edi; mov 16(%%esp),%%ecx; mov %%esp,%%esi; "
> +        "1: mov %%esp,%%esi; addl $2f-1b,(%%esp); "
>          "sub %%esi,%%ecx; cmp %3,%%ecx; jbe 2f; "
>          "xor %%esi,%%esi; jmp 3f; "
>          "2: rep movsb; mov %%esp,%%esi; 3: pop %%eax; "
> -        "pop %%ebp; pop %%edi; pop %%edx; pop %%ecx; pop %%ebx; pop %%eax"
> +        "pop %%ebp; pop %%edx; pop %%ebx; pop %%eax"
>  #endif
> -        : "=S" (wqv->esp)
> -        : "c" (cpu_info), "D" (wqv->stack), "i" (PAGE_SIZE)
> +        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
> +        : "i" (PAGE_SIZE), "1" (cpu_info), "2" (wqv->stack)
>          : "memory" );
>  
>      if ( unlikely(wqv->esp == 0) )
> @@ -200,7 +201,7 @@ void check_wakeup_from_wait(void)
>      }
>  
>      asm volatile (
> -        "mov %1,%%"__OP"sp; rep movsb; jmp *(%%"__OP"sp)"
> +        "mov %1,%%"__OP"sp; jmp *(%0)"
>          : : "S" (wqv->stack), "D" (wqv->esp),
>          "c" ((char *)get_cpu_info() - (char *)wqv->esp)
>          : "memory" );
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86: fix wait code asm() constraints

[Jan Beulich]

>>> On 03.08.12 at 12:04, Keir Fraser <keir@xen.org> wrote:
> On 03/08/2012 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> In __prepare_to_wait(), properly mark early clobbered registers. By
>> doing so, we at once eliminate the need to save/restore rCX and rDI.
>> 
>> In check_wakeup_from_wait(), make the current constraints match by
>> removing the code that actuall alters registers. By adjusting the
>> resume address in __prepare_to_wait(), we can simply re-use the copying
>> operation there (rather than doing a second pointless copy in the
>> opposite direction after branching to the resume point), which at once
>> eliminates the need for re-loading rCX and rDI inside the asm().
> 
> First of all, this is code improvement, rather than a bug fix, right? The
> asm constraints are correct for the code as it is, I believe.

No, the constraints aren't really correct at present (yet this is
not visible as a functional bug in any way) - from a formal
perspective, the early clobber specification is needed on _any_
operand that doesn't retain its value throughout an asm(). Any
future compiler could derive something from this that we don't
intend.

> It also seems the patch splits into two independent parts:
> 
>  A. Not sure whether the trade-off of the rCX/rDI save/restore versus more
> complex asm constraints makes sense.
> 
>  B. Separately, the adjustment of the restore return address, and avoiding
> needing to reload rCX/rDI after label 1, as well as avoiding the copy in
> check_wakeup_from_wait(), is very nice.
> 
> I'm inclined to take the second part only, and make it clearer in the
> changeset comment that it is not a bug fix.
> 
> What do you think?

The patch could be split, yes, but where exactly the split(s)
should be isn't that obvious to me. And as it's fixing the same
kind of issue on both asm()-s, it seemed sensible to keep the
changes together.

Jan

Re: [PATCH] x86: fix wait code asm() constraints

[Keir Fraser]

On 03/08/2012 11:34, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 03.08.12 at 12:04, Keir Fraser <keir@xen.org> wrote:
>> On 03/08/2012 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>> In __prepare_to_wait(), properly mark early clobbered registers. By
>>> doing so, we at once eliminate the need to save/restore rCX and rDI.
>>> 
>>> In check_wakeup_from_wait(), make the current constraints match by
>>> removing the code that actuall alters registers. By adjusting the
>>> resume address in __prepare_to_wait(), we can simply re-use the copying
>>> operation there (rather than doing a second pointless copy in the
>>> opposite direction after branching to the resume point), which at once
>>> eliminates the need for re-loading rCX and rDI inside the asm().
>> 
>> First of all, this is code improvement, rather than a bug fix, right? The
>> asm constraints are correct for the code as it is, I believe.
> 
> No, the constraints aren't really correct at present (yet this is
> not visible as a functional bug in any way) - from a formal
> perspective, the early clobber specification is needed on _any_
> operand that doesn't retain its value throughout an asm(). Any
> future compiler could derive something from this that we don't
> intend.

I'm confused. The registers have the same values at the start and the end of
the asm statement. How can it possibly matter, even in theory, whether they
temporarily change in the middle? Is this fairly strong assumption written
down in the gcc documentation anywhere?

>> It also seems the patch splits into two independent parts:
>> 
>>  A. Not sure whether the trade-off of the rCX/rDI save/restore versus more
>> complex asm constraints makes sense.
>> 
>>  B. Separately, the adjustment of the restore return address, and avoiding
>> needing to reload rCX/rDI after label 1, as well as avoiding the copy in
>> check_wakeup_from_wait(), is very nice.
>> 
>> I'm inclined to take the second part only, and make it clearer in the
>> changeset comment that it is not a bug fix.
>> 
>> What do you think?
> 
> The patch could be split, yes, but where exactly the split(s)
> should be isn't that obvious to me. And as it's fixing the same
> kind of issue on both asm()-s, it seemed sensible to keep the
> changes together.

Yes, that confused me too -- the output constraints on the second asm can
hardly be wrong, or at least matter, since it never returns! Execution state
is completely reloaded within the asm statement.

 -- Keir

> Jan
> 

Re: [PATCH] x86: fix wait code asm() constraints

[Jan Beulich]

>>> On 03.08.12 at 13:00, Keir Fraser <keir.xen@gmail.com> wrote:
> On 03/08/2012 11:34, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>>>>> On 03.08.12 at 12:04, Keir Fraser <keir@xen.org> wrote:
>>> On 03/08/2012 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> 
>>>> In __prepare_to_wait(), properly mark early clobbered registers. By
>>>> doing so, we at once eliminate the need to save/restore rCX and rDI.
>>>> 
>>>> In check_wakeup_from_wait(), make the current constraints match by
>>>> removing the code that actuall alters registers. By adjusting the
>>>> resume address in __prepare_to_wait(), we can simply re-use the copying
>>>> operation there (rather than doing a second pointless copy in the
>>>> opposite direction after branching to the resume point), which at once
>>>> eliminates the need for re-loading rCX and rDI inside the asm().
>>> 
>>> First of all, this is code improvement, rather than a bug fix, right? The
>>> asm constraints are correct for the code as it is, I believe.
>> 
>> No, the constraints aren't really correct at present (yet this is
>> not visible as a functional bug in any way) - from a formal
>> perspective, the early clobber specification is needed on _any_
>> operand that doesn't retain its value throughout an asm(). Any
>> future compiler could derive something from this that we don't
>> intend.
> 
> I'm confused. The registers have the same values at the start and the end of
> the asm statement. How can it possibly matter, even in theory, whether they
> temporarily change in the middle? Is this fairly strong assumption written
> down in the gcc documentation anywhere?

It's in the specification of the & modifier:

"‘&’ Means (in a particular alternative) that this operand is an
 earlyclobber operand, which is modified before the instruction
 is finished using the input operands. Therefore, this operand
 may not lie in a register that is used as an input operand or as
 part of any memory address."

Of course, here we're not having any other operands, which
is why at least at present getting this wrong does no harm.

>>> It also seems the patch splits into two independent parts:
>>> 
>>>  A. Not sure whether the trade-off of the rCX/rDI save/restore versus more
>>> complex asm constraints makes sense.
>>> 
>>>  B. Separately, the adjustment of the restore return address, and avoiding
>>> needing to reload rCX/rDI after label 1, as well as avoiding the copy in
>>> check_wakeup_from_wait(), is very nice.
>>> 
>>> I'm inclined to take the second part only, and make it clearer in the
>>> changeset comment that it is not a bug fix.
>>> 
>>> What do you think?
>> 
>> The patch could be split, yes, but where exactly the split(s)
>> should be isn't that obvious to me. And as it's fixing the same
>> kind of issue on both asm()-s, it seemed sensible to keep the
>> changes together.
> 
> Yes, that confused me too -- the output constraints on the second asm can
> hardly be wrong, or at least matter, since it never returns! Execution state
> is completely reloaded within the asm statement.

Formally they're wrong too without that change. And the
fact that the asm() does not "return" is irrelevant here, as
the restriction is because of the potential use of the register
inside the asm(), after it already got modified.

Formally it's also not permitted for the asm() to branch
elsewhere, but that is violated in so many places (Linux not
the least) that they can hardly dare to ever come up with
something breaking this.

"Speaking of labels, jumps from one asm to another are not
 supported. The compiler’s optimizers do not know about these
 jumps, and therefore they cannot take account of them when
 deciding how to optimize."

Plus, this likely is really targeting jumps from one asm to another
_within_ one function, albeit that's not being said explicitly.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: fix wait code asm() constraints

[Keir Fraser]

On 03/08/2012 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:

> In __prepare_to_wait(), properly mark early clobbered registers. By
> doing so, we at once eliminate the need to save/restore rCX and rDI.

Okay, this patch has my blessing as is. But please add a remark that the
existing constraints are falling foul of a strict reading of the gcc
specification, and are actually okay in practice (being very
straightforward, no memory constraints, etc). I really thought you had found
a bug in practice, but this was not the case.

> In check_wakeup_from_wait(), make the current constraints match by
> removing the code that actuall alters registers. By adjusting the
> resume address in __prepare_to_wait(), we can simply re-use the copying
> operation there (rather than doing a second pointless copy in the
> opposite direction after branching to the resume point), which at once
> eliminates the need for re-loading rCX and rDI inside the asm().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -126,6 +126,7 @@ static void __prepare_to_wait(struct wai
>  {
>      char *cpu_info = (char *)get_cpu_info();
>      struct vcpu *curr = current;
> +    unsigned long dummy;
>  
>      ASSERT(wqv->esp == 0);
>  
> @@ -140,27 +141,27 @@ static void __prepare_to_wait(struct wai
>  
>      asm volatile (
>  #ifdef CONFIG_X86_64
> -        "push %%rax; push %%rbx; push %%rcx; push %%rdx; push %%rdi; "
> +        "push %%rax; push %%rbx; push %%rdx; "
>          "push %%rbp; push %%r8; push %%r9; push %%r10; push %%r11; "
>          "push %%r12; push %%r13; push %%r14; push %%r15; call 1f; "
> -        "1: mov 80(%%rsp),%%rdi; mov 96(%%rsp),%%rcx; mov %%rsp,%%rsi; "
> +        "1: mov %%rsp,%%rsi; addq $2f-1b,(%%rsp); "
>          "sub %%rsi,%%rcx; cmp %3,%%rcx; jbe 2f; "
>          "xor %%esi,%%esi; jmp 3f; "
>          "2: rep movsb; mov %%rsp,%%rsi; 3: pop %%rax; "
>          "pop %%r15; pop %%r14; pop %%r13; pop %%r12; "
>          "pop %%r11; pop %%r10; pop %%r9; pop %%r8; "
> -        "pop %%rbp; pop %%rdi; pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax"
> +        "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
>  #else
> -        "push %%eax; push %%ebx; push %%ecx; push %%edx; push %%edi; "
> +        "push %%eax; push %%ebx; push %%edx; "
>          "push %%ebp; call 1f; "
> -        "1: mov 8(%%esp),%%edi; mov 16(%%esp),%%ecx; mov %%esp,%%esi; "
> +        "1: mov %%esp,%%esi; addl $2f-1b,(%%esp); "
>          "sub %%esi,%%ecx; cmp %3,%%ecx; jbe 2f; "
>          "xor %%esi,%%esi; jmp 3f; "
>          "2: rep movsb; mov %%esp,%%esi; 3: pop %%eax; "
> -        "pop %%ebp; pop %%edi; pop %%edx; pop %%ecx; pop %%ebx; pop %%eax"
> +        "pop %%ebp; pop %%edx; pop %%ebx; pop %%eax"
>  #endif
> -        : "=S" (wqv->esp)
> -        : "c" (cpu_info), "D" (wqv->stack), "i" (PAGE_SIZE)
> +        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
> +        : "i" (PAGE_SIZE), "1" (cpu_info), "2" (wqv->stack)
>          : "memory" );
>  
>      if ( unlikely(wqv->esp == 0) )
> @@ -200,7 +201,7 @@ void check_wakeup_from_wait(void)
>      }
>  
>      asm volatile (
> -        "mov %1,%%"__OP"sp; rep movsb; jmp *(%%"__OP"sp)"
> +        "mov %1,%%"__OP"sp; jmp *(%0)"
>          : : "S" (wqv->stack), "D" (wqv->esp),
>          "c" ((char *)get_cpu_info() - (char *)wqv->esp)
>          : "memory" );
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86: fix wait code asm() constraints

[Keir Fraser]

On 03/08/2012 12:36, "Jan Beulich" <JBeulich@suse.com> wrote:

>> I'm confused. The registers have the same values at the start and the end of
>> the asm statement. How can it possibly matter, even in theory, whether they
>> temporarily change in the middle? Is this fairly strong assumption written
>> down in the gcc documentation anywhere?
> 
> It's in the specification of the & modifier:
> 
> "Œ&¹ Means (in a particular alternative) that this operand is an
>  earlyclobber operand, which is modified before the instruction
>  is finished using the input operands. Therefore, this operand
>  may not lie in a register that is used as an input operand or as
>  part of any memory address."
> 
> Of course, here we're not having any other operands, which
> is why at least at present getting this wrong does no harm.

Yep, okay, that makes sense. Especially the use of an input operand to form
a memory address, although of course that cannot happen in our specific case
here. I have acked your patch, although I'd like an update to the patch
comment.

 Thanks,
 -- Keir

Re: [PATCH] x86: fix wait code asm() constraints

[Jan Beulich]

>>> On 03.08.12 at 14:08, Keir Fraser <keir.xen@gmail.com> wrote:
> On 03/08/2012 12:36, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>>> I'm confused. The registers have the same values at the start and the end of
>>> the asm statement. How can it possibly matter, even in theory, whether they
>>> temporarily change in the middle? Is this fairly strong assumption written
>>> down in the gcc documentation anywhere?
>> 
>> It's in the specification of the & modifier:
>> 
>> "Œ&¹ Means (in a particular alternative) that this operand is an
>>  earlyclobber operand, which is modified before the instruction
>>  is finished using the input operands. Therefore, this operand
>>  may not lie in a register that is used as an input operand or as
>>  part of any memory address."
>> 
>> Of course, here we're not having any other operands, which
>> is why at least at present getting this wrong does no harm.
> 
> Yep, okay, that makes sense. Especially the use of an input operand to form
> a memory address, although of course that cannot happen in our specific case
> here. I have acked your patch, although I'd like an update to the patch
> comment.

How about this as an added initial paragraph?

This fixes theoretical issues with those constraints - operands that
get clobbered before consuming all input operands must be marked so
according the the gcc documentation. Beyond that, the change is merely
code improvement, not a bug fix.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: fix wait code asm() constraints

[Keir Fraser]

On 03/08/2012 13:15, "Jan Beulich" <JBeulich@suse.com> wrote:

>> Yep, okay, that makes sense. Especially the use of an input operand to form
>> a memory address, although of course that cannot happen in our specific case
>> here. I have acked your patch, although I'd like an update to the patch
>> comment.
> 
> How about this as an added initial paragraph?
> 
> This fixes theoretical issues with those constraints - operands that
> get clobbered before consuming all input operands must be marked so
> according the the gcc documentation. Beyond that, the change is merely
> code improvement, not a bug fix.

Perfect!

 K.