From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH] kvm-unit-tests: Add test case for accessing bpl via modr/m Date: Thu, 6 Jun 2013 12:41:35 +0300 Message-ID: <20130606094135.GA4725@redhat.com> References: <1370495024-3712-1-git-send-email-yzt356@gmail.com> <20130606054502.GN4725@redhat.com> <20130606070155.GS4725@redhat.com> <20130606074514.GW4725@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm , Paolo Bonzini To: =?utf-8?B?5p2O5pil5aWHIDxBcnRodXIgQ2h1bnFpIExpPg==?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:28158 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751254Ab3FFJli convert rfc822-to-8bit (ORCPT ); Thu, 6 Jun 2013 05:41:38 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jun 06, 2013 at 05:33:52PM +0800, =E6=9D=8E=E6=98=A5=E5=A5=87 <= Arthur Chunqi Li> wrote: > On Thu, Jun 6, 2013 at 3:45 PM, Gleb Natapov wrote: > > On Thu, Jun 06, 2013 at 03:42:56PM +0800, =E6=9D=8E=E6=98=A5=E5=A5=87= wrote: > >> On Thu, Jun 6, 2013 at 3:01 PM, Gleb Natapov wro= te: > >> > On Thu, Jun 06, 2013 at 02:47:49PM +0800, =E6=9D=8E=E6=98=A5=E5=A5= =87 wrote: > >> >> On Thu, Jun 6, 2013 at 1:45 PM, Gleb Natapov = wrote: > >> >> > On Thu, Jun 06, 2013 at 01:03:44PM +0800, Arthur Chunqi Li wr= ote: > >> >> >> Test access to %bpl via modr/m addressing mode. This case ca= n test another bug in the boot of RHEL5.9 64-bit. > >> >> >> > >> >> > We have growing number of instructions tests using the same t= lb trick. I > >> >> > think it is time to make the code more generic. Create a func= tion that > >> >> > receives instruction to check and all the tlb games will be d= one 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 tes= t > >> > reliability. Since it relies on tlb to be out of sync with actua= l page > >> > table a vmexit in a wrong time can break this assumption and wro= ng > >> > instruction will be emulated (the one from insn_page instead of > >> > alt_insn_page). If we make the instruction on insn_page place sp= ecial > >> > value somewhere and check it after the test we can see if the wr= ong > >> > 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 ge= t > result may cause the parameter table too long. Do you have any > suggestions about this? >=20 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 > >> >> >> --- > >> >> >> 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 =3D=3D 0x8400); > >> >> >> } > >> >> >> > >> >> >> +static void test_bpl_modrm(uint64_t *mem, uint8_t *insn_pag= e, > >> >> >> + uint8_t *alt_insn_page, void *insn_r= am) > >> >> >> +{ > >> >> >> + ulong *cr3 =3D (ulong *)read_cr3(); > >> >> >> + uint16_t cx =3D 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] =3D 0x66; // mov $0x4321, %cx > >> >> >> + insn_page[1] =3D 0xb9; > >> >> >> + insn_page[2] =3D 0x21; > >> >> >> + insn_page[3] =3D 0x43; > >> >> >> + insn_page[4] =3D 0x89; // mov %eax, (%rax) > >> >> >> + insn_page[5] =3D 0x00; > >> >> >> + insn_page[6] =3D 0x90; // nop > >> >> >> + // Place mov %cl, %bpl in alt_insn_page for emulator t= o execuate > >> >> >> + // If emulator mistaken addressing %bpl, %cl may be mo= ved to %ch > >> >> >> + // %cx will be broken to 0x2121, not 0x4321 > >> >> >> + alt_insn_page[4] =3D 0x40; > >> >> >> + alt_insn_page[5] =3D 0x88; > >> >> >> + alt_insn_page[6] =3D 0xcd; > >> >> >> + > >> >> >> + // Load the code TLB with insn_page, but point the pag= e 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 instru= ction 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_ra= m); > >> >> >> + asm volatile("call *%0" : : "r"(insn_ram), "a"(mem)); > >> >> >> + asm volatile("":"=3Dc"(cx)); > >> >> > Why not add the constrain to previous asm? > >> >> I will merge them in next version. > >> >> > >> >> > > >> >> >> + report("access bpl in modr/m", cx =3D=3D 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, fa= ils); > >> >> >> return fails ? 1 : 0; > >> >> >> } > >> >> >> -- > >> >> >> 1.7.9.5 > >> >> > > >> >> > -- > >> >> > Gleb. > >> > > >> > -- > >> > Gleb. > > > > -- > > Gleb. -- Gleb.