Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing

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

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

On 2013-08-15 09:40, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 3:30 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>> Add testing for CR0/4 shadowing.
>>
>> A few sentences on the test strategy would be good.
>>
>>>
>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>> ---
>>>  lib/x86/vm.h    |    4 +
>>>  x86/vmx_tests.c |  218 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 222 insertions(+)
>>>
>>> diff --git a/lib/x86/vm.h b/lib/x86/vm.h
>>> index eff6f72..6e0ce2b 100644
>>> --- a/lib/x86/vm.h
>>> +++ b/lib/x86/vm.h
>>> @@ -17,9 +17,13 @@
>>>  #define PTE_ADDR    (0xffffffffff000ull)
>>>
>>>  #define X86_CR0_PE      0x00000001
>>> +#define X86_CR0_MP      0x00000002
>>> +#define X86_CR0_TS      0x00000008
>>>  #define X86_CR0_WP      0x00010000
>>>  #define X86_CR0_PG      0x80000000
>>>  #define X86_CR4_VMXE   0x00000001
>>> +#define X86_CR4_TSD     0x00000004
>>> +#define X86_CR4_DE      0x00000008
>>>  #define X86_CR4_PSE     0x00000010
>>>  #define X86_CR4_PAE     0x00000020
>>>  #define X86_CR4_PCIDE  0x00020000
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index 61b0cef..44be3f4 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -5,12 +5,18 @@
>>>
>>>  u64 ia32_pat;
>>>  u64 ia32_efer;
>>> +u32 stage;
>>>
>>>  static inline void vmcall()
>>>  {
>>>       asm volatile("vmcall");
>>>  }
>>>
>>> +static inline void set_stage(u32 s)
>>> +{
>>> +     asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>>> +}
>>> +
>>
>> Why do we need "state = s" as assembler instruction?
> This is due to assembler optimization. If we simply use "state = s",
> assembler will sometimes optimize it and state may not be set indeed.

volatile u32 stage? And we have barrier() to avoid reordering.

