[PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Zachary Amsden]

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

The 128-bit multiply in pvclock.h was missing an output constraint for 
EDX which caused a register corruption to appear.  Thanks to Ulrich for 
diagnosing the EDX corruption and Avi for providing this fix (which now 
I am sending back to you Avi..)

Zach

[-- Attachment #2: x86-kvm-pvclock-scale-delta-constraint-fix.patch --]
[-- Type: text/plain, Size: 957 bytes --]

>From Avi and Ulrich, fix constraints for 128-bit multiply

Signed-off-by: Zachary Amsden <zamsden@redhat.com>

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 31d84ac..a518c0a 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -22,6 +22,8 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
 	u64 product;
 #ifdef __i386__
 	u32 tmp1, tmp2;
+#else
+	ulong tmp;
 #endif
 
 	if (shift < 0)
@@ -42,8 +44,11 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
 		: "a" ((u32)delta), "1" ((u32)(delta >> 32)), "2" (mul_frac) );
 #elif defined(__x86_64__)
 	__asm__ (
-		"mul %%rdx ; shrd $32,%%rdx,%%rax"
-		: "=a" (product) : "0" (delta), "d" ((u64)mul_frac) );
+		"mul %[mul_frac] ; shrd $32, %[hi], %[lo]"
+		: [lo]"=a"(product),
+		  [hi]"=d"(tmp)
+		: "0"(delta),
+		  [mul_frac]"rm"((u64)mul_frac));
 #else
 #error implement me!
 #endif

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Avi Kivity]

On 06/16/2011 06:50 AM, Zachary Amsden wrote:
> The 128-bit multiply in pvclock.h was missing an output constraint for 
> EDX which caused a register corruption to appear.  Thanks to Ulrich 
> for diagnosing the EDX corruption and Avi for providing this fix 
> (which now I am sending back to you Avi..)

Applied and queued, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Ian Campbell]

On Sun, 2011-06-19 at 15:44 +0300, Avi Kivity wrote:
> On 06/16/2011 06:50 AM, Zachary Amsden wrote:
> > The 128-bit multiply in pvclock.h was missing an output constraint for 
> > EDX which caused a register corruption to appear.

Was there any particular symptom associated with corruption at that
particular point or just general badness?

>   Thanks to Ulrich 
> > for diagnosing the EDX corruption and Avi for providing this fix 
> > (which now I am sending back to you Avi..)
> 
> Applied and queued, thanks.

Seems like a stable candidate also?

Ian.

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Avi Kivity]

On 06/22/2011 12:35 PM, Ian Campbell wrote:
> On Sun, 2011-06-19 at 15:44 +0300, Avi Kivity wrote:
> >  On 06/16/2011 06:50 AM, Zachary Amsden wrote:
> >  >  The 128-bit multiply in pvclock.h was missing an output constraint for
> >  >  EDX which caused a register corruption to appear.
>
> Was there any particular symptom associated with corruption at that
> particular point or just general badness?
>
> >    Thanks to Ulrich
> >  >  for diagnosing the EDX corruption and Avi for providing this fix
> >  >  (which now I am sending back to you Avi..)
> >
> >  Applied and queued, thanks.
>
> Seems like a stable candidate also?

Yes.  I see it's merged; Marcelo, can you backport it?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Avi Kivity]

On 06/22/2011 12:35 PM, Ian Campbell wrote:
> On Sun, 2011-06-19 at 15:44 +0300, Avi Kivity wrote:
> >  On 06/16/2011 06:50 AM, Zachary Amsden wrote:
> >  >  The 128-bit multiply in pvclock.h was missing an output constraint for
> >  >  EDX which caused a register corruption to appear.
>
> Was there any particular symptom associated with corruption at that
> particular point or just general badness?
>

Time went backwards and forwards as vcpus were migrated across cpus.

The problem was in the host, not the guest, so Xen should not be affected.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Ian Campbell]

On Wed, 2011-06-22 at 10:40 +0100, Avi Kivity wrote:
> On 06/22/2011 12:35 PM, Ian Campbell wrote:
> > On Sun, 2011-06-19 at 15:44 +0300, Avi Kivity wrote:
> > >  On 06/16/2011 06:50 AM, Zachary Amsden wrote:
> > >  >  The 128-bit multiply in pvclock.h was missing an output constraint for
> > >  >  EDX which caused a register corruption to appear.
> >
> > Was there any particular symptom associated with corruption at that
> > particular point or just general badness?
> >
> 
> Time went backwards and forwards as vcpus were migrated across cpus.

Oops!

> The problem was in the host, not the guest, so Xen should not be affected.

Good to know, thanks!

Ian.

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Duncan Sands]

Hi Zachary, this patch can cause a compile failure if the compiler chooses to
use a memory location for mul_frac rather than a register (as allowed by the
"m" in the constraints):

   pvclock.s:229: Error: no instruction mnemonic suffix given and no register 
operands; can't size instruction

In this example the assembler is:

         #APP
         mul -48(%rbp) ; shrd $32, %rdx, %rax
         #NO_APP

A simple solution is to use mulq, as in the following patch:

Signed-off-by: Duncan Sands <baldrick@free.fr>

--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -44,7 +44,7 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, 
int shift)
                 : "a" ((u32)delta), "1" ((u32)(delta >> 32)), "2" (mul_frac) );
  #elif defined(__x86_64__)
         __asm__ (
-               "mul %[mul_frac] ; shrd $32, %[hi], %[lo]"
+               "mulq %[mul_frac] ; shrd $32, %[hi], %[lo]"
                 : [lo]"=a"(product),
                   [hi]"=d"(tmp)
                 : "0"(delta),

Re: [PATCH x86 kvm] Fix register corruption in pvclock_scale_delta

[Avi Kivity]

On 08/30/2011 11:58 AM, Duncan Sands wrote:
> Hi Zachary, this patch can cause a compile failure if the compiler 
> chooses to
> use a memory location for mul_frac rather than a register (as allowed 
> by the
> "m" in the constraints):
>
>   pvclock.s:229: Error: no instruction mnemonic suffix given and no 
> register operands; can't size instruction
>
> In this example the assembler is:
>
>         #APP
>         mul -48(%rbp) ; shrd $32, %rdx, %rax
>         #NO_APP
>
> A simple solution is to use mulq, as in the following patch:
>
> Signed-off-by: Duncan Sands <baldrick@free.fr>
>
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -44,7 +44,7 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 
> mul_frac, int shift)
>                 : "a" ((u32)delta), "1" ((u32)(delta >> 32)), "2" 
> (mul_frac) );
>  #elif defined(__x86_64__)
>         __asm__ (
> -               "mul %[mul_frac] ; shrd $32, %[hi], %[lo]"
> +               "mulq %[mul_frac] ; shrd $32, %[hi], %[lo]"
>                 : [lo]"=a"(product),
>                   [hi]"=d"(tmp)
>                 : "0"(delta),

Thanks, applied.

-- 
error compiling committee.c: too many arguments to function