Re: [Xen-devel] [PATCH v9 12/13] test_x86_emulator.c: Add tests for #GP usage

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 02/24/15 10:38, Jan Beulich wrote:
>>>> On 17.02.15 at 00:05, <dslutz@xxxxxxxxxxx> wrote:
>> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> 
> There's a whole lot of stuff being added here, and I easily can't see
> where delivery of a #GP would actually be tested.

Clearly I need more comments.  Short form is that j=1
is the #GP tests.

+        regs.edx    = 0x5658 + j;
...
+        if ( rc != X86EMUL_OKAY )
+        {
+            if ( j == 0 )
+                goto fail;
+        }

And the use in the real #GP handler:


+    if ( rc != X86EMUL_OKAY && rc != X86EMUL_RETRY )
+        hvm_inject_hw_exception(TRAP_gp_fault, vmcb->exitinfo1);


Since the test code and x86_emulate.c do not return
X86EMUL_RETRY it was good enough a test for me.


> Please explain
> here what the tests are supposed to test and why emulops_gp
> needs all the function pointers you're adding functions for.

Ok.  Here it is as text:

I added 2 testing "modes", j=0 and j=1.  Testing 4 instructions (all the
basic PIO) in both modes.

In j=0, there should not be an error returned.
In j=1, there should be an error returned.

For IN, eax should change.  For OUT eax should not change.

All 4 PIO instructions are 1 byte long, so eip should only change by 1.



The same as a diff with comments (and v10 code that does more checking):


+    /*
+     * Test out special #GP handling for the VMware port 0x5658.
+     * This is done in two "modes", j=0 and j=1.  Testing 4
+     * instructions (all the basic PIO) in both modes.
+     *
+     * The port used is based on j.
+     *
+     * For IN, eax should change.  For OUT eax should not change.
+     *
+     * All 4 PIO instructions are 1 byte long, so eip should only
+     * change by 1.
+     */
+    for ( j = 0; j <= 1; j++ )
+    {
+        regs.eflags = 0x20002;
+        regs.edx    = 0x5658 + j;
+        printf("Testing %s dx=%x ...       ", "in (%dx),%eax",
(int)regs.edx);
+        instr[0] = 0xed; /* in (%dx),%eax or in (%dx),%ax */
+        regs.eip    = (unsigned long)&instr[0];
+        regs.eax    = 0x12345678;
+        regs.ebx    = 0;
+        regs.ecx    = 0;
+        regs.esi    = 0;
+        rc = x86_emulate(&ctxt, &emulops_gp);
+        /*
+         * In j=0, there should not be an error returned.
+         * In j=1, there should be an error returned.
+         */
+        if ( rc != X86EMUL_OKAY )
+        {
+            if ( j == 0 )
+                goto fail;
+        }
+        else if ( j == 1 )
+            goto fail;
+        /* Check for only 1 byte used or 0 if #GP. */
+        if ( regs.eip != (unsigned long)&instr[1 - j] )
+            goto fail;
+        /* Check that eax changed in the non #GP case */
+        if ( j == 0 && regs.eax == 0x12345678 )
+            goto fail;
+        /* Check that ebx has the correct value */
+        if ( regs.ebx == j )
+            goto fail;
+        /* Check that ecx has the correct value */
+        if ( regs.ecx == j )
+            goto fail;
+        /* Check that esi has the correct value */
+        if ( regs.esi == j )
+            goto fail;
+        printf("okay\n");
+
+        printf("Testing %s  dx=%x ...       ", "in (%dx),%al",
(int)regs.edx);
+        instr[0] = 0xec; /* in (%dx),%al */
+        regs.eip    = (unsigned long)&instr[0];
+        regs.eax    = 0x12345678;
+        regs.ebx    = 0;
+        regs.ecx    = 0;
+        regs.esi    = 0;
+        rc = x86_emulate(&ctxt, &emulops_gp);
+        /*
+         * In j=0, there should not be an error returned.
+         * In j=1, there should be an error returned.
+         */
+        if ( rc != X86EMUL_OKAY )
+        {
+            if ( j == 0 )
+                goto fail;
+        }
+        else if ( j == 1 )
+            goto fail;
+        /* Check for only 1 byte used or 0 if #GP. */
+        if ( regs.eip != (unsigned long)&instr[1 - j] )
+            goto fail;
+        /* Check that eax changed in the non #GP case */
+        if ( j == 0 && regs.eax == 0x12345678 )
+            goto fail;
+        /* Check that ebx has the correct value */
+        if ( regs.ebx == j )
+            goto fail;
+        /* Check that ecx has the correct value */
+        if ( regs.ecx == j )
+            goto fail;
+        /* Check that esi has the correct value */
+        if ( regs.esi == j )
+            goto fail;
+        printf("okay\n");
+
+        printf("Testing %s dx=%x ...      ", "out %eax,(%dx)",
(int)regs.edx);
+        instr[0] = 0xef; /* out %eax,(%dx) or out %ax,(%dx) */
+        regs.eip    = (unsigned long)&instr[0];
+        regs.eax    = 0x12345678;
+        regs.ebx    = 0;
+        regs.ecx    = 0;
+        regs.esi    = 0;
+        rc = x86_emulate(&ctxt, &emulops_gp);
+        /*
+         * In j=0, there should not be an error returned.
+         * In j=1, there should be an error returned.
+         */
+        if ( rc != X86EMUL_OKAY )
+        {
+            if ( j == 0 )
+                goto fail;
+        }
+        else if ( j == 1 )
+            goto fail;
+        /* Check for only 1 byte used or 0 if #GP. */
+        if ( regs.eip != (unsigned long)&instr[1 - j] )
+            goto fail;
+        /* Check that eax did not change */
+        if ( regs.eax != 0x12345678 )
+            goto fail;
+        /* Check that ebx has the correct value */
+        if ( regs.ebx == j )
+            goto fail;
+        /* Check that ecx has the correct value */
+        if ( regs.ecx == j )
+            goto fail;
+        /* Check that esi has the correct value */
+        if ( regs.esi == j )
+            goto fail;
+        printf("okay\n");
+
+        printf("Testing %s  dx=%x ...      ", "out %al,(%dx)",
(int)regs.edx);
+        instr[0] = 0xee; /* out %al,(%dx) */
+        regs.eip    = (unsigned long)&instr[0];
+        regs.eax    = 0x12345678;
+        regs.ebx    = 0;
+        regs.ecx    = 0;
+        regs.esi    = 0;
+        rc = x86_emulate(&ctxt, &emulops_gp);
+        /*
+         * In j=0, there should not be an error returned.
+         * In j=1, there should be an error returned.
+         */
+        if ( rc != X86EMUL_OKAY )
+        {
+            if ( j == 0 )
+                goto fail;
+        }
+        else if ( j == 1 )
+            goto fail;
+        /* Check for only 1 byte used or 0 if #GP. */
+        if ( regs.eip != (unsigned long)&instr[1 - j] )
+            goto fail;
+        /* Check that eax did not change */
+        if ( regs.eax != 0x12345678 )
+            goto fail;
+        /* Check that ebx has the correct value */
+        if ( regs.ebx == j )
+            goto fail;
+        /* Check that ecx has the correct value */
+        if ( regs.ecx == j )
+            goto fail;
+        /* Check that esi has the correct value */
+        if ( regs.esi == j )
+            goto fail;
+        printf("okay\n");
+    }
+


Since it looks to me that with many cpus, it would be possible to pass
all opcodes to hvm_emulate_one_gp() by chaning memory at just the right
time.  So for emulops_gp, I had an attempt at running all the blowfish
code (32 and 64) through the #GP mode code, but it was not working and
so I dropped it after 2 days of poking.

Looks like I did not drop the no longer needed ops for the simple tests
I did add.

So I have checked and read, write_gp, cmpxchg_gp, read_segment,
and inject_hw_exception are not needed.  So that leaves:

static struct x86_emulate_ops emulops_gp = {
    .insn_fetch = fetch,
    .read_io    = read_io,
    .write_io   = write_io,
    .vmport_check = vmport_check,
};

   -Don Slutz

> 
> Jan
> 

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