Re: [PATCH v16 2/9] linux-user: Add LoongArch signal support

[Richard Henderson]

On 6/14/22 02:05, Song Gao wrote:
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   linux-user/loongarch64/signal.c        | 283 +++++++++++++++++++++++++
>   linux-user/loongarch64/target_signal.h |  13 ++
>   2 files changed, 296 insertions(+)
>   create mode 100644 linux-user/loongarch64/signal.c
>   create mode 100644 linux-user/loongarch64/target_signal.h

You copied too much directly from the kernel, without changing to match the host/guest 
split that is present in qemu.

> +struct target_ctx_layout {
> +    struct target_sctx_info *addr;

abi_ulong.

> +    unsigned int size;
> +};
> +
> +struct target_extctx_layout {
> +    unsigned long size;

unsigned int -- it only needs to hold sizeof(target_fpu_context) + 
sizeof(target_sctx_info).  Use of "unsigned long" in qemu is generally incorrect.

Both of these two structures should drop the "target_" prefix from the name, because they 
do not appear in target memory.  They are implementation details of this file.

> +static void *get_ctx(struct target_sctx_info *info)
> +{
> +    return (void *)((char *)info + sizeof(struct target_sctx_info));
> +}

Return type should be struct target_sctx_info *.

> +static unsigned long extframe_alloc(struct target_extctx_layout *extctx,
> +                                    struct target_ctx_layout *layout,
> +                                    size_t size, unsigned long base)
> +{
> +    unsigned long new_base = base - size;
> +
> +    new_base -= sizeof(struct target_sctx_info);
> +    layout->addr = (void *)new_base;
> +    layout->size = (unsigned int)(base - new_base);
> +    extctx->size += layout->size;

All of these unsigned long should be abi_ulong.
The cast into layout->addr is wrong.

> +static unsigned long setup_extcontext(struct target_extctx_layout *extctx,
> +                                      unsigned long sp)
> +{
> +    unsigned long new_sp = sp;
> +
> +    memset(extctx, 0, sizeof(struct target_extctx_layout));
> +    new_sp -= sizeof(struct target_sctx_info);
> +
> +    extctx->end.addr = (void *) new_sp;
> +    extctx->end.size = (unsigned int)sizeof(struct target_sctx_info);
> +    extctx->size += extctx->end.size;
> +    extctx->flags = SC_USED_FP;
> +
> +    new_sp = extframe_alloc(extctx, &extctx->fpu,
> +                            sizeof(struct target_fpu_context), new_sp);
> +
> +    return new_sp;
> +}

More unsigned long and casting errors.


> +static void restore_sigcontext(CPULoongArchState *env,
> +                               struct target_sigcontext *sc)
> +{
> +    int i;
> +    struct target_extctx_layout extctx;
> +
> +    memset(&extctx, 0, sizeof(struct target_extctx_layout));
> +
> +    __get_user(extctx.flags, &sc->sc_flags);
> +
> +    extctx.fpu.addr = (struct target_sctx_info *)&sc->sc_extcontext;

This is wrong.  You're missing all of the code from parse_extcontext().


r~