>>
>>>  void basic_init()
>>>  {
>>>  }
>>> @@ -257,6 +263,216 @@ static int test_ctrl_efer_exit_handler()
>>>       return VMX_TEST_VMEXIT;
>>>  }
>>>
>>> +u32 guest_cr0, guest_cr4;
>>> +
>>> +static void cr_shadowing_main()
>>> +{
>>> +     u32 cr0, cr4, tmp;
>>> +
>>> +     // Test read through
>>> +     set_stage(0);
>>> +     guest_cr0 = read_cr0();
>>> +     if (stage == 1)
>>> +             report("Read through CR0", 0);
>>> +     else
>>> +             vmcall();
>>> +     set_stage(1);
>>> +     guest_cr4 = read_cr4();
>>> +     if (stage == 2)
>>> +             report("Read through CR4", 0);
>>> +     else
>>> +             vmcall();
>>> +     // Test write through
>>> +     guest_cr0 = guest_cr0 ^ (X86_CR0_TS | X86_CR0_MP);
>>> +     guest_cr4 = guest_cr4 ^ (X86_CR4_TSD | X86_CR4_DE);
>>> +     set_stage(2);
>>> +     write_cr0(guest_cr0);
>>> +     if (stage == 3)
>>> +             report("Write throuth CR0", 0);
>>> +     else
>>> +             vmcall();
>>> +     set_stage(3);
>>> +     write_cr4(guest_cr4);
>>> +     if (stage == 4)
>>> +             report("Write through CR4", 0);
>>> +     else
>>> +             vmcall();
>>> +     // Test read shadow
>>> +     set_stage(4);
>>> +     vmcall();
>>> +     cr0 = read_cr0();
>>> +     if (stage != 5) {
>>> +             if (cr0 == guest_cr0)
>>> +                     report("Read shadowing CR0", 1);
>>> +             else
>>> +                     report("Read shadowing CR0", 0);
>>> +     }
>>> +     set_stage(5);
>>> +     cr4 = read_cr4();
>>> +     if (stage != 6) {
>>> +             if (cr4 == guest_cr4)
>>> +                     report("Read shadowing CR4", 1);
>>> +             else
>>> +                     report("Read shadowing CR4", 0);
>>> +     }
>>> +     // Test write shadow (same value with shadow)
>>> +     set_stage(6);
>>> +     write_cr0(guest_cr0);
>>> +     if (stage == 7)
>>> +             report("Write shadowing CR0 (same value with shadow)", 0);
>>> +     else
>>> +             vmcall();
>>> +     set_stage(7);
>>> +     write_cr4(guest_cr4);
>>> +     if (stage == 8)
>>> +             report("Write shadowing CR4 (same value with shadow)", 0);
>>> +     else
>>> +             vmcall();
>>> +     // Test write shadow (different value)
>>> +     set_stage(8);
>>> +     tmp = guest_cr0 ^ X86_CR0_TS;
>>> +     asm volatile("mov %0, %%rsi\n\t"
>>> +             "mov %%rsi, %%cr0\n\t"
>>> +             ::"m"(tmp)
>>> +             :"rsi", "memory", "cc");
>>> +     if (stage != 9)
>>> +             report("Write shadowing different X86_CR0_TS", 0);
>>> +     else
>>> +             report("Write shadowing different X86_CR0_TS", 1);
>>> +     set_stage(9);
>>> +     tmp = guest_cr0 ^ X86_CR0_MP;
>>> +     asm volatile("mov %0, %%rsi\n\t"
>>> +             "mov %%rsi, %%cr0\n\t"
>>> +             ::"m"(tmp)
>>> +             :"rsi", "memory", "cc");
>>> +     if (stage != 10)
>>> +             report("Write shadowing different X86_CR0_MP", 0);
>>> +     else
>>> +             report("Write shadowing different X86_CR0_MP", 1);
>>> +     set_stage(10);
>>> +     tmp = guest_cr4 ^ X86_CR4_TSD;
>>> +     asm volatile("mov %0, %%rsi\n\t"
>>> +             "mov %%rsi, %%cr4\n\t"
>>> +             ::"m"(tmp)
>>> +             :"rsi", "memory", "cc");
>>> +     if (stage != 11)
>>> +             report("Write shadowing different X86_CR4_TSD", 0);
>>> +     else
>>> +             report("Write shadowing different X86_CR4_TSD", 1);
>>> +     set_stage(11);
>>> +     tmp = guest_cr4 ^ X86_CR4_DE;
>>> +     asm volatile("mov %0, %%rsi\n\t"
>>> +             "mov %%rsi, %%cr4\n\t"
>>> +             ::"m"(tmp)
>>> +             :"rsi", "memory", "cc");
>>> +     if (stage != 12)
>>> +             report("Write shadowing different X86_CR4_DE", 0);
>>> +     else
>>> +             report("Write shadowing different X86_CR4_DE", 1);
>>> +}
>>> +
>>> +static int cr_shadowing_exit_handler()
>>> +{
>>> +     u64 guest_rip;
>>> +     ulong reason;
>>> +     u32 insn_len;
>>> +     u32 exit_qual;
>>> +
>>> +     guest_rip = vmcs_read(GUEST_RIP);
>>> +     reason = vmcs_read(EXI_REASON) & 0xff;
>>> +     insn_len = vmcs_read(EXI_INST_LEN);
>>> +     exit_qual = vmcs_read(EXI_QUALIFICATION);
>>> +     switch (reason) {
>>> +     case VMX_VMCALL:
>>> +             switch (stage) {
>>> +             case 0:
>>> +                     if (guest_cr0 == vmcs_read(GUEST_CR0))
>>> +                             report("Read through CR0", 1);
>>> +                     else
>>> +                             report("Read through CR0", 0);
>>> +                     break;
>>> +             case 1:
>>> +                     if (guest_cr4 == vmcs_read(GUEST_CR4))
>>> +                             report("Read through CR4", 1);
>>> +                     else
>>> +                             report("Read through CR4", 0);
>>> +                     break;
>>> +             case 2:
>>> +                     if (guest_cr0 == vmcs_read(GUEST_CR0))
>>> +                             report("Write through CR0", 1);
>>> +                     else
>>> +                             report("Write through CR0", 0);
>>> +                     break;
>>> +             case 3:
>>> +                     if (guest_cr4 == vmcs_read(GUEST_CR4))
>>> +                             report("Write through CR4", 1);
>>> +                     else
>>> +                             report("Write through CR4", 0);
>>> +                     break;
>>> +             case 4:
>>> +                     guest_cr0 = vmcs_read(GUEST_CR0) ^ (X86_CR0_TS | X86_CR0_MP);
>>> +                     guest_cr4 = vmcs_read(GUEST_CR4) ^ (X86_CR4_TSD | X86_CR4_DE);
>>> +                     vmcs_write(CR0_MASK, X86_CR0_TS | X86_CR0_MP);
>>> +                     vmcs_write(CR0_READ_SHADOW, guest_cr0 & (X86_CR0_TS | X86_CR0_MP));
>>> +                     vmcs_write(CR4_MASK, X86_CR4_TSD | X86_CR4_DE);
>>> +                     vmcs_write(CR4_READ_SHADOW, guest_cr4 & (X86_CR4_TSD | X86_CR4_DE));
>>> +                     break;
>>> +             case 6:
>>> +                     if (guest_cr0 == (vmcs_read(GUEST_CR0) ^ (X86_CR0_TS | X86_CR0_MP)))
>>> +                             report("Write shadowing CR0 (same value)", 1);
>>> +                     else
>>> +                             report("Write shadowing CR0 (same value)", 0);
>>> +                     break;
>>> +             case 7:
>>> +                     if (guest_cr4 == (vmcs_read(GUEST_CR4) ^ (X86_CR4_TSD | X86_CR4_DE)))
>>> +                             report("Write shadowing CR4 (same value)", 1);
>>> +                     else
>>> +                             report("Write shadowing CR4 (same value)", 0);
>>> +                     break;
>>> +             default:
>>
>> report("something went wrong", 0); ???
> Actually, we cannot reach here for any reason. If necessary, we can
> put an error message here.

It's always better to document this, specifically via code, instead of
letting the reader wonder.

>>
>>> +                     break;
>>> +             }
>>> +             vmcs_write(GUEST_RIP, guest_rip + insn_len);
>>> +             return VMX_TEST_RESUME;
>>> +     case VMX_CR:
>>> +             switch (stage) {
>>> +             case 4:
>>> +                     report("Read shadowing CR0", 0);
>>> +                     set_stage(stage + 1);
>>> +                     break;
>>> +             case 5:
>>> +                     report("Read shadowing CR4", 0);
>>> +                     set_stage(stage + 1);
>>> +                     break;
>>> +             case 6:
>>> +                     report("Write shadowing CR0 (same value)", 0);
>>> +                     set_stage(stage + 1);
>>> +                     break;
>>> +             case 7:
>>> +                     report("Write shadowing CR4 (same value)", 0);
>>> +                     set_stage(stage + 1);
>>> +                     break;
>>> +             case 8:
>>> +             case 9:
>>> +                     if (exit_qual == 0x600)
>>> +                             set_stage(stage + 1);
>>
>> What is this qualification about? Yes, ESI to CR0, but that takes a
>> manual to find out. And what if exit_qual is something else? Is that a
>> test error?
> For stage 9, it is a "MOV to CR0/4" vmexit, qualification indicates
> the detail of this exit. Take a look at the codes above of stage 9, I
> think it means a test error if exit_qual is something else.

Again, better make it clear in the code what the expected exit_qual
encodes and that any other value is an error.

Jan



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