diff mbox

[PULL,v2,22/25] linux-user: Implement signals for openrisc

Message ID 20180702151023.24532-23-shorne@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stafford Horne July 2, 2018, 3:10 p.m. UTC
From: Richard Henderson <richard.henderson@linaro.org>

All of the existing code was boilerplate from elsewhere,
and would crash the guest upon the first signal.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Stafford Horne <shorne@gmail.com>

---
v2:
  Add a comment to the new definition of target_pt_regs.
  Install the signal mask into the ucontext.
v3:
  Incorporate feedback from Laurent.
---
 linux-user/openrisc/signal.c         | 217 +++++++++++----------------
 linux-user/openrisc/target_syscall.h |  28 +---
 linux-user/signal.c                  |   2 +-
 target/openrisc/cpu.c                |   1 +
 4 files changed, 94 insertions(+), 154 deletions(-)

Comments

Philippe Mathieu-Daudé July 3, 2018, 10:34 p.m. UTC | #1
Hi Stafford,

On 07/02/2018 12:10 PM, Stafford Horne wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> All of the existing code was boilerplate from elsewhere,
> and would crash the guest upon the first signal.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> 
> ---
> v2:
>   Add a comment to the new definition of target_pt_regs.
>   Install the signal mask into the ucontext.
> v3:
>   Incorporate feedback from Laurent.

Oops this is not supposed to enter git history, something is wrong with
your pull request script.

> ---
>  linux-user/openrisc/signal.c         | 217 +++++++++++----------------
>  linux-user/openrisc/target_syscall.h |  28 +---
>  linux-user/signal.c                  |   2 +-
>  target/openrisc/cpu.c                |   1 +
>  4 files changed, 94 insertions(+), 154 deletions(-)

<snip>
Stafford Horne July 3, 2018, 11:51 p.m. UTC | #2
Hello,

On Wed, Jul 4, 2018, 7:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi Stafford,
>
> On 07/02/2018 12:10 PM, Stafford Horne wrote:
> > From: Richard Henderson <richard.henderson@linaro.org>
> >
> > All of the existing code was boilerplate from elsewhere,
> > and would crash the guest upon the first signal.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> >
> > ---
> > v2:
> >   Add a comment to the new definition of target_pt_regs.
> >   Install the signal mask into the ucontext.
> > v3:
> >   Incorporate feedback from Laurent.
>
> Oops this is not supposed to enter git history, something is wrong with
> your pull request script.
>

Sorry, I brought these in from Richards tree and saw them but thought it
was fine to keep the annotations.

I'll remove them from patches in the future.

Richard, how do you usually remove these before your pr?  I suppose you
have some kind of script?

-stafford




> > ---
> >  linux-user/openrisc/signal.c         | 217 +++++++++++----------------
> >  linux-user/openrisc/target_syscall.h |  28 +---
> >  linux-user/signal.c                  |   2 +-
> >  target/openrisc/cpu.c                |   1 +
> >  4 files changed, 94 insertions(+), 154 deletions(-)
>
> <snip>
>
Richard Henderson July 4, 2018, 8:54 p.m. UTC | #3
On 07/03/2018 04:51 PM, Stafford Horne wrote:
> Richard, how do you usually remove these before your pr?  I suppose you have
> some kind of script?

No, I just edit the things by hand.


r~
Eric Blake July 6, 2018, 10:22 p.m. UTC | #4
On 07/04/2018 03:54 PM, Richard Henderson wrote:
> On 07/03/2018 04:51 PM, Stafford Horne wrote:
>> Richard, how do you usually remove these before your pr?  I suppose you have
>> some kind of script?
> 
> No, I just edit the things by hand.

I do it by always using 'git am' to suck in patches from the mailing 
list, including my own patches, and even if I already have the patch 
locally.  That is, my workflow for PR bypasses my local work tree to 
ensure I don't leave local artifacts around in the PR.
Stafford Horne July 6, 2018, 11:02 p.m. UTC | #5
On Fri, Jul 06, 2018 at 05:22:15PM -0500, Eric Blake wrote:
> On 07/04/2018 03:54 PM, Richard Henderson wrote:
> > On 07/03/2018 04:51 PM, Stafford Horne wrote:
> > > Richard, how do you usually remove these before your pr?  I suppose you have
> > > some kind of script?
> > 
> > No, I just edit the things by hand.
> 
> I do it by always using 'git am' to suck in patches from the mailing list,
> including my own patches, and even if I already have the patch locally.
> That is, my workflow for PR bypasses my local work tree to ensure I don't
> leave local artifacts around in the PR.

