| Message ID | 1360521050-29680-5-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On 2013-02-10 10:30, Richard Henderson wrote: > Signed-off-by: Richard Henderson<rth@twiddle.net> > --- > linux-user/signal.c | 157 +++++++++++++---------------------------------- > target-mips/cpu.h | 3 + > target-mips/dsp_helper.c | 16 ++++- > 3 files changed, 58 insertions(+), 118 deletions(-) Probably not required by anything I'm ever planning to test, but why not? It's certainly easy to fill in these blanks... r~
On 10 February 2013 18:30, Richard Henderson <rth@twiddle.net> wrote: > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > 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
On 2013-02-11 09:28, Peter Maydell wrote: > 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 :-) I suppose such a cleanup should be in a separate patch, probably right before this one... >> - /* 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? The same way we handle all other self-modifying code: with the read-only bit on pages for which we have TBs. I'm deleting kernel code that's been cut-and-pasted into here and then commented out. > 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. I can't think of a reason it's not safe. We're not modifying the env->CP0_Status field, so if the dsp is disabled it can't be enabled by user fiddling. Again the comment about the data just being garbage... r~
On 11 February 2013 17:43, Richard Henderson <rth@twiddle.net> wrote: >> On 2013-02-11 09:28, Peter Maydell wrote: >>> - /* 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? > The same way we handle all other self-modifying code: with > the read-only bit on pages for which we have TBs. OK. > I'm deleting kernel code that's been cut-and-pasted into here > and then commented out. Yeah; I couldn't remember the mechanism we used (ie whether we allowed stale code to hang around on guest architectures that require explicit cache flush after self modifying code). Having looked through the code you're right that we don't need to do anything. >> 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. > > > I can't think of a reason it's not safe. We're not modifying > the env->CP0_Status field, so if the dsp is disabled it can't > be enabled by user fiddling. Again the comment about the data > just being garbage... The difference is that the guest signal ABI allows the stack frame space to be garbage, so we know it's safe. Whereas allowing the guest to write garbage into our CPU state struct means we have to be sure that there's nothing in target-mips/ which ever assumes "dsp state fields are zero in a non-dsp config because nothing will have written to them" or similar. More pithily, it's fine to feed other people garbage but we should be more cautious about eating it ourselves :-) -- PMM
On 2013-02-11 09:55, Peter Maydell wrote: > Whereas allowing the guest to write garbage into our CPU state struct > means we have to be sure that there's nothing in target-mips/ which > ever assumes "dsp state fields are zero in a non-dsp config because > nothing will have written to them" or similar. Ok, afaict everything about the dsp is gated by CP0_Status bit CP0St_MX. r~
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); - /* flush_cache_sigtramp((unsigned long) tramp); */ 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]); + } err |= __put_user(regs->active_tc.HI[0], &sc->sc_mdhi); err |= __put_user(regs->active_tc.LO[0], &sc->sc_mdlo); - /* Not used yet, but might be useful if we ever have DSP suppport */ -#if 0 - if (cpu_has_dsp) { - err |= __put_user(mfhi1(), &sc->sc_hi1); - err |= __put_user(mflo1(), &sc->sc_lo1); - err |= __put_user(mfhi2(), &sc->sc_hi2); - err |= __put_user(mflo2(), &sc->sc_lo2); - err |= __put_user(mfhi3(), &sc->sc_hi3); - err |= __put_user(mflo3(), &sc->sc_lo3); - err |= __put_user(rddsp(DSP_MASK), &sc->sc_dsp); - } - /* same with 64 bit */ -#ifdef CONFIG_64BIT - err |= __put_user(regs->hi, &sc->sc_hi[0]); - err |= __put_user(regs->lo, &sc->sc_lo[0]); - if (cpu_has_dsp) { - err |= __put_user(mfhi1(), &sc->sc_hi[1]); - err |= __put_user(mflo1(), &sc->sc_lo[1]); - err |= __put_user(mfhi2(), &sc->sc_hi[2]); - err |= __put_user(mflo2(), &sc->sc_lo[2]); - err |= __put_user(mfhi3(), &sc->sc_hi[3]); - err |= __put_user(mflo3(), &sc->sc_lo[3]); - err |= __put_user(rddsp(DSP_MASK), &sc->sc_dsp); + /* Rather than checking for dsp existance, always copy. The storage + would just be garbage otherwise. */ + err |= __put_user(regs->active_tc.HI[1], &sc->sc_hi1); + err |= __put_user(regs->active_tc.HI[2], &sc->sc_hi2); + err |= __put_user(regs->active_tc.HI[3], &sc->sc_hi3); + err |= __put_user(regs->active_tc.LO[1], &sc->sc_lo1); + err |= __put_user(regs->active_tc.LO[2], &sc->sc_lo2); + err |= __put_user(regs->active_tc.LO[3], &sc->sc_lo3); + { + uint32_t dsp = cpu_rddsp(0x3ff, regs); + err |= __put_user(dsp, &sc->sc_dsp); } -#endif -#endif - -#if 0 - err |= __put_user(!!used_math(), &sc->sc_used_math); - if (!used_math()) - goto out; + err |= __put_user(1, &sc->sc_used_math); - /* - * Save FPU state to signal context. Signal handler will "inherit" - * current FPU state. - */ - preempt_disable(); - - if (!is_fpu_owner()) { - own_fpu(); - restore_fp(current); + for (i = 0; i < 32; ++i) { + err |= __put_user(regs->active_fpu.fpr[i].d, &sc->sc_fpregs[i]); } - err |= save_fp_context(sc); - preempt_enable(); - out: -#endif return err; } @@ -2602,68 +2564,33 @@ static inline int restore_sigcontext(CPUMIPSState *regs, struct target_sigcontext *sc) { int err = 0; + int i; err |= __get_user(regs->CP0_EPC, &sc->sc_pc); err |= __get_user(regs->active_tc.HI[0], &sc->sc_mdhi); err |= __get_user(regs->active_tc.LO[0], &sc->sc_mdlo); -#define restore_gp_reg(i) do { \ - err |= __get_user(regs->active_tc.gpr[i], &sc->sc_regs[i]); \ - } while(0) - restore_gp_reg( 1); restore_gp_reg( 2); restore_gp_reg( 3); - restore_gp_reg( 4); restore_gp_reg( 5); restore_gp_reg( 6); - restore_gp_reg( 7); restore_gp_reg( 8); restore_gp_reg( 9); - restore_gp_reg(10); restore_gp_reg(11); restore_gp_reg(12); - restore_gp_reg(13); restore_gp_reg(14); restore_gp_reg(15); - restore_gp_reg(16); restore_gp_reg(17); restore_gp_reg(18); - restore_gp_reg(19); restore_gp_reg(20); restore_gp_reg(21); - restore_gp_reg(22); restore_gp_reg(23); restore_gp_reg(24); - restore_gp_reg(25); restore_gp_reg(26); restore_gp_reg(27); - restore_gp_reg(28); restore_gp_reg(29); restore_gp_reg(30); - restore_gp_reg(31); -#undef restore_gp_reg - -#if 0 - if (cpu_has_dsp) { - err |= __get_user(treg, &sc->sc_hi1); mthi1(treg); - err |= __get_user(treg, &sc->sc_lo1); mtlo1(treg); - err |= __get_user(treg, &sc->sc_hi2); mthi2(treg); - err |= __get_user(treg, &sc->sc_lo2); mtlo2(treg); - err |= __get_user(treg, &sc->sc_hi3); mthi3(treg); - err |= __get_user(treg, &sc->sc_lo3); mtlo3(treg); - err |= __get_user(treg, &sc->sc_dsp); wrdsp(treg, DSP_MASK); - } -#ifdef CONFIG_64BIT - err |= __get_user(regs->hi, &sc->sc_hi[0]); - err |= __get_user(regs->lo, &sc->sc_lo[0]); - if (cpu_has_dsp) { - err |= __get_user(treg, &sc->sc_hi[1]); mthi1(treg); - err |= __get_user(treg, &sc->sc_lo[1]); mthi1(treg); - err |= __get_user(treg, &sc->sc_hi[2]); mthi2(treg); - err |= __get_user(treg, &sc->sc_lo[2]); mthi2(treg); - err |= __get_user(treg, &sc->sc_hi[3]); mthi3(treg); - err |= __get_user(treg, &sc->sc_lo[3]); mthi3(treg); - err |= __get_user(treg, &sc->sc_dsp); wrdsp(treg, DSP_MASK); + for (i = 1; i < 32; ++i) { + err |= __get_user(regs->active_tc.gpr[i], &sc->sc_regs[i]); } -#endif - - err |= __get_user(used_math, &sc->sc_used_math); - conditional_used_math(used_math); - preempt_disable(); + 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); + } - if (used_math()) { - /* restore fpu context if we have used it before */ - own_fpu(); - err |= restore_fp_context(sc); - } else { - /* signal handler may have used FPU. Give it up. */ - lose_fpu(); + 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 *); + #define CPU_SAVE_VERSION 3 /* MMU modes definitions. We carefully match the indices with our diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c index 96cb044..3937815 100644 --- a/target-mips/dsp_helper.c +++ b/target-mips/dsp_helper.c @@ -3637,7 +3637,7 @@ void helper_dmthlip(target_ulong rs, target_ulong ac, CPUMIPSState *env) } #endif -void helper_wrdsp(target_ulong rs, target_ulong mask_num, CPUMIPSState *env) +void cpu_wrdsp(uint32_t rs, uint32_t mask_num, CPUMIPSState *env) { uint8_t mask[6]; uint8_t i; @@ -3703,7 +3703,12 @@ void helper_wrdsp(target_ulong rs, target_ulong mask_num, CPUMIPSState *env) env->active_tc.DSPControl = dsp; } -target_ulong helper_rddsp(target_ulong masknum, CPUMIPSState *env) +void helper_wrdsp(target_ulong rs, target_ulong mask_num, CPUMIPSState *env) +{ + return cpu_wrdsp(rs, mask_num, env); +} + +uint32_t cpu_rddsp(uint32_t mask_num, CPUMIPSState *env) { uint8_t mask[6]; uint32_t ruler, i; @@ -3712,7 +3717,7 @@ target_ulong helper_rddsp(target_ulong masknum, CPUMIPSState *env) ruler = 0x01; for (i = 0; i < 6; i++) { - mask[i] = (masknum & ruler) >> i ; + mask[i] = (mask_num & ruler) >> i ; ruler = ruler << 1; } @@ -3754,6 +3759,11 @@ target_ulong helper_rddsp(target_ulong masknum, CPUMIPSState *env) return temp; } +target_ulong helper_rddsp(target_ulong mask_num, CPUMIPSState *env) +{ + return cpu_rddsp(mask_num, env); +} + #undef MIPSDSP_LHI #undef MIPSDSP_LLO
Signed-off-by: Richard Henderson <rth@twiddle.net> --- linux-user/signal.c | 157 +++++++++++++---------------------------------- target-mips/cpu.h | 3 + target-mips/dsp_helper.c | 16 ++++- 3 files changed, 58 insertions(+), 118 deletions(-)