Re: [PATCH v11 19/26] linux-user: Add LoongArch signal support

[Richard Henderson <richard.henderson@linaro.org>]

On 11/19/21 7:13 AM, Song Gao wrote:
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   linux-user/loongarch64/signal.c        | 162 +++++++++++++++++++++++++++++++++
>   linux-user/loongarch64/target_signal.h |  29 ++++++
>   2 files changed, 191 insertions(+)
>   create mode 100644 linux-user/loongarch64/signal.c
>   create mode 100644 linux-user/loongarch64/target_signal.h
> 
> diff --git a/linux-user/loongarch64/signal.c b/linux-user/loongarch64/signal.c
> new file mode 100644
> index 0000000..8fbc827
> --- /dev/null
> +++ b/linux-user/loongarch64/signal.c
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * LoongArch emulation of Linux signals
> + *
> + * Copyright (c) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu.h"
> +#include "signal-common.h"
> +#include "user-internals.h"
> +#include "linux-user/trace.h"
> +
> +struct target_sigcontext {
> +    uint64_t   sc_pc;
> +    uint64_t   sc_gpr[32];
> +    uint64_t   sc_fpr[32];
> +    uint64_t   sc_fcc;
> +    uint32_t   sc_fcsr;
> +    uint32_t   sc_flags;
> +};

Does not match
https://github.com/loongson/linux/blob/loongarch-next/arch/loongarch/include/uapi/asm/sigcontext.h

> +
> +struct target_ucontext {
> +    target_ulong tuc_flags;
> +    target_ulong tuc_link;
> +    target_stack_t tuc_stack;
> +    target_ulong pad0;
> +    struct target_sigcontext tuc_mcontext;
> +    target_sigset_t tuc_sigmask;
> +};

Does not match
https://github.com/loongson/linux/blob/loongarch-next/arch/loongarch/include/uapi/asm/ucontext.h

> +static inline void setup_sigcontext(CPULoongArchState *env,
> +                                    struct target_sigcontext *sc)

Drop all of the the inline markers.

> +{
> +    int i;
> +
> +    __put_user(env->pc, &sc->sc_pc);
> +
> +    __put_user(0, &sc->sc_gpr[0]);
> +    for (i = 1; i < 32; ++i) {
> +        __put_user(env->gpr[i], &sc->sc_gpr[i]);
> +    }
> +
> +    for (i = 0; i < 32; ++i) {
> +        __put_user(env->fpr[i], &sc->sc_fpr[i]);
> +    }
> +}

Missing fcsr and fcc.

I'll note that the kernel is missing sets of vscr and scr[0-3].  IMO they should at least 
be zeroed in advance of supporting the vector extension.

> +static inline void
> +restore_sigcontext(CPULoongArchState *env, struct target_sigcontext *sc)
> +{
> +    int i;
> +
> +    __get_user(env->pc, &sc->sc_pc);
> +
> +    for (i = 1; i < 32; ++i) {
> +        __get_user(env->gpr[i], &sc->sc_gpr[i]);
> +    }
> +
> +    for (i = 0; i < 32; ++i) {
> +        __get_user(env->fpr[i], &sc->sc_fpr[i]);
> +    }
> +}

Similarly.

> +    return (sp - frame_size) & ~7;

include/asm/asm.h:#define ALMASK        ~15
kernel/signal.c:        return (void __user *)((sp - frame_size) & ALMASK);

> +    env->pc = env->gpr[20] = ka->_sa_handler;

There is no set of gpr[20].

> +void setup_sigtramp(abi_ulong sigtramp_page)
> +{
> +    uint32_t *tramp = lock_user(VERIFY_WRITE, sigtramp_page, 8, 0);
> +    assert(tramp != NULL);
> +
> +    __put_user(0x03822c0b, tramp + 0);  /* ori     a7, a7, 0x8b */

The comment is incorrect: "ori a7, zero, 0x8b", but the hex is right.

> +/* this struct defines a stack used during syscall handling */
> +typedef struct target_sigaltstack {
> +        abi_long ss_sp;
> +        abi_int ss_flags;
> +        abi_ulong ss_size;
> +} target_stack_t;
> +
> +/*
> + * sigaltstack controls
> + */
> +#define TARGET_SS_ONSTACK     1
> +#define TARGET_SS_DISABLE     2
> +
> +#define TARGET_MINSIGSTKSZ    2048
> +#define TARGET_SIGSTKSZ       8192

We should move these to generic/signal.h.


r~