Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps

[Jan Kiszka <jan.kiszka@web.de>]

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

On 2013-08-15 10:20, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 4:13 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-15 10:09, Arthur Chunqi Li wrote:
>>> On Thu, Aug 15, 2013 at 3:58 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2013-08-15 09:51, Arthur Chunqi Li wrote:
>>>>> On Thu, Aug 15, 2013 at 3:40 PM, Jan Kiszka <jan.kiszka@web.de> 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@gmail.com>
>>>>>>> ---
>>>>>>>  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.
>>
>> The point is that, according to our understanding of the SDM, we should
>> even see a trap in this wrap-around scenario if both bitmaps are cleared.
> Well, yep. I get the same understanding when I had first glance at
> SDM, but currently IO will pass if every bits cleared. This is the
> only pending problem that I asked Paolo and Gleb in a previous mail
> thread, and they are both too busy as you told me and no response
> until now :)

That is strange when looking at kvm:

static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
                                       struct vmcs12 *vmcs12)
...
                if (port < 0x8000)
                        bitmap = vmcs12->io_bitmap_a;
                else if (port < 0x10000)
                        bitmap = vmcs12->io_bitmap_b;
                else
                        return 1;

Are you testing with kvm.git next?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]