| Message ID | 1463414992-8357-6-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16/05/16 19:09, Peter Maydell wrote: > The exception_action() function in user-exec.c is just a call to > cpu_loop_exit() for every target CPU except i386. Since this > function is only called if the target's handle_mmu_fault() hook has > indicated an MMU fault, and that hook is only called from the > handle_cpu_signal() code path, we can simply move the x86-specific > setup into that hook, which allows us to remove the TARGET_I386 > ifdef from user-exec.c. > > Of the actions that were done by the call to raise_interrupt_err(): > * cpu_svm_check_intercept_param() is a no-op in user mode > * check_exception() is a no-op since double faults are impossible > for user-mode > * assignments to cs->exception_index and env->error_code are no-ops > * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since > pc is 0 > which leaves just setting env_>exception_is_int and > env->exception_next_eip as actions that need to be added to > x86_cpu_handle_mmu_fault(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target-i386/helper.c | 2 ++ > user-exec.c | 16 +--------------- > 2 files changed, 3 insertions(+), 15 deletions(-) > > diff --git a/target-i386/helper.c b/target-i386/helper.c > index bf3e762..e1dde46 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr, > env->error_code = (is_write << PG_ERROR_W_BIT); > env->error_code |= PG_ERROR_U_MASK; > cs->exception_index = EXCP0E_PAGE; > + env->exception_is_int = 0; > + env->exception_next_eip = env->eip; 'env->eip' was updated by restore_state_to_opc() from cpu_restore_state_from_tb() from cpu_restore_state() from handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_ calling exception_action(). Kind regards, Sergey > return 1; > } > > diff --git a/user-exec.c b/user-exec.c > index ad669f4..439bb37 100644 > --- a/user-exec.c > +++ b/user-exec.c > @@ -39,18 +39,6 @@ > > //#define DEBUG_SIGNAL > > -static void exception_action(CPUState *cpu) > -{ > -#if defined(TARGET_I386) > - X86CPU *x86_cpu = X86_CPU(cpu); > - CPUX86State *env1 = &x86_cpu->env; > - > - raise_exception_err(env1, cpu->exception_index, env1->error_code); > -#else > - cpu_loop_exit(cpu); > -#endif > -} > - > /* exit the current TB from a signal handler. The host registers are > restored in a state compatible with the CPU emulator > */ > @@ -119,10 +107,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address, > /* now we have a real cpu fault */ > cpu_restore_state(cpu, pc); > > - /* we restore the process signal mask as the sigreturn should > - do it (XXX: use sigsetjmp) */ > sigprocmask(SIG_SETMASK, old_set, NULL); > - exception_action(cpu); > + cpu_loop_exit(cpu); > > /* never comes here */ > return 1;
On 16 May 2016 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > On 16/05/16 19:09, Peter Maydell wrote: >> The exception_action() function in user-exec.c is just a call to >> cpu_loop_exit() for every target CPU except i386. Since this >> function is only called if the target's handle_mmu_fault() hook has >> indicated an MMU fault, and that hook is only called from the >> handle_cpu_signal() code path, we can simply move the x86-specific >> setup into that hook, which allows us to remove the TARGET_I386 >> ifdef from user-exec.c. >> >> Of the actions that were done by the call to raise_interrupt_err(): >> * cpu_svm_check_intercept_param() is a no-op in user mode >> * check_exception() is a no-op since double faults are impossible >> for user-mode >> * assignments to cs->exception_index and env->error_code are no-ops >> * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since >> pc is 0 >> which leaves just setting env_>exception_is_int and >> env->exception_next_eip as actions that need to be added to >> x86_cpu_handle_mmu_fault(). >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> target-i386/helper.c | 2 ++ >> user-exec.c | 16 +--------------- >> 2 files changed, 3 insertions(+), 15 deletions(-) >> >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index bf3e762..e1dde46 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr, >> env->error_code = (is_write << PG_ERROR_W_BIT); >> env->error_code |= PG_ERROR_U_MASK; >> cs->exception_index = EXCP0E_PAGE; >> + env->exception_is_int = 0; >> + env->exception_next_eip = env->eip; > > 'env->eip' was updated by restore_state_to_opc() from > cpu_restore_state_from_tb() from cpu_restore_state() from > handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_ > calling exception_action(). Oops, nice catch. (I wonder if any of the other target architectures are not correctly doing things in their handle_mmu_fault function because the cpu_restore_state() call happens later?) thanks -- PMM
On 16 May 2016 at 19:33, Peter Maydell <peter.maydell@linaro.org> wrote: > On 16 May 2016 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote: >> 'env->eip' was updated by restore_state_to_opc() from >> cpu_restore_state_from_tb() from cpu_restore_state() from >> handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_ >> calling exception_action(). > > Oops, nice catch. (I wonder if any of the other target architectures > are not correctly doing things in their handle_mmu_fault function > because the cpu_restore_state() call happens later?) Looking at the other target architectures they're OK because they don't do very much in the handle_mmu_fault function. Since every single handle_mmu_fault function always returns 1 (ignoring one or two clearly softmmu-only versions) we could in theory call cpu_restore_state() before the handle_mmu_fault hook. However since in the softmmu case the equivalent code is also called in a pre-restore-state setup it seems more consistent to keep the user-exec.c code the order it is now. So the target-i386 code needs rearranging a bit I guess (perhaps to save the offset rather than the actual next eip?) I think patches 1..4 are still worthwhile even if we drop this one for now, though. thanks -- PMM
On 16 May 2016 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > On 16/05/16 19:09, Peter Maydell wrote: >> The exception_action() function in user-exec.c is just a call to >> cpu_loop_exit() for every target CPU except i386. Since this >> function is only called if the target's handle_mmu_fault() hook has >> indicated an MMU fault, and that hook is only called from the >> handle_cpu_signal() code path, we can simply move the x86-specific >> setup into that hook, which allows us to remove the TARGET_I386 >> ifdef from user-exec.c. >> >> Of the actions that were done by the call to raise_interrupt_err(): >> * cpu_svm_check_intercept_param() is a no-op in user mode >> * check_exception() is a no-op since double faults are impossible >> for user-mode >> * assignments to cs->exception_index and env->error_code are no-ops >> * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since >> pc is 0 >> which leaves just setting env_>exception_is_int and >> env->exception_next_eip as actions that need to be added to >> x86_cpu_handle_mmu_fault(). >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> target-i386/helper.c | 2 ++ >> user-exec.c | 16 +--------------- >> 2 files changed, 3 insertions(+), 15 deletions(-) >> >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index bf3e762..e1dde46 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr, >> env->error_code = (is_write << PG_ERROR_W_BIT); >> env->error_code |= PG_ERROR_U_MASK; >> cs->exception_index = EXCP0E_PAGE; >> + env->exception_is_int = 0; >> + env->exception_next_eip = env->eip; > > 'env->eip' was updated by restore_state_to_opc() from > cpu_restore_state_from_tb() from cpu_restore_state() from > handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_ > calling exception_action(). I looked a bit closer at this, and I think that we're actually OK to just not set exception_next_eip (or to set it wrongly, according to taste). In the softmmu equivalent code path, tlb_fill() will call raise_exception_err_ra() for a page fault. This code path also ends up doing env->exception_next_eip = env->eip before it has done a cpu_restore_state() [in this case the restore-state happens in cpu_loop_exit_restore()]. But this is OK because we only use env->exception_next_eip as the value to pass into do_interrupt_all(), which states that next_eip is only valid if is_int is true (ie we got here from an int instruction or syscall). For do_interrupt_user() it is also true that next_eip only matters if is_int (or if into is EXCP_SYSCALL), so we can safely just not update exception_next_eip here in x86_cpu_handle_mmu_fault(). So I think the best approach is: (1) in this patch, set exception_next_eip to -1 (clearly indicating that it is not supposed to be valid) (2) add a comment to do_interrupt_user() like the one for do_interrupt_all() noting that next_eip is only valid in some cases. thanks -- PMM
diff --git a/target-i386/helper.c b/target-i386/helper.c index bf3e762..e1dde46 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr, env->error_code = (is_write << PG_ERROR_W_BIT); env->error_code |= PG_ERROR_U_MASK; cs->exception_index = EXCP0E_PAGE; + env->exception_is_int = 0; + env->exception_next_eip = env->eip; return 1; } diff --git a/user-exec.c b/user-exec.c index ad669f4..439bb37 100644 --- a/user-exec.c +++ b/user-exec.c @@ -39,18 +39,6 @@ //#define DEBUG_SIGNAL -static void exception_action(CPUState *cpu) -{ -#if defined(TARGET_I386) - X86CPU *x86_cpu = X86_CPU(cpu); - CPUX86State *env1 = &x86_cpu->env; - - raise_exception_err(env1, cpu->exception_index, env1->error_code); -#else - cpu_loop_exit(cpu); -#endif -} - /* exit the current TB from a signal handler. The host registers are restored in a state compatible with the CPU emulator */ @@ -119,10 +107,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address, /* now we have a real cpu fault */ cpu_restore_state(cpu, pc); - /* we restore the process signal mask as the sigreturn should - do it (XXX: use sigsetjmp) */ sigprocmask(SIG_SETMASK, old_set, NULL); - exception_action(cpu); + cpu_loop_exit(cpu); /* never comes here */ return 1;
The exception_action() function in user-exec.c is just a call to cpu_loop_exit() for every target CPU except i386. Since this function is only called if the target's handle_mmu_fault() hook has indicated an MMU fault, and that hook is only called from the handle_cpu_signal() code path, we can simply move the x86-specific setup into that hook, which allows us to remove the TARGET_I386 ifdef from user-exec.c. Of the actions that were done by the call to raise_interrupt_err(): * cpu_svm_check_intercept_param() is a no-op in user mode * check_exception() is a no-op since double faults are impossible for user-mode * assignments to cs->exception_index and env->error_code are no-ops * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since pc is 0 which leaves just setting env_>exception_is_int and env->exception_next_eip as actions that need to be added to x86_cpu_handle_mmu_fault(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target-i386/helper.c | 2 ++ user-exec.c | 16 +--------------- 2 files changed, 3 insertions(+), 15 deletions(-)