Re: [Qemu-devel] [PATCH 04/10] mips-linux-user: Save and restore fpu and dsp from sigcontext

[Peter Maydell]

On 10 February 2013 18:30, Richard Henderson <address@hidden> wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/signal.c      | 157 
> +++++++++++++----------------------------------
>  target-mips/cpu.h        |   3 +
>  target-mips/dsp_helper.c |  16 ++++-
>  3 files changed, 58 insertions(+), 118 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 7da676f..a38eaa1 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -2508,18 +2508,17 @@ struct target_rt_sigframe {
>  /* Install trampoline to jump back from signal handler */
>  static inline int install_sigtramp(unsigned int *tramp,   unsigned int 
> syscall)
>  {
> -    int err;
> +    int err = 0;
>
>      /*
> -    * Set up the return code ...
> -    *
> -    *         li      v0, __NR__foo_sigreturn
> -    *         syscall
> -    */
> +     * Set up the return code ...
> +     *
> +     *         li      v0, __NR__foo_sigreturn
> +     *         syscall
> +     */
>
> -    err = __put_user(0x24020000 + syscall, tramp + 0);
> +    err |= __put_user(0x24020000 + syscall, tramp + 0);
>      err |= __put_user(0x0000000c          , tramp + 1);

Since this code is writing to a region of memory which it
has obtained via lock_user_struct(), __put_user() is
guaranteed not to fail. So you might as well not worry
about its return code. MIPS setup_rt_frame() is already
written in this style, for instance.

If you're trying to write to a region of memory which isn't
in the frame struct (which I don't think you are here)
then defining DEBUG_REMAP will probably break things :-)

Nobody is looking at the return value from install_sigtramp()
anyhow, so you could make it void if you like.

> -    /* flush_cache_sigtramp((unsigned long) tramp); */

So, what does cause any stale TB we might have happened to
have lying around for this address to be flushed from the
TB cache?

>      return err;
>  }
>
> @@ -2527,74 +2526,37 @@ static inline int
>  setup_sigcontext(CPUMIPSState *regs, struct target_sigcontext *sc)
>  {
>      int err = 0;
> +    int i;
>
>      err |= __put_user(regs->active_tc.PC, &sc->sc_pc);
>
> -#define save_gp_reg(i) do {                                            \
> -        err |= __put_user(regs->active_tc.gpr[i], &sc->sc_regs[i]);    \
> -    } while(0)
> -    __put_user(0, &sc->sc_regs[0]); save_gp_reg(1); save_gp_reg(2);
> -    save_gp_reg(3); save_gp_reg(4); save_gp_reg(5); save_gp_reg(6);
> -    save_gp_reg(7); save_gp_reg(8); save_gp_reg(9); save_gp_reg(10);
> -    save_gp_reg(11); save_gp_reg(12); save_gp_reg(13); save_gp_reg(14);
> -    save_gp_reg(15); save_gp_reg(16); save_gp_reg(17); save_gp_reg(18);
> -    save_gp_reg(19); save_gp_reg(20); save_gp_reg(21); save_gp_reg(22);
> -    save_gp_reg(23); save_gp_reg(24); save_gp_reg(25); save_gp_reg(26);
> -    save_gp_reg(27); save_gp_reg(28); save_gp_reg(29); save_gp_reg(30);
> -    save_gp_reg(31);
> -#undef save_gp_reg
> +    __put_user(0, &sc->sc_regs[0]);
> +    for (i = 1; i < 32; ++i) {
> +        err |= __put_user(regs->active_tc.gpr[i], &sc->sc_regs[i]);
> +    }

This is much nicer than that macro (though the same remarks
about not needing to check __put_user return values apply).


> +    /* Rather than checking for dsp existance, always copy.  The storage
> +       would just be garbage otherwise.  */

"existence".

> +    err |= __get_user(regs->active_tc.HI[1], &sc->sc_hi1);
> +    err |= __get_user(regs->active_tc.HI[2], &sc->sc_hi2);
> +    err |= __get_user(regs->active_tc.HI[3], &sc->sc_hi3);
> +    err |= __get_user(regs->active_tc.LO[1], &sc->sc_lo1);
> +    err |= __get_user(regs->active_tc.LO[2], &sc->sc_lo2);
> +    err |= __get_user(regs->active_tc.LO[3], &sc->sc_lo3);
> +    {
> +        uint32_t dsp;
> +        err |= __get_user(dsp, &sc->sc_dsp);
> +        cpu_wrdsp(dsp, 0x3ff, regs);
> +    }

Unconditionally copying the DSP state fields into the signal
context struct is OK, but is it really safe to copy whatever
the guest provides us back into the CPU state fields even if
there's no DSP? I think I'd prefer to see a cpu_has_dsp guard
here.

> +    for (i = 0; i < 32; ++i) {
> +        err |= __get_user(regs->active_fpu.fpr[i].d, &sc->sc_fpregs[i]);
>      }
>
> -    preempt_enable();
> -#endif
>      return err;
>  }

> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 5963d62..2183d06 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -504,6 +504,9 @@ void mips_cpu_list (FILE *f, fprintf_function 
> cpu_fprintf);
>  #define cpu_signal_handler cpu_mips_signal_handler
>  #define cpu_list mips_cpu_list
>
> +extern void cpu_wrdsp(uint32_t, uint32_t, CPUMIPSState *);
> +extern uint32_t cpu_rddsp(uint32_t, CPUMIPSState *);

If you included the argument names in these prototypes then
it would make it consistent with the other code in the file,
assist the reader and shut checkpatch.pl up :-)

-- PMM