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

[gaosong]

在 2023/10/29 上午5:35, Richard Henderson 写道:
> 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.
Ok,
> > @@ -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 think extctx_flags is incorrectly named, fp_alive_flags or 
vec_alive_flags would be more appropriate.
The flags function like the kernel's 'thread_lsx_context_live', 
'thread_lasx_context_live' functions, checking if the LSX/LASX 
instructions are used.
If we don't use the LSX/LASX instructions, we don't need to use 
lsx_context/lasx_context even though the LSX/LASX enable bit is set.

and  EXTCTX_FLAGS_FPU is not required.

Thanks.
Song Gao
> 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~