cmpxchg emulation returns wrong ZF

Hi,

attached patch corrects a bug in cmpxchg emulation in the hypervisor.

BS2000 running as HVM-domain on 4 vcpus (no HAP) hit an error due to this bug after several days.

Juergen

Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 636 47950
Fujitsu Technolgy Solutions               e-mail: [email protected]
Otto-Hahn-Ring 6                        Internet: ts.fujitsu.com
D-81739 Muenchen                 Company details: ts.fujitsu.com/imprint.html

The cmpxchg emulation for accesses to page tables of guests doesn't handle races correct.
ops->cmpxchg might return X86EMUL_CMPXCHG_FAILED if the addressed memory location changed after checking the old contents. In this case ZF was not changed and could remain 1 instead of being set to 0.

# HG changeset patch
# User [email protected]
# Date 1249540842 -7200
# Node ID 26adbdb6cb1d59d95e0a65b6a0d38fa8e95b9f51 # Parent 68e8b8379244e293c55875e7dc3692fc81d3d212 handle race on cmpxchg emulation

diff -r 68e8b8379244 -r 26adbdb6cb1d xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4124,6 +4124,7 @@
         op_bytes *= 2;
 
         /* Get actual old value. */
+cmpxchg_failed:
         for ( i = 0; i < (op_bytes/sizeof(long)); i++ )
             if ( (rc = read_ulong(ea.mem.seg, ea.mem.off + i*sizeof(long),
                                   &old[i], sizeof(long), ctxt, ops)) != 0 )

@@ -4151,10 +4152,13 @@
         else
         {
             /* Expected == actual: attempt atomic cmpxchg and set ZF. */
-            if ( (rc = ops->cmpxchg(ea.mem.seg, ea.mem.off, old,
-                                    new, op_bytes, ctxt)) != 0 )
-                goto done;
-            _regs.eflags |= EFLG_ZF;
+            rc = ops->cmpxchg(ea.mem.seg, ea.mem.off, old, new, op_bytes, ctxt);
+            if ( rc == 0 )
+                _regs.eflags |= EFLG_ZF;
+            else if ( rc == X86EMUL_CMPXCHG_FAILED )
+                goto cmpxchg_failed;
+            else
+                goto done;
         }
         break;
     }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Why don't you just clear ZF in that case? I think it is intentional that the code doesn't loop inside the hypervisor, since that loop is non-preemptible (whereas returning to the guest and re-issuing the instruction is).

Further, I'm not really clear why that change is necessary at all: In the code prior to the patch, register state is not being updated if ops->cmpxchg() failed, and hence the old value of ZF is simply being retained - which is the correct thing to do when intending to re-start the instruction.

Jan

Oh yes, you are right!
I missed that eip isn't updated then, too.

Please forget that patch. I'll continue to investigate the problem...

Juergen

Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 636 47950
Fujitsu Technolgy Solutions               e-mail: [email protected]
Otto-Hahn-Ring 6                        Internet: ts.fujitsu.com
D-81739 Muenchen                 Company details: ts.fujitsu.com/imprint.html

You'll have to give more details as I don't see the bug that this patch fixes. Changeset comment says "ops->cmpxchg might return X86EMUL_CMPXCHG_FAILED if the addressed memory location changed after checking the old contents. In this case ZF was not changed and could remain 1 instead of being set to 0." Now, firstly the patch does not directly alter ZF when X86EMUL_CMPXCHG_FAILED. Secondly, the X86EMUL_CMPXCHG_FAILED is supposed to be safe to propagate to the caller of x86_emulate(), who can then choose to retry. Most callers implicitly retry by treating similar to X86EMUL_OKAY -- returning to guest context where the instruction gets reattempted due to EIP not having changed. That last point is crucial to the correctness of course: Indeed we are not messing with EFLAGS.ZF on that return code, but then we are not updating *any* state (including the program counter) so it is supposed to be as if the instruction was not executed (which is obviously correct, since it wasn't).

-- Keir