Re: Possible problem emulating movntq, movss

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/08/2014 11:47, Andrei LUTAS wrote:
> Hello there,
>
> On 8/6/2014 12:54 PM, Jan Beulich wrote:
>>>>> On 06.08.14 at 10:57, <rcojocaru@bitdefender.com> wrote:
>>> We found that our HVM guests froze when trying to emulate movntq
>>> instructions. The solution seems to be to replace "goto done;" with
>>> "break;" at line 4191 (when handling "case 0x7f:") in
>>> xen/arch/x86/x86_emulate/x86_emulate.c. Otherwise the writeback part
>>> doesn't happen.
>>>
>>> If you're happy with the fix I can prepare a patch, otherwise please
>>> let
>>> me know if we're missing something.
>> No, that doesn't look right: There's nothing left to be written back at
>> that point (registers get updated with the instruction executed via the
>> on-stack stub, and memory gets written with immediately preceding
>> ops->write(). So without you being more specific about _what_ you
>> see going wrong I don't think I can give further advice.
> Except for maybe the instruction pointer? That doesn't seem to be updated
> anywhereexcept during the write-back phase (or maybe I'm missing the
> spot).
> The problem is that the guest gets stuck with the instruction pointer
> pointing to the sameinstruction (in our particular case it is
> "MOVDQU xmm0, xmmword ptr [rdx + rcx - 0x10]"),entering in an infinite
> loop (EPT violation - emulate), since the IP doesn't seem to be updated.
>>
>> Furthermore what you write is kind of inconsistent: For one, opcode
>> 0x7f is movq/movdq[au] rather than movntdq (admitted they're
>> being handled by the same code block, but you ought to be rather
>> precise here). And then the subject of your mail mentions movss, but
>> the body doesn't at all - is that because you mean the same would
>> apply to that other similar code block?
> A quick look reveals that 0x0f 0x2b/0x28/0x29/0x10/0x11 and 0x0f
> 0xe7/0x6f/0x7f
> encodings are all affected. While other places may be affected too,
> these two
> encoding sets seem to be the only ones where "goto done;" is used
> unconditionally instead of a "break;" - all otheruses of "goto done;" are
> made when an error is encountered.
>
>>
>> As to Andrew asking for added tests: movq, movdqu, and vmovdqu
>> are all being tested with both operation directions (covering one of
>> the two code blocks in question), and the set of tests for movsd,
>> movaps, vmovsd, and vmovaps should be sufficient to cover the
>> other of the two code blocks too.
>>
>> Jan
> Best regards,
> Andrei.

It would appear that for some instructions, these movxxx included, the
test harness does not verify that the instruction pointer has been
suitably updated.

It is plausible that this is a real bug and the unit tests are
erroneously passing.  I would suggest starting by adding instruction
pointer checks to the test harness first to confirm whether there is a bug.

~Andrew