Hi Eric,

How do you carry the Change list for each patch brought in with 'git am' i.e.
v2,v3 when you have updates that need respin?  Do you just update that manually
on the .patch files after 'git format-patch'?

I found and tried `git notes` recently which seems to be meant for this.

From 'git help notes'

   Notes can also be added to patches prepared with git format-patch by
   using the --notes option. Such notes are added as a patch commentary
   after a three dash separator line.

However, I found the notes get lots pretty easily during rebases and would not
survive a 'git am'.

I am sure many have their own methods that work.  So far I have been doing
manually just updating the .patch files after 'format-patch'.

-Stafford
Alex Bennée July 7, 2018, 7:04 a.m. UTC | #6
Stafford Horne <shorne@gmail.com> writes:

> Hello,
>
> On Wed, Jul 4, 2018, 7:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
>> Hi Stafford,
>>
>> On 07/02/2018 12:10 PM, Stafford Horne wrote:
>> > From: Richard Henderson <richard.henderson@linaro.org>
<snip>
>
> Sorry, I brought these in from Richards tree and saw them but thought it
> was fine to keep the annotations.
>
> I'll remove them from patches in the future.
>
> Richard, how do you usually remove these before your pr?  I suppose you
> have some kind of script?
>

For reference these are my exact-steps:

  We need to ensure we have added our signoff and there is no ---
  ephemera left from commit history.

  ,----
  | errors=0
  | commits=0
  | while read rev; do
  |     author=$(git show -s --format='%an <%ae>' $rev)
  |     body=$(git show -s --format='%B' $rev)
  |
  |     # Check for Author signoff
  |     if ! grep -q "^Signed-off-by: $author" <<< "$body"; then
  |         errors=$((errors+1))
  |         echo "$rev missing signoff by $author"
  |     fi
  |
  |     # Check for my signoff
  |     if ! grep -q "^Signed-off-by: $signoff" <<< "$body"; then
  |         errors=$((errors+1))
  |         echo "$rev missing your signoff ($signoff)"
  |     fi
  |
  |     # Check for stray history
  |     if grep -q "^--" <<< "$body"; then
  |         errors=$((errors+1))
  |         echo "$rev may have history in it"
  |     fi
  |
  |     commits=$((commits+1))
  | done < <(git rev-list "origin/master..HEAD")
  |
  | echo "Found $errors errors over $commits commits"
  `----

  If we want to strip history we can run the following:

  ,----
  | current=$(git rev-parse --abbrev-ref HEAD)
  | if [ -d "${series}.pull" ]; then
  |   rm -rf ${series}.pull
  | fi
  | git format-patch origin/master..${current} -p -o ${series}.pull
  | prb=${current}-pr
  | echo ${prb}
  | git checkout -b ${prb} origin/master
  | git am ${series}.pull/*
  | rm -rf ${series}.pull
  `----

--
Alex Bennée
Laurent Vivier July 7, 2018, 8:10 a.m. UTC | #7
Le 07/07/2018 à 01:02, Stafford Horne a écrit :
> On Fri, Jul 06, 2018 at 05:22:15PM -0500, Eric Blake wrote:
>> On 07/04/2018 03:54 PM, Richard Henderson wrote:
>>> On 07/03/2018 04:51 PM, Stafford Horne wrote:
>>>> Richard, how do you usually remove these before your pr?  I suppose you have
>>>> some kind of script?
>>>
>>> No, I just edit the things by hand.
>>
>> I do it by always using 'git am' to suck in patches from the mailing list,
>> including my own patches, and even if I already have the patch locally.
>> That is, my workflow for PR bypasses my local work tree to ensure I don't
>> leave local artifacts around in the PR.
> 
> Hi Eric,
> 
> How do you carry the Change list for each patch brought in with 'git am' i.e.
> v2,v3 when you have updates that need respin?  Do you just update that manually
> on the .patch files after 'git format-patch'?
> 
> I found and tried `git notes` recently which seems to be meant for this.
> 
> From 'git help notes'
> 
>    Notes can also be added to patches prepared with git format-patch by
>    using the --notes option. Such notes are added as a patch commentary
>    after a three dash separator line.
> 
> However, I found the notes get lots pretty easily during rebases and would not
> survive a 'git am'.

You need to set notes.rewriteref=refs/notes/commits

Thanks,
Laurent
diff mbox

Patch

diff --git a/linux-user/openrisc/signal.c b/linux-user/openrisc/signal.c
index 8be0b74001..232ad82b98 100644
--- a/linux-user/openrisc/signal.c
+++ b/linux-user/openrisc/signal.c
@@ -21,124 +21,69 @@ 
 #include "signal-common.h"
 #include "linux-user/trace.h"
 
