| Message ID | 20201105212314.9628-3-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: fix various sparc64 guest bugs | expand |
On 11/5/20 1:23 PM, Peter Maydell wrote: > Because QEMU's user-mode emulation just directly accesses guest CPU > state, for SPARC the guest register window state is not the same in > the sparc64_get_context() and sparc64_set_context() functions as it > is for the real kernel's versions of those functions. Specifically, > for the kernel it has saved the user space state such that the O* > registers go into a pt_regs struct as UREG_I*, and the I* registers > have been spilled onto the userspace stack. For QEMU, we haven't > done that, so the guest's O* registers are still in WREG_O* and the > I* registers in WREG_I*. > > The code was already accessing the O* registers correctly for QEMU, > but had copied the kernel code for accessing the I* registers off the > userspace stack. Replace this with direct accesses to fp and i7 in > the CPU state, and add a comment explaining why we differ from the > kernel code here. > > This fix is sufficient to get bash to a shell prompt. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I'm really pretty unsure about our handling of SPARC register > windows here. This fix works, but should we instead be > ensuring that the flush_windows() call cpu_loop() does > before handling this trap has written the I* regs to the > stack ??? > --- Ach, I was so close to being right the last time I tried to clean up this code. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Le 05/11/2020 à 22:23, Peter Maydell a écrit : > Because QEMU's user-mode emulation just directly accesses guest CPU > state, for SPARC the guest register window state is not the same in > the sparc64_get_context() and sparc64_set_context() functions as it > is for the real kernel's versions of those functions. Specifically, > for the kernel it has saved the user space state such that the O* > registers go into a pt_regs struct as UREG_I*, and the I* registers > have been spilled onto the userspace stack. For QEMU, we haven't > done that, so the guest's O* registers are still in WREG_O* and the > I* registers in WREG_I*. > > The code was already accessing the O* registers correctly for QEMU, > but had copied the kernel code for accessing the I* registers off the > userspace stack. Replace this with direct accesses to fp and i7 in > the CPU state, and add a comment explaining why we differ from the > kernel code here. > > This fix is sufficient to get bash to a shell prompt. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I'm really pretty unsure about our handling of SPARC register > windows here. This fix works, but should we instead be > ensuring that the flush_windows() call cpu_loop() does > before handling this trap has written the I* regs to the > stack ??? > --- > linux-user/sparc/signal.c | 47 ++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c > index 57ea1593bfc..c315704b389 100644 > --- a/linux-user/sparc/signal.c > +++ b/linux-user/sparc/signal.c > @@ -403,7 +403,6 @@ void sparc64_set_context(CPUSPARCState *env) > struct target_ucontext *ucp; > target_mc_gregset_t *grp; > abi_ulong pc, npc, tstate; > - abi_ulong fp, i7, w_addr; > unsigned int i; > > ucp_addr = env->regwptr[WREG_O0]; > @@ -447,6 +446,15 @@ void sparc64_set_context(CPUSPARCState *env) > __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5])); > __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6])); > __get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7])); > + > + /* > + * Note that unlike the kernel, we didn't need to mess with the > + * guest register window state to save it into a pt_regs to run > + * the kernel. So for us the guest's O regs are still in WREG_O* > + * (unlike the kernel which has put them in UREG_I* in a pt_regs) > + * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't > + * need to be written back to userspace memory. > + */ > __get_user(env->regwptr[WREG_O0], (&(*grp)[SPARC_MC_O0])); > __get_user(env->regwptr[WREG_O1], (&(*grp)[SPARC_MC_O1])); > __get_user(env->regwptr[WREG_O2], (&(*grp)[SPARC_MC_O2])); > @@ -456,18 +464,9 @@ void sparc64_set_context(CPUSPARCState *env) > __get_user(env->regwptr[WREG_O6], (&(*grp)[SPARC_MC_O6])); > __get_user(env->regwptr[WREG_O7], (&(*grp)[SPARC_MC_O7])); > > - __get_user(fp, &(ucp->tuc_mcontext.mc_fp)); > - __get_user(i7, &(ucp->tuc_mcontext.mc_i7)); > + __get_user(env->regwptr[WREG_FP], &(ucp->tuc_mcontext.mc_fp)); > + __get_user(env->regwptr[WREG_I7], &(ucp->tuc_mcontext.mc_i7)); > > - w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6]; > - if (put_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]), > - abi_ulong) != 0) { > - goto do_sigsegv; > - } > - if (put_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]), > - abi_ulong) != 0) { > - goto do_sigsegv; > - } > /* FIXME this does not match how the kernel handles the FPU in > * its sparc64_set_context implementation. In particular the FPU > * is only restored if fenab is non-zero in: > @@ -501,7 +500,6 @@ void sparc64_get_context(CPUSPARCState *env) > struct target_ucontext *ucp; > target_mc_gregset_t *grp; > target_mcontext_t *mcp; > - abi_ulong fp, i7, w_addr; > int err; > unsigned int i; > target_sigset_t target_set; > @@ -553,6 +551,15 @@ void sparc64_get_context(CPUSPARCState *env) > __put_user(env->gregs[5], &((*grp)[SPARC_MC_G5])); > __put_user(env->gregs[6], &((*grp)[SPARC_MC_G6])); > __put_user(env->gregs[7], &((*grp)[SPARC_MC_G7])); > + > + /* > + * Note that unlike the kernel, we didn't need to mess with the > + * guest register window state to save it into a pt_regs to run > + * the kernel. So for us the guest's O regs are still in WREG_O* > + * (unlike the kernel which has put them in UREG_I* in a pt_regs) > + * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't > + * need to be fished out of userspace memory. > + */ > __put_user(env->regwptr[WREG_O0], &((*grp)[SPARC_MC_O0])); > __put_user(env->regwptr[WREG_O1], &((*grp)[SPARC_MC_O1])); > __put_user(env->regwptr[WREG_O2], &((*grp)[SPARC_MC_O2])); > @@ -562,18 +569,8 @@ void sparc64_get_context(CPUSPARCState *env) > __put_user(env->regwptr[WREG_O6], &((*grp)[SPARC_MC_O6])); > __put_user(env->regwptr[WREG_O7], &((*grp)[SPARC_MC_O7])); > > - w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6]; > - fp = i7 = 0; > - if (get_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]), > - abi_ulong) != 0) { > - goto do_sigsegv; > - } > - if (get_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]), > - abi_ulong) != 0) { > - goto do_sigsegv; > - } > - __put_user(fp, &(mcp->mc_fp)); > - __put_user(i7, &(mcp->mc_i7)); > + __put_user(env->regwptr[WREG_FP], &(mcp->mc_fp)); > + __put_user(env->regwptr[WREG_I7], &(mcp->mc_i7)); > > { > uint32_t *dst = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs; > Applied to my linux-user-for-5.2 branch. Thanks, Laurent
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c index 57ea1593bfc..c315704b389 100644 --- a/linux-user/sparc/signal.c +++ b/linux-user/sparc/signal.c @@ -403,7 +403,6 @@ void sparc64_set_context(CPUSPARCState *env) struct target_ucontext *ucp; target_mc_gregset_t *grp; abi_ulong pc, npc, tstate; - abi_ulong fp, i7, w_addr; unsigned int i; ucp_addr = env->regwptr[WREG_O0]; @@ -447,6 +446,15 @@ void sparc64_set_context(CPUSPARCState *env) __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5])); __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6])); __get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7])); + + /* + * Note that unlike the kernel, we didn't need to mess with the + * guest register window state to save it into a pt_regs to run + * the kernel. So for us the guest's O regs are still in WREG_O* + * (unlike the kernel which has put them in UREG_I* in a pt_regs) + * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't + * need to be written back to userspace memory. + */ __get_user(env->regwptr[WREG_O0], (&(*grp)[SPARC_MC_O0])); __get_user(env->regwptr[WREG_O1], (&(*grp)[SPARC_MC_O1])); __get_user(env->regwptr[WREG_O2], (&(*grp)[SPARC_MC_O2])); @@ -456,18 +464,9 @@ void sparc64_set_context(CPUSPARCState *env) __get_user(env->regwptr[WREG_O6], (&(*grp)[SPARC_MC_O6])); __get_user(env->regwptr[WREG_O7], (&(*grp)[SPARC_MC_O7])); - __get_user(fp, &(ucp->tuc_mcontext.mc_fp)); - __get_user(i7, &(ucp->tuc_mcontext.mc_i7)); + __get_user(env->regwptr[WREG_FP], &(ucp->tuc_mcontext.mc_fp)); + __get_user(env->regwptr[WREG_I7], &(ucp->tuc_mcontext.mc_i7)); - w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6]; - if (put_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]), - abi_ulong) != 0) { - goto do_sigsegv; - } - if (put_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]), - abi_ulong) != 0) { - goto do_sigsegv; - } /* FIXME this does not match how the kernel handles the FPU in * its sparc64_set_context implementation. In particular the FPU * is only restored if fenab is non-zero in: @@ -501,7 +500,6 @@ void sparc64_get_context(CPUSPARCState *env) struct target_ucontext *ucp; target_mc_gregset_t *grp; target_mcontext_t *mcp; - abi_ulong fp, i7, w_addr; int err; unsigned int i; target_sigset_t target_set; @@ -553,6 +551,15 @@ void sparc64_get_context(CPUSPARCState *env) __put_user(env->gregs[5], &((*grp)[SPARC_MC_G5])); __put_user(env->gregs[6], &((*grp)[SPARC_MC_G6])); __put_user(env->gregs[7], &((*grp)[SPARC_MC_G7])); + + /* + * Note that unlike the kernel, we didn't need to mess with the + * guest register window state to save it into a pt_regs to run + * the kernel. So for us the guest's O regs are still in WREG_O* + * (unlike the kernel which has put them in UREG_I* in a pt_regs) + * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't + * need to be fished out of userspace memory. + */ __put_user(env->regwptr[WREG_O0], &((*grp)[SPARC_MC_O0])); __put_user(env->regwptr[WREG_O1], &((*grp)[SPARC_MC_O1])); __put_user(env->regwptr[WREG_O2], &((*grp)[SPARC_MC_O2])); @@ -562,18 +569,8 @@ void sparc64_get_context(CPUSPARCState *env) __put_user(env->regwptr[WREG_O6], &((*grp)[SPARC_MC_O6])); __put_user(env->regwptr[WREG_O7], &((*grp)[SPARC_MC_O7])); - w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6]; - fp = i7 = 0; - if (get_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]), - abi_ulong) != 0) { - goto do_sigsegv; - } - if (get_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]), - abi_ulong) != 0) { - goto do_sigsegv; - } - __put_user(fp, &(mcp->mc_fp)); - __put_user(i7, &(mcp->mc_i7)); + __put_user(env->regwptr[WREG_FP], &(mcp->mc_fp)); + __put_user(env->regwptr[WREG_I7], &(mcp->mc_i7)); { uint32_t *dst = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs;
Because QEMU's user-mode emulation just directly accesses guest CPU state, for SPARC the guest register window state is not the same in the sparc64_get_context() and sparc64_set_context() functions as it is for the real kernel's versions of those functions. Specifically, for the kernel it has saved the user space state such that the O* registers go into a pt_regs struct as UREG_I*, and the I* registers have been spilled onto the userspace stack. For QEMU, we haven't done that, so the guest's O* registers are still in WREG_O* and the I* registers in WREG_I*. The code was already accessing the O* registers correctly for QEMU, but had copied the kernel code for accessing the I* registers off the userspace stack. Replace this with direct accesses to fp and i7 in the CPU state, and add a comment explaining why we differ from the kernel code here. This fix is sufficient to get bash to a shell prompt. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I'm really pretty unsure about our handling of SPARC register windows here. This fix works, but should we instead be ensuring that the flush_windows() call cpu_loop() does before handling this trap has written the I* regs to the stack ??? --- linux-user/sparc/signal.c | 47 ++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 25 deletions(-)