* [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER
2013-08-13 15:56 [PATCH 0/4] kvm-unit-tests: Add a series of test cases Arthur Chunqi Li
@ 2013-08-13 15:56 ` Arthur Chunqi Li
2013-08-15 7:17 ` Jan Kiszka
2013-08-13 15:56 ` [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing Arthur Chunqi Li
` (2 subsequent siblings)
3 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-13 15:56 UTC (permalink / raw)
To: kvm; +Cc: jan.kiszka, gleb, pbonzini, Arthur Chunqi Li
Add test cases for ENT_LOAD_PAT, ENT_LOAD_EFER, EXI_LOAD_PAT,
EXI_SAVE_PAT, EXI_LOAD_EFER, EXI_SAVE_PAT flags in enter/exit
control fields.
Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
---
x86/vmx.h | 7 +++
x86/vmx_tests.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 192 insertions(+)
diff --git a/x86/vmx.h b/x86/vmx.h
index 28595d8..18961f1 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -152,10 +152,12 @@ enum Encoding {
GUEST_DEBUGCTL = 0x2802ul,
GUEST_DEBUGCTL_HI = 0x2803ul,
GUEST_EFER = 0x2806ul,
+ GUEST_PAT = 0x2804ul,
GUEST_PERF_GLOBAL_CTRL = 0x2808ul,
GUEST_PDPTE = 0x280aul,
/* 64-Bit Host State */
+ HOST_PAT = 0x2c00ul,
HOST_EFER = 0x2c02ul,
HOST_PERF_GLOBAL_CTRL = 0x2c04ul,
@@ -330,11 +332,15 @@ enum Ctrl_exi {
EXI_HOST_64 = 1UL << 9,
EXI_LOAD_PERF = 1UL << 12,
EXI_INTA = 1UL << 15,
+ EXI_SAVE_PAT = 1UL << 18,
+ EXI_LOAD_PAT = 1UL << 19,
+ EXI_SAVE_EFER = 1UL << 20,
EXI_LOAD_EFER = 1UL << 21,
};
enum Ctrl_ent {
ENT_GUEST_64 = 1UL << 9,
+ ENT_LOAD_PAT = 1UL << 14,
ENT_LOAD_EFER = 1UL << 15,
};
@@ -354,6 +360,7 @@ enum Ctrl0 {
CPU_NMI_WINDOW = 1ul << 22,
CPU_IO = 1ul << 24,
CPU_IO_BITMAP = 1ul << 25,
+ CPU_MSR_BITMAP = 1ul << 28,
CPU_SECONDARY = 1ul << 31,
};
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index c1b39f4..61b0cef 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -1,4 +1,15 @@
#include "vmx.h"
+#include "msr.h"
+#include "processor.h"
+#include "vm.h"
+
+u64 ia32_pat;
+u64 ia32_efer;
+
+static inline void vmcall()
+{
+ asm volatile("vmcall");
+}
void basic_init()
{
@@ -76,6 +87,176 @@ int vmenter_exit_handler()
return VMX_TEST_VMEXIT;
}
+void msr_bmp_init()
+{
+ void *msr_bitmap;
+ u32 ctrl_cpu0;
+
+ msr_bitmap = alloc_page();
+ memset(msr_bitmap, 0x0, PAGE_SIZE);
+ ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
+ ctrl_cpu0 |= CPU_MSR_BITMAP;
+ vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
+ vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
+}
+
+static void test_ctrl_pat_init()
+{
+ u64 ctrl_ent;
+ u64 ctrl_exi;
+
+ msr_bmp_init();
+ ctrl_ent = vmcs_read(ENT_CONTROLS);
+ ctrl_exi = vmcs_read(EXI_CONTROLS);
+ vmcs_write(ENT_CONTROLS, ctrl_ent | ENT_LOAD_PAT);
+ vmcs_write(EXI_CONTROLS, ctrl_exi | (EXI_SAVE_PAT | EXI_LOAD_PAT));
+ ia32_pat = rdmsr(MSR_IA32_CR_PAT);
+ vmcs_write(GUEST_PAT, 0x0);
+ vmcs_write(HOST_PAT, ia32_pat);
+}
+
+static void test_ctrl_pat_main()
+{
+ u64 guest_ia32_pat;
+
+ guest_ia32_pat = rdmsr(MSR_IA32_CR_PAT);
+ if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT))
+ printf("\tENT_LOAD_PAT is not supported.\n");
+ else {
+ if (guest_ia32_pat != 0) {
+ report("Entry load PAT", 0);
+ return;
+ }
+ }
+ wrmsr(MSR_IA32_CR_PAT, 0x6);
+ vmcall();
+ guest_ia32_pat = rdmsr(MSR_IA32_CR_PAT);
+ if (ctrl_enter_rev.clr & ENT_LOAD_PAT) {
+ if (guest_ia32_pat != ia32_pat) {
+ report("Entry load PAT", 0);
+ return;
+ }
+ report("Entry load PAT", 1);
+ }
+}
+
+static int test_ctrl_pat_exit_handler()
+{
+ u64 guest_rip;
+ ulong reason;
+ u64 guest_pat;
+
+ guest_rip = vmcs_read(GUEST_RIP);
+ reason = vmcs_read(EXI_REASON) & 0xff;
+ switch (reason) {
+ case VMX_VMCALL:
+ guest_pat = vmcs_read(GUEST_PAT);
+ if (!(ctrl_exit_rev.clr & EXI_SAVE_PAT)) {
+ printf("\tEXI_SAVE_PAT is not supported\n");
+ vmcs_write(GUEST_PAT, 0x6);
+ } else {
+ if (guest_pat == 0x6)
+ report("Exit save PAT", 1);
+ else
+ report("Exit save PAT", 0);
+ }
+ if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT))
+ printf("\tEXI_LOAD_PAT is not supported\n");
+ else {
+ if (rdmsr(MSR_IA32_CR_PAT) == ia32_pat)
+ report("Exit load PAT", 1);
+ else
+ report("Exit load PAT", 0);
+ }
+ vmcs_write(GUEST_PAT, ia32_pat);
+ vmcs_write(GUEST_RIP, guest_rip + 3);
+ return VMX_TEST_RESUME;
+ default:
+ printf("ERROR : Undefined exit reason, reason = %d.\n", reason);
+ break;
+ }
+ return VMX_TEST_VMEXIT;
+}
+
+static void test_ctrl_efer_init()
+{
+ u64 ctrl_ent;
+ u64 ctrl_exi;
+
+ msr_bmp_init();
+ ctrl_ent = vmcs_read(ENT_CONTROLS) | ENT_LOAD_EFER;
+ ctrl_exi = vmcs_read(EXI_CONTROLS) | EXI_SAVE_EFER | EXI_LOAD_EFER;
+ vmcs_write(ENT_CONTROLS, ctrl_ent & ctrl_enter_rev.clr);
+ vmcs_write(EXI_CONTROLS, ctrl_exi & ctrl_exit_rev.clr);
+ ia32_efer = rdmsr(MSR_EFER);
+ vmcs_write(GUEST_EFER, ia32_efer ^ EFER_NX);
+ vmcs_write(HOST_EFER, ia32_efer ^ EFER_NX);
+}
+
+static void test_ctrl_efer_main()
+{
+ u64 guest_ia32_efer;
+
+ guest_ia32_efer = rdmsr(MSR_EFER);
+ if (!(ctrl_enter_rev.clr & ENT_LOAD_EFER))
+ printf("\tENT_LOAD_EFER is not supported.\n");
+ else {
+ if (guest_ia32_efer != (ia32_efer ^ EFER_NX)) {
+ report("Entry load EFER", 0);
+ return;
+ }
+ }
+ wrmsr(MSR_EFER, ia32_efer);
+ vmcall();
+ guest_ia32_efer = rdmsr(MSR_EFER);
+ if (ctrl_enter_rev.clr & ENT_LOAD_EFER) {
+ if (guest_ia32_efer != ia32_efer) {
+ report("Entry load EFER", 0);
+ return;
+ }
+ report("Entry load EFER", 1);
+ }
+}
+
+static int test_ctrl_efer_exit_handler()
+{
+ u64 guest_rip;
+ ulong reason;
+ u64 guest_efer;
+
+ guest_rip = vmcs_read(GUEST_RIP);
+ reason = vmcs_read(EXI_REASON) & 0xff;
+ switch (reason) {
+ case VMX_VMCALL:
+ guest_efer = vmcs_read(GUEST_EFER);
+ if (!(ctrl_exit_rev.clr & EXI_SAVE_EFER)) {
+ printf("\tEXI_SAVE_EFER is not supported\n");
+ vmcs_write(GUEST_EFER, ia32_efer);
+ } else {
+ if (guest_efer == ia32_efer)
+ report("Exit save EFER", 1);
+ else
+ report("Exit save EFER", 0);
+ }
+ if (!(ctrl_exit_rev.clr & EXI_LOAD_EFER)) {
+ printf("\tEXI_LOAD_EFER is not supported\n");
+ wrmsr(MSR_EFER, ia32_efer ^ EFER_NX);
+ } else {
+ if (rdmsr(MSR_EFER) == (ia32_efer ^ EFER_NX))
+ report("Exit load EFER", 1);
+ else
+ report("Exit load EFER", 0);
+ }
+ vmcs_write(GUEST_PAT, ia32_efer);
+ vmcs_write(GUEST_RIP, guest_rip + 3);
+ return VMX_TEST_RESUME;
+ default:
+ printf("ERROR : 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[] = {
@@ -83,5 +264,9 @@ struct vmx_test vmx_tests[] = {
basic_syscall_handler, {0} },
{ "vmenter", basic_init, vmenter_main, vmenter_exit_handler,
basic_syscall_handler, {0} },
+ { "control field PAT", test_ctrl_pat_init, test_ctrl_pat_main,
+ test_ctrl_pat_exit_handler, basic_syscall_handler, {0} },
+ { "control field EFER", test_ctrl_efer_init, test_ctrl_efer_main,
+ test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
{ NULL, NULL, NULL, NULL, NULL, {0} },
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER
2013-08-13 15:56 ` [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER Arthur Chunqi Li
@ 2013-08-15 7:17 ` Jan Kiszka
2013-08-15 7:41 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 7:17 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, gleb, pbonzini
[-- Attachment #1: Type: text/plain, Size: 7560 bytes --]
On 2013-08-13 17:56, Arthur Chunqi Li wrote:
> Add test cases for ENT_LOAD_PAT, ENT_LOAD_EFER, EXI_LOAD_PAT,
> EXI_SAVE_PAT, EXI_LOAD_EFER, EXI_SAVE_PAT flags in enter/exit
> control fields.
>
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> ---
> x86/vmx.h | 7 +++
> x86/vmx_tests.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 192 insertions(+)
>
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 28595d8..18961f1 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -152,10 +152,12 @@ enum Encoding {
> GUEST_DEBUGCTL = 0x2802ul,
> GUEST_DEBUGCTL_HI = 0x2803ul,
> GUEST_EFER = 0x2806ul,
> + GUEST_PAT = 0x2804ul,
> GUEST_PERF_GLOBAL_CTRL = 0x2808ul,
> GUEST_PDPTE = 0x280aul,
>
> /* 64-Bit Host State */
> + HOST_PAT = 0x2c00ul,
> HOST_EFER = 0x2c02ul,
> HOST_PERF_GLOBAL_CTRL = 0x2c04ul,
>
> @@ -330,11 +332,15 @@ enum Ctrl_exi {
> EXI_HOST_64 = 1UL << 9,
> EXI_LOAD_PERF = 1UL << 12,
> EXI_INTA = 1UL << 15,
> + EXI_SAVE_PAT = 1UL << 18,
> + EXI_LOAD_PAT = 1UL << 19,
> + EXI_SAVE_EFER = 1UL << 20,
> EXI_LOAD_EFER = 1UL << 21,
> };
>
> enum Ctrl_ent {
> ENT_GUEST_64 = 1UL << 9,
> + ENT_LOAD_PAT = 1UL << 14,
> ENT_LOAD_EFER = 1UL << 15,
> };
>
> @@ -354,6 +360,7 @@ enum Ctrl0 {
> CPU_NMI_WINDOW = 1ul << 22,
> CPU_IO = 1ul << 24,
> CPU_IO_BITMAP = 1ul << 25,
> + CPU_MSR_BITMAP = 1ul << 28,
> CPU_SECONDARY = 1ul << 31,
> };
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index c1b39f4..61b0cef 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1,4 +1,15 @@
> #include "vmx.h"
> +#include "msr.h"
> +#include "processor.h"
> +#include "vm.h"
> +
> +u64 ia32_pat;
> +u64 ia32_efer;
> +
> +static inline void vmcall()
> +{
> + asm volatile("vmcall");
> +}
>
> void basic_init()
> {
> @@ -76,6 +87,176 @@ int vmenter_exit_handler()
> return VMX_TEST_VMEXIT;
> }
>
> +void msr_bmp_init()
> +{
> + void *msr_bitmap;
> + u32 ctrl_cpu0;
> +
> + msr_bitmap = alloc_page();
> + memset(msr_bitmap, 0x0, PAGE_SIZE);
> + ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
> + ctrl_cpu0 |= CPU_MSR_BITMAP;
> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
> + vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
> +}
Better safe this function for the test case where you actually stress
the bitmap.
Jan
> +
> +static void test_ctrl_pat_init()
> +{
> + u64 ctrl_ent;
> + u64 ctrl_exi;
> +
> + msr_bmp_init();
> + ctrl_ent = vmcs_read(ENT_CONTROLS);
> + ctrl_exi = vmcs_read(EXI_CONTROLS);
> + vmcs_write(ENT_CONTROLS, ctrl_ent | ENT_LOAD_PAT);
> + vmcs_write(EXI_CONTROLS, ctrl_exi | (EXI_SAVE_PAT | EXI_LOAD_PAT));
> + ia32_pat = rdmsr(MSR_IA32_CR_PAT);
> + vmcs_write(GUEST_PAT, 0x0);
> + vmcs_write(HOST_PAT, ia32_pat);
> +}
> +
> +static void test_ctrl_pat_main()
> +{
> + u64 guest_ia32_pat;
> +
> + guest_ia32_pat = rdmsr(MSR_IA32_CR_PAT);
> + if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT))
> + printf("\tENT_LOAD_PAT is not supported.\n");
> + else {
> + if (guest_ia32_pat != 0) {
> + report("Entry load PAT", 0);
> + return;
> + }
> + }
> + wrmsr(MSR_IA32_CR_PAT, 0x6);
> + vmcall();
> + guest_ia32_pat = rdmsr(MSR_IA32_CR_PAT);
> + if (ctrl_enter_rev.clr & ENT_LOAD_PAT) {
> + if (guest_ia32_pat != ia32_pat) {
> + report("Entry load PAT", 0);
> + return;
> + }
> + report("Entry load PAT", 1);
> + }
> +}
> +
> +static int test_ctrl_pat_exit_handler()
> +{
> + u64 guest_rip;
> + ulong reason;
> + u64 guest_pat;
> +
> + guest_rip = vmcs_read(GUEST_RIP);
> + reason = vmcs_read(EXI_REASON) & 0xff;
> + switch (reason) {
> + case VMX_VMCALL:
> + guest_pat = vmcs_read(GUEST_PAT);
> + if (!(ctrl_exit_rev.clr & EXI_SAVE_PAT)) {
> + printf("\tEXI_SAVE_PAT is not supported\n");
> + vmcs_write(GUEST_PAT, 0x6);
> + } else {
> + if (guest_pat == 0x6)
> + report("Exit save PAT", 1);
> + else
> + report("Exit save PAT", 0);
> + }
> + if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT))
> + printf("\tEXI_LOAD_PAT is not supported\n");
> + else {
> + if (rdmsr(MSR_IA32_CR_PAT) == ia32_pat)
> + report("Exit load PAT", 1);
> + else
> + report("Exit load PAT", 0);
> + }
> + vmcs_write(GUEST_PAT, ia32_pat);
> + vmcs_write(GUEST_RIP, guest_rip + 3);
> + return VMX_TEST_RESUME;
> + default:
> + printf("ERROR : Undefined exit reason, reason = %d.\n", reason);
> + break;
> + }
> + return VMX_TEST_VMEXIT;
> +}
> +
> +static void test_ctrl_efer_init()
> +{
> + u64 ctrl_ent;
> + u64 ctrl_exi;
> +
> + msr_bmp_init();
> + ctrl_ent = vmcs_read(ENT_CONTROLS) | ENT_LOAD_EFER;
> + ctrl_exi = vmcs_read(EXI_CONTROLS) | EXI_SAVE_EFER | EXI_LOAD_EFER;
> + vmcs_write(ENT_CONTROLS, ctrl_ent & ctrl_enter_rev.clr);
> + vmcs_write(EXI_CONTROLS, ctrl_exi & ctrl_exit_rev.clr);
> + ia32_efer = rdmsr(MSR_EFER);
> + vmcs_write(GUEST_EFER, ia32_efer ^ EFER_NX);
> + vmcs_write(HOST_EFER, ia32_efer ^ EFER_NX);
> +}
> +
> +static void test_ctrl_efer_main()
> +{
> + u64 guest_ia32_efer;
> +
> + guest_ia32_efer = rdmsr(MSR_EFER);
> + if (!(ctrl_enter_rev.clr & ENT_LOAD_EFER))
> + printf("\tENT_LOAD_EFER is not supported.\n");
> + else {
> + if (guest_ia32_efer != (ia32_efer ^ EFER_NX)) {
> + report("Entry load EFER", 0);
> + return;
> + }
> + }
> + wrmsr(MSR_EFER, ia32_efer);
> + vmcall();
> + guest_ia32_efer = rdmsr(MSR_EFER);
> + if (ctrl_enter_rev.clr & ENT_LOAD_EFER) {
> + if (guest_ia32_efer != ia32_efer) {
> + report("Entry load EFER", 0);
> + return;
> + }
> + report("Entry load EFER", 1);
> + }
> +}
> +
> +static int test_ctrl_efer_exit_handler()
> +{
> + u64 guest_rip;
> + ulong reason;
> + u64 guest_efer;
> +
> + guest_rip = vmcs_read(GUEST_RIP);
> + reason = vmcs_read(EXI_REASON) & 0xff;
> + switch (reason) {
> + case VMX_VMCALL:
> + guest_efer = vmcs_read(GUEST_EFER);
> + if (!(ctrl_exit_rev.clr & EXI_SAVE_EFER)) {
> + printf("\tEXI_SAVE_EFER is not supported\n");
> + vmcs_write(GUEST_EFER, ia32_efer);
> + } else {
> + if (guest_efer == ia32_efer)
> + report("Exit save EFER", 1);
> + else
> + report("Exit save EFER", 0);
> + }
> + if (!(ctrl_exit_rev.clr & EXI_LOAD_EFER)) {
> + printf("\tEXI_LOAD_EFER is not supported\n");
> + wrmsr(MSR_EFER, ia32_efer ^ EFER_NX);
> + } else {
> + if (rdmsr(MSR_EFER) == (ia32_efer ^ EFER_NX))
> + report("Exit load EFER", 1);
> + else
> + report("Exit load EFER", 0);
> + }
> + vmcs_write(GUEST_PAT, ia32_efer);
> + vmcs_write(GUEST_RIP, guest_rip + 3);
> + return VMX_TEST_RESUME;
> + default:
> + printf("ERROR : 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[] = {
> @@ -83,5 +264,9 @@ struct vmx_test vmx_tests[] = {
> basic_syscall_handler, {0} },
> { "vmenter", basic_init, vmenter_main, vmenter_exit_handler,
> basic_syscall_handler, {0} },
> + { "control field PAT", test_ctrl_pat_init, test_ctrl_pat_main,
> + test_ctrl_pat_exit_handler, basic_syscall_handler, {0} },
> + { "control field EFER", test_ctrl_efer_init, test_ctrl_efer_main,
> + test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
> { NULL, NULL, NULL, NULL, NULL, {0} },
> };
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER
2013-08-15 7:17 ` Jan Kiszka
@ 2013-08-15 7:41 ` Arthur Chunqi Li
2013-08-15 7:48 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 7:41 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 3:17 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>> Add test cases for ENT_LOAD_PAT, ENT_LOAD_EFER, EXI_LOAD_PAT,
>> EXI_SAVE_PAT, EXI_LOAD_EFER, EXI_SAVE_PAT flags in enter/exit
>> control fields.
>>
>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> ---
>> x86/vmx.h | 7 +++
>> x86/vmx_tests.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 192 insertions(+)
>>
>> diff --git a/x86/vmx.h b/x86/vmx.h
>> index 28595d8..18961f1 100644
>> --- a/x86/vmx.h
>> +++ b/x86/vmx.h
>> @@ -152,10 +152,12 @@ enum Encoding {
>> GUEST_DEBUGCTL = 0x2802ul,
>> GUEST_DEBUGCTL_HI = 0x2803ul,
>> GUEST_EFER = 0x2806ul,
>> + GUEST_PAT = 0x2804ul,
>> GUEST_PERF_GLOBAL_CTRL = 0x2808ul,
>> GUEST_PDPTE = 0x280aul,
>>
>> /* 64-Bit Host State */
>> + HOST_PAT = 0x2c00ul,
>> HOST_EFER = 0x2c02ul,
>> HOST_PERF_GLOBAL_CTRL = 0x2c04ul,
>>
>> @@ -330,11 +332,15 @@ enum Ctrl_exi {
>> EXI_HOST_64 = 1UL << 9,
>> EXI_LOAD_PERF = 1UL << 12,
>> EXI_INTA = 1UL << 15,
>> + EXI_SAVE_PAT = 1UL << 18,
>> + EXI_LOAD_PAT = 1UL << 19,
>> + EXI_SAVE_EFER = 1UL << 20,
>> EXI_LOAD_EFER = 1UL << 21,
>> };
>>
>> enum Ctrl_ent {
>> ENT_GUEST_64 = 1UL << 9,
>> + ENT_LOAD_PAT = 1UL << 14,
>> ENT_LOAD_EFER = 1UL << 15,
>> };
>>
>> @@ -354,6 +360,7 @@ enum Ctrl0 {
>> CPU_NMI_WINDOW = 1ul << 22,
>> CPU_IO = 1ul << 24,
>> CPU_IO_BITMAP = 1ul << 25,
>> + CPU_MSR_BITMAP = 1ul << 28,
>> CPU_SECONDARY = 1ul << 31,
>> };
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index c1b39f4..61b0cef 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -1,4 +1,15 @@
>> #include "vmx.h"
>> +#include "msr.h"
>> +#include "processor.h"
>> +#include "vm.h"
>> +
>> +u64 ia32_pat;
>> +u64 ia32_efer;
>> +
>> +static inline void vmcall()
>> +{
>> + asm volatile("vmcall");
>> +}
>>
>> void basic_init()
>> {
>> @@ -76,6 +87,176 @@ int vmenter_exit_handler()
>> return VMX_TEST_VMEXIT;
>> }
>>
>> +void msr_bmp_init()
>> +{
>> + void *msr_bitmap;
>> + u32 ctrl_cpu0;
>> +
>> + msr_bitmap = alloc_page();
>> + memset(msr_bitmap, 0x0, PAGE_SIZE);
>> + ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
>> + ctrl_cpu0 |= CPU_MSR_BITMAP;
>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
>> + vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
>> +}
>
> Better safe this function for the test case where you actually stress
> the bitmap.
What do you mean by "safe"?
Arthur
>
> Jan
>
>> +
>> +static void test_ctrl_pat_init()
>> +{
>> + u64 ctrl_ent;
>> + u64 ctrl_exi;
>> +
>> + msr_bmp_init();
>> + ctrl_ent = vmcs_read(ENT_CONTROLS);
>> + ctrl_exi = vmcs_read(EXI_CONTROLS);
>> + vmcs_write(ENT_CONTROLS, ctrl_ent | ENT_LOAD_PAT);
>> + vmcs_write(EXI_CONTROLS, ctrl_exi | (EXI_SAVE_PAT | EXI_LOAD_PAT));
>> + ia32_pat = rdmsr(MSR_IA32_CR_PAT);
>> + vmcs_write(GUEST_PAT, 0x0);
>> + vmcs_write(HOST_PAT, ia32_pat);
>> +}
>> +
>> +static void test_ctrl_pat_main()
>> +{
>> + u64 guest_ia32_pat;
>> +
>> + guest_ia32_pat = rdmsr(MSR_IA32_CR_PAT);
>> + if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT))
>> + printf("\tENT_LOAD_PAT is not supported.\n");
>> + else {
>> + if (guest_ia32_pat != 0) {
>> + report("Entry load PAT", 0);
>> + return;
>> + }
>> + }
>> + wrmsr(MSR_IA32_CR_PAT, 0x6);
>> + vmcall();
>> + guest_ia32_pat = rdmsr(MSR_IA32_CR_PAT);
>> + if (ctrl_enter_rev.clr & ENT_LOAD_PAT) {
>> + if (guest_ia32_pat != ia32_pat) {
>> + report("Entry load PAT", 0);
>> + return;
>> + }
>> + report("Entry load PAT", 1);
>> + }
>> +}
>> +
>> +static int test_ctrl_pat_exit_handler()
>> +{
>> + u64 guest_rip;
>> + ulong reason;
>> + u64 guest_pat;
>> +
>> + guest_rip = vmcs_read(GUEST_RIP);
>> + reason = vmcs_read(EXI_REASON) & 0xff;
>> + switch (reason) {
>> + case VMX_VMCALL:
>> + guest_pat = vmcs_read(GUEST_PAT);
>> + if (!(ctrl_exit_rev.clr & EXI_SAVE_PAT)) {
>> + printf("\tEXI_SAVE_PAT is not supported\n");
>> + vmcs_write(GUEST_PAT, 0x6);
>> + } else {
>> + if (guest_pat == 0x6)
>> + report("Exit save PAT", 1);
>> + else
>> + report("Exit save PAT", 0);
>> + }
>> + if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT))
>> + printf("\tEXI_LOAD_PAT is not supported\n");
>> + else {
>> + if (rdmsr(MSR_IA32_CR_PAT) == ia32_pat)
>> + report("Exit load PAT", 1);
>> + else
>> + report("Exit load PAT", 0);
>> + }
>> + vmcs_write(GUEST_PAT, ia32_pat);
>> + vmcs_write(GUEST_RIP, guest_rip + 3);
>> + return VMX_TEST_RESUME;
>> + default:
>> + printf("ERROR : Undefined exit reason, reason = %d.\n", reason);
>> + break;
>> + }
>> + return VMX_TEST_VMEXIT;
>> +}
>> +
>> +static void test_ctrl_efer_init()
>> +{
>> + u64 ctrl_ent;
>> + u64 ctrl_exi;
>> +
>> + msr_bmp_init();
>> + ctrl_ent = vmcs_read(ENT_CONTROLS) | ENT_LOAD_EFER;
>> + ctrl_exi = vmcs_read(EXI_CONTROLS) | EXI_SAVE_EFER | EXI_LOAD_EFER;
>> + vmcs_write(ENT_CONTROLS, ctrl_ent & ctrl_enter_rev.clr);
>> + vmcs_write(EXI_CONTROLS, ctrl_exi & ctrl_exit_rev.clr);
>> + ia32_efer = rdmsr(MSR_EFER);
>> + vmcs_write(GUEST_EFER, ia32_efer ^ EFER_NX);
>> + vmcs_write(HOST_EFER, ia32_efer ^ EFER_NX);
>> +}
>> +
>> +static void test_ctrl_efer_main()
>> +{
>> + u64 guest_ia32_efer;
>> +
>> + guest_ia32_efer = rdmsr(MSR_EFER);
>> + if (!(ctrl_enter_rev.clr & ENT_LOAD_EFER))
>> + printf("\tENT_LOAD_EFER is not supported.\n");
>> + else {
>> + if (guest_ia32_efer != (ia32_efer ^ EFER_NX)) {
>> + report("Entry load EFER", 0);
>> + return;
>> + }
>> + }
>> + wrmsr(MSR_EFER, ia32_efer);
>> + vmcall();
>> + guest_ia32_efer = rdmsr(MSR_EFER);
>> + if (ctrl_enter_rev.clr & ENT_LOAD_EFER) {
>> + if (guest_ia32_efer != ia32_efer) {
>> + report("Entry load EFER", 0);
>> + return;
>> + }
>> + report("Entry load EFER", 1);
>> + }
>> +}
>> +
>> +static int test_ctrl_efer_exit_handler()
>> +{
>> + u64 guest_rip;
>> + ulong reason;
>> + u64 guest_efer;
>> +
>> + guest_rip = vmcs_read(GUEST_RIP);
>> + reason = vmcs_read(EXI_REASON) & 0xff;
>> + switch (reason) {
>> + case VMX_VMCALL:
>> + guest_efer = vmcs_read(GUEST_EFER);
>> + if (!(ctrl_exit_rev.clr & EXI_SAVE_EFER)) {
>> + printf("\tEXI_SAVE_EFER is not supported\n");
>> + vmcs_write(GUEST_EFER, ia32_efer);
>> + } else {
>> + if (guest_efer == ia32_efer)
>> + report("Exit save EFER", 1);
>> + else
>> + report("Exit save EFER", 0);
>> + }
>> + if (!(ctrl_exit_rev.clr & EXI_LOAD_EFER)) {
>> + printf("\tEXI_LOAD_EFER is not supported\n");
>> + wrmsr(MSR_EFER, ia32_efer ^ EFER_NX);
>> + } else {
>> + if (rdmsr(MSR_EFER) == (ia32_efer ^ EFER_NX))
>> + report("Exit load EFER", 1);
>> + else
>> + report("Exit load EFER", 0);
>> + }
>> + vmcs_write(GUEST_PAT, ia32_efer);
>> + vmcs_write(GUEST_RIP, guest_rip + 3);
>> + return VMX_TEST_RESUME;
>> + default:
>> + printf("ERROR : 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[] = {
>> @@ -83,5 +264,9 @@ struct vmx_test vmx_tests[] = {
>> basic_syscall_handler, {0} },
>> { "vmenter", basic_init, vmenter_main, vmenter_exit_handler,
>> basic_syscall_handler, {0} },
>> + { "control field PAT", test_ctrl_pat_init, test_ctrl_pat_main,
>> + test_ctrl_pat_exit_handler, basic_syscall_handler, {0} },
>> + { "control field EFER", test_ctrl_efer_init, test_ctrl_efer_main,
>> + test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
>> { NULL, NULL, NULL, NULL, NULL, {0} },
>> };
>>
>
>
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER
2013-08-15 7:41 ` Arthur Chunqi Li
@ 2013-08-15 7:48 ` Jan Kiszka
2013-08-15 8:05 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 7:48 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]
On 2013-08-15 09:41, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 3:17 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>> Add test cases for ENT_LOAD_PAT, ENT_LOAD_EFER, EXI_LOAD_PAT,
>>> EXI_SAVE_PAT, EXI_LOAD_EFER, EXI_SAVE_PAT flags in enter/exit
>>> control fields.
>>>
>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>> ---
>>> x86/vmx.h | 7 +++
>>> x86/vmx_tests.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 192 insertions(+)
>>>
>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>> index 28595d8..18961f1 100644
>>> --- a/x86/vmx.h
>>> +++ b/x86/vmx.h
>>> @@ -152,10 +152,12 @@ enum Encoding {
>>> GUEST_DEBUGCTL = 0x2802ul,
>>> GUEST_DEBUGCTL_HI = 0x2803ul,
>>> GUEST_EFER = 0x2806ul,
>>> + GUEST_PAT = 0x2804ul,
>>> GUEST_PERF_GLOBAL_CTRL = 0x2808ul,
>>> GUEST_PDPTE = 0x280aul,
>>>
>>> /* 64-Bit Host State */
>>> + HOST_PAT = 0x2c00ul,
>>> HOST_EFER = 0x2c02ul,
>>> HOST_PERF_GLOBAL_CTRL = 0x2c04ul,
>>>
>>> @@ -330,11 +332,15 @@ enum Ctrl_exi {
>>> EXI_HOST_64 = 1UL << 9,
>>> EXI_LOAD_PERF = 1UL << 12,
>>> EXI_INTA = 1UL << 15,
>>> + EXI_SAVE_PAT = 1UL << 18,
>>> + EXI_LOAD_PAT = 1UL << 19,
>>> + EXI_SAVE_EFER = 1UL << 20,
>>> EXI_LOAD_EFER = 1UL << 21,
>>> };
>>>
>>> enum Ctrl_ent {
>>> ENT_GUEST_64 = 1UL << 9,
>>> + ENT_LOAD_PAT = 1UL << 14,
>>> ENT_LOAD_EFER = 1UL << 15,
>>> };
>>>
>>> @@ -354,6 +360,7 @@ enum Ctrl0 {
>>> CPU_NMI_WINDOW = 1ul << 22,
>>> CPU_IO = 1ul << 24,
>>> CPU_IO_BITMAP = 1ul << 25,
>>> + CPU_MSR_BITMAP = 1ul << 28,
>>> CPU_SECONDARY = 1ul << 31,
>>> };
>>>
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index c1b39f4..61b0cef 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -1,4 +1,15 @@
>>> #include "vmx.h"
>>> +#include "msr.h"
>>> +#include "processor.h"
>>> +#include "vm.h"
>>> +
>>> +u64 ia32_pat;
>>> +u64 ia32_efer;
>>> +
>>> +static inline void vmcall()
>>> +{
>>> + asm volatile("vmcall");
>>> +}
>>>
>>> void basic_init()
>>> {
>>> @@ -76,6 +87,176 @@ int vmenter_exit_handler()
>>> return VMX_TEST_VMEXIT;
>>> }
>>>
>>> +void msr_bmp_init()
>>> +{
>>> + void *msr_bitmap;
>>> + u32 ctrl_cpu0;
>>> +
>>> + msr_bitmap = alloc_page();
>>> + memset(msr_bitmap, 0x0, PAGE_SIZE);
>>> + ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
>>> + ctrl_cpu0 |= CPU_MSR_BITMAP;
>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
>>> + vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
>>> +}
>>
>> Better safe this function for the test case where you actually stress
>> the bitmap.
> What do you mean by "safe"?
I meant the other "save": This function serves no purpose here. Let's
only introduce it when that changes, i.e. when you actually test the MSR
bitmap.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER
2013-08-15 7:48 ` Jan Kiszka
@ 2013-08-15 8:05 ` Arthur Chunqi Li
2013-08-15 8:09 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 8:05 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 3:48 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2013-08-15 09:41, Arthur Chunqi Li wrote:
>> On Thu, Aug 15, 2013 at 3:17 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>> Add test cases for ENT_LOAD_PAT, ENT_LOAD_EFER, EXI_LOAD_PAT,
>>>> EXI_SAVE_PAT, EXI_LOAD_EFER, EXI_SAVE_PAT flags in enter/exit
>>>> control fields.
>>>>
>>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>>> ---
>>>> x86/vmx.h | 7 +++
>>>> x86/vmx_tests.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 192 insertions(+)
>>>>
>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>> index 28595d8..18961f1 100644
>>>> --- a/x86/vmx.h
>>>> +++ b/x86/vmx.h
>>>> @@ -152,10 +152,12 @@ enum Encoding {
>>>> GUEST_DEBUGCTL = 0x2802ul,
>>>> GUEST_DEBUGCTL_HI = 0x2803ul,
>>>> GUEST_EFER = 0x2806ul,
>>>> + GUEST_PAT = 0x2804ul,
>>>> GUEST_PERF_GLOBAL_CTRL = 0x2808ul,
>>>> GUEST_PDPTE = 0x280aul,
>>>>
>>>> /* 64-Bit Host State */
>>>> + HOST_PAT = 0x2c00ul,
>>>> HOST_EFER = 0x2c02ul,
>>>> HOST_PERF_GLOBAL_CTRL = 0x2c04ul,
>>>>
>>>> @@ -330,11 +332,15 @@ enum Ctrl_exi {
>>>> EXI_HOST_64 = 1UL << 9,
>>>> EXI_LOAD_PERF = 1UL << 12,
>>>> EXI_INTA = 1UL << 15,
>>>> + EXI_SAVE_PAT = 1UL << 18,
>>>> + EXI_LOAD_PAT = 1UL << 19,
>>>> + EXI_SAVE_EFER = 1UL << 20,
>>>> EXI_LOAD_EFER = 1UL << 21,
>>>> };
>>>>
>>>> enum Ctrl_ent {
>>>> ENT_GUEST_64 = 1UL << 9,
>>>> + ENT_LOAD_PAT = 1UL << 14,
>>>> ENT_LOAD_EFER = 1UL << 15,
>>>> };
>>>>
>>>> @@ -354,6 +360,7 @@ enum Ctrl0 {
>>>> CPU_NMI_WINDOW = 1ul << 22,
>>>> CPU_IO = 1ul << 24,
>>>> CPU_IO_BITMAP = 1ul << 25,
>>>> + CPU_MSR_BITMAP = 1ul << 28,
>>>> CPU_SECONDARY = 1ul << 31,
>>>> };
>>>>
>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>> index c1b39f4..61b0cef 100644
>>>> --- a/x86/vmx_tests.c
>>>> +++ b/x86/vmx_tests.c
>>>> @@ -1,4 +1,15 @@
>>>> #include "vmx.h"
>>>> +#include "msr.h"
>>>> +#include "processor.h"
>>>> +#include "vm.h"
>>>> +
>>>> +u64 ia32_pat;
>>>> +u64 ia32_efer;
>>>> +
>>>> +static inline void vmcall()
>>>> +{
>>>> + asm volatile("vmcall");
>>>> +}
>>>>
>>>> void basic_init()
>>>> {
>>>> @@ -76,6 +87,176 @@ int vmenter_exit_handler()
>>>> return VMX_TEST_VMEXIT;
>>>> }
>>>>
>>>> +void msr_bmp_init()
>>>> +{
>>>> + void *msr_bitmap;
>>>> + u32 ctrl_cpu0;
>>>> +
>>>> + msr_bitmap = alloc_page();
>>>> + memset(msr_bitmap, 0x0, PAGE_SIZE);
>>>> + ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
>>>> + ctrl_cpu0 |= CPU_MSR_BITMAP;
>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
>>>> + vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
>>>> +}
>>>
>>> Better safe this function for the test case where you actually stress
>>> the bitmap.
>> What do you mean by "safe"?
>
> I meant the other "save": This function serves no purpose here. Let's
> only introduce it when that changes, i.e. when you actually test the MSR
> bitmap.
No, the function is meaningful here. We need directly access to MSRs
in guest and if msr bitmap is not set, any access to MSRs will cause
vmexit. Here we just let all rdmsr/wrmsr pass in guest.
Arthur
>
> Jan
>
>
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER
2013-08-15 8:05 ` Arthur Chunqi Li
@ 2013-08-15 8:09 ` Jan Kiszka
0 siblings, 0 replies; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:09 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3843 bytes --]
On 2013-08-15 10:05, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 3:48 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-15 09:41, Arthur Chunqi Li wrote:
>>> On Thu, Aug 15, 2013 at 3:17 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>>> Add test cases for ENT_LOAD_PAT, ENT_LOAD_EFER, EXI_LOAD_PAT,
>>>>> EXI_SAVE_PAT, EXI_LOAD_EFER, EXI_SAVE_PAT flags in enter/exit
>>>>> control fields.
>>>>>
>>>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>>>> ---
>>>>> x86/vmx.h | 7 +++
>>>>> x86/vmx_tests.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 192 insertions(+)
>>>>>
>>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>>> index 28595d8..18961f1 100644
>>>>> --- a/x86/vmx.h
>>>>> +++ b/x86/vmx.h
>>>>> @@ -152,10 +152,12 @@ enum Encoding {
>>>>> GUEST_DEBUGCTL = 0x2802ul,
>>>>> GUEST_DEBUGCTL_HI = 0x2803ul,
>>>>> GUEST_EFER = 0x2806ul,
>>>>> + GUEST_PAT = 0x2804ul,
>>>>> GUEST_PERF_GLOBAL_CTRL = 0x2808ul,
>>>>> GUEST_PDPTE = 0x280aul,
>>>>>
>>>>> /* 64-Bit Host State */
>>>>> + HOST_PAT = 0x2c00ul,
>>>>> HOST_EFER = 0x2c02ul,
>>>>> HOST_PERF_GLOBAL_CTRL = 0x2c04ul,
>>>>>
>>>>> @@ -330,11 +332,15 @@ enum Ctrl_exi {
>>>>> EXI_HOST_64 = 1UL << 9,
>>>>> EXI_LOAD_PERF = 1UL << 12,
>>>>> EXI_INTA = 1UL << 15,
>>>>> + EXI_SAVE_PAT = 1UL << 18,
>>>>> + EXI_LOAD_PAT = 1UL << 19,
>>>>> + EXI_SAVE_EFER = 1UL << 20,
>>>>> EXI_LOAD_EFER = 1UL << 21,
>>>>> };
>>>>>
>>>>> enum Ctrl_ent {
>>>>> ENT_GUEST_64 = 1UL << 9,
>>>>> + ENT_LOAD_PAT = 1UL << 14,
>>>>> ENT_LOAD_EFER = 1UL << 15,
>>>>> };
>>>>>
>>>>> @@ -354,6 +360,7 @@ enum Ctrl0 {
>>>>> CPU_NMI_WINDOW = 1ul << 22,
>>>>> CPU_IO = 1ul << 24,
>>>>> CPU_IO_BITMAP = 1ul << 25,
>>>>> + CPU_MSR_BITMAP = 1ul << 28,
>>>>> CPU_SECONDARY = 1ul << 31,
>>>>> };
>>>>>
>>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>>> index c1b39f4..61b0cef 100644
>>>>> --- a/x86/vmx_tests.c
>>>>> +++ b/x86/vmx_tests.c
>>>>> @@ -1,4 +1,15 @@
>>>>> #include "vmx.h"
>>>>> +#include "msr.h"
>>>>> +#include "processor.h"
>>>>> +#include "vm.h"
>>>>> +
>>>>> +u64 ia32_pat;
>>>>> +u64 ia32_efer;
>>>>> +
>>>>> +static inline void vmcall()
>>>>> +{
>>>>> + asm volatile("vmcall");
>>>>> +}
>>>>>
>>>>> void basic_init()
>>>>> {
>>>>> @@ -76,6 +87,176 @@ int vmenter_exit_handler()
>>>>> return VMX_TEST_VMEXIT;
>>>>> }
>>>>>
>>>>> +void msr_bmp_init()
>>>>> +{
>>>>> + void *msr_bitmap;
>>>>> + u32 ctrl_cpu0;
>>>>> +
>>>>> + msr_bitmap = alloc_page();
>>>>> + memset(msr_bitmap, 0x0, PAGE_SIZE);
>>>>> + ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
>>>>> + ctrl_cpu0 |= CPU_MSR_BITMAP;
>>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
>>>>> + vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
>>>>> +}
>>>>
>>>> Better safe this function for the test case where you actually stress
>>>> the bitmap.
>>> What do you mean by "safe"?
>>
>> I meant the other "save": This function serves no purpose here. Let's
>> only introduce it when that changes, i.e. when you actually test the MSR
>> bitmap.
> No, the function is meaningful here. We need directly access to MSRs
> in guest and if msr bitmap is not set, any access to MSRs will cause
> vmexit. Here we just let all rdmsr/wrmsr pass in guest.
Ah, sorry. Forgot that the default is "trap", not "pass".
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-13 15:56 [PATCH 0/4] kvm-unit-tests: Add a series of test cases Arthur Chunqi Li
2013-08-13 15:56 ` [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER Arthur Chunqi Li
@ 2013-08-13 15:56 ` Arthur Chunqi Li
2013-08-15 7:30 ` Jan Kiszka
2013-08-13 15:56 ` [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps Arthur Chunqi Li
2013-08-13 15:56 ` [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception Arthur Chunqi Li
3 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-13 15:56 UTC (permalink / raw)
To: kvm; +Cc: jan.kiszka, gleb, pbonzini, Arthur Chunqi Li
Add testing for CR0/4 shadowing.
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");
+}
+
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:
+ 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);
+ break;
+ case 10:
+ case 11:
+ if (exit_qual == 0x604)
+ set_stage(stage + 1);
+ default:
+ break;
+ }
+ vmcs_write(GUEST_RIP, guest_rip + insn_len);
+ return VMX_TEST_RESUME;
+ default:
+ printf("Unknown exit reason, %d\n", reason);
+ print_vmexit_info();
+ }
+ 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[] = {
@@ -268,5 +484,7 @@ struct vmx_test vmx_tests[] = {
test_ctrl_pat_exit_handler, basic_syscall_handler, {0} },
{ "control field EFER", test_ctrl_efer_init, test_ctrl_efer_main,
test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
+ { "CR shadowing", basic_init, cr_shadowing_main,
+ cr_shadowing_exit_handler, basic_syscall_handler, {0} },
{ NULL, NULL, NULL, NULL, NULL, {0} },
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-13 15:56 ` [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing Arthur Chunqi Li
@ 2013-08-15 7:30 ` Jan Kiszka
2013-08-15 7:40 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 7:30 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, gleb, pbonzini
[-- Attachment #1: Type: text/plain, Size: 8260 bytes --]
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?
> 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); ???
> + 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?
> + break;
> + case 10:
> + case 11:
> + if (exit_qual == 0x604)
> + set_stage(stage + 1);
Same as above.
> + default:
...?
> + break;
> + }
> + vmcs_write(GUEST_RIP, guest_rip + insn_len);
> + return VMX_TEST_RESUME;
> + default:
> + printf("Unknown exit reason, %d\n", reason);
> + print_vmexit_info();
> + }
> + 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[] = {
> @@ -268,5 +484,7 @@ struct vmx_test vmx_tests[] = {
> test_ctrl_pat_exit_handler, basic_syscall_handler, {0} },
> { "control field EFER", test_ctrl_efer_init, test_ctrl_efer_main,
> test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
> + { "CR shadowing", basic_init, cr_shadowing_main,
> + cr_shadowing_exit_handler, basic_syscall_handler, {0} },
> { NULL, NULL, NULL, NULL, NULL, {0} },
> };
>
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-15 7:30 ` Jan Kiszka
@ 2013-08-15 7:40 ` Arthur Chunqi Li
2013-08-15 7:47 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 7:40 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
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.
>
>> 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.
>
>> + 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.
Arthur
>
>> + break;
>> + case 10:
>> + case 11:
>> + if (exit_qual == 0x604)
>> + set_stage(stage + 1);
>
> Same as above.
>
>> + default:
>
> ...?
>
>> + break;
>> + }
>> + vmcs_write(GUEST_RIP, guest_rip + insn_len);
>> + return VMX_TEST_RESUME;
>> + default:
>> + printf("Unknown exit reason, %d\n", reason);
>> + print_vmexit_info();
>> + }
>> + 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[] = {
>> @@ -268,5 +484,7 @@ struct vmx_test vmx_tests[] = {
>> test_ctrl_pat_exit_handler, basic_syscall_handler, {0} },
>> { "control field EFER", test_ctrl_efer_init, test_ctrl_efer_main,
>> test_ctrl_efer_exit_handler, basic_syscall_handler, {0} },
>> + { "CR shadowing", basic_init, cr_shadowing_main,
>> + cr_shadowing_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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-15 7:40 ` Arthur Chunqi Li
@ 2013-08-15 7:47 ` Jan Kiszka
2013-08-15 7:59 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 7:47 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-15 7:47 ` Jan Kiszka
@ 2013-08-15 7:59 ` Arthur Chunqi Li
2013-08-15 8:07 ` Jan Kiszka
2013-08-18 14:07 ` Paolo Bonzini
0 siblings, 2 replies; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 7:59 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 3:47 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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.
Reordering here is not a big deal here, though it is actually needed
here. I occurred the following problem:
stage = 1;
do something that causes vmexit;
stage = 2;
Then the compiler will optimize "stage = 1" and "stage = 2" to one
instruction "stage =2", since instructions between them don't use
"stage". Can volatile solve this problem?
Arthur
>
>>>
>>>> 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
>
>
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-15 7:59 ` Arthur Chunqi Li
@ 2013-08-15 8:07 ` Jan Kiszka
2013-08-18 14:07 ` Paolo Bonzini
1 sibling, 0 replies; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:07 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]
On 2013-08-15 09:59, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 3:47 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> 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.
> Reordering here is not a big deal here, though it is actually needed
> here. I occurred the following problem:
>
> stage = 1;
> do something that causes vmexit;
> stage = 2;
>
> Then the compiler will optimize "stage = 1" and "stage = 2" to one
> instruction "stage =2", since instructions between them don't use
> "stage". Can volatile solve this problem?
Yep.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-15 7:59 ` Arthur Chunqi Li
2013-08-15 8:07 ` Jan Kiszka
@ 2013-08-18 14:07 ` Paolo Bonzini
2013-08-18 14:32 ` Gmail
1 sibling, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2013-08-18 14:07 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: Jan Kiszka, kvm, Gleb Natapov
Il 15/08/2013 09:59, Arthur Chunqi Li ha scritto:
>> > volatile u32 stage? And we have barrier() to avoid reordering.
> Reordering here is not a big deal here, though it is actually needed
> here. I occurred the following problem:
>
> stage = 1;
> do something that causes vmexit;
> stage = 2;
>
> Then the compiler will optimize "stage = 1" and "stage = 2" to one
> instruction "stage =2", since instructions between them don't use
> "stage". Can volatile solve this problem?
I'm not sure if volatile stores are reordered against non-volatile stores.
Better keep set_stage() but write it as
barrier();
stage = s;
barrier();
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing
2013-08-18 14:07 ` Paolo Bonzini
@ 2013-08-18 14:32 ` Gmail
0 siblings, 0 replies; 32+ messages in thread
From: Gmail @ 2013-08-18 14:32 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Jan Kiszka, kvm, Gleb Natapov
在 2013-8-18，22:07，Paolo Bonzini <pbonzini@redhat.com> 写道：
> Il 15/08/2013 09:59, Arthur Chunqi Li ha scritto:
>>>> volatile u32 stage? And we have barrier() to avoid reordering.
>> Reordering here is not a big deal here, though it is actually needed
>> here. I occurred the following problem:
>>
>> stage = 1;
>> do something that causes vmexit;
>> stage = 2;
>>
>> Then the compiler will optimize "stage = 1" and "stage = 2" to one
>> instruction "stage =2", since instructions between them don't use
>> "stage". Can volatile solve this problem?
>
> I'm not sure if volatile stores are reordered against non-volatile stores.
>
> Better keep set_stage() but write it as
>
> barrier();
> stage = s;
> barrier();
Yep. I have done it like this in the 2nd version.
Arthur
>
> Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-13 15:56 [PATCH 0/4] kvm-unit-tests: Add a series of test cases Arthur Chunqi Li
2013-08-13 15:56 ` [PATCH 1/4] kvm-unit-tests: VMX: Add test cases for PAT and EFER Arthur Chunqi Li
2013-08-13 15:56 ` [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing Arthur Chunqi Li
@ 2013-08-13 15:56 ` Arthur Chunqi Li
2013-08-15 7:40 ` Jan Kiszka
2013-08-13 15:56 ` [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception Arthur Chunqi Li
3 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-13 15:56 UTC (permalink / raw)
To: kvm; +Cc: jan.kiszka, gleb, pbonzini, Arthur Chunqi Li
Add test cases for I/O bitmaps, including corner cases.
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);
+*/
+ // 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);
+ // 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);
+ memset(io_bitmap_b, 0x0, PAGE_SIZE);
+ 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);
+ 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:
+ 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} },
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-13 15:56 ` [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps Arthur Chunqi Li
@ 2013-08-15 7:40 ` Jan Kiszka
2013-08-15 7:51 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 7:40 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, gleb, pbonzini
[-- Attachment #1: Type: text/plain, Size: 7098 bytes --]
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.
> + 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
...?
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 7:40 ` Jan Kiszka
@ 2013-08-15 7:51 ` Arthur Chunqi Li
2013-08-15 7:58 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 7:51 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
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.
>
>> + 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.
>
> 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 7:51 ` Arthur Chunqi Li
@ 2013-08-15 7:58 ` Jan Kiszka
2013-08-15 8:09 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 7:58 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 10080 bytes --]
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?
>>
>>> + 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
>>
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 7:58 ` Jan Kiszka
@ 2013-08-15 8:09 ` Arthur Chunqi Li
2013-08-15 8:13 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 8:09 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
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.
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 8:09 ` Arthur Chunqi Li
@ 2013-08-15 8:13 ` Jan Kiszka
2013-08-15 8:20 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:13 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 5894 bytes --]
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.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 8:13 ` Jan Kiszka
@ 2013-08-15 8:20 ` Arthur Chunqi Li
2013-08-15 8:23 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 8:20 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
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 :)
Arthur
>
> Jan
>
>
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 8:20 ` Arthur Chunqi Li
@ 2013-08-15 8:23 ` Jan Kiszka
2013-08-15 10:43 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:23 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps
2013-08-15 8:23 ` Jan Kiszka
@ 2013-08-15 10:43 ` Arthur Chunqi Li
0 siblings, 0 replies; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 10:43 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 4:23 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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?
Sorry, there's something wrong with my previous test. It is now acts
as what we expect.
Arthur
>
> Jan
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-13 15:56 [PATCH 0/4] kvm-unit-tests: Add a series of test cases Arthur Chunqi Li
` (2 preceding siblings ...)
2013-08-13 15:56 ` [PATCH 3/4] kvm-unit-tests: VMX: Add test cases for I/O bitmaps Arthur Chunqi Li
@ 2013-08-13 15:56 ` Arthur Chunqi Li
2013-08-15 8:06 ` Jan Kiszka
3 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-13 15:56 UTC (permalink / raw)
To: kvm; +Cc: jan.kiszka, gleb, pbonzini, Arthur Chunqi Li
Add test cases for instruction interception, including three types:
1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
RDPMC/RDTSC/MONITOR/PAUSE)
2. Secondary Processor-Based VM-Execution Controls (WBINVD)
3. No control flag (CPUID/INVD)
Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
---
x86/vmx.c | 3 +-
x86/vmx.h | 7 ++++
x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 125 insertions(+), 2 deletions(-)
diff --git a/x86/vmx.c b/x86/vmx.c
index ca36d35..c346070 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -336,8 +336,7 @@ static void init_vmx(void)
: MSR_IA32_VMX_ENTRY_CTLS);
ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
: MSR_IA32_VMX_PROCBASED_CTLS);
- if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
- ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
+ ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
diff --git a/x86/vmx.h b/x86/vmx.h
index dba8b20..d81d25d 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -354,6 +354,9 @@ enum Ctrl0 {
CPU_INTR_WINDOW = 1ul << 2,
CPU_HLT = 1ul << 7,
CPU_INVLPG = 1ul << 9,
+ CPU_MWAIT = 1ul << 10,
+ CPU_RDPMC = 1ul << 11,
+ CPU_RDTSC = 1ul << 12,
CPU_CR3_LOAD = 1ul << 15,
CPU_CR3_STORE = 1ul << 16,
CPU_TPR_SHADOW = 1ul << 21,
@@ -361,6 +364,8 @@ enum Ctrl0 {
CPU_IO = 1ul << 24,
CPU_IO_BITMAP = 1ul << 25,
CPU_MSR_BITMAP = 1ul << 28,
+ CPU_MONITOR = 1ul << 29,
+ CPU_PAUSE = 1ul << 30,
CPU_SECONDARY = 1ul << 31,
};
@@ -368,6 +373,8 @@ enum Ctrl1 {
CPU_EPT = 1ul << 1,
CPU_VPID = 1ul << 5,
CPU_URG = 1ul << 7,
+ CPU_WBINVD = 1ul << 6,
+ CPU_RDRAND = 1ul << 11,
};
#define SAVE_GPR \
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ad28c4c..66187f4 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
}
+static inline u32 get_stage()
+{
+ u32 s;
+ asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
+ return s;
+}
+
void basic_init()
{
}
@@ -638,6 +645,114 @@ static int iobmp_exit_handler()
return VMX_TEST_VMEXIT;
}
+asm(
+ "insn_hlt: hlt;ret\n\t"
+ "insn_invlpg: invlpg 0x12345678;ret\n\t"
+ "insn_mwait: mwait;ret\n\t"
+ "insn_rdpmc: rdpmc;ret\n\t"
+ "insn_rdtsc: rdtsc;ret\n\t"
+ "insn_monitor: monitor;ret\n\t"
+ "insn_pause: pause;ret\n\t"
+ "insn_wbinvd: wbinvd;ret\n\t"
+ "insn_cpuid: cpuid;ret\n\t"
+ "insn_invd: invd;ret\n\t"
+);
+extern void insn_hlt();
+extern void insn_invlpg();
+extern void insn_mwait();
+extern void insn_rdpmc();
+extern void insn_rdtsc();
+extern void insn_monitor();
+extern void insn_pause();
+extern void insn_wbinvd();
+extern void insn_cpuid();
+extern void insn_invd();
+
+u32 cur_insn;
+
+struct insn_table {
+ const char *name;
+ u32 flag;
+ void (*insn_func)();
+ u32 type;
+ u32 reason;
+ ulong exit_qual;
+ u32 insn_info;
+};
+
+static struct insn_table insn_table[] = {
+ // Flags for Primary Processor-Based VM-Execution Controls
+ {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
+ {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
+ {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
+ {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
+ {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
+ {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
+ {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
+ // Flags for Secondary Processor-Based VM-Execution Controls
+ {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
+ // Flags for Non-Processor-Based
+ {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
+ {"INVD", 0, insn_invd, 2, 13, 0, 0},
+ {NULL},
+};
+
+static void insn_intercept_init()
+{
+ u32 ctrl_cpu[2];
+
+ ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
+ ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
+ CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
+ ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
+ vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
+ ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
+ ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
+ ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
+ vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
+}
+
+static void insn_intercept_main()
+{
+ cur_insn = 0;
+ while(insn_table[cur_insn].name != NULL) {
+ set_stage(cur_insn);
+ if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
+ (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
+ printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
+ } else {
+ insn_table[cur_insn].insn_func();
+ if (stage != cur_insn + 1)
+ report(insn_table[cur_insn].name, 0);
+ }
+ cur_insn ++;
+ }
+}
+
+static int insn_intercept_exit_handler()
+{
+ u64 guest_rip;
+ u32 reason;
+ ulong exit_qual;
+ u32 insn_len;
+ u32 insn_info;
+
+ 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);
+ insn_info = vmcs_read(EXI_INST_INFO);
+ if (cur_insn == get_stage() &&
+ insn_table[cur_insn].reason == reason &&
+ insn_table[cur_insn].exit_qual == exit_qual &&
+ insn_table[cur_insn].insn_info == insn_info) {
+ report(insn_table[cur_insn].name, 1);
+ set_stage(stage + 1);
+ }
+ vmcs_write(GUEST_RIP, guest_rip + insn_len);
+ return VMX_TEST_RESUME;
+}
+
/* name/init/guest_main/exit_handler/syscall_handler/guest_regs
basic_* just implement some basic functions */
struct vmx_test vmx_tests[] = {
@@ -653,5 +768,7 @@ struct vmx_test vmx_tests[] = {
cr_shadowing_exit_handler, basic_syscall_handler, {0} },
{ "I/O bitmap", iobmp_init, iobmp_main, iobmp_exit_handler,
basic_syscall_handler, {0} },
+ { "instruction intercept", insn_intercept_init, insn_intercept_main,
+ insn_intercept_exit_handler, basic_syscall_handler, {0} },
{ NULL, NULL, NULL, NULL, NULL, {0} },
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-13 15:56 ` [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception Arthur Chunqi Li
@ 2013-08-15 8:06 ` Jan Kiszka
2013-08-15 8:16 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:06 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, gleb, pbonzini
[-- Attachment #1: Type: text/plain, Size: 7013 bytes --]
On 2013-08-13 17:56, Arthur Chunqi Li wrote:
> Add test cases for instruction interception, including three types:
> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
> RDPMC/RDTSC/MONITOR/PAUSE)
> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
> 3. No control flag (CPUID/INVD)
>
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> ---
> x86/vmx.c | 3 +-
> x86/vmx.h | 7 ++++
> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index ca36d35..c346070 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -336,8 +336,7 @@ static void init_vmx(void)
> : MSR_IA32_VMX_ENTRY_CTLS);
> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
> : MSR_IA32_VMX_PROCBASED_CTLS);
> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>
> diff --git a/x86/vmx.h b/x86/vmx.h
> index dba8b20..d81d25d 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -354,6 +354,9 @@ enum Ctrl0 {
> CPU_INTR_WINDOW = 1ul << 2,
> CPU_HLT = 1ul << 7,
> CPU_INVLPG = 1ul << 9,
> + CPU_MWAIT = 1ul << 10,
> + CPU_RDPMC = 1ul << 11,
> + CPU_RDTSC = 1ul << 12,
> CPU_CR3_LOAD = 1ul << 15,
> CPU_CR3_STORE = 1ul << 16,
> CPU_TPR_SHADOW = 1ul << 21,
> @@ -361,6 +364,8 @@ enum Ctrl0 {
> CPU_IO = 1ul << 24,
> CPU_IO_BITMAP = 1ul << 25,
> CPU_MSR_BITMAP = 1ul << 28,
> + CPU_MONITOR = 1ul << 29,
> + CPU_PAUSE = 1ul << 30,
> CPU_SECONDARY = 1ul << 31,
> };
>
> @@ -368,6 +373,8 @@ enum Ctrl1 {
> CPU_EPT = 1ul << 1,
> CPU_VPID = 1ul << 5,
> CPU_URG = 1ul << 7,
> + CPU_WBINVD = 1ul << 6,
> + CPU_RDRAND = 1ul << 11,
> };
>
> #define SAVE_GPR \
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index ad28c4c..66187f4 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
> }
>
> +static inline u32 get_stage()
> +{
> + u32 s;
> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
> + return s;
> +}
Tagging "stage" volatile will obsolete this special assembly.
> +
> void basic_init()
> {
> }
> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
> return VMX_TEST_VMEXIT;
> }
>
> +asm(
> + "insn_hlt: hlt;ret\n\t"
> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
> + "insn_mwait: mwait;ret\n\t"
> + "insn_rdpmc: rdpmc;ret\n\t"
> + "insn_rdtsc: rdtsc;ret\n\t"
> + "insn_monitor: monitor;ret\n\t"
> + "insn_pause: pause;ret\n\t"
> + "insn_wbinvd: wbinvd;ret\n\t"
> + "insn_cpuid: cpuid;ret\n\t"
> + "insn_invd: invd;ret\n\t"
> +);
> +extern void insn_hlt();
> +extern void insn_invlpg();
> +extern void insn_mwait();
> +extern void insn_rdpmc();
> +extern void insn_rdtsc();
> +extern void insn_monitor();
> +extern void insn_pause();
> +extern void insn_wbinvd();
> +extern void insn_cpuid();
> +extern void insn_invd();
> +
> +u32 cur_insn;
> +
> +struct insn_table {
> + const char *name;
> + u32 flag;
> + void (*insn_func)();
> + u32 type;
What do the "type" values mean?
> + u32 reason;
> + ulong exit_qual;
> + u32 insn_info;
For none of the instructions you test, EXI_INST_INFO will have valid
content on exit. So you must not check it anyway.
> +};
> +
> +static struct insn_table insn_table[] = {
> + // Flags for Primary Processor-Based VM-Execution Controls
> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
> + // Flags for Secondary Processor-Based VM-Execution Controls
> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
> + // Flags for Non-Processor-Based
> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
> + {"INVD", 0, insn_invd, 2, 13, 0, 0},
> + {NULL},
> +};
> +
> +static void insn_intercept_init()
> +{
> + u32 ctrl_cpu[2];
> +
> + ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
> +}
> +
> +static void insn_intercept_main()
> +{
> + cur_insn = 0;
> + while(insn_table[cur_insn].name != NULL) {
> + set_stage(cur_insn);
> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
> + (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
> + printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
Coding style, specifically overlong lines.
> + } else {
> + insn_table[cur_insn].insn_func();
> + if (stage != cur_insn + 1)
> + report(insn_table[cur_insn].name, 0);
Would be good to test the inverse as well, i.e. the intercept-free
execution.
> + }
> + cur_insn ++;
> + }
> +}
> +
> +static int insn_intercept_exit_handler()
> +{
> + u64 guest_rip;
> + u32 reason;
> + ulong exit_qual;
> + u32 insn_len;
> + u32 insn_info;
> +
> + 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);
> + insn_info = vmcs_read(EXI_INST_INFO);
> + if (cur_insn == get_stage() &&
> + insn_table[cur_insn].reason == reason &&
> + insn_table[cur_insn].exit_qual == exit_qual &&
> + insn_table[cur_insn].insn_info == insn_info) {
> + report(insn_table[cur_insn].name, 1);
> + set_stage(stage + 1);
> + }
> + vmcs_write(GUEST_RIP, guest_rip + insn_len);
> + return VMX_TEST_RESUME;
> +}
> +
> /* name/init/guest_main/exit_handler/syscall_handler/guest_regs
> basic_* just implement some basic functions */
> struct vmx_test vmx_tests[] = {
> @@ -653,5 +768,7 @@ struct vmx_test vmx_tests[] = {
> cr_shadowing_exit_handler, basic_syscall_handler, {0} },
> { "I/O bitmap", iobmp_init, iobmp_main, iobmp_exit_handler,
> basic_syscall_handler, {0} },
> + { "instruction intercept", insn_intercept_init, insn_intercept_main,
> + insn_intercept_exit_handler, basic_syscall_handler, {0} },
> { NULL, NULL, NULL, NULL, NULL, {0} },
> };
>
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-15 8:06 ` Jan Kiszka
@ 2013-08-15 8:16 ` Arthur Chunqi Li
2013-08-15 8:20 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 8:16 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>> Add test cases for instruction interception, including three types:
>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
>> RDPMC/RDTSC/MONITOR/PAUSE)
>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
>> 3. No control flag (CPUID/INVD)
>>
>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> ---
>> x86/vmx.c | 3 +-
>> x86/vmx.h | 7 ++++
>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 125 insertions(+), 2 deletions(-)
>>
>> diff --git a/x86/vmx.c b/x86/vmx.c
>> index ca36d35..c346070 100644
>> --- a/x86/vmx.c
>> +++ b/x86/vmx.c
>> @@ -336,8 +336,7 @@ static void init_vmx(void)
>> : MSR_IA32_VMX_ENTRY_CTLS);
>> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
>> : MSR_IA32_VMX_PROCBASED_CTLS);
>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
>> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>>
>> diff --git a/x86/vmx.h b/x86/vmx.h
>> index dba8b20..d81d25d 100644
>> --- a/x86/vmx.h
>> +++ b/x86/vmx.h
>> @@ -354,6 +354,9 @@ enum Ctrl0 {
>> CPU_INTR_WINDOW = 1ul << 2,
>> CPU_HLT = 1ul << 7,
>> CPU_INVLPG = 1ul << 9,
>> + CPU_MWAIT = 1ul << 10,
>> + CPU_RDPMC = 1ul << 11,
>> + CPU_RDTSC = 1ul << 12,
>> CPU_CR3_LOAD = 1ul << 15,
>> CPU_CR3_STORE = 1ul << 16,
>> CPU_TPR_SHADOW = 1ul << 21,
>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>> CPU_IO = 1ul << 24,
>> CPU_IO_BITMAP = 1ul << 25,
>> CPU_MSR_BITMAP = 1ul << 28,
>> + CPU_MONITOR = 1ul << 29,
>> + CPU_PAUSE = 1ul << 30,
>> CPU_SECONDARY = 1ul << 31,
>> };
>>
>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>> CPU_EPT = 1ul << 1,
>> CPU_VPID = 1ul << 5,
>> CPU_URG = 1ul << 7,
>> + CPU_WBINVD = 1ul << 6,
>> + CPU_RDRAND = 1ul << 11,
>> };
>>
>> #define SAVE_GPR \
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index ad28c4c..66187f4 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>> }
>>
>> +static inline u32 get_stage()
>> +{
>> + u32 s;
>> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
>> + return s;
>> +}
>
> Tagging "stage" volatile will obsolete this special assembly.
>
>> +
>> void basic_init()
>> {
>> }
>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
>> return VMX_TEST_VMEXIT;
>> }
>>
>> +asm(
>> + "insn_hlt: hlt;ret\n\t"
>> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
>> + "insn_mwait: mwait;ret\n\t"
>> + "insn_rdpmc: rdpmc;ret\n\t"
>> + "insn_rdtsc: rdtsc;ret\n\t"
>> + "insn_monitor: monitor;ret\n\t"
>> + "insn_pause: pause;ret\n\t"
>> + "insn_wbinvd: wbinvd;ret\n\t"
>> + "insn_cpuid: cpuid;ret\n\t"
>> + "insn_invd: invd;ret\n\t"
>> +);
>> +extern void insn_hlt();
>> +extern void insn_invlpg();
>> +extern void insn_mwait();
>> +extern void insn_rdpmc();
>> +extern void insn_rdtsc();
>> +extern void insn_monitor();
>> +extern void insn_pause();
>> +extern void insn_wbinvd();
>> +extern void insn_cpuid();
>> +extern void insn_invd();
>> +
>> +u32 cur_insn;
>> +
>> +struct insn_table {
>> + const char *name;
>> + u32 flag;
>> + void (*insn_func)();
>> + u32 type;
>
> What do the "type" values mean?
For intercepted instructions we have three type: controlled by Primary
Processor-Based VM-Execution Controls, controlled by Secondary
Controls and always intercepted. The testing process is different for
different types.
>
>> + u32 reason;
>> + ulong exit_qual;
>> + u32 insn_info;
>
> For none of the instructions you test, EXI_INST_INFO will have valid
> content on exit. So you must not check it anyway.
Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now.
Since for all intercepts these three vmcs fields are enough to
determine everything, I put it here for future use.
>
>> +};
>> +
>> +static struct insn_table insn_table[] = {
>> + // Flags for Primary Processor-Based VM-Execution Controls
>> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
>> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
>> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
>> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
>> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
>> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
>> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
>> + // Flags for Secondary Processor-Based VM-Execution Controls
>> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
>> + // Flags for Non-Processor-Based
>> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
>> + {"INVD", 0, insn_invd, 2, 13, 0, 0},
>> + {NULL},
>> +};
>> +
>> +static void insn_intercept_init()
>> +{
>> + u32 ctrl_cpu[2];
>> +
>> + ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
>> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
>> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
>> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
>> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
>> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
>> +}
>> +
>> +static void insn_intercept_main()
>> +{
>> + cur_insn = 0;
>> + while(insn_table[cur_insn].name != NULL) {
>> + set_stage(cur_insn);
>> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
>> + (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
>> + printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
>
>
> Coding style, specifically overlong lines.
>
>> + } else {
>> + insn_table[cur_insn].insn_func();
>> + if (stage != cur_insn + 1)
>> + report(insn_table[cur_insn].name, 0);
>
> Would be good to test the inverse as well, i.e. the intercept-free
> execution.
Since this is called "insn_intercept", I think we'd better test
intercept-free execution insns in a separate test suite.
Arthur
>
>> + }
>> + cur_insn ++;
>> + }
>> +}
>> +
>> +static int insn_intercept_exit_handler()
>> +{
>> + u64 guest_rip;
>> + u32 reason;
>> + ulong exit_qual;
>> + u32 insn_len;
>> + u32 insn_info;
>> +
>> + 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);
>> + insn_info = vmcs_read(EXI_INST_INFO);
>> + if (cur_insn == get_stage() &&
>> + insn_table[cur_insn].reason == reason &&
>> + insn_table[cur_insn].exit_qual == exit_qual &&
>> + insn_table[cur_insn].insn_info == insn_info) {
>> + report(insn_table[cur_insn].name, 1);
>> + set_stage(stage + 1);
>> + }
>> + vmcs_write(GUEST_RIP, guest_rip + insn_len);
>> + return VMX_TEST_RESUME;
>> +}
>> +
>> /* name/init/guest_main/exit_handler/syscall_handler/guest_regs
>> basic_* just implement some basic functions */
>> struct vmx_test vmx_tests[] = {
>> @@ -653,5 +768,7 @@ struct vmx_test vmx_tests[] = {
>> cr_shadowing_exit_handler, basic_syscall_handler, {0} },
>> { "I/O bitmap", iobmp_init, iobmp_main, iobmp_exit_handler,
>> basic_syscall_handler, {0} },
>> + { "instruction intercept", insn_intercept_init, insn_intercept_main,
>> + insn_intercept_exit_handler, basic_syscall_handler, {0} },
>> { NULL, NULL, NULL, NULL, NULL, {0} },
>> };
>>
>
> Jan
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-15 8:16 ` Arthur Chunqi Li
@ 2013-08-15 8:20 ` Jan Kiszka
2013-08-15 8:35 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:20 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 7750 bytes --]
On 2013-08-15 10:16, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>> Add test cases for instruction interception, including three types:
>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
>>> RDPMC/RDTSC/MONITOR/PAUSE)
>>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
>>> 3. No control flag (CPUID/INVD)
>>>
>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>> ---
>>> x86/vmx.c | 3 +-
>>> x86/vmx.h | 7 ++++
>>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 125 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/x86/vmx.c b/x86/vmx.c
>>> index ca36d35..c346070 100644
>>> --- a/x86/vmx.c
>>> +++ b/x86/vmx.c
>>> @@ -336,8 +336,7 @@ static void init_vmx(void)
>>> : MSR_IA32_VMX_ENTRY_CTLS);
>>> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
>>> : MSR_IA32_VMX_PROCBASED_CTLS);
>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
>>> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>>> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>>>
>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>> index dba8b20..d81d25d 100644
>>> --- a/x86/vmx.h
>>> +++ b/x86/vmx.h
>>> @@ -354,6 +354,9 @@ enum Ctrl0 {
>>> CPU_INTR_WINDOW = 1ul << 2,
>>> CPU_HLT = 1ul << 7,
>>> CPU_INVLPG = 1ul << 9,
>>> + CPU_MWAIT = 1ul << 10,
>>> + CPU_RDPMC = 1ul << 11,
>>> + CPU_RDTSC = 1ul << 12,
>>> CPU_CR3_LOAD = 1ul << 15,
>>> CPU_CR3_STORE = 1ul << 16,
>>> CPU_TPR_SHADOW = 1ul << 21,
>>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>>> CPU_IO = 1ul << 24,
>>> CPU_IO_BITMAP = 1ul << 25,
>>> CPU_MSR_BITMAP = 1ul << 28,
>>> + CPU_MONITOR = 1ul << 29,
>>> + CPU_PAUSE = 1ul << 30,
>>> CPU_SECONDARY = 1ul << 31,
>>> };
>>>
>>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>>> CPU_EPT = 1ul << 1,
>>> CPU_VPID = 1ul << 5,
>>> CPU_URG = 1ul << 7,
>>> + CPU_WBINVD = 1ul << 6,
>>> + CPU_RDRAND = 1ul << 11,
>>> };
>>>
>>> #define SAVE_GPR \
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index ad28c4c..66187f4 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
>>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>>> }
>>>
>>> +static inline u32 get_stage()
>>> +{
>>> + u32 s;
>>> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
>>> + return s;
>>> +}
>>
>> Tagging "stage" volatile will obsolete this special assembly.
>>
>>> +
>>> void basic_init()
>>> {
>>> }
>>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
>>> return VMX_TEST_VMEXIT;
>>> }
>>>
>>> +asm(
>>> + "insn_hlt: hlt;ret\n\t"
>>> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
>>> + "insn_mwait: mwait;ret\n\t"
>>> + "insn_rdpmc: rdpmc;ret\n\t"
>>> + "insn_rdtsc: rdtsc;ret\n\t"
>>> + "insn_monitor: monitor;ret\n\t"
>>> + "insn_pause: pause;ret\n\t"
>>> + "insn_wbinvd: wbinvd;ret\n\t"
>>> + "insn_cpuid: cpuid;ret\n\t"
>>> + "insn_invd: invd;ret\n\t"
>>> +);
>>> +extern void insn_hlt();
>>> +extern void insn_invlpg();
>>> +extern void insn_mwait();
>>> +extern void insn_rdpmc();
>>> +extern void insn_rdtsc();
>>> +extern void insn_monitor();
>>> +extern void insn_pause();
>>> +extern void insn_wbinvd();
>>> +extern void insn_cpuid();
>>> +extern void insn_invd();
>>> +
>>> +u32 cur_insn;
>>> +
>>> +struct insn_table {
>>> + const char *name;
>>> + u32 flag;
>>> + void (*insn_func)();
>>> + u32 type;
>>
>> What do the "type" values mean?
> For intercepted instructions we have three type: controlled by Primary
> Processor-Based VM-Execution Controls, controlled by Secondary
> Controls and always intercepted. The testing process is different for
> different types.
This was a rhetorical questions. ;) Could you make the values symbolic?
>>
>>> + u32 reason;
>>> + ulong exit_qual;
>>> + u32 insn_info;
>>
>> For none of the instructions you test, EXI_INST_INFO will have valid
>> content on exit. So you must not check it anyway.
> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now.
> Since for all intercepts these three vmcs fields are enough to
> determine everything, I put it here for future use.
OK, but don't test its value when it's undefined - like in all cases
implemented here.
>>
>>> +};
>>> +
>>> +static struct insn_table insn_table[] = {
>>> + // Flags for Primary Processor-Based VM-Execution Controls
>>> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
>>> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
>>> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
>>> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
>>> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
>>> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
>>> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
>>> + // Flags for Secondary Processor-Based VM-Execution Controls
>>> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
>>> + // Flags for Non-Processor-Based
>>> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
>>> + {"INVD", 0, insn_invd, 2, 13, 0, 0},
>>> + {NULL},
>>> +};
>>> +
>>> +static void insn_intercept_init()
>>> +{
>>> + u32 ctrl_cpu[2];
>>> +
>>> + ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
>>> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
>>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
>>> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
>>> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
>>> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
>>> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
>>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
>>> +}
>>> +
>>> +static void insn_intercept_main()
>>> +{
>>> + cur_insn = 0;
>>> + while(insn_table[cur_insn].name != NULL) {
>>> + set_stage(cur_insn);
>>> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
>>> + (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
>>> + printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
>>
>>
>> Coding style, specifically overlong lines.
>>
>>> + } else {
>>> + insn_table[cur_insn].insn_func();
>>> + if (stage != cur_insn + 1)
>>> + report(insn_table[cur_insn].name, 0);
>>
>> Would be good to test the inverse as well, i.e. the intercept-free
>> execution.
> Since this is called "insn_intercept", I think we'd better test
> intercept-free execution insns in a separate test suite.
You have all the infrastructure here now. Isn't it trivial to check
weather stage makes no progress when the intercept bit is cleared instead?
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-15 8:20 ` Jan Kiszka
@ 2013-08-15 8:35 ` Arthur Chunqi Li
2013-08-15 8:40 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 8:35 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2013-08-15 10:16, Arthur Chunqi Li wrote:
>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>> Add test cases for instruction interception, including three types:
>>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
>>>> RDPMC/RDTSC/MONITOR/PAUSE)
>>>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
>>>> 3. No control flag (CPUID/INVD)
>>>>
>>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>>> ---
>>>> x86/vmx.c | 3 +-
>>>> x86/vmx.h | 7 ++++
>>>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 125 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/x86/vmx.c b/x86/vmx.c
>>>> index ca36d35..c346070 100644
>>>> --- a/x86/vmx.c
>>>> +++ b/x86/vmx.c
>>>> @@ -336,8 +336,7 @@ static void init_vmx(void)
>>>> : MSR_IA32_VMX_ENTRY_CTLS);
>>>> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
>>>> : MSR_IA32_VMX_PROCBASED_CTLS);
>>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
>>>> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>>>> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>>>>
>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>> index dba8b20..d81d25d 100644
>>>> --- a/x86/vmx.h
>>>> +++ b/x86/vmx.h
>>>> @@ -354,6 +354,9 @@ enum Ctrl0 {
>>>> CPU_INTR_WINDOW = 1ul << 2,
>>>> CPU_HLT = 1ul << 7,
>>>> CPU_INVLPG = 1ul << 9,
>>>> + CPU_MWAIT = 1ul << 10,
>>>> + CPU_RDPMC = 1ul << 11,
>>>> + CPU_RDTSC = 1ul << 12,
>>>> CPU_CR3_LOAD = 1ul << 15,
>>>> CPU_CR3_STORE = 1ul << 16,
>>>> CPU_TPR_SHADOW = 1ul << 21,
>>>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>>>> CPU_IO = 1ul << 24,
>>>> CPU_IO_BITMAP = 1ul << 25,
>>>> CPU_MSR_BITMAP = 1ul << 28,
>>>> + CPU_MONITOR = 1ul << 29,
>>>> + CPU_PAUSE = 1ul << 30,
>>>> CPU_SECONDARY = 1ul << 31,
>>>> };
>>>>
>>>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>>>> CPU_EPT = 1ul << 1,
>>>> CPU_VPID = 1ul << 5,
>>>> CPU_URG = 1ul << 7,
>>>> + CPU_WBINVD = 1ul << 6,
>>>> + CPU_RDRAND = 1ul << 11,
>>>> };
>>>>
>>>> #define SAVE_GPR \
>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>> index ad28c4c..66187f4 100644
>>>> --- a/x86/vmx_tests.c
>>>> +++ b/x86/vmx_tests.c
>>>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
>>>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>>>> }
>>>>
>>>> +static inline u32 get_stage()
>>>> +{
>>>> + u32 s;
>>>> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
>>>> + return s;
>>>> +}
>>>
>>> Tagging "stage" volatile will obsolete this special assembly.
>>>
>>>> +
>>>> void basic_init()
>>>> {
>>>> }
>>>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
>>>> return VMX_TEST_VMEXIT;
>>>> }
>>>>
>>>> +asm(
>>>> + "insn_hlt: hlt;ret\n\t"
>>>> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
>>>> + "insn_mwait: mwait;ret\n\t"
>>>> + "insn_rdpmc: rdpmc;ret\n\t"
>>>> + "insn_rdtsc: rdtsc;ret\n\t"
>>>> + "insn_monitor: monitor;ret\n\t"
>>>> + "insn_pause: pause;ret\n\t"
>>>> + "insn_wbinvd: wbinvd;ret\n\t"
>>>> + "insn_cpuid: cpuid;ret\n\t"
>>>> + "insn_invd: invd;ret\n\t"
>>>> +);
>>>> +extern void insn_hlt();
>>>> +extern void insn_invlpg();
>>>> +extern void insn_mwait();
>>>> +extern void insn_rdpmc();
>>>> +extern void insn_rdtsc();
>>>> +extern void insn_monitor();
>>>> +extern void insn_pause();
>>>> +extern void insn_wbinvd();
>>>> +extern void insn_cpuid();
>>>> +extern void insn_invd();
>>>> +
>>>> +u32 cur_insn;
>>>> +
>>>> +struct insn_table {
>>>> + const char *name;
>>>> + u32 flag;
>>>> + void (*insn_func)();
>>>> + u32 type;
>>>
>>> What do the "type" values mean?
>> For intercepted instructions we have three type: controlled by Primary
>> Processor-Based VM-Execution Controls, controlled by Secondary
>> Controls and always intercepted. The testing process is different for
>> different types.
>
> This was a rhetorical questions. ;) Could you make the values symbolic?
OK. It's better to rename it to "ctrl_field" and define some macros
such as CTRL_CPU0, CTRL_CPU1, CTRL_NONE to make it more readable.
>
>>>
>>>> + u32 reason;
>>>> + ulong exit_qual;
>>>> + u32 insn_info;
>>>
>>> For none of the instructions you test, EXI_INST_INFO will have valid
>>> content on exit. So you must not check it anyway.
>> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now.
>> Since for all intercepts these three vmcs fields are enough to
>> determine everything, I put it here for future use.
>
> OK, but don't test its value when it's undefined - like in all cases
> implemented here.
Only test the used field will make it more complex because we need to
define which field is used in insn_table. Besides, if any of these
three fields is unused, it will be set to 0; and I think writing like
this is OK since we just write a test case.
Arthur
>
>>>
>>>> +};
>>>> +
>>>> +static struct insn_table insn_table[] = {
>>>> + // Flags for Primary Processor-Based VM-Execution Controls
>>>> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
>>>> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
>>>> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
>>>> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
>>>> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
>>>> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
>>>> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
>>>> + // Flags for Secondary Processor-Based VM-Execution Controls
>>>> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
>>>> + // Flags for Non-Processor-Based
>>>> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
>>>> + {"INVD", 0, insn_invd, 2, 13, 0, 0},
>>>> + {NULL},
>>>> +};
>>>> +
>>>> +static void insn_intercept_init()
>>>> +{
>>>> + u32 ctrl_cpu[2];
>>>> +
>>>> + ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
>>>> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
>>>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
>>>> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
>>>> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
>>>> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
>>>> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
>>>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
>>>> +}
>>>> +
>>>> +static void insn_intercept_main()
>>>> +{
>>>> + cur_insn = 0;
>>>> + while(insn_table[cur_insn].name != NULL) {
>>>> + set_stage(cur_insn);
>>>> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
>>>> + (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
>>>> + printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
>>>
>>>
>>> Coding style, specifically overlong lines.
>>>
>>>> + } else {
>>>> + insn_table[cur_insn].insn_func();
>>>> + if (stage != cur_insn + 1)
>>>> + report(insn_table[cur_insn].name, 0);
>>>
>>> Would be good to test the inverse as well, i.e. the intercept-free
>>> execution.
>> Since this is called "insn_intercept", I think we'd better test
>> intercept-free execution insns in a separate test suite.
>
> You have all the infrastructure here now. Isn't it trivial to check
> weather stage makes no progress when the intercept bit is cleared instead?
No, thus maybe we need to rename it to "instruction test" :)
Arthur
>
> Jan
>
>
--
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-15 8:35 ` Arthur Chunqi Li
@ 2013-08-15 8:40 ` Jan Kiszka
2013-08-15 8:48 ` Arthur Chunqi Li
0 siblings, 1 reply; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 8:40 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 8879 bytes --]
On 2013-08-15 10:35, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-15 10:16, Arthur Chunqi Li wrote:
>>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>>> Add test cases for instruction interception, including three types:
>>>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
>>>>> RDPMC/RDTSC/MONITOR/PAUSE)
>>>>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
>>>>> 3. No control flag (CPUID/INVD)
>>>>>
>>>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>>>> ---
>>>>> x86/vmx.c | 3 +-
>>>>> x86/vmx.h | 7 ++++
>>>>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 125 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/x86/vmx.c b/x86/vmx.c
>>>>> index ca36d35..c346070 100644
>>>>> --- a/x86/vmx.c
>>>>> +++ b/x86/vmx.c
>>>>> @@ -336,8 +336,7 @@ static void init_vmx(void)
>>>>> : MSR_IA32_VMX_ENTRY_CTLS);
>>>>> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
>>>>> : MSR_IA32_VMX_PROCBASED_CTLS);
>>>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
>>>>> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>>>>> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>>>>>
>>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>>> index dba8b20..d81d25d 100644
>>>>> --- a/x86/vmx.h
>>>>> +++ b/x86/vmx.h
>>>>> @@ -354,6 +354,9 @@ enum Ctrl0 {
>>>>> CPU_INTR_WINDOW = 1ul << 2,
>>>>> CPU_HLT = 1ul << 7,
>>>>> CPU_INVLPG = 1ul << 9,
>>>>> + CPU_MWAIT = 1ul << 10,
>>>>> + CPU_RDPMC = 1ul << 11,
>>>>> + CPU_RDTSC = 1ul << 12,
>>>>> CPU_CR3_LOAD = 1ul << 15,
>>>>> CPU_CR3_STORE = 1ul << 16,
>>>>> CPU_TPR_SHADOW = 1ul << 21,
>>>>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>>>>> CPU_IO = 1ul << 24,
>>>>> CPU_IO_BITMAP = 1ul << 25,
>>>>> CPU_MSR_BITMAP = 1ul << 28,
>>>>> + CPU_MONITOR = 1ul << 29,
>>>>> + CPU_PAUSE = 1ul << 30,
>>>>> CPU_SECONDARY = 1ul << 31,
>>>>> };
>>>>>
>>>>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>>>>> CPU_EPT = 1ul << 1,
>>>>> CPU_VPID = 1ul << 5,
>>>>> CPU_URG = 1ul << 7,
>>>>> + CPU_WBINVD = 1ul << 6,
>>>>> + CPU_RDRAND = 1ul << 11,
>>>>> };
>>>>>
>>>>> #define SAVE_GPR \
>>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>>> index ad28c4c..66187f4 100644
>>>>> --- a/x86/vmx_tests.c
>>>>> +++ b/x86/vmx_tests.c
>>>>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
>>>>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>>>>> }
>>>>>
>>>>> +static inline u32 get_stage()
>>>>> +{
>>>>> + u32 s;
>>>>> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
>>>>> + return s;
>>>>> +}
>>>>
>>>> Tagging "stage" volatile will obsolete this special assembly.
>>>>
>>>>> +
>>>>> void basic_init()
>>>>> {
>>>>> }
>>>>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
>>>>> return VMX_TEST_VMEXIT;
>>>>> }
>>>>>
>>>>> +asm(
>>>>> + "insn_hlt: hlt;ret\n\t"
>>>>> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
>>>>> + "insn_mwait: mwait;ret\n\t"
>>>>> + "insn_rdpmc: rdpmc;ret\n\t"
>>>>> + "insn_rdtsc: rdtsc;ret\n\t"
>>>>> + "insn_monitor: monitor;ret\n\t"
>>>>> + "insn_pause: pause;ret\n\t"
>>>>> + "insn_wbinvd: wbinvd;ret\n\t"
>>>>> + "insn_cpuid: cpuid;ret\n\t"
>>>>> + "insn_invd: invd;ret\n\t"
>>>>> +);
>>>>> +extern void insn_hlt();
>>>>> +extern void insn_invlpg();
>>>>> +extern void insn_mwait();
>>>>> +extern void insn_rdpmc();
>>>>> +extern void insn_rdtsc();
>>>>> +extern void insn_monitor();
>>>>> +extern void insn_pause();
>>>>> +extern void insn_wbinvd();
>>>>> +extern void insn_cpuid();
>>>>> +extern void insn_invd();
>>>>> +
>>>>> +u32 cur_insn;
>>>>> +
>>>>> +struct insn_table {
>>>>> + const char *name;
>>>>> + u32 flag;
>>>>> + void (*insn_func)();
>>>>> + u32 type;
>>>>
>>>> What do the "type" values mean?
>>> For intercepted instructions we have three type: controlled by Primary
>>> Processor-Based VM-Execution Controls, controlled by Secondary
>>> Controls and always intercepted. The testing process is different for
>>> different types.
>>
>> This was a rhetorical questions. ;) Could you make the values symbolic?
> OK. It's better to rename it to "ctrl_field" and define some macros
> such as CTRL_CPU0, CTRL_CPU1, CTRL_NONE to make it more readable.
>>
>>>>
>>>>> + u32 reason;
>>>>> + ulong exit_qual;
>>>>> + u32 insn_info;
>>>>
>>>> For none of the instructions you test, EXI_INST_INFO will have valid
>>>> content on exit. So you must not check it anyway.
>>> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now.
>>> Since for all intercepts these three vmcs fields are enough to
>>> determine everything, I put it here for future use.
>>
>> OK, but don't test its value when it's undefined - like in all cases
>> implemented here.
> Only test the used field will make it more complex because we need to
> define which field is used in insn_table. Besides, if any of these
> three fields is unused, it will be set to 0;
Please point me to the paragraph in the SDM where this is stated.
> and I think writing like
> this is OK since we just write a test case.
>
> Arthur
>>
>>>>
>>>>> +};
>>>>> +
>>>>> +static struct insn_table insn_table[] = {
>>>>> + // Flags for Primary Processor-Based VM-Execution Controls
>>>>> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
>>>>> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
>>>>> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
>>>>> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
>>>>> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
>>>>> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
>>>>> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
>>>>> + // Flags for Secondary Processor-Based VM-Execution Controls
>>>>> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
>>>>> + // Flags for Non-Processor-Based
>>>>> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
>>>>> + {"INVD", 0, insn_invd, 2, 13, 0, 0},
>>>>> + {NULL},
>>>>> +};
>>>>> +
>>>>> +static void insn_intercept_init()
>>>>> +{
>>>>> + u32 ctrl_cpu[2];
>>>>> +
>>>>> + ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
>>>>> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
>>>>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
>>>>> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
>>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
>>>>> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
>>>>> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
>>>>> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
>>>>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
>>>>> +}
>>>>> +
>>>>> +static void insn_intercept_main()
>>>>> +{
>>>>> + cur_insn = 0;
>>>>> + while(insn_table[cur_insn].name != NULL) {
>>>>> + set_stage(cur_insn);
>>>>> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
>>>>> + (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
>>>>> + printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
>>>>
>>>>
>>>> Coding style, specifically overlong lines.
>>>>
>>>>> + } else {
>>>>> + insn_table[cur_insn].insn_func();
>>>>> + if (stage != cur_insn + 1)
>>>>> + report(insn_table[cur_insn].name, 0);
>>>>
>>>> Would be good to test the inverse as well, i.e. the intercept-free
>>>> execution.
>>> Since this is called "insn_intercept", I think we'd better test
>>> intercept-free execution insns in a separate test suite.
>>
>> You have all the infrastructure here now. Isn't it trivial to check
>> weather stage makes no progress when the intercept bit is cleared instead?
> No, thus maybe we need to rename it to "instruction test" :)
This is an instruction intercept control test.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-15 8:40 ` Jan Kiszka
@ 2013-08-15 8:48 ` Arthur Chunqi Li
2013-08-15 9:15 ` Jan Kiszka
0 siblings, 1 reply; 32+ messages in thread
From: Arthur Chunqi Li @ 2013-08-15 8:48 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Gleb Natapov, Paolo Bonzini
On Thu, Aug 15, 2013 at 4:40 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2013-08-15 10:35, Arthur Chunqi Li wrote:
>> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2013-08-15 10:16, Arthur Chunqi Li wrote:
>>>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>>>> Add test cases for instruction interception, including three types:
>>>>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
>>>>>> RDPMC/RDTSC/MONITOR/PAUSE)
>>>>>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
>>>>>> 3. No control flag (CPUID/INVD)
>>>>>>
>>>>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>>>>> ---
>>>>>> x86/vmx.c | 3 +-
>>>>>> x86/vmx.h | 7 ++++
>>>>>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 125 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/x86/vmx.c b/x86/vmx.c
>>>>>> index ca36d35..c346070 100644
>>>>>> --- a/x86/vmx.c
>>>>>> +++ b/x86/vmx.c
>>>>>> @@ -336,8 +336,7 @@ static void init_vmx(void)
>>>>>> : MSR_IA32_VMX_ENTRY_CTLS);
>>>>>> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
>>>>>> : MSR_IA32_VMX_PROCBASED_CTLS);
>>>>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
>>>>>> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>>> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>>>>>> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>>>>>>
>>>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>>>> index dba8b20..d81d25d 100644
>>>>>> --- a/x86/vmx.h
>>>>>> +++ b/x86/vmx.h
>>>>>> @@ -354,6 +354,9 @@ enum Ctrl0 {
>>>>>> CPU_INTR_WINDOW = 1ul << 2,
>>>>>> CPU_HLT = 1ul << 7,
>>>>>> CPU_INVLPG = 1ul << 9,
>>>>>> + CPU_MWAIT = 1ul << 10,
>>>>>> + CPU_RDPMC = 1ul << 11,
>>>>>> + CPU_RDTSC = 1ul << 12,
>>>>>> CPU_CR3_LOAD = 1ul << 15,
>>>>>> CPU_CR3_STORE = 1ul << 16,
>>>>>> CPU_TPR_SHADOW = 1ul << 21,
>>>>>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>>>>>> CPU_IO = 1ul << 24,
>>>>>> CPU_IO_BITMAP = 1ul << 25,
>>>>>> CPU_MSR_BITMAP = 1ul << 28,
>>>>>> + CPU_MONITOR = 1ul << 29,
>>>>>> + CPU_PAUSE = 1ul << 30,
>>>>>> CPU_SECONDARY = 1ul << 31,
>>>>>> };
>>>>>>
>>>>>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>>>>>> CPU_EPT = 1ul << 1,
>>>>>> CPU_VPID = 1ul << 5,
>>>>>> CPU_URG = 1ul << 7,
>>>>>> + CPU_WBINVD = 1ul << 6,
>>>>>> + CPU_RDRAND = 1ul << 11,
>>>>>> };
>>>>>>
>>>>>> #define SAVE_GPR \
>>>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>>>> index ad28c4c..66187f4 100644
>>>>>> --- a/x86/vmx_tests.c
>>>>>> +++ b/x86/vmx_tests.c
>>>>>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
>>>>>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>>>>>> }
>>>>>>
>>>>>> +static inline u32 get_stage()
>>>>>> +{
>>>>>> + u32 s;
>>>>>> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
>>>>>> + return s;
>>>>>> +}
>>>>>
>>>>> Tagging "stage" volatile will obsolete this special assembly.
>>>>>
>>>>>> +
>>>>>> void basic_init()
>>>>>> {
>>>>>> }
>>>>>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
>>>>>> return VMX_TEST_VMEXIT;
>>>>>> }
>>>>>>
>>>>>> +asm(
>>>>>> + "insn_hlt: hlt;ret\n\t"
>>>>>> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
>>>>>> + "insn_mwait: mwait;ret\n\t"
>>>>>> + "insn_rdpmc: rdpmc;ret\n\t"
>>>>>> + "insn_rdtsc: rdtsc;ret\n\t"
>>>>>> + "insn_monitor: monitor;ret\n\t"
>>>>>> + "insn_pause: pause;ret\n\t"
>>>>>> + "insn_wbinvd: wbinvd;ret\n\t"
>>>>>> + "insn_cpuid: cpuid;ret\n\t"
>>>>>> + "insn_invd: invd;ret\n\t"
>>>>>> +);
>>>>>> +extern void insn_hlt();
>>>>>> +extern void insn_invlpg();
>>>>>> +extern void insn_mwait();
>>>>>> +extern void insn_rdpmc();
>>>>>> +extern void insn_rdtsc();
>>>>>> +extern void insn_monitor();
>>>>>> +extern void insn_pause();
>>>>>> +extern void insn_wbinvd();
>>>>>> +extern void insn_cpuid();
>>>>>> +extern void insn_invd();
>>>>>> +
>>>>>> +u32 cur_insn;
>>>>>> +
>>>>>> +struct insn_table {
>>>>>> + const char *name;
>>>>>> + u32 flag;
>>>>>> + void (*insn_func)();
>>>>>> + u32 type;
>>>>>
>>>>> What do the "type" values mean?
>>>> For intercepted instructions we have three type: controlled by Primary
>>>> Processor-Based VM-Execution Controls, controlled by Secondary
>>>> Controls and always intercepted. The testing process is different for
>>>> different types.
>>>
>>> This was a rhetorical questions. ;) Could you make the values symbolic?
>> OK. It's better to rename it to "ctrl_field" and define some macros
>> such as CTRL_CPU0, CTRL_CPU1, CTRL_NONE to make it more readable.
>>>
>>>>>
>>>>>> + u32 reason;
>>>>>> + ulong exit_qual;
>>>>>> + u32 insn_info;
>>>>>
>>>>> For none of the instructions you test, EXI_INST_INFO will have valid
>>>>> content on exit. So you must not check it anyway.
>>>> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now.
>>>> Since for all intercepts these three vmcs fields are enough to
>>>> determine everything, I put it here for future use.
>>>
>>> OK, but don't test its value when it's undefined - like in all cases
>>> implemented here.
>> Only test the used field will make it more complex because we need to
>> define which field is used in insn_table. Besides, if any of these
>> three fields is unused, it will be set to 0;
>
> Please point me to the paragraph in the SDM where this is stated.
For qualification, see 27.2.1, "For all other VM exits, this field is
cleared.". For instruction information, see 27.2.4, "For all other VM
exits, the field is undefined."
Well, "undefined" doesn't means "cleared" :(, though it is actually
cleared in KVM. Do you think we actually need a flag to indicate which
fields need to test?
>
>> and I think writing like
>> this is OK since we just write a test case.
>>
>> Arthur
>>>
>>>>>
>>>>>> +};
>>>>>> +
>>>>>> +static struct insn_table insn_table[] = {
>>>>>> + // Flags for Primary Processor-Based VM-Execution Controls
>>>>>> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0},
>>>>>> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0},
>>>>>> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0},
>>>>>> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0},
>>>>>> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0},
>>>>>> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0},
>>>>>> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0},
>>>>>> + // Flags for Secondary Processor-Based VM-Execution Controls
>>>>>> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0},
>>>>>> + // Flags for Non-Processor-Based
>>>>>> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0},
>>>>>> + {"INVD", 0, insn_invd, 2, 13, 0, 0},
>>>>>> + {NULL},
>>>>>> +};
>>>>>> +
>>>>>> +static void insn_intercept_init()
>>>>>> +{
>>>>>> + u32 ctrl_cpu[2];
>>>>>> +
>>>>>> + ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
>>>>>> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
>>>>>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
>>>>>> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
>>>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
>>>>>> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
>>>>>> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
>>>>>> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
>>>>>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
>>>>>> +}
>>>>>> +
>>>>>> +static void insn_intercept_main()
>>>>>> +{
>>>>>> + cur_insn = 0;
>>>>>> + while(insn_table[cur_insn].name != NULL) {
>>>>>> + set_stage(cur_insn);
>>>>>> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
>>>>>> + (insn_table[cur_insn].type == 1 && !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
>>>>>> + printf("\tCPU_CTRL.CPU_%s is not supported.\n", insn_table[cur_insn].name);
>>>>>
>>>>>
>>>>> Coding style, specifically overlong lines.
>>>>>
>>>>>> + } else {
>>>>>> + insn_table[cur_insn].insn_func();
>>>>>> + if (stage != cur_insn + 1)
>>>>>> + report(insn_table[cur_insn].name, 0);
>>>>>
>>>>> Would be good to test the inverse as well, i.e. the intercept-free
>>>>> execution.
>>>> Since this is called "insn_intercept", I think we'd better test
>>>> intercept-free execution insns in a separate test suite.
>>>
>>> You have all the infrastructure here now. Isn't it trivial to check
>>> weather stage makes no progress when the intercept bit is cleared instead?
>> No, thus maybe we need to rename it to "instruction test" :)
>
> This is an instruction intercept control test.
Well, OK.
Arthur
>
> Jan
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception
2013-08-15 8:48 ` Arthur Chunqi Li
@ 2013-08-15 9:15 ` Jan Kiszka
0 siblings, 0 replies; 32+ messages in thread
From: Jan Kiszka @ 2013-08-15 9:15 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, Gleb Natapov, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 6889 bytes --]
On 2013-08-15 10:48, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 4:40 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-15 10:35, Arthur Chunqi Li wrote:
>>> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2013-08-15 10:16, Arthur Chunqi Li wrote:
>>>>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote:
>>>>>>> Add test cases for instruction interception, including three types:
>>>>>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/
>>>>>>> RDPMC/RDTSC/MONITOR/PAUSE)
>>>>>>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD)
>>>>>>> 3. No control flag (CPUID/INVD)
>>>>>>>
>>>>>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>>>>>> ---
>>>>>>> x86/vmx.c | 3 +-
>>>>>>> x86/vmx.h | 7 ++++
>>>>>>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 125 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/x86/vmx.c b/x86/vmx.c
>>>>>>> index ca36d35..c346070 100644
>>>>>>> --- a/x86/vmx.c
>>>>>>> +++ b/x86/vmx.c
>>>>>>> @@ -336,8 +336,7 @@ static void init_vmx(void)
>>>>>>> : MSR_IA32_VMX_ENTRY_CTLS);
>>>>>>> ctrl_cpu_rev[0].val = rdmsr(basic.ctrl ? MSR_IA32_VMX_TRUE_PROC
>>>>>>> : MSR_IA32_VMX_PROCBASED_CTLS);
>>>>>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY)
>>>>>>> - ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>>>> + ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>>>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>>>>>>> ept_vpid.val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
>>>>>>>
>>>>>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>>>>>> index dba8b20..d81d25d 100644
>>>>>>> --- a/x86/vmx.h
>>>>>>> +++ b/x86/vmx.h
>>>>>>> @@ -354,6 +354,9 @@ enum Ctrl0 {
>>>>>>> CPU_INTR_WINDOW = 1ul << 2,
>>>>>>> CPU_HLT = 1ul << 7,
>>>>>>> CPU_INVLPG = 1ul << 9,
>>>>>>> + CPU_MWAIT = 1ul << 10,
>>>>>>> + CPU_RDPMC = 1ul << 11,
>>>>>>> + CPU_RDTSC = 1ul << 12,
>>>>>>> CPU_CR3_LOAD = 1ul << 15,
>>>>>>> CPU_CR3_STORE = 1ul << 16,
>>>>>>> CPU_TPR_SHADOW = 1ul << 21,
>>>>>>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>>>>>>> CPU_IO = 1ul << 24,
>>>>>>> CPU_IO_BITMAP = 1ul << 25,
>>>>>>> CPU_MSR_BITMAP = 1ul << 28,
>>>>>>> + CPU_MONITOR = 1ul << 29,
>>>>>>> + CPU_PAUSE = 1ul << 30,
>>>>>>> CPU_SECONDARY = 1ul << 31,
>>>>>>> };
>>>>>>>
>>>>>>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>>>>>>> CPU_EPT = 1ul << 1,
>>>>>>> CPU_VPID = 1ul << 5,
>>>>>>> CPU_URG = 1ul << 7,
>>>>>>> + CPU_WBINVD = 1ul << 6,
>>>>>>> + CPU_RDRAND = 1ul << 11,
>>>>>>> };
>>>>>>>
>>>>>>> #define SAVE_GPR \
>>>>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>>>>> index ad28c4c..66187f4 100644
>>>>>>> --- a/x86/vmx_tests.c
>>>>>>> +++ b/x86/vmx_tests.c
>>>>>>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s)
>>>>>>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc");
>>>>>>> }
>>>>>>>
>>>>>>> +static inline u32 get_stage()
>>>>>>> +{
>>>>>>> + u32 s;
>>>>>>> + asm volatile("mov stage, %0\n\t":"=r"(s)::"memory", "cc");
>>>>>>> + return s;
>>>>>>> +}
>>>>>>
>>>>>> Tagging "stage" volatile will obsolete this special assembly.
>>>>>>
>>>>>>> +
>>>>>>> void basic_init()
>>>>>>> {
>>>>>>> }
>>>>>>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler()
>>>>>>> return VMX_TEST_VMEXIT;
>>>>>>> }
>>>>>>>
>>>>>>> +asm(
>>>>>>> + "insn_hlt: hlt;ret\n\t"
>>>>>>> + "insn_invlpg: invlpg 0x12345678;ret\n\t"
>>>>>>> + "insn_mwait: mwait;ret\n\t"
>>>>>>> + "insn_rdpmc: rdpmc;ret\n\t"
>>>>>>> + "insn_rdtsc: rdtsc;ret\n\t"
>>>>>>> + "insn_monitor: monitor;ret\n\t"
>>>>>>> + "insn_pause: pause;ret\n\t"
>>>>>>> + "insn_wbinvd: wbinvd;ret\n\t"
>>>>>>> + "insn_cpuid: cpuid;ret\n\t"
>>>>>>> + "insn_invd: invd;ret\n\t"
>>>>>>> +);
>>>>>>> +extern void insn_hlt();
>>>>>>> +extern void insn_invlpg();
>>>>>>> +extern void insn_mwait();
>>>>>>> +extern void insn_rdpmc();
>>>>>>> +extern void insn_rdtsc();
>>>>>>> +extern void insn_monitor();
>>>>>>> +extern void insn_pause();
>>>>>>> +extern void insn_wbinvd();
>>>>>>> +extern void insn_cpuid();
>>>>>>> +extern void insn_invd();
>>>>>>> +
>>>>>>> +u32 cur_insn;
>>>>>>> +
>>>>>>> +struct insn_table {
>>>>>>> + const char *name;
>>>>>>> + u32 flag;
>>>>>>> + void (*insn_func)();
>>>>>>> + u32 type;
>>>>>>
>>>>>> What do the "type" values mean?
>>>>> For intercepted instructions we have three type: controlled by Primary
>>>>> Processor-Based VM-Execution Controls, controlled by Secondary
>>>>> Controls and always intercepted. The testing process is different for
>>>>> different types.
>>>>
>>>> This was a rhetorical questions. ;) Could you make the values symbolic?
>>> OK. It's better to rename it to "ctrl_field" and define some macros
>>> such as CTRL_CPU0, CTRL_CPU1, CTRL_NONE to make it more readable.
>>>>
>>>>>>
>>>>>>> + u32 reason;
>>>>>>> + ulong exit_qual;
>>>>>>> + u32 insn_info;
>>>>>>
>>>>>> For none of the instructions you test, EXI_INST_INFO will have valid
>>>>>> content on exit. So you must not check it anyway.
>>>>> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now.
>>>>> Since for all intercepts these three vmcs fields are enough to
>>>>> determine everything, I put it here for future use.
>>>>
>>>> OK, but don't test its value when it's undefined - like in all cases
>>>> implemented here.
>>> Only test the used field will make it more complex because we need to
>>> define which field is used in insn_table. Besides, if any of these
>>> three fields is unused, it will be set to 0;
>>
>> Please point me to the paragraph in the SDM where this is stated.
> For qualification, see 27.2.1, "For all other VM exits, this field is
> cleared.". For instruction information, see 27.2.4, "For all other VM
> exits, the field is undefined."
>
> Well, "undefined" doesn't means "cleared" :(, though it is actually
> cleared in KVM. Do you think we actually need a flag to indicate which
> fields need to test?
Yes, that's safer. Even if existing hardware or KVM clears it so far,
different implementations may not follow this, and your test may even
start to randomly fail.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread