Re: [PATCH v1 5/6] linux-user/loongarch64: Add LSX sigcontext save/restore

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

On 10/9/23 20:37, Song Gao wrote:
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   linux-user/loongarch64/signal.c | 107 ++++++++++++++++++++++++++------
>   1 file changed, 87 insertions(+), 20 deletions(-)
> 
> diff --git a/linux-user/loongarch64/signal.c b/linux-user/loongarch64/signal.c
> index 277e9f5757..4b09e50a5f 100644
> --- a/linux-user/loongarch64/signal.c
> +++ b/linux-user/loongarch64/signal.c
> @@ -33,6 +33,14 @@ struct target_fpu_context {
>       uint32_t fcsr;
>   } QEMU_ALIGNED(FPU_CTX_ALIGN);
>   
> +#define LSX_CTX_MAGIC           0x53580001
> +#define LSX_CTX_ALIGN           16
> +struct target_lsx_context {
> +    uint64_t regs[2 * 32];
> +    uint64_t fcc;
> +    uint32_t fcsr;
> +} QEMU_ALIGNED(LSX_CTX_ALIGN);

It probably doesn't matter here because fo the alignment, but all types within target 
structures should be using abi_{ullong,uint} etc.


> @@ -99,8 +109,15 @@ static abi_ptr setup_extcontext(struct extctx_layout *extctx, abi_ptr sp)
>   
>       /* For qemu, there is no lazy fp context switch, so fp always present. */
>       extctx->flags = SC_USED_FP;
> -    sp = extframe_alloc(extctx, &extctx->fpu,
> +
> +    if (env->extctx_flags & EXTCTX_FLAGS_LSX) {
> +        sp = extframe_alloc(extctx, &extctx->lsx,
> +                        sizeof(struct target_lsx_context), LSX_CTX_ALIGN, sp);
> +
> +    } else if (env->extctx_flags & EXTCTX_FLAGS_FPU) {
> +        sp = extframe_alloc(extctx, &extctx->fpu,
>                           sizeof(struct target_fpu_context), FPU_CTX_ALIGN, sp);
> +    }

I think this is overly complicated.  (1) The fpu is always present, and (2) you don't need 
a special flag on env, you can check the same CSR bits as for system mode.

I'll note that while this layout matches the kernel, it is an unfortunate set of data 
structures.  Any program has to look for all of {FPU,LSX,LASX}_CTX_MAGIC in order to find 
the basic fp registers.


r~