From: Bandan Das <bsd@redhat.com>
To: Jan Kiszka <jan.kiszka@web.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm <kvm@vger.kernel.org>
Subject: Re: [PATCH 2/5] VMX: Only use get_stage accessor
Date: Mon, 16 Jun 2014 13:19:25 -0400 [thread overview]
Message-ID: <jpg38f44vs2.fsf@nelium.bos.redhat.com> (raw)
In-Reply-To: <914ca67ac7207c5c13cc2737b654ac2899f58270.1402842281.git.jan.kiszka@web.de> (Jan Kiszka's message of "Sun, 15 Jun 2014 16:24:38 +0200")
Jan Kiszka <jan.kiszka@web.de> writes:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Consistently make sure we are not affected by any compiler reordering
> when evaluating the current stage.
Should we prevent accidental calls to the variable directly by moving
get/set to vmx.c or a separate file in lib/x86 altogether ?
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> x86/vmx_tests.c | 80 ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index d0ce365..bf7aa2c 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -415,13 +415,13 @@ static void cr_shadowing_main()
> // Test read through
> set_stage(0);
> guest_cr0 = read_cr0();
> - if (stage == 1)
> + if (get_stage() == 1)
> report("Read through CR0", 0);
> else
> vmcall();
> set_stage(1);
> guest_cr4 = read_cr4();
> - if (stage == 2)
> + if (get_stage() == 2)
> report("Read through CR4", 0);
> else
> vmcall();
> @@ -430,13 +430,13 @@ static void cr_shadowing_main()
> guest_cr4 = guest_cr4 ^ (X86_CR4_TSD | X86_CR4_DE);
> set_stage(2);
> write_cr0(guest_cr0);
> - if (stage == 3)
> + if (get_stage() == 3)
> report("Write throuth CR0", 0);
> else
> vmcall();
> set_stage(3);
> write_cr4(guest_cr4);
> - if (stage == 4)
> + if (get_stage() == 4)
> report("Write through CR4", 0);
> else
> vmcall();
> @@ -444,7 +444,7 @@ static void cr_shadowing_main()
> set_stage(4);
> vmcall();
> cr0 = read_cr0();
> - if (stage != 5) {
> + if (get_stage() != 5) {
> if (cr0 == guest_cr0)
> report("Read shadowing CR0", 1);
> else
> @@ -452,7 +452,7 @@ static void cr_shadowing_main()
> }
> set_stage(5);
> cr4 = read_cr4();
> - if (stage != 6) {
> + if (get_stage() != 6) {
> if (cr4 == guest_cr4)
> report("Read shadowing CR4", 1);
> else
> @@ -461,13 +461,13 @@ static void cr_shadowing_main()
> // Test write shadow (same value with shadow)
> set_stage(6);
> write_cr0(guest_cr0);
> - if (stage == 7)
> + if (get_stage() == 7)
> report("Write shadowing CR0 (same value with shadow)", 0);
> else
> vmcall();
> set_stage(7);
> write_cr4(guest_cr4);
> - if (stage == 8)
> + if (get_stage() == 8)
> report("Write shadowing CR4 (same value with shadow)", 0);
> else
> vmcall();
> @@ -478,7 +478,7 @@ static void cr_shadowing_main()
> "mov %%rsi, %%cr0\n\t"
> ::"m"(tmp)
> :"rsi", "memory", "cc");
> - if (stage != 9)
> + if (get_stage() != 9)
> report("Write shadowing different X86_CR0_TS", 0);
> else
> report("Write shadowing different X86_CR0_TS", 1);
> @@ -488,7 +488,7 @@ static void cr_shadowing_main()
> "mov %%rsi, %%cr0\n\t"
> ::"m"(tmp)
> :"rsi", "memory", "cc");
> - if (stage != 10)
> + if (get_stage() != 10)
> report("Write shadowing different X86_CR0_MP", 0);
> else
> report("Write shadowing different X86_CR0_MP", 1);
> @@ -498,7 +498,7 @@ static void cr_shadowing_main()
> "mov %%rsi, %%cr4\n\t"
> ::"m"(tmp)
> :"rsi", "memory", "cc");
> - if (stage != 11)
> + if (get_stage() != 11)
> report("Write shadowing different X86_CR4_TSD", 0);
> else
> report("Write shadowing different X86_CR4_TSD", 1);
> @@ -508,7 +508,7 @@ static void cr_shadowing_main()
> "mov %%rsi, %%cr4\n\t"
> ::"m"(tmp)
> :"rsi", "memory", "cc");
> - if (stage != 12)
> + if (get_stage() != 12)
> report("Write shadowing different X86_CR4_DE", 0);
> else
> report("Write shadowing different X86_CR4_DE", 1);
> @@ -584,31 +584,31 @@ static int cr_shadowing_exit_handler()
> switch (get_stage()) {
> case 4:
> report("Read shadowing CR0", 0);
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 5:
> report("Read shadowing CR4", 0);
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 6:
> report("Write shadowing CR0 (same value)", 0);
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 7:
> report("Write shadowing CR4 (same value)", 0);
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 8:
> case 9:
> // 0x600 encodes "mov %esi, %cr0"
> if (exit_qual == 0x600)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 10:
> case 11:
> // 0x604 encodes "mov %esi, %cr4"
> if (exit_qual == 0x604)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> default:
> // Should not reach here
> @@ -648,7 +648,7 @@ static void iobmp_main()
> set_stage(0);
> inb(0x5000);
> outb(0x0, 0x5000);
> - if (stage != 0)
> + if (get_stage() != 0)
> report("I/O bitmap - I/O pass", 0);
> else
> report("I/O bitmap - I/O pass", 1);
> @@ -656,39 +656,39 @@ static void iobmp_main()
> ((u8 *)io_bitmap_a)[0] = 0xFF;
> set_stage(2);
> inb(0x0);
> - if (stage != 3)
> + if (get_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)
> + if (get_stage() != 4)
> report("I/O bitmap - trap out", 0);
> else
> report("I/O bitmap - trap out", 1);
> set_stage(4);
> inl(0x0);
> - if (stage != 5)
> + if (get_stage() != 5)
> report("I/O bitmap - I/O width, long", 0);
> // test low/high IO port
> set_stage(5);
> ((u8 *)io_bitmap_a)[0x5000 / 8] = (1 << (0x5000 % 8));
> inb(0x5000);
> - if (stage == 6)
> + if (get_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)
> + if (get_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)
> + if (get_stage() == 8)
> report("I/O bitmap - partial pass", 1);
> else
> report("I/O bitmap - partial pass", 0);
> @@ -697,18 +697,18 @@ static void iobmp_main()
> memset(io_bitmap_a, 0x0, PAGE_SIZE);
> memset(io_bitmap_b, 0x0, PAGE_SIZE);
> inl(0xFFFF);
> - if (stage == 9)
> + if (get_stage() == 9)
> report("I/O bitmap - overrun", 1);
> else
> report("I/O bitmap - overrun", 0);
> set_stage(9);
> vmcall();
> outb(0x0, 0x0);
> - report("I/O bitmap - ignore unconditional exiting", stage == 9);
> + report("I/O bitmap - ignore unconditional exiting", get_stage() == 9);
> set_stage(10);
> vmcall();
> outb(0x0, 0x0);
> - report("I/O bitmap - unconditional exiting", stage == 11);
> + report("I/O bitmap - unconditional exiting", get_stage() == 11);
> }
>
> static int iobmp_exit_handler()
> @@ -726,7 +726,7 @@ static int iobmp_exit_handler()
> switch (get_stage()) {
> case 0:
> case 1:
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 2:
> if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_BYTE)
> @@ -737,7 +737,7 @@ static int iobmp_exit_handler()
> report("I/O bitmap - I/O direction, in", 0);
> else
> report("I/O bitmap - I/O direction, in", 1);
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 3:
> if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_WORD)
> @@ -748,36 +748,36 @@ static int iobmp_exit_handler()
> report("I/O bitmap - I/O direction, out", 1);
> else
> report("I/O bitmap - I/O direction, out", 0);
> - set_stage(stage + 1);
> + set_stage(get_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);
> + set_stage(get_stage() + 1);
> break;
> case 5:
> if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x5000)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 6:
> if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x9000)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 7:
> if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x4FFF)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 8:
> if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0xFFFF)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> case 9:
> case 10:
> ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
> vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0 & ~CPU_IO);
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> break;
> default:
> // Should not reach here
> @@ -948,13 +948,13 @@ static void insn_intercept_main()
> case INSN_CPU0:
> case INSN_CPU1:
> case INSN_ALWAYS_TRAP:
> - if (stage != cur_insn + 1)
> + if (get_stage() != cur_insn + 1)
> report(insn_table[cur_insn].name, 0);
> else
> report(insn_table[cur_insn].name, 1);
> break;
> case INSN_NEVER_TRAP:
> - if (stage == cur_insn + 1)
> + if (get_stage() == cur_insn + 1)
> report(insn_table[cur_insn].name, 0);
> else
> report(insn_table[cur_insn].name, 1);
> @@ -985,7 +985,7 @@ static int insn_intercept_exit_handler()
> if (insn_table[cur_insn].test_field & FIELD_INSN_INFO)
> pass = pass && insn_table[cur_insn].insn_info == insn_info;
> if (pass)
> - set_stage(stage + 1);
> + set_stage(get_stage() + 1);
> vmcs_write(GUEST_RIP, guest_rip + insn_len);
> return VMX_TEST_RESUME;
> }
next prev parent reply other threads:[~2014-06-16 17:19 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-15 14:24 [PATCH 0/5] kvm-unit-tests: more instr. interceptions, debug control migration Jan Kiszka
2014-06-15 14:24 ` [PATCH 1/5] VMX: Add tests for CR3 and CR8 interception Jan Kiszka
2014-06-16 10:53 ` Paolo Bonzini
2014-06-16 11:31 ` Jan Kiszka
2014-06-15 14:24 ` [PATCH 2/5] VMX: Only use get_stage accessor Jan Kiszka
2014-06-16 17:19 ` Bandan Das [this message]
2014-06-15 14:24 ` [PATCH 3/5] VMX: Test both interception and execution of instructions Jan Kiszka
2014-06-15 14:24 ` [PATCH 4/5] VMX: Validate capability MSRs Jan Kiszka
2014-06-16 11:00 ` Paolo Bonzini
2014-06-16 11:26 ` Jan Kiszka
2014-06-15 14:24 ` [PATCH 5/5] VMX: Test behavior on set and cleared save/load debug controls Jan Kiszka
2014-06-16 11:02 ` Paolo Bonzini
2014-06-16 11:28 ` Jan Kiszka
2014-06-16 11:03 ` [PATCH 0/5] kvm-unit-tests: more instr. interceptions, debug control migration Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jpg38f44vs2.fsf@nelium.bos.redhat.com \
--to=bsd@redhat.com \
--cc=jan.kiszka@web.de \
--cc=kvm@vger.kernel.org \
--cc=pbonzini@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox