All of lore.kernel.org
 help / color / mirror / Atom feed
* Possible problem emulating movntq, movss
@ 2014-08-06  8:57 Razvan Cojocaru
  2014-08-06  9:22 ` Andrew Cooper
  2014-08-06  9:54 ` Jan Beulich
  0 siblings, 2 replies; 11+ messages in thread
From: Razvan Cojocaru @ 2014-08-06  8:57 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, keir, Jan Beulich

Hello,

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.


Thanks,
Razvan Cojocaru

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06  8:57 Possible problem emulating movntq, movss Razvan Cojocaru
@ 2014-08-06  9:22 ` Andrew Cooper
  2014-08-06  9:54 ` Jan Beulich
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2014-08-06  9:22 UTC (permalink / raw)
  To: Razvan Cojocaru, xen-devel; +Cc: keir, Jan Beulich

On 06/08/2014 09:57, Razvan Cojocaru wrote:
> Hello,
>
> 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.
>
>
> Thanks,
> Razvan Cojocaru

I can't comment on the fix right now, but will take a look at it in due
course.

If you are making a change, please can you add a testcase for movntq and
movss to tools/tests/x86_emulator/test_x86_emulator.c which is the
unittest harness for x86_emulate.

~Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06  8:57 Possible problem emulating movntq, movss Razvan Cojocaru
  2014-08-06  9:22 ` Andrew Cooper
@ 2014-08-06  9:54 ` Jan Beulich
  2014-08-06 10:39   ` Razvan Cojocaru
  2014-08-06 10:47   ` Andrei LUTAS
  1 sibling, 2 replies; 11+ messages in thread
From: Jan Beulich @ 2014-08-06  9:54 UTC (permalink / raw)
  To: Razvan Cojocaru; +Cc: Andrew Cooper, keir, xen-devel

>>> 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.

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?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06  9:54 ` Jan Beulich
@ 2014-08-06 10:39   ` Razvan Cojocaru
  2014-08-06 10:47   ` Andrei LUTAS
  1 sibling, 0 replies; 11+ messages in thread
From: Razvan Cojocaru @ 2014-08-06 10:39 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andrew Cooper, keir, xen-devel

On 08/06/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.

I understand. My colleague and fellow xen-devel subscriber Andrei Lutas
has found the issue and the solution, and will reply with more details.

> 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?

Indeed, my assumptions were exactly those: movq/movdq[au] is handled in
the same code block (it is, in fact, arguably the beginning of said code
block) as movntdq, and movss seems to be handled in the same manner
("goto done;" vs "break;"), and if I understood Andrei correctly, poses
the same problem.


Thanks,
Razvan Cojocaru

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06  9:54 ` Jan Beulich
  2014-08-06 10:39   ` Razvan Cojocaru
@ 2014-08-06 10:47   ` Andrei LUTAS
  2014-08-06 11:05     ` Andrew Cooper
                       ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Andrei LUTAS @ 2014-08-06 10:47 UTC (permalink / raw)
  To: Jan Beulich, Razvan Cojocaru; +Cc: Andrew Cooper, keir, xen-devel

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.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06 10:47   ` Andrei LUTAS
@ 2014-08-06 11:05     ` Andrew Cooper
  2014-08-06 11:22       ` Razvan Cojocaru
  2014-08-06 12:16     ` Jan Beulich
  2014-08-06 12:29     ` Jan Beulich
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2014-08-06 11:05 UTC (permalink / raw)
  To: Andrei LUTAS, Jan Beulich, Razvan Cojocaru; +Cc: keir, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06 11:05     ` Andrew Cooper
@ 2014-08-06 11:22       ` Razvan Cojocaru
  0 siblings, 0 replies; 11+ messages in thread
From: Razvan Cojocaru @ 2014-08-06 11:22 UTC (permalink / raw)
  To: Andrew Cooper, Andrei LUTAS, Jan Beulich; +Cc: keir, xen-devel

On 08/06/2014 02:05 PM, Andrew Cooper wrote:
> 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.

Quick and dirty test:

655     printf("%-40s", "Testing movdqu %xmm2,(%ecx)...");
656     if ( stack_exec && cpu_has_sse2 )
657     {
658         extern const unsigned char movdqu_to_mem[];
659
660         asm volatile ( "pcmpeqb %%xmm2, %%xmm2\n"
661                        ".pushsection .test, \"a\", @progbits\n"
662                        "movdqu_to_mem: movdqu %%xmm2, (%0)\n"
663                        ".popsection" :: "c" (NULL) );
664
665         memcpy(instr, movdqu_to_mem, 15);
666         memset(res, 0x55, 64);
667         memset(res + 8, 0xff, 16);
668         regs.eip    = (unsigned long)&instr[0];
669         regs.ecx    = (unsigned long)res;
670         rc = x86_emulate(&ctxt, &emulops);
671         if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
672             goto fail;
673
674         if ( regs.eip == (unsigned long)&instr[0] )
675             printf("eip has NOT been updated!\n");
676         else
677             printf("okay\n");
678     }
679     else
680         printf("skipped\n");

