On Thu, Jun 6, 2013 at 3:45 PM, Gleb Natapov <gleb@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
>> >> >> ---
>> >> >> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
