| Message ID | 20210203012842.101447-3-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nSVM: Test host RFLAGS.TF on VMRUN | expand |
On 03/02/21 02:28, Krish Sadhukhan wrote: > Add helper functions to set host RFLAGS.TF immediately before the VMRUN > instruction. These will be used by the next patch to test Single Stepping > on the VMRUN instruction from the host's perspective. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> I think you can use prepare_gif_clear to set RFLAGS.TF and the exception handler can: 1) look for VMRUN at the interrupted EIP. If it is there store the VMRUN address and set a flag. 2) on the next #DB (flag set), store the EIP and clear the flag The finished callback then checks that the EIP was stored and that the two EIPs are 3 bytes apart (the length of a VMRUN). Paolo > --- > x86/svm.c | 24 ++++++++++++++++++++++-- > x86/svm.h | 3 +++ > x86/svm_tests.c | 1 - > 3 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/x86/svm.c b/x86/svm.c > index a1808c7..547f62a 100644 > --- a/x86/svm.c > +++ b/x86/svm.c > @@ -179,6 +179,17 @@ void vmcb_ident(struct vmcb *vmcb) > } > } > > +static bool ss_bp_on_vmrun = false; > +void set_ss_bp_on_vmrun(void) > +{ > + ss_bp_on_vmrun = true; > +} > + > +void unset_ss_bp_on_vmrun(void) > +{ > + ss_bp_on_vmrun = false; > +} > + > struct regs regs; > > struct regs get_regs(void) > @@ -215,6 +226,12 @@ struct svm_test *v2_test; > "mov regs, %%r15\n\t" \ > "mov %%r15, 0x1f8(%%rax)\n\t" \ > LOAD_GPR_C \ > + "cmpb $0, %[ss_bp]\n\t" \ > + "je 1f\n\t" \ > + "pushf; pop %%r8\n\t" \ > + "or $0x100, %%r8\n\t" \ > + "push %%r8; popf\n\t" \ > + "1: " \ > "vmrun %%rax\n\t" \ > SAVE_GPR_C \ > "mov 0x170(%%rax), %%r15\n\t" \ > @@ -234,7 +251,8 @@ int svm_vmrun(void) > asm volatile ( > ASM_VMRUN_CMD > : > - : "a" (virt_to_phys(vmcb)) > + : "a" (virt_to_phys(vmcb)), > + [ss_bp]"m"(ss_bp_on_vmrun) > : "memory", "r15"); > > return (vmcb->control.exit_code); > @@ -253,6 +271,7 @@ static void test_run(struct svm_test *test) > do { > struct svm_test *the_test = test; > u64 the_vmcb = vmcb_phys; > + > asm volatile ( > "clgi;\n\t" // semi-colon needed for LLVM compatibility > "sti \n\t" > @@ -266,7 +285,8 @@ static void test_run(struct svm_test *test) > "=b" (the_vmcb) // callee save register! > : [test] "0" (the_test), > [vmcb_phys] "1"(the_vmcb), > - [PREPARE_GIF_CLEAR] "i" (offsetof(struct svm_test, prepare_gif_clear)) > + [PREPARE_GIF_CLEAR] "i" (offsetof(struct svm_test, prepare_gif_clear)), > + [ss_bp]"m"(ss_bp_on_vmrun) > : "rax", "rcx", "rdx", "rsi", > "r8", "r9", "r10", "r11" , "r12", "r13", "r14", "r15", > "memory"); > diff --git a/x86/svm.h b/x86/svm.h > index a0863b8..d521972 100644 > --- a/x86/svm.h > +++ b/x86/svm.h > @@ -391,6 +391,9 @@ void vmcb_ident(struct vmcb *vmcb); > struct regs get_regs(void); > void vmmcall(void); > int svm_vmrun(void); > +int svm_vmrun1(void); > +void set_ss_bp_on_vmrun(void); > +void unset_ss_bp_on_vmrun(void); > void test_set_guest(test_guest_func func); > > extern struct vmcb *vmcb; > diff --git a/x86/svm_tests.c b/x86/svm_tests.c > index 29a0b59..7bf3624 100644 > --- a/x86/svm_tests.c > +++ b/x86/svm_tests.c > @@ -2046,7 +2046,6 @@ static void basic_guest_main(struct svm_test *test) > { > } > > - > #define SVM_TEST_REG_RESERVED_BITS(start, end, inc, str_name, reg, val, \ > resv_mask) \ > { \ >
On 2/3/21 12:15 AM, Paolo Bonzini wrote: > On 03/02/21 02:28, Krish Sadhukhan wrote: >> Add helper functions to set host RFLAGS.TF immediately before the VMRUN >> instruction. These will be used by the next patch to test Single >> Stepping >> on the VMRUN instruction from the host's perspective. >> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > > I think you can use prepare_gif_clear to set RFLAGS.TF and the > exception handler can: > > 1) look for VMRUN at the interrupted EIP. If it is there store the > VMRUN address and set a flag. > > 2) on the next #DB (flag set), store the EIP and clear the flag > > The finished callback then checks that the EIP was stored and that the > two EIPs are 3 bytes apart (the length of a VMRUN). Thanks for the suggestion. It worked fine and I have sent out v2. However, I couldn't use the two RIPs (VMRUN and post-VMRUN) to check the result because the post-VMRUN RIP was more than the length of the VMRUN instruction i.e., when #DB handler got executed following guest exit, the RIP had moved forward a few instructions from VMRUN. So, I have used the same mechanism I used in v1, to check the results. > > Paolo > >> --- >> x86/svm.c | 24 ++++++++++++++++++++++-- >> x86/svm.h | 3 +++ >> x86/svm_tests.c | 1 - >> 3 files changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/x86/svm.c b/x86/svm.c >> index a1808c7..547f62a 100644 >> --- a/x86/svm.c >> +++ b/x86/svm.c >> @@ -179,6 +179,17 @@ void vmcb_ident(struct vmcb *vmcb) >> } >> } >> +static bool ss_bp_on_vmrun = false; >> +void set_ss_bp_on_vmrun(void) >> +{ >> + ss_bp_on_vmrun = true; >> +} >> + >> +void unset_ss_bp_on_vmrun(void) >> +{ >> + ss_bp_on_vmrun = false; >> +} >> + >> struct regs regs; >> struct regs get_regs(void) >> @@ -215,6 +226,12 @@ struct svm_test *v2_test; >> "mov regs, %%r15\n\t" \ >> "mov %%r15, 0x1f8(%%rax)\n\t" \ >> LOAD_GPR_C \ >> + "cmpb $0, %[ss_bp]\n\t" \ >> + "je 1f\n\t" \ >> + "pushf; pop %%r8\n\t" \ >> + "or $0x100, %%r8\n\t" \ >> + "push %%r8; popf\n\t" \ >> + "1: " \ >> "vmrun %%rax\n\t" \ >> SAVE_GPR_C \ >> "mov 0x170(%%rax), %%r15\n\t" \ >> @@ -234,7 +251,8 @@ int svm_vmrun(void) >> asm volatile ( >> ASM_VMRUN_CMD >> : >> - : "a" (virt_to_phys(vmcb)) >> + : "a" (virt_to_phys(vmcb)), >> + [ss_bp]"m"(ss_bp_on_vmrun) >> : "memory", "r15"); >> return (vmcb->control.exit_code); >> @@ -253,6 +271,7 @@ static void test_run(struct svm_test *test) >> do { >> struct svm_test *the_test = test; >> u64 the_vmcb = vmcb_phys; >> + >> asm volatile ( >> "clgi;\n\t" // semi-colon needed for LLVM compatibility >> "sti \n\t" >> @@ -266,7 +285,8 @@ static void test_run(struct svm_test *test) >> "=b" (the_vmcb) // callee save register! >> : [test] "0" (the_test), >> [vmcb_phys] "1"(the_vmcb), >> - [PREPARE_GIF_CLEAR] "i" (offsetof(struct svm_test, >> prepare_gif_clear)) >> + [PREPARE_GIF_CLEAR] "i" (offsetof(struct svm_test, >> prepare_gif_clear)), >> + [ss_bp]"m"(ss_bp_on_vmrun) >> : "rax", "rcx", "rdx", "rsi", >> "r8", "r9", "r10", "r11" , "r12", "r13", "r14", "r15", >> "memory"); >> diff --git a/x86/svm.h b/x86/svm.h >> index a0863b8..d521972 100644 >> --- a/x86/svm.h >> +++ b/x86/svm.h >> @@ -391,6 +391,9 @@ void vmcb_ident(struct vmcb *vmcb); >> struct regs get_regs(void); >> void vmmcall(void); >> int svm_vmrun(void); >> +int svm_vmrun1(void); >> +void set_ss_bp_on_vmrun(void); >> +void unset_ss_bp_on_vmrun(void); >> void test_set_guest(test_guest_func func); >> extern struct vmcb *vmcb; >> diff --git a/x86/svm_tests.c b/x86/svm_tests.c >> index 29a0b59..7bf3624 100644 >> --- a/x86/svm_tests.c >> +++ b/x86/svm_tests.c >> @@ -2046,7 +2046,6 @@ static void basic_guest_main(struct svm_test >> *test) >> { >> } >> - >> #define SVM_TEST_REG_RESERVED_BITS(start, end, inc, str_name, reg, >> val, \ >> resv_mask) \ >> { \ >> >
On 05/02/21 01:20, Krish Sadhukhan wrote: >> >> I think you can use prepare_gif_clear to set RFLAGS.TF and the >> exception handler can: >> >> 1) look for VMRUN at the interrupted EIP. If it is there store the >> VMRUN address and set a flag. >> >> 2) on the next #DB (flag set), store the EIP and clear the flag >> >> The finished callback then checks that the EIP was stored and that the >> two EIPs are 3 bytes apart (the length of a VMRUN). > > > Thanks for the suggestion. It worked fine and I have sent out v2. > > However, I couldn't use the two RIPs (VMRUN and post-VMRUN) to check the > result because the post-VMRUN RIP was more than the length of the VMRUN > instruction i.e., when #DB handler got executed following guest exit, > the RIP had moved forward a few instructions from VMRUN. So, I have used > the same mechanism I used in v1, to check the results. Where did it move to? (And could it be a KVM bug?) Paolo
On 2/5/21 12:21 AM, Paolo Bonzini wrote: > On 05/02/21 01:20, Krish Sadhukhan wrote: >>> >>> I think you can use prepare_gif_clear to set RFLAGS.TF and the >>> exception handler can: >>> >>> 1) look for VMRUN at the interrupted EIP. If it is there store the >>> VMRUN address and set a flag. >>> >>> 2) on the next #DB (flag set), store the EIP and clear the flag >>> >>> The finished callback then checks that the EIP was stored and that >>> the two EIPs are 3 bytes apart (the length of a VMRUN). >> >> >> Thanks for the suggestion. It worked fine and I have sent out v2. >> >> However, I couldn't use the two RIPs (VMRUN and post-VMRUN) to check >> the result because the post-VMRUN RIP was more than the length of the >> VMRUN instruction i.e., when #DB handler got executed following guest >> exit, the RIP had moved forward a few instructions from VMRUN. So, I >> have used the same mechanism I used in v1, to check the results. > > Where did it move to? (And could it be a KVM bug?) It moved to the next-to-next instruction and it turned out to be a KVM (SVM) bug. I have added a fix to v3 that I have sent out. > > Paolo >
diff --git a/x86/svm.c b/x86/svm.c index a1808c7..547f62a 100644 --- a/x86/svm.c +++ b/x86/svm.c @@ -179,6 +179,17 @@ void vmcb_ident(struct vmcb *vmcb) } } +static bool ss_bp_on_vmrun = false; +void set_ss_bp_on_vmrun(void) +{ + ss_bp_on_vmrun = true; +} + +void unset_ss_bp_on_vmrun(void) +{ + ss_bp_on_vmrun = false; +} + struct regs regs; struct regs get_regs(void) @@ -215,6 +226,12 @@ struct svm_test *v2_test; "mov regs, %%r15\n\t" \ "mov %%r15, 0x1f8(%%rax)\n\t" \ LOAD_GPR_C \ + "cmpb $0, %[ss_bp]\n\t" \ + "je 1f\n\t" \ + "pushf; pop %%r8\n\t" \ + "or $0x100, %%r8\n\t" \ + "push %%r8; popf\n\t" \ + "1: " \ "vmrun %%rax\n\t" \ SAVE_GPR_C \ "mov 0x170(%%rax), %%r15\n\t" \ @@ -234,7 +251,8 @@ int svm_vmrun(void) asm volatile ( ASM_VMRUN_CMD : - : "a" (virt_to_phys(vmcb)) + : "a" (virt_to_phys(vmcb)), + [ss_bp]"m"(ss_bp_on_vmrun) : "memory", "r15"); return (vmcb->control.exit_code); @@ -253,6 +271,7 @@ static void test_run(struct svm_test *test) do { struct svm_test *the_test = test; u64 the_vmcb = vmcb_phys; + asm volatile ( "clgi;\n\t" // semi-colon needed for LLVM compatibility "sti \n\t" @@ -266,7 +285,8 @@ static void test_run(struct svm_test *test) "=b" (the_vmcb) // callee save register! : [test] "0" (the_test), [vmcb_phys] "1"(the_vmcb), - [PREPARE_GIF_CLEAR] "i" (offsetof(struct svm_test, prepare_gif_clear)) + [PREPARE_GIF_CLEAR] "i" (offsetof(struct svm_test, prepare_gif_clear)), + [ss_bp]"m"(ss_bp_on_vmrun) : "rax", "rcx", "rdx", "rsi", "r8", "r9", "r10", "r11" , "r12", "r13", "r14", "r15", "memory"); diff --git a/x86/svm.h b/x86/svm.h index a0863b8..d521972 100644 --- a/x86/svm.h +++ b/x86/svm.h @@ -391,6 +391,9 @@ void vmcb_ident(struct vmcb *vmcb); struct regs get_regs(void); void vmmcall(void); int svm_vmrun(void); +int svm_vmrun1(void); +void set_ss_bp_on_vmrun(void); +void unset_ss_bp_on_vmrun(void); void test_set_guest(test_guest_func func); extern struct vmcb *vmcb; diff --git a/x86/svm_tests.c b/x86/svm_tests.c index 29a0b59..7bf3624 100644 --- a/x86/svm_tests.c +++ b/x86/svm_tests.c @@ -2046,7 +2046,6 @@ static void basic_guest_main(struct svm_test *test) { } - #define SVM_TEST_REG_RESERVED_BITS(start, end, inc, str_name, reg, val, \ resv_mask) \ { \
Add helper functions to set host RFLAGS.TF immediately before the VMRUN instruction. These will be used by the next patch to test Single Stepping on the VMRUN instruction from the host's perspective. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- x86/svm.c | 24 ++++++++++++++++++++++-- x86/svm.h | 3 +++ x86/svm_tests.c | 1 - 3 files changed, 25 insertions(+), 3 deletions(-)