Output:

Testing movdqu %xmm2,(%ecx)...          eip has NOT been updated!


Thanks,
Razvan Cojocaru

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06 10:47   ` Andrei LUTAS
  2014-08-06 11:05     ` Andrew Cooper
@ 2014-08-06 12:16     ` Jan Beulich
  2014-08-06 12:50       ` Jan Beulich
  2014-08-06 12:29     ` Jan Beulich
  2 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-08-06 12:16 UTC (permalink / raw)
  To: Razvan Cojocaru, Andrei LUTAS; +Cc: Andrew Cooper, keir, xen-devel

>>> On 06.08.14 at 12:47, <vlutas@bitdefender.com> wrote:
> 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.

Now that is indeed a problem, but not solved by simply replacing
the "goto done" with "break". I'll look into getting you a proper fix.

Jan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06 10:47   ` Andrei LUTAS
  2014-08-06 11:05     ` Andrew Cooper
  2014-08-06 12:16     ` Jan Beulich
@ 2014-08-06 12:29     ` Jan Beulich
  2 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2014-08-06 12:29 UTC (permalink / raw)
  To: Razvan Cojocaru, Andrei LUTAS; +Cc: Andrew Cooper, keir, xen-devel

>>> On 06.08.14 at 12:47, <vlutas@bitdefender.com> wrote:
> 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.

With two other exceptions: The get_rep_prefix() use seems to be
broken too, while the use after the swint label looks to be correct
(ops->inject_sw_interrupt() ought to do all necessary updating).

Jan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06 12:16     ` Jan Beulich
@ 2014-08-06 12:50       ` Jan Beulich
  2014-08-07  8:09         ` Razvan Cojocaru
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2014-08-06 12:50 UTC (permalink / raw)
  To: Razvan Cojocaru, Andrei LUTAS; +Cc: Andrew Cooper, keir, xen-devel

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

>>> On 06.08.14 at 14:16, <JBeulich@suse.com> wrote:
>>>> On 06.08.14 at 12:47, <vlutas@bitdefender.com> wrote:
>> 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.
> 
> Now that is indeed a problem, but not solved by simply replacing
> the "goto done" with "break". I'll look into getting you a proper fix.

Mind giving this one (lightly tested only) a try?

Jan

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -720,29 +720,26 @@ do{ uint8_t stub[] = { _bytes, 0xc3 };  
     put_fpu(&fic);                                                      \
 } while (0)
 
-static unsigned long __get_rep_prefix(
-    struct cpu_user_regs *int_regs,
-    struct cpu_user_regs *ext_regs,
+static unsigned long _get_rep_prefix(
+    const struct cpu_user_regs *int_regs,
     int ad_bytes)
 {
-    unsigned long ecx = ((ad_bytes == 2) ? (uint16_t)int_regs->ecx :
-                         (ad_bytes == 4) ? (uint32_t)int_regs->ecx :
-                         int_regs->ecx);
-
-    /* Skip the instruction if no repetitions are required. */
-    if ( ecx == 0 )
-        ext_regs->eip = int_regs->eip;
-
-    return ecx;
+    return (ad_bytes == 2) ? (uint16_t)int_regs->ecx :
+           (ad_bytes == 4) ? (uint32_t)int_regs->ecx :
+           int_regs->ecx;
 }
 
 #define get_rep_prefix() ({                                             \
     unsigned long max_reps = 1;                                         \
     if ( rep_prefix() )                                                 \
-        max_reps = __get_rep_prefix(&_regs, ctxt->regs, ad_bytes);      \
+        max_reps = _get_rep_prefix(&_regs, ad_bytes);                   \
     if ( max_reps == 0 )                                                \
-        goto done;                                                      \
-   max_reps;                                                            \
+    {                                                                   \
+        /* Skip the instruction if no repetitions are required. */      \
+        dst.type = OP_NONE;                                             \
+        goto writeback;                                                 \
+    }                                                                   \
+    max_reps;                                                           \
 })
 
 static void __put_rep_prefix(
@@ -3921,7 +3918,8 @@ x86_emulate(
         if ( !rc && (b & 1) && (ea.type == OP_MEM) )
             rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
                             ea.bytes, ctxt);
-        goto done;
+        dst.type = OP_NONE;
+        break;
     }
 
     case 0x20: /* mov cr,reg */
@@ -4188,7 +4186,8 @@ x86_emulate(
         if ( !rc && (b != 0x6f) && (ea.type == OP_MEM) )
             rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
                             ea.bytes, ctxt);
-        goto done;
+        dst.type = OP_NONE;
+        break;
     }
 
     case 0x80 ... 0x8f: /* jcc (near) */ {


[-- Attachment #2: x86emul-IP-updates.patch --]
[-- Type: text/plain, Size: 2705 bytes --]

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -720,29 +720,26 @@ do{ uint8_t stub[] = { _bytes, 0xc3 };  
     put_fpu(&fic);                                                      \
 } while (0)
 
-static unsigned long __get_rep_prefix(
-    struct cpu_user_regs *int_regs,
-    struct cpu_user_regs *ext_regs,
+static unsigned long _get_rep_prefix(
+    const struct cpu_user_regs *int_regs,
     int ad_bytes)
 {
-    unsigned long ecx = ((ad_bytes == 2) ? (uint16_t)int_regs->ecx :
-                         (ad_bytes == 4) ? (uint32_t)int_regs->ecx :
-                         int_regs->ecx);
-
-    /* Skip the instruction if no repetitions are required. */
-    if ( ecx == 0 )
-        ext_regs->eip = int_regs->eip;
-
-    return ecx;
+    return (ad_bytes == 2) ? (uint16_t)int_regs->ecx :
+           (ad_bytes == 4) ? (uint32_t)int_regs->ecx :
+           int_regs->ecx;
 }
 
 #define get_rep_prefix() ({                                             \
     unsigned long max_reps = 1;                                         \
     if ( rep_prefix() )                                                 \
-        max_reps = __get_rep_prefix(&_regs, ctxt->regs, ad_bytes);      \
+        max_reps = _get_rep_prefix(&_regs, ad_bytes);                   \
     if ( max_reps == 0 )                                                \
-        goto done;                                                      \
-   max_reps;                                                            \
+    {                                                                   \
+        /* Skip the instruction if no repetitions are required. */      \
+        dst.type = OP_NONE;                                             \
+        goto writeback;                                                 \
+    }                                                                   \
+    max_reps;                                                           \
 })
 
 static void __put_rep_prefix(
@@ -3921,7 +3918,8 @@ x86_emulate(
         if ( !rc && (b & 1) && (ea.type == OP_MEM) )
             rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
                             ea.bytes, ctxt);
-        goto done;
+        dst.type = OP_NONE;
+        break;
     }
 
     case 0x20: /* mov cr,reg */
@@ -4188,7 +4186,8 @@ x86_emulate(
         if ( !rc && (b != 0x6f) && (ea.type == OP_MEM) )
             rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
                             ea.bytes, ctxt);
-        goto done;
+        dst.type = OP_NONE;
+        break;
     }
 
     case 0x80 ... 0x8f: /* jcc (near) */ {

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Possible problem emulating movntq, movss
  2014-08-06 12:50       ` Jan Beulich
@ 2014-08-07  8:09         ` Razvan Cojocaru
  0 siblings, 0 replies; 11+ messages in thread
From: Razvan Cojocaru @ 2014-08-07  8:09 UTC (permalink / raw)
  To: Jan Beulich, Andrei LUTAS; +Cc: Andrew Cooper, keir, xen-devel

On 08/06/2014 03:50 PM, Jan Beulich wrote:
>>>> On 06.08.14 at 14:16, <JBeulich@suse.com> wrote:
>>>>> On 06.08.14 at 12:47, <vlutas@bitdefender.com> wrote:
>>> 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.
>>
>> Now that is indeed a problem, but not solved by simply replacing
>> the "goto done" with "break". I'll look into getting you a proper fix.
> 
> Mind giving this one (lightly tested only) a try?

We've tested this with both a modified version of test_x86_emulator.c
(small patch to follow) and our HVM guest, and everything seems to be
working.


Thanks,
Razvan Cojocaru

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-08-07  8:09 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-06  8:57 Possible problem emulating movntq, movss Razvan Cojocaru
2014-08-06  9:22 ` Andrew Cooper
2014-08-06  9:54 ` Jan Beulich
2014-08-06 10:39   ` Razvan Cojocaru
2014-08-06 10:47   ` Andrei LUTAS
2014-08-06 11:05     ` Andrew Cooper
2014-08-06 11:22       ` Razvan Cojocaru
2014-08-06 12:16     ` Jan Beulich
2014-08-06 12:50       ` Jan Beulich
2014-08-07  8:09         ` Razvan Cojocaru
2014-08-06 12:29     ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.