-struct target_sigcontext {
+typedef struct target_sigcontext {
     struct target_pt_regs regs;
     abi_ulong oldmask;
-    abi_ulong usp;
-};
+} target_sigcontext;
 
-struct target_ucontext {
+typedef struct target_ucontext {
     abi_ulong tuc_flags;
     abi_ulong tuc_link;
     target_stack_t tuc_stack;
-    struct target_sigcontext tuc_mcontext;
+    target_sigcontext tuc_mcontext;
     target_sigset_t tuc_sigmask;   /* mask last for extensibility */
-};
+} target_ucontext;
 
-struct target_rt_sigframe {
-    abi_ulong pinfo;
-    uint64_t puc;
+typedef struct target_rt_sigframe {
     struct target_siginfo info;
-    struct target_sigcontext sc;
-    struct target_ucontext uc;
-    unsigned char retcode[16];  /* trampoline code */
-};
-
-/* This is the asm-generic/ucontext.h version */
-#if 0
-static int restore_sigcontext(CPUOpenRISCState *regs,
-                              struct target_sigcontext *sc)
-{
-    unsigned int err = 0;
-    unsigned long old_usp;
-
-    /* Alwys make any pending restarted system call return -EINTR */
-    current_thread_info()->restart_block.fn = do_no_restart_syscall;
+    target_ucontext uc;
+    uint32_t retcode[4];  /* trampoline code */
+} target_rt_sigframe;
 
-    /* restore the regs from &sc->regs (same as sc, since regs is first)
-     * (sc is already checked for VERIFY_READ since the sigframe was
-     *  checked in sys_sigreturn previously)
-     */
+static void restore_sigcontext(CPUOpenRISCState *env, target_sigcontext *sc)
+{
+    int i;
+    abi_ulong v;
 
-    if (copy_from_user(regs, &sc, sizeof(struct target_pt_regs))) {
-        goto badframe;
+    for (i = 0; i < 32; ++i) {
+        __get_user(v, &sc->regs.gpr[i]);
+        cpu_set_gpr(env, i, v);
     }
+    __get_user(env->pc, &sc->regs.pc);
 
-    /* make sure the U-flag is set so user-mode cannot fool us */
-
-    regs->sr &= ~SR_SM;
-
-    /* restore the old USP as it was before we stacked the sc etc.
-     * (we cannot just pop the sigcontext since we aligned the sp and
-     *  stuff after pushing it)
-     */
-
-    __get_user(old_usp, &sc->usp);
-    phx_signal("old_usp 0x%lx", old_usp);
-
-    __PHX__ REALLY           /* ??? */
-    wrusp(old_usp);
-    regs->gpr[1] = old_usp;
-
-    /* TODO: the other ports use regs->orig_XX to disable syscall checks
-     * after this completes, but we don't use that mechanism. maybe we can
-     * use it now ?
-     */
-
-    return err;
-
-badframe:
-    return 1;
+    /* Make sure the supervisor flag is clear.  */
+    __get_user(v, &sc->regs.sr);
+    cpu_set_sr(env, v & ~SR_SM);
 }
-#endif
 
 /* Set up a signal frame.  */
 
-static void setup_sigcontext(struct target_sigcontext *sc,
-                             CPUOpenRISCState *regs,
-                             unsigned long mask)
+static void setup_sigcontext(target_sigcontext *sc, CPUOpenRISCState *env)
 {
-    unsigned long usp = cpu_get_gpr(regs, 1);
-
-    /* copy the regs. they are first in sc so we can use sc directly */
+    int i;
 
-    /*copy_to_user(&sc, regs, sizeof(struct target_pt_regs));*/
-
-    /* Set the frametype to CRIS_FRAME_NORMAL for the execution of
-       the signal handler. The frametype will be restored to its previous
-       value in restore_sigcontext. */
-    /*regs->frametype = CRIS_FRAME_NORMAL;*/
-
-    /* then some other stuff */
-    __put_user(mask, &sc->oldmask);
-    __put_user(usp, &sc->usp);
-}
+    for (i = 0; i < 32; ++i) {
+        __put_user(cpu_get_gpr(env, i), &sc->regs.gpr[i]);
+    }
 
-static inline unsigned long align_sigframe(unsigned long sp)
-{
-    return sp & ~3UL;
+    __put_user(env->pc, &sc->regs.pc);
+    __put_user(cpu_get_sr(env), &sc->regs.sr);
 }
 
 static inline abi_ulong get_sigframe(struct target_sigaction *ka,
-                                     CPUOpenRISCState *regs,
+                                     CPUOpenRISCState *env,
                                      size_t frame_size)
 {
-    unsigned long sp = get_sp_from_cpustate(regs);
-    int onsigstack = on_sig_stack(sp);
-
-    /* redzone */
-    sp = target_sigsp(sp, ka);
-
-    sp = align_sigframe(sp - frame_size);
+    target_ulong sp = get_sp_from_cpustate(env);
 
-    /*
-     * If we are on the alternate signal stack and would overflow it, don't.
-     * Return an always-bogus address instead so we will die with SIGSEGV.
+    /* Honor redzone now.  If we swap to signal stack, no need to waste
+     * the 128 bytes by subtracting afterward.
      */
+    sp -= 128;
 
-    if (onsigstack && !likely(on_sig_stack(sp))) {
-        return -1L;
-    }
+    sp = target_sigsp(sp, ka);
+    sp -= frame_size;
+    sp = QEMU_ALIGN_DOWN(sp, 4);
 
     return sp;
 }
@@ -147,11 +92,9 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
                     target_siginfo_t *info,
                     target_sigset_t *set, CPUOpenRISCState *env)
 {
-    int err = 0;
     abi_ulong frame_addr;
-    unsigned long return_ip;
-    struct target_rt_sigframe *frame;
-    abi_ulong info_addr, uc_addr;
+    target_rt_sigframe *frame;
+    int i;
 
     frame_addr = get_sigframe(ka, env, sizeof(*frame));
     trace_user_setup_rt_frame(env, frame_addr);
@@ -159,47 +102,37 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
         goto give_sigsegv;
     }
 
-    info_addr = frame_addr + offsetof(struct target_rt_sigframe, info);
-    __put_user(info_addr, &frame->pinfo);
-    uc_addr = frame_addr + offsetof(struct target_rt_sigframe, uc);
-    __put_user(uc_addr, &frame->puc);
-
     if (ka->sa_flags & SA_SIGINFO) {
         tswap_siginfo(&frame->info, info);
     }
 
-    /*err |= __clear_user(&frame->uc, offsetof(ucontext_t, uc_mcontext));*/
     __put_user(0, &frame->uc.tuc_flags);
     __put_user(0, &frame->uc.tuc_link);
-    target_save_altstack(&frame->uc.tuc_stack, env);
-    setup_sigcontext(&frame->sc, env, set->sig[0]);
 
-    /*err |= copy_to_user(frame->uc.tuc_sigmask, set, sizeof(*set));*/
-
-    /* trampoline - the desired return ip is the retcode itself */
-    return_ip = (unsigned long)&frame->retcode;
-    /* This is l.ori r11,r0,__NR_sigreturn, l.sys 1 */
-    __put_user(0xa960, (short *)(frame->retcode + 0));
-    __put_user(TARGET_NR_rt_sigreturn, (short *)(frame->retcode + 2));
-    __put_user(0x20000001, (unsigned long *)(frame->retcode + 4));
-    __put_user(0x15000000, (unsigned long *)(frame->retcode + 8));
-
-    if (err) {
-        goto give_sigsegv;
+    target_save_altstack(&frame->uc.tuc_stack, env);
+    setup_sigcontext(&frame->uc.tuc_mcontext, env);
+    for (i = 0; i < TARGET_NSIG_WORDS; ++i) {
+        __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
     }
 
-    /* TODO what is the current->exec_domain stuff and invmap ? */
+    /* This is l.ori r11,r0,__NR_sigreturn; l.sys 1; l.nop; l.nop */
+    __put_user(0xa9600000 | TARGET_NR_rt_sigreturn, frame->retcode + 0);
+    __put_user(0x20000001, frame->retcode + 1);
+    __put_user(0x15000000, frame->retcode + 2);
+    __put_user(0x15000000, frame->retcode + 3);
 
     /* Set up registers for signal handler */
-    env->pc = (unsigned long)ka->_sa_handler; /* what we enter NOW */
-    cpu_set_gpr(env, 9, (unsigned long)return_ip);     /* what we enter LATER */
-    cpu_set_gpr(env, 3, (unsigned long)sig);           /* arg 1: signo */
-    cpu_set_gpr(env, 4, (unsigned long)&frame->info);  /* arg 2: (siginfo_t*) */
-    cpu_set_gpr(env, 5, (unsigned long)&frame->uc);    /* arg 3: ucontext */
-
-    /* actually move the usp to reflect the stacked frame */
-    cpu_set_gpr(env, 1, (unsigned long)frame);
-
+    cpu_set_gpr(env, 9, frame_addr + offsetof(target_rt_sigframe, retcode));
+    cpu_set_gpr(env, 3, sig);
+    cpu_set_gpr(env, 4, frame_addr + offsetof(target_rt_sigframe, info));
+    cpu_set_gpr(env, 5, frame_addr + offsetof(target_rt_sigframe, uc));
+    cpu_set_gpr(env, 1, frame_addr);
+
+    /* For debugging convenience, set ppc to the insn that faulted.  */
+    env->ppc = env->pc;
+    /* When setting the PC for the signal handler, exit delay slot.  */
+    env->pc = ka->_sa_handler;
+    env->dflag = 0;
     return;
 
 give_sigsegv:
@@ -207,16 +140,34 @@  give_sigsegv:
     force_sigsegv(sig);
 }
 
-long do_sigreturn(CPUOpenRISCState *env)
-{
-    trace_user_do_sigreturn(env, 0);
-    fprintf(stderr, "do_sigreturn: not implemented\n");
-    return -TARGET_ENOSYS;
-}
-
 long do_rt_sigreturn(CPUOpenRISCState *env)
 {
+    abi_ulong frame_addr = get_sp_from_cpustate(env);
+    target_rt_sigframe *frame;
+    sigset_t set;
+
     trace_user_do_rt_sigreturn(env, 0);
-    fprintf(stderr, "do_rt_sigreturn: not implemented\n");
-    return -TARGET_ENOSYS;
+    if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
+        goto badframe;
+    }
+    if (frame_addr & 3) {
+        goto badframe;
+    }
+
+    target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
+    set_sigmask(&set);
+
+    restore_sigcontext(env, &frame->uc.tuc_mcontext);
+    if (do_sigaltstack(frame_addr + offsetof(target_rt_sigframe, uc.tuc_stack),
+                       0, frame_addr) == -EFAULT) {
+        goto badframe;
+    }
+
+    unlock_user_struct(frame, frame_addr, 0);
+    return cpu_get_gpr(env, 11);
+
+ badframe:
+    unlock_user_struct(frame, frame_addr, 0);
+    force_sig(TARGET_SIGSEGV);
+    return 0;
 }
diff --git a/linux-user/openrisc/target_syscall.h b/linux-user/openrisc/target_syscall.h
index 03104f80af..d586d2a018 100644
--- a/linux-user/openrisc/target_syscall.h
+++ b/linux-user/openrisc/target_syscall.h
@@ -1,27 +1,15 @@ 
 #ifndef OPENRISC_TARGET_SYSCALL_H
 #define OPENRISC_TARGET_SYSCALL_H
 
+/* Note that in linux/arch/openrisc/include/uapi/asm/ptrace.h,
+ * this is called user_regs_struct.  Given that this is what
+ * is used within struct sigcontext we need this definition.
+ * However, elfload.c wants this name.
+ */
 struct target_pt_regs {
-    union {
-        struct {
-            /* Named registers */
-            uint32_t sr;       /* Stored in place of r0 */
-            target_ulong sp;   /* r1 */
-        };
-        struct {
-            /* Old style */
-            target_ulong offset[2];
-            target_ulong gprs[30];
-        };
-        struct {
-            /* New style */
-            target_ulong gpr[32];
-        };
-    };
-    target_ulong pc;
-    target_ulong orig_gpr11;   /* For restarting system calls */
-    uint32_t syscallno;        /* Syscall number (used by strace) */
-    target_ulong dummy;     /* Cheap alignment fix */
+    abi_ulong gpr[32];
+    abi_ulong pc;
+    abi_ulong sr;
 };
 
 #define UNAME_MACHINE "openrisc"
diff --git a/linux-user/signal.c b/linux-user/signal.c
index be2815b45d..602b631b92 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -236,7 +236,7 @@  int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
     return 0;
 }
 
-#if !defined(TARGET_OPENRISC) && !defined(TARGET_NIOS2)
+#if !defined(TARGET_NIOS2)
 /* Just set the guest's signal mask to the specified value; the
  * caller is assumed to have called block_signals() already.
  */
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index e01ce9ed1c..fb7cb5c507 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -27,6 +27,7 @@  static void openrisc_cpu_set_pc(CPUState *cs, vaddr value)
     OpenRISCCPU *cpu = OPENRISC_CPU(cs);
 
     cpu->env.pc = value;
+    cpu->env.dflag = 0;
 }
 
 static bool openrisc_cpu_has_work(CPUState *cs)