* [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
@ 2013-06-06 5:03 Arthur Chunqi Li
2013-06-06 5:45 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Arthur Chunqi Li @ 2013-06-06 5:03 UTC (permalink / raw)
To: kvm; +Cc: gleb, pbonzini, Arthur Chunqi Li
Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
---
x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/x86/emulator.c b/x86/emulator.c
index 96576e5..3563971 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
report("test", *mem == 0x8400);
}
+static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
+ uint8_t *alt_insn_page, void *insn_ram)
+{
+ ulong *cr3 = (ulong *)read_cr3();
+ uint16_t cx = 0;
+
+ // Pad with RET instructions
+ memset(insn_page, 0xc3, 4096);
+ memset(alt_insn_page, 0xc3, 4096);
+ // Place a trapping instruction in the page to trigger a VMEXIT
+ insn_page[0] = 0x66; // mov $0x4321, %cx
+ insn_page[1] = 0xb9;
+ insn_page[2] = 0x21;
+ insn_page[3] = 0x43;
+ insn_page[4] = 0x89; // mov %eax, (%rax)
+ insn_page[5] = 0x00;
+ insn_page[6] = 0x90; // nop
+ // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
+ // If emulator mistaken addressing %bpl, %cl may be moved to %ch
+ // %cx will be broken to 0x2121, not 0x4321
+ alt_insn_page[4] = 0x40;
+ alt_insn_page[5] = 0x88;
+ alt_insn_page[6] = 0xcd;
+
+ // Load the code TLB with insn_page, but point the page tables at
+ // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
+ // This will make the CPU trap on the insn_page instruction but the
+ // hypervisor will see alt_insn_page.
+ install_page(cr3, virt_to_phys(insn_page), insn_ram);
+ // Load code TLB
+ invlpg(insn_ram);
+ asm volatile("call *%0" : : "r"(insn_ram+3));
+ // Trap, let hypervisor emulate at alt_insn_page
+ install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
+ asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
+ asm volatile("":"=c"(cx));
+ report("access bpl in modr/m", cx == 0x4321);
+}
+
int main()
{
void *mem;
@@ -964,6 +1003,8 @@ int main()
test_string_io_mmio(mem);
+ test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
+
printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
return fails ? 1 : 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 5:03 [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m Arthur Chunqi Li
@ 2013-06-06 5:45 ` Gleb Natapov
2013-06-06 6:47 ` 李春奇 <Arthur Chunqi Li>
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-06-06 5:45 UTC (permalink / raw)
To: Arthur Chunqi Li; +Cc: kvm, pbonzini
On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
>
We have growing number of instructions tests using the same tlb trick. I
think it is time to make the code more generic. Create a function that
receives instruction to check and all the tlb games will be done by that
function.
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> ---
> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/x86/emulator.c b/x86/emulator.c
> index 96576e5..3563971 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
> report("test", *mem == 0x8400);
> }
>
> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
> + uint8_t *alt_insn_page, void *insn_ram)
> +{
> + ulong *cr3 = (ulong *)read_cr3();
> + uint16_t cx = 0;
> +
> + // Pad with RET instructions
> + memset(insn_page, 0xc3, 4096);
> + memset(alt_insn_page, 0xc3, 4096);
> + // Place a trapping instruction in the page to trigger a VMEXIT
> + insn_page[0] = 0x66; // mov $0x4321, %cx
> + insn_page[1] = 0xb9;
> + insn_page[2] = 0x21;
> + insn_page[3] = 0x43;
> + insn_page[4] = 0x89; // mov %eax, (%rax)
> + insn_page[5] = 0x00;
> + insn_page[6] = 0x90; // nop
> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
> + // %cx will be broken to 0x2121, not 0x4321
> + alt_insn_page[4] = 0x40;
> + alt_insn_page[5] = 0x88;
> + alt_insn_page[6] = 0xcd;
> +
> + // Load the code TLB with insn_page, but point the page tables at
> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
> + // This will make the CPU trap on the insn_page instruction but the
> + // hypervisor will see alt_insn_page.
> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
> + // Load code TLB
> + invlpg(insn_ram);
> + asm volatile("call *%0" : : "r"(insn_ram+3));
> + // Trap, let hypervisor emulate at alt_insn_page
> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
> + asm volatile("":"=c"(cx));
Why not add the constrain to previous asm?
> + report("access bpl in modr/m", cx == 0x4321);
> +}
> +
> int main()
> {
> void *mem;
> @@ -964,6 +1003,8 @@ int main()
>
> test_string_io_mmio(mem);
>
> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
> +
> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
> return fails ? 1 : 0;
> }
> --
> 1.7.9.5
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 5:45 ` Gleb Natapov
@ 2013-06-06 6:47 ` 李春奇 <Arthur Chunqi Li>
2013-06-06 7:01 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: 李春奇 <Arthur Chunqi Li> @ 2013-06-06 6:47 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, Paolo Bonzini
On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
>> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
>>
> We have growing number of instructions tests using the same tlb trick. I
> think it is time to make the code more generic. Create a function that
> receives instruction to check and all the tlb games will be done by that
> function.
Should I do some work to merge all these test cases?
>
>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> ---
>> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>>
>> diff --git a/x86/emulator.c b/x86/emulator.c
>> index 96576e5..3563971 100644
>> --- a/x86/emulator.c
>> +++ b/x86/emulator.c
>> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
>> report("test", *mem == 0x8400);
>> }
>>
>> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
>> + uint8_t *alt_insn_page, void *insn_ram)
>> +{
>> + ulong *cr3 = (ulong *)read_cr3();
>> + uint16_t cx = 0;
>> +
>> + // Pad with RET instructions
>> + memset(insn_page, 0xc3, 4096);
>> + memset(alt_insn_page, 0xc3, 4096);
>> + // Place a trapping instruction in the page to trigger a VMEXIT
>> + insn_page[0] = 0x66; // mov $0x4321, %cx
>> + insn_page[1] = 0xb9;
>> + insn_page[2] = 0x21;
>> + insn_page[3] = 0x43;
>> + insn_page[4] = 0x89; // mov %eax, (%rax)
>> + insn_page[5] = 0x00;
>> + insn_page[6] = 0x90; // nop
>> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
>> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
>> + // %cx will be broken to 0x2121, not 0x4321
>> + alt_insn_page[4] = 0x40;
>> + alt_insn_page[5] = 0x88;
>> + alt_insn_page[6] = 0xcd;
>> +
>> + // Load the code TLB with insn_page, but point the page tables at
>> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
>> + // This will make the CPU trap on the insn_page instruction but the
>> + // hypervisor will see alt_insn_page.
>> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
>> + // Load code TLB
>> + invlpg(insn_ram);
>> + asm volatile("call *%0" : : "r"(insn_ram+3));
>> + // Trap, let hypervisor emulate at alt_insn_page
>> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
>> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
>> + asm volatile("":"=c"(cx));
> Why not add the constrain to previous asm?
I will merge them in next version.
>
>> + report("access bpl in modr/m", cx == 0x4321);
>> +}
>> +
>> int main()
>> {
>> void *mem;
>> @@ -964,6 +1003,8 @@ int main()
>>
>> test_string_io_mmio(mem);
>>
>> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
>> +
>> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
>> return fails ? 1 : 0;
>> }
>> --
>> 1.7.9.5
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 6:47 ` 李春奇 <Arthur Chunqi Li>
@ 2013-06-06 7:01 ` Gleb Natapov
2013-06-06 7:42 ` 李春奇 <Arthur Chunqi Li>
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-06-06 7:01 UTC (permalink / raw)
To: 李春奇 <Arthur Chunqi Li>; +Cc: kvm, Paolo Bonzini
On Thu, Jun 06, 2013 at 02:47:49PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
> On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
> >> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
> >>
> > We have growing number of instructions tests using the same tlb trick. I
> > think it is time to make the code more generic. Create a function that
> > receives instruction to check and all the tlb games will be done by that
> > function.
> Should I do some work to merge all these test cases?
>
It would be nice, yes. I also have an idea on how to improve test
reliability. Since it relies on tlb to be out of sync with actual page
table a vmexit in a wrong time can break this assumption and wrong
instruction will be emulated (the one from insn_page instead of
alt_insn_page). If we make the instruction on insn_page place special
value somewhere and check it after the test we can see if the wrong
instruction was executed and rerun the test.
> >
> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> >> ---
> >> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 41 insertions(+)
> >>
> >> diff --git a/x86/emulator.c b/x86/emulator.c
> >> index 96576e5..3563971 100644
> >> --- a/x86/emulator.c
> >> +++ b/x86/emulator.c
> >> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
> >> report("test", *mem == 0x8400);
> >> }
> >>
> >> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
> >> + uint8_t *alt_insn_page, void *insn_ram)
> >> +{
> >> + ulong *cr3 = (ulong *)read_cr3();
> >> + uint16_t cx = 0;
> >> +
> >> + // Pad with RET instructions
> >> + memset(insn_page, 0xc3, 4096);
> >> + memset(alt_insn_page, 0xc3, 4096);
> >> + // Place a trapping instruction in the page to trigger a VMEXIT
> >> + insn_page[0] = 0x66; // mov $0x4321, %cx
> >> + insn_page[1] = 0xb9;
> >> + insn_page[2] = 0x21;
> >> + insn_page[3] = 0x43;
> >> + insn_page[4] = 0x89; // mov %eax, (%rax)
> >> + insn_page[5] = 0x00;
> >> + insn_page[6] = 0x90; // nop
> >> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
> >> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
> >> + // %cx will be broken to 0x2121, not 0x4321
> >> + alt_insn_page[4] = 0x40;
> >> + alt_insn_page[5] = 0x88;
> >> + alt_insn_page[6] = 0xcd;
> >> +
> >> + // Load the code TLB with insn_page, but point the page tables at
> >> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
> >> + // This will make the CPU trap on the insn_page instruction but the
> >> + // hypervisor will see alt_insn_page.
> >> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
> >> + // Load code TLB
> >> + invlpg(insn_ram);
> >> + asm volatile("call *%0" : : "r"(insn_ram+3));
> >> + // Trap, let hypervisor emulate at alt_insn_page
> >> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
> >> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
> >> + asm volatile("":"=c"(cx));
> > Why not add the constrain to previous asm?
> I will merge them in next version.
>
> >
> >> + report("access bpl in modr/m", cx == 0x4321);
> >> +}
> >> +
> >> int main()
> >> {
> >> void *mem;
> >> @@ -964,6 +1003,8 @@ int main()
> >>
> >> test_string_io_mmio(mem);
> >>
> >> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
> >> +
> >> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
> >> return fails ? 1 : 0;
> >> }
> >> --
> >> 1.7.9.5
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 7:01 ` Gleb Natapov
@ 2013-06-06 7:42 ` 李春奇 <Arthur Chunqi Li>
2013-06-06 7:45 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: 李春奇 <Arthur Chunqi Li> @ 2013-06-06 7:42 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, Paolo Bonzini
On Thu, Jun 6, 2013 at 3:01 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Thu, Jun 06, 2013 at 02:47:49PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
>> On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> > On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
>> >> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
>> >>
>> > We have growing number of instructions tests using the same tlb trick. I
>> > think it is time to make the code more generic. Create a function that
>> > receives instruction to check and all the tlb games will be done by that
>> > function.
>> Should I do some work to merge all these test cases?
>>
> It would be nice, yes. I also have an idea on how to improve test
> reliability. Since it relies on tlb to be out of sync with actual page
> table a vmexit in a wrong time can break this assumption and wrong
> instruction will be emulated (the one from insn_page instead of
> alt_insn_page). If we make the instruction on insn_page place special
> value somewhere and check it after the test we can see if the wrong
> instruction was executed and rerun the test.
If I commit the patch to merge these test cases, should the patch base
on what I have commit before (after patched of this mail), or base on
the master thread?
Arthur
>
>> >
>> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> >> ---
>> >> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> >> 1 file changed, 41 insertions(+)
>> >>
>> >> diff --git a/x86/emulator.c b/x86/emulator.c
>> >> index 96576e5..3563971 100644
>> >> --- a/x86/emulator.c
>> >> +++ b/x86/emulator.c
>> >> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
>> >> report("test", *mem == 0x8400);
>> >> }
>> >>
>> >> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
>> >> + uint8_t *alt_insn_page, void *insn_ram)
>> >> +{
>> >> + ulong *cr3 = (ulong *)read_cr3();
>> >> + uint16_t cx = 0;
>> >> +
>> >> + // Pad with RET instructions
>> >> + memset(insn_page, 0xc3, 4096);
>> >> + memset(alt_insn_page, 0xc3, 4096);
>> >> + // Place a trapping instruction in the page to trigger a VMEXIT
>> >> + insn_page[0] = 0x66; // mov $0x4321, %cx
>> >> + insn_page[1] = 0xb9;
>> >> + insn_page[2] = 0x21;
>> >> + insn_page[3] = 0x43;
>> >> + insn_page[4] = 0x89; // mov %eax, (%rax)
>> >> + insn_page[5] = 0x00;
>> >> + insn_page[6] = 0x90; // nop
>> >> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
>> >> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
>> >> + // %cx will be broken to 0x2121, not 0x4321
>> >> + alt_insn_page[4] = 0x40;
>> >> + alt_insn_page[5] = 0x88;
>> >> + alt_insn_page[6] = 0xcd;
>> >> +
>> >> + // Load the code TLB with insn_page, but point the page tables at
>> >> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
>> >> + // This will make the CPU trap on the insn_page instruction but the
>> >> + // hypervisor will see alt_insn_page.
>> >> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
>> >> + // Load code TLB
>> >> + invlpg(insn_ram);
>> >> + asm volatile("call *%0" : : "r"(insn_ram+3));
>> >> + // Trap, let hypervisor emulate at alt_insn_page
>> >> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
>> >> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
>> >> + asm volatile("":"=c"(cx));
>> > Why not add the constrain to previous asm?
>> I will merge them in next version.
>>
>> >
>> >> + report("access bpl in modr/m", cx == 0x4321);
>> >> +}
>> >> +
>> >> int main()
>> >> {
>> >> void *mem;
>> >> @@ -964,6 +1003,8 @@ int main()
>> >>
>> >> test_string_io_mmio(mem);
>> >>
>> >> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
>> >> +
>> >> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
>> >> return fails ? 1 : 0;
>> >> }
>> >> --
>> >> 1.7.9.5
>> >
>> > --
>> > Gleb.
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 7:42 ` 李春奇 <Arthur Chunqi Li>
@ 2013-06-06 7:45 ` Gleb Natapov
2013-06-06 9:33 ` 李春奇 <Arthur Chunqi Li>
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-06-06 7:45 UTC (permalink / raw)
To: 李春奇 <Arthur Chunqi Li>; +Cc: kvm, Paolo Bonzini
On Thu, Jun 06, 2013 at 03:42:56PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
> On Thu, Jun 6, 2013 at 3:01 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Thu, Jun 06, 2013 at 02:47:49PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
> >> On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> > On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
> >> >> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
> >> >>
> >> > We have growing number of instructions tests using the same tlb trick. I
> >> > think it is time to make the code more generic. Create a function that
> >> > receives instruction to check and all the tlb games will be done by that
> >> > function.
> >> Should I do some work to merge all these test cases?
> >>
> > It would be nice, yes. I also have an idea on how to improve test
> > reliability. Since it relies on tlb to be out of sync with actual page
> > table a vmexit in a wrong time can break this assumption and wrong
> > instruction will be emulated (the one from insn_page instead of
> > alt_insn_page). If we make the instruction on insn_page place special
> > value somewhere and check it after the test we can see if the wrong
> > instruction was executed and rerun the test.
> If I commit the patch to merge these test cases, should the patch base
> on what I have commit before (after patched of this mail), or base on
> the master thread?
>
Master. First patch provides the infrastructure. Second converts
existing users. Third adds new tests.
> Arthur
>
> >
> >> >
> >> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> >> >> ---
> >> >> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >> >> 1 file changed, 41 insertions(+)
> >> >>
> >> >> diff --git a/x86/emulator.c b/x86/emulator.c
> >> >> index 96576e5..3563971 100644
> >> >> --- a/x86/emulator.c
> >> >> +++ b/x86/emulator.c
> >> >> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
> >> >> report("test", *mem == 0x8400);
> >> >> }
> >> >>
> >> >> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
> >> >> + uint8_t *alt_insn_page, void *insn_ram)
> >> >> +{
> >> >> + ulong *cr3 = (ulong *)read_cr3();
> >> >> + uint16_t cx = 0;
> >> >> +
> >> >> + // Pad with RET instructions
> >> >> + memset(insn_page, 0xc3, 4096);
> >> >> + memset(alt_insn_page, 0xc3, 4096);
> >> >> + // Place a trapping instruction in the page to trigger a VMEXIT
> >> >> + insn_page[0] = 0x66; // mov $0x4321, %cx
> >> >> + insn_page[1] = 0xb9;
> >> >> + insn_page[2] = 0x21;
> >> >> + insn_page[3] = 0x43;
> >> >> + insn_page[4] = 0x89; // mov %eax, (%rax)
> >> >> + insn_page[5] = 0x00;
> >> >> + insn_page[6] = 0x90; // nop
> >> >> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
> >> >> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
> >> >> + // %cx will be broken to 0x2121, not 0x4321
> >> >> + alt_insn_page[4] = 0x40;
> >> >> + alt_insn_page[5] = 0x88;
> >> >> + alt_insn_page[6] = 0xcd;
> >> >> +
> >> >> + // Load the code TLB with insn_page, but point the page tables at
> >> >> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
> >> >> + // This will make the CPU trap on the insn_page instruction but the
> >> >> + // hypervisor will see alt_insn_page.
> >> >> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
> >> >> + // Load code TLB
> >> >> + invlpg(insn_ram);
> >> >> + asm volatile("call *%0" : : "r"(insn_ram+3));
> >> >> + // Trap, let hypervisor emulate at alt_insn_page
> >> >> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
> >> >> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
> >> >> + asm volatile("":"=c"(cx));
> >> > Why not add the constrain to previous asm?
> >> I will merge them in next version.
> >>
> >> >
> >> >> + report("access bpl in modr/m", cx == 0x4321);
> >> >> +}
> >> >> +
> >> >> int main()
> >> >> {
> >> >> void *mem;
> >> >> @@ -964,6 +1003,8 @@ int main()
> >> >>
> >> >> test_string_io_mmio(mem);
> >> >>
> >> >> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
> >> >> +
> >> >> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
> >> >> return fails ? 1 : 0;
> >> >> }
> >> >> --
> >> >> 1.7.9.5
> >> >
> >> > --
> >> > Gleb.
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 7:45 ` Gleb Natapov
@ 2013-06-06 9:33 ` 李春奇 <Arthur Chunqi Li>
2013-06-06 9:41 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: 李春奇 <Arthur Chunqi Li> @ 2013-06-06 9:33 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, Paolo Bonzini
On Thu, Jun 6, 2013 at 3:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Thu, Jun 06, 2013 at 03:42:56PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
>> On Thu, Jun 6, 2013 at 3:01 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> > On Thu, Jun 06, 2013 at 02:47:49PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
>> >> On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> >> > On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
>> >> >> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
>> >> >>
>> >> > We have growing number of instructions tests using the same tlb trick. I
>> >> > think it is time to make the code more generic. Create a function that
>> >> > receives instruction to check and all the tlb games will be done by that
>> >> > function.
>> >> Should I do some work to merge all these test cases?
>> >>
>> > It would be nice, yes. I also have an idea on how to improve test
>> > reliability. Since it relies on tlb to be out of sync with actual page
>> > table a vmexit in a wrong time can break this assumption and wrong
>> > instruction will be emulated (the one from insn_page instead of
>> > alt_insn_page). If we make the instruction on insn_page place special
>> > value somewhere and check it after the test we can see if the wrong
>> > instruction was executed and rerun the test.
>> If I commit the patch to merge these test cases, should the patch base
>> on what I have commit before (after patched of this mail), or base on
>> the master thread?
>>
> Master. First patch provides the infrastructure. Second converts
> existing users. Third adds new tests.
There are some problems packaging it. For some test cases some
registers should be set before call alt_insn_page and some registers
should be returned after it (such as test_movabs). If the emulator
part is packed, the initialization before and result got after it may
be invalid. Add two functions designed by caller to initialize and get
result may cause the parameter table too long. Do you have any
suggestions about this?
>
>> Arthur
>>
>> >
>> >> >
>> >> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> >> >> ---
>> >> >> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> >> >> 1 file changed, 41 insertions(+)
>> >> >>
>> >> >> diff --git a/x86/emulator.c b/x86/emulator.c
>> >> >> index 96576e5..3563971 100644
>> >> >> --- a/x86/emulator.c
>> >> >> +++ b/x86/emulator.c
>> >> >> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
>> >> >> report("test", *mem == 0x8400);
>> >> >> }
>> >> >>
>> >> >> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
>> >> >> + uint8_t *alt_insn_page, void *insn_ram)
>> >> >> +{
>> >> >> + ulong *cr3 = (ulong *)read_cr3();
>> >> >> + uint16_t cx = 0;
>> >> >> +
>> >> >> + // Pad with RET instructions
>> >> >> + memset(insn_page, 0xc3, 4096);
>> >> >> + memset(alt_insn_page, 0xc3, 4096);
>> >> >> + // Place a trapping instruction in the page to trigger a VMEXIT
>> >> >> + insn_page[0] = 0x66; // mov $0x4321, %cx
>> >> >> + insn_page[1] = 0xb9;
>> >> >> + insn_page[2] = 0x21;
>> >> >> + insn_page[3] = 0x43;
>> >> >> + insn_page[4] = 0x89; // mov %eax, (%rax)
>> >> >> + insn_page[5] = 0x00;
>> >> >> + insn_page[6] = 0x90; // nop
>> >> >> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
>> >> >> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
>> >> >> + // %cx will be broken to 0x2121, not 0x4321
>> >> >> + alt_insn_page[4] = 0x40;
>> >> >> + alt_insn_page[5] = 0x88;
>> >> >> + alt_insn_page[6] = 0xcd;
>> >> >> +
>> >> >> + // Load the code TLB with insn_page, but point the page tables at
>> >> >> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
>> >> >> + // This will make the CPU trap on the insn_page instruction but the
>> >> >> + // hypervisor will see alt_insn_page.
>> >> >> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
>> >> >> + // Load code TLB
>> >> >> + invlpg(insn_ram);
>> >> >> + asm volatile("call *%0" : : "r"(insn_ram+3));
>> >> >> + // Trap, let hypervisor emulate at alt_insn_page
>> >> >> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
>> >> >> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
>> >> >> + asm volatile("":"=c"(cx));
>> >> > Why not add the constrain to previous asm?
>> >> I will merge them in next version.
>> >>
>> >> >
>> >> >> + report("access bpl in modr/m", cx == 0x4321);
>> >> >> +}
>> >> >> +
>> >> >> int main()
>> >> >> {
>> >> >> void *mem;
>> >> >> @@ -964,6 +1003,8 @@ int main()
>> >> >>
>> >> >> test_string_io_mmio(mem);
>> >> >>
>> >> >> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
>> >> >> +
>> >> >> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
>> >> >> return fails ? 1 : 0;
>> >> >> }
>> >> >> --
>> >> >> 1.7.9.5
>> >> >
>> >> > --
>> >> > Gleb.
>> >
>> > --
>> > Gleb.
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m
2013-06-06 9:33 ` 李春奇 <Arthur Chunqi Li>
@ 2013-06-06 9:41 ` Gleb Natapov
0 siblings, 0 replies; 8+ messages in thread
From: Gleb Natapov @ 2013-06-06 9:41 UTC (permalink / raw)
To: 李春奇 <Arthur Chunqi Li>; +Cc: kvm, Paolo Bonzini
On Thu, Jun 06, 2013 at 05:33:52PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
> On Thu, Jun 6, 2013 at 3:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Thu, Jun 06, 2013 at 03:42:56PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
> >> On Thu, Jun 6, 2013 at 3:01 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> > On Thu, Jun 06, 2013 at 02:47:49PM +0800, 李春奇 <Arthur Chunqi Li> wrote:
> >> >> On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> >> > On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wrote:
> >> >> >> Test access to %bpl via modr/m addressing mode. This case can test another bug in the boot of RHEL5.9 64-bit.
> >> >> >>
> >> >> > We have growing number of instructions tests using the same tlb trick. I
> >> >> > think it is time to make the code more generic. Create a function that
> >> >> > receives instruction to check and all the tlb games will be done by that
> >> >> > function.
> >> >> Should I do some work to merge all these test cases?
> >> >>
> >> > It would be nice, yes. I also have an idea on how to improve test
> >> > reliability. Since it relies on tlb to be out of sync with actual page
> >> > table a vmexit in a wrong time can break this assumption and wrong
> >> > instruction will be emulated (the one from insn_page instead of
> >> > alt_insn_page). If we make the instruction on insn_page place special
> >> > value somewhere and check it after the test we can see if the wrong
> >> > instruction was executed and rerun the test.
> >> If I commit the patch to merge these test cases, should the patch base
> >> on what I have commit before (after patched of this mail), or base on
> >> the master thread?
> >>
> > Master. First patch provides the infrastructure. Second converts
> > existing users. Third adds new tests.
> There are some problems packaging it. For some test cases some
> registers should be set before call alt_insn_page and some registers
> should be returned after it (such as test_movabs). If the emulator
> part is packed, the initialization before and result got after it may
> be invalid. Add two functions designed by caller to initialize and get
> result may cause the parameter table too long. Do you have any
> suggestions about this?
>
You can do what realmode.c does. It has inregs and outregs structures,
registers are set to inregs values before a test and saved to outregs
after. You can examine outregs to check correctness instead of checking
HW registers directly.
> >
> >> Arthur
> >>
> >> >
> >> >> >
> >> >> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> >> >> >> ---
> >> >> >> x86/emulator.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >> >> >> 1 file changed, 41 insertions(+)
> >> >> >>
> >> >> >> diff --git a/x86/emulator.c b/x86/emulator.c
> >> >> >> index 96576e5..3563971 100644
> >> >> >> --- a/x86/emulator.c
> >> >> >> +++ b/x86/emulator.c
> >> >> >> @@ -901,6 +901,45 @@ static void test_simplealu(u32 *mem)
> >> >> >> report("test", *mem == 0x8400);
> >> >> >> }
> >> >> >>
> >> >> >> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_page,
> >> >> >> + uint8_t *alt_insn_page, void *insn_ram)
> >> >> >> +{
> >> >> >> + ulong *cr3 = (ulong *)read_cr3();
> >> >> >> + uint16_t cx = 0;
> >> >> >> +
> >> >> >> + // Pad with RET instructions
> >> >> >> + memset(insn_page, 0xc3, 4096);
> >> >> >> + memset(alt_insn_page, 0xc3, 4096);
> >> >> >> + // Place a trapping instruction in the page to trigger a VMEXIT
> >> >> >> + insn_page[0] = 0x66; // mov $0x4321, %cx
> >> >> >> + insn_page[1] = 0xb9;
> >> >> >> + insn_page[2] = 0x21;
> >> >> >> + insn_page[3] = 0x43;
> >> >> >> + insn_page[4] = 0x89; // mov %eax, (%rax)
> >> >> >> + insn_page[5] = 0x00;
> >> >> >> + insn_page[6] = 0x90; // nop
> >> >> >> + // Place mov %cl, %bpl in alt_insn_page for emulator to execuate
> >> >> >> + // If emulator mistaken addressing %bpl, %cl may be moved to %ch
> >> >> >> + // %cx will be broken to 0x2121, not 0x4321
> >> >> >> + alt_insn_page[4] = 0x40;
> >> >> >> + alt_insn_page[5] = 0x88;
> >> >> >> + alt_insn_page[6] = 0xcd;
> >> >> >> +
> >> >> >> + // Load the code TLB with insn_page, but point the page tables at
> >> >> >> + // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
> >> >> >> + // This will make the CPU trap on the insn_page instruction but the
> >> >> >> + // hypervisor will see alt_insn_page.
> >> >> >> + install_page(cr3, virt_to_phys(insn_page), insn_ram);
> >> >> >> + // Load code TLB
> >> >> >> + invlpg(insn_ram);
> >> >> >> + asm volatile("call *%0" : : "r"(insn_ram+3));
> >> >> >> + // Trap, let hypervisor emulate at alt_insn_page
> >> >> >> + install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
> >> >> >> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem));
> >> >> >> + asm volatile("":"=c"(cx));
> >> >> > Why not add the constrain to previous asm?
> >> >> I will merge them in next version.
> >> >>
> >> >> >
> >> >> >> + report("access bpl in modr/m", cx == 0x4321);
> >> >> >> +}
> >> >> >> +
> >> >> >> int main()
> >> >> >> {
> >> >> >> void *mem;
> >> >> >> @@ -964,6 +1003,8 @@ int main()
> >> >> >>
> >> >> >> test_string_io_mmio(mem);
> >> >> >>
> >> >> >> + test_bpl_modrm(mem, insn_page, alt_insn_page, insn_ram);
> >> >> >> +
> >> >> >> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
> >> >> >> return fails ? 1 : 0;
> >> >> >> }
> >> >> >> --
> >> >> >> 1.7.9.5
> >> >> >
> >> >> > --
> >> >> > Gleb.
> >> >
> >> > --
> >> > Gleb.
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-06-06 9:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-06 5:03 [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m Arthur Chunqi Li
2013-06-06 5:45 ` Gleb Natapov
2013-06-06 6:47 ` 李春奇 <Arthur Chunqi Li>
2013-06-06 7:01 ` Gleb Natapov
2013-06-06 7:42 ` 李春奇 <Arthur Chunqi Li>
2013-06-06 7:45 ` Gleb Natapov
2013-06-06 9:33 ` 李春奇 <Arthur Chunqi Li>
2013-06-06 9:41 ` Gleb Natapov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).