Re: [PATCH v1 2/6] target/loongarch: Add set_vec_extctx to set LSX/LASX instructions extctx_flags

[Richard Henderson]

On 10/29/23 20:28, gaosong wrote:
> 在 2023/10/29 上午5:40, Richard Henderson 写道:
> > On 10/9/23 20:36, Song Gao wrote:
> > > Signed-off-by: Song Gao <gaosong@loongson.cn>
> > > ---
> > >   target/loongarch/insn_trans/trans_vec.c.inc | 12 ++++++++++++
> > >   target/loongarch/internals.h                |  2 ++
> > >   2 files changed, 14 insertions(+)
> > >
> > > diff --git a/target/loongarch/insn_trans/trans_vec.c.inc 
> > > b/target/loongarch/insn_trans/trans_vec.c.inc
> > > index 98f856bb29..aef16ef44a 100644
> > > --- a/target/loongarch/insn_trans/trans_vec.c.inc
> > > +++ b/target/loongarch/insn_trans/trans_vec.c.inc
> > > @@ -23,8 +23,20 @@ static bool check_vec(DisasContext *ctx, uint32_t oprsz)
> > >     #else
> > >   +static void set_vec_extctx(DisasContext *ctx, uint32_t oprsz)
> > > +{
> > > +    if (oprsz == 16) {
> > > +        ctx->extctx_flags |= EXTCTX_FLAGS_LSX;
> > > +    }
> > > +
> > > +    if (oprsz == 32) {
> > > +        ctx->extctx_flags |= EXTCTX_FLAGS_LASX;
> > > +    }
> > > +}
> > > +
> > >   static bool check_vec(DisasContext *ctx, uint32_t oprsz)
> > >   {
> > > +    set_vec_extctx(ctx, oprsz);
> > >       return true;
> > >   }
> >
> > This doesn't do anything.  Nothing copies the changed value back to env.
> > Anyway, I think this is the wrong way to go about it.
> Oh, It is on patch1.
>
> @@ -294,6 +296,7 @@ static void loongarch_tr_translate_insn(DisasContextBase *dcbase, 
> CPUState *cs)
>           generate_exception(ctx, EXCCODE_INE);
>       }
>
> +    env->extctx_flags |= ctx->extctx_flags;

Ah, well, this is also incorrect.

This copy only happens at translation time, not at execution time.

Anyway, I think my previous suggestion is better:

> > If you want to track what the program is using, you should do it exactly like the real 
> > kernel: disable the execution unit, have the program trap, and the enable the execution 
> > unit when the trap occurs. At this point, CSR_EUEN enable bits contain exactly which 
> > units have been used by the program.


r~