Xen devel - Aug 2009 - [Patch] 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: juergen.gross@ts.fujitsu.com
Otto-Hahn-Ring 6                        Internet: ts.fujitsu.com
D-81739 Muenchen                 Company details: ts.fujitsu.com/imprint.html


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

>>> Juergen Gross <juergen.gross@ts.fujitsu.com> 06.08.09 08:49
>>>
>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.
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


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

Jan Beulich wrote:
>>>> Juergen Gross <juergen.gross@ts.fujitsu.com> 06.08.09
08:49 >>>
>> 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.
> 
> 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.
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: juergen.gross@ts.fujitsu.com
Otto-Hahn-Ring 6                        Internet: ts.fujitsu.com
D-81739 Muenchen                 Company details: ts.fujitsu.com/imprint.html

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

On 06/08/2009 07:49, "Juergen Gross"
<juergen.gross@ts.fujitsu.com> wrote:
> 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.
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



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