On Thu, Aug 15, 2013 at 3:58 PM, Jan Kiszka <jan.kiszka@xxxxxx> wrote:
> On 2013-08-15 09:51, Arthur Chunqi Li wrote:
>> On Thu, Aug 15, 2013 at 3:40 PM, Jan Kiszka <jan.kiszka@xxxxxx> wrote:
>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>> Add test cases for I/O bitmaps, including corner cases.
>>>
>>> Would be good to briefly list the corner cases here.
>>>
>>>>
>>>> Signed-off-by: Arthur Chunqi Li <yzt356@xxxxxxxxx>
>>>> ---
>>>> x86/vmx.h | 6 +-
>>>> x86/vmx_tests.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 170 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>> index 18961f1..dba8b20 100644
>>>> --- a/x86/vmx.h
>>>> +++ b/x86/vmx.h
>>>> @@ -417,15 +417,15 @@ enum Ctrl1 {
>>>> "popf\n\t"
>>>>
>>>> #define VMX_IO_SIZE_MASK 0x7
>>>> -#define _VMX_IO_BYTE 1
>>>> -#define _VMX_IO_WORD 2
>>>> +#define _VMX_IO_BYTE 0
>>>> +#define _VMX_IO_WORD 1
>>>> #define _VMX_IO_LONG 3
>>>> #define VMX_IO_DIRECTION_MASK (1ul << 3)
>>>> #define VMX_IO_IN (1ul << 3)
>>>> #define VMX_IO_OUT 0
>>>> #define VMX_IO_STRING (1ul << 4)
>>>> #define VMX_IO_REP (1ul << 5)
>>>> -#define VMX_IO_OPRAND_DX (1ul << 6)
>>>> +#define VMX_IO_OPRAND_IMM (1ul << 6)
>>>> #define VMX_IO_PORT_MASK 0xFFFF0000
>>>> #define VMX_IO_PORT_SHIFT 16
>>>>
>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>> index 44be3f4..ad28c4c 100644
>>>> --- a/x86/vmx_tests.c
>>>> +++ b/x86/vmx_tests.c
>>>> @@ -2,10 +2,13 @@
>>>> #include "msr.h"
>>>> #include "processor.h"
>>>> #include "vm.h"
>>>> +#include "io.h"
>>>>
>>>> u64 ia32_pat;
>>>> u64 ia32_efer;
>>>> u32 stage;
>>>> +void *io_bitmap_a, *io_bitmap_b;
>>>> +u16 ioport;
>>>>
>>>> static inline void vmcall()
>>>> {
>>>> @@ -473,6 +476,168 @@ static int cr_shadowing_exit_handler()
>>>> return VMX_TEST_VMEXIT;
>>>> }
>>>>
>>>> +static void iobmp_init()
>>>> +{
>>>> + u32 ctrl_cpu0;
>>>> +
>>>> + io_bitmap_a = alloc_page();
>>>> + io_bitmap_a = alloc_page();
>>>> + memset(io_bitmap_a, 0x0, PAGE_SIZE);
>>>> + memset(io_bitmap_b, 0x0, PAGE_SIZE);
>>>> + ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
>>>> + ctrl_cpu0 |= CPU_IO_BITMAP;
>>>> + ctrl_cpu0 &= (~CPU_IO);
>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
>>>> + vmcs_write(IO_BITMAP_A, (u64)io_bitmap_a);
>>>> + vmcs_write(IO_BITMAP_B, (u64)io_bitmap_b);
>>>> +}
>>>> +
>>>> +static void iobmp_main()
>>>> +{
>>>> +/*
>>>> + data = (u8 *)io_bitmap_b;
>>>> + ioport = 0xffff;
>>>> + data[(ioport - 0x8000) /8] |= (1 << (ioport % 8));
>>>> + inb(ioport);
>>>> + outb(0, ioport);
>>>> +*/
>>>
>>> Forgotten debug code?
>>>
>>>> + // stage 0, test IO pass
>>>> + set_stage(0);
>>>> + inb(0x5000);
>>>> + outb(0x0, 0x5000);
>>>> + if (stage != 0)
>>>> + report("I/O bitmap - I/O pass", 0);
>>>> + else
>>>> + report("I/O bitmap - I/O pass", 1);
>>>> + // test IO width, in/out
>>>> + ((u8 *)io_bitmap_a)[0] = 0xFF;
>>>> + set_stage(2);
>>>> + inb(0x0);
>>>> + if (stage != 3)
>>>> + report("I/O bitmap - trap in", 0);
>>>> + else
>>>> + report("I/O bitmap - trap in", 1);
>>>> + set_stage(3);
>>>> + outw(0x0, 0x0);
>>>> + if (stage != 4)
>>>> + report("I/O bitmap - trap out", 0);
>>>> + else
>>>> + report("I/O bitmap - trap out", 1);
>>>> + set_stage(4);
>>>> + inl(0x0);
>>>
>>> Forgot to check the progress?
>>>
>>>> + // test low/high IO port
>>>> + set_stage(5);
>>>> + ((u8 *)io_bitmap_a)[0x5000 / 8] = (1 << (0x5000 % 8));
>>>> + inb(0x5000);
>>>> + if (stage == 6)
>>>> + report("I/O bitmap - I/O port, low part", 1);
>>>> + else
>>>> + report("I/O bitmap - I/O port, low part", 0);
>>>> + set_stage(6);
>>>> + ((u8 *)io_bitmap_b)[0x1000 / 8] = (1 << (0x1000 % 8));
>>>> + inb(0x9000);
>>>> + if (stage == 7)
>>>> + report("I/O bitmap - I/O port, high part", 1);
>>>> + else
>>>> + report("I/O bitmap - I/O port, high part", 0);
>>>> + // test partial pass
>>>> + set_stage(7);
>>>> + inl(0x4FFF);
>>>> + if (stage == 8)
>>>> + report("I/O bitmap - partial pass", 1);
>>>> + else
>>>> + report("I/O bitmap - partial pass", 0);
>>>> + // test overrun
>>>> + set_stage(8);
>>>> + memset(io_bitmap_b, 0xFF, PAGE_SIZE);
>>>> + inl(0xFFFF);
>>>
>>> Let's check the expected stage also here.
>> The check is below "if (stage == 9)", the following "memset" is just
>> used to prevent I/O mask to printf.
>
> Right, there is an i/o instruction missing below after the second memset
> - or I cannot follow what you are trying to test. The above inl would
> always trigger, independent of the wrap-around. Only if you clear both
> bitmaps, we get to the "interesting" scenario. So something is still
> wrong here, no?
Yes, we need to memset io_bit_map_a to 0 here. The above inl and the
test "if (stage == 9)" are cooperatively used to test I/O overrun:
test 4 bits width "in" to 0xFFFF.
Arthur
>
>>>
>>>> + memset(io_bitmap_b, 0x0, PAGE_SIZE);
>>>
>>> Note that you still have io_bitmap_a[0] != 0 here. You probably want to
>>> clear it in order to have a clean setup.
>>>
>>>> + if (stage == 9)
>>>> + report("I/O bitmap - overrun", 1);
>>>> + else
>>>> + report("I/O bitmap - overrun", 0);
>>>> +
>>>> + return;
>>>> +}
>>>> +
>>>> +static int iobmp_exit_handler()
>>>> +{
>>>> + u64 guest_rip;
>>>> + ulong reason, exit_qual;
>>>> + u32 insn_len;
>>>> + //u32 ctrl_cpu0;
>>>> +
>>>> + guest_rip = vmcs_read(GUEST_RIP);
>>>> + reason = vmcs_read(EXI_REASON) & 0xff;
>>>> + exit_qual = vmcs_read(EXI_QUALIFICATION);
>>>> + insn_len = vmcs_read(EXI_INST_LEN);
>>>> + switch (reason) {
>>>> + case VMX_IO:
>>>> + switch (stage) {
>>>> + case 2:
>>>> + if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_BYTE)
>>>> + report("I/O bitmap - I/O width, byte", 0);
>>>> + else
>>>> + report("I/O bitmap - I/O width, byte", 1);
>>>> + if (!(exit_qual & VMX_IO_IN))
>>>> + report("I/O bitmap - I/O direction, in", 0);
>>>> + else
>>>> + report("I/O bitmap - I/O direction, in", 1);
>>>> + set_stage(stage + 1);
>>>> + break;
>>>> + case 3:
>>>> + if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_WORD)
>>>> + report("I/O bitmap - I/O width, word", 0);
>>>> + else
>>>> + report("I/O bitmap - I/O width, word", 1);
>>>> + if (!(exit_qual & VMX_IO_IN))
>>>> + report("I/O bitmap - I/O direction, out", 1);
>>>> + else
>>>> + report("I/O bitmap - I/O direction, out", 0);
>>>> + set_stage(stage + 1);
>>>> + break;
>>>> + case 4:
>>>> + if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_LONG)
>>>> + report("I/O bitmap - I/O width, long", 0);
>>>> + else
>>>> + report("I/O bitmap - I/O width, long", 1);
>>>> + set_stage(stage + 1);
>>>> + break;
>>>> + case 5:
>>>> + if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x5000)
>>>> + set_stage(stage + 1);
>>>
>>> else
>>> ...?
>> We don't need "else" here, if we don't increase "stage" here, it means
>> test fail, which is the same as if it doesn't cause vmexit.
>
> OK.
>
> Jan
>
>>>
>>> Here and below.
>>>
>>>> + break;
>>>> + case 6:
>>>> + if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x9000)
>>>> + set_stage(stage + 1);
>>>> + break;
>>>> + case 7:
>>>> + if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x4FFF)
>>>> + set_stage(stage + 1);
>>>> + //ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
>>>> + //ctrl_cpu0 &= (~CPU_IO_BITMAP);
>>>> + //vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
>>>> + break;
>>>> + case 8:
>>>> + if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0xFFFF)
>>>> + set_stage(stage + 1);
>>>> + break;
>>>> + case 0:
>>>> + case 1:
>>>> + set_stage(stage + 1);
>>>> + default:
>>>
>>> Unexpected stage?
>>>
>>>> + break;
>>>> + }
>>>> + vmcs_write(GUEST_RIP, guest_rip + insn_len);
>>>> + return VMX_TEST_RESUME;
>>>> + default:
>>>> + printf("guest_rip = 0x%llx\n", guest_rip);
>>>> + printf("\tERROR : Undefined exit reason, reason = %d.\n", reason);
>>>> + break;
>>>> + }
>>>> + return VMX_TEST_VMEXIT;
>>>> +}
>>>> +
>>>> /* name/init/guest_main/exit_handler/syscall_handler/guest_regs
>>>> basic_* just implement some basic functions */
>>>> struct vmx_test vmx_tests[] = {
>>>> @@ -486,5 +651,7 @@ struct vmx_test vmx_tests[] = {
>>>> test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
>>>> { "CR shadowing", basic_init, cr_shadowing_main,
>>>> cr_shadowing_exit_handler, basic_syscall_handler, {0} },
>>>> + { "I/O bitmap", iobmp_init, iobmp_main, iobmp_exit_handler,
>>>> + basic_syscall_handler, {0} },
>>>> { NULL, NULL, NULL, NULL, NULL, {0} },
>>>> };
>>>>
>>>
>>> Jan
>>>
>>
>>
>>
>
>
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
