Re: [PATCH v6 06/14] target/riscv: rvk: add support for zknd/zkne extension in RV32

[Weiwei Li]

在 2022/2/28 上午3:05, Richard Henderson 写道:
> On 2/27/22 04:25, Weiwei Li wrote:
> > +#define AES_SHIFROWS_LO(RS1, RS2) ( \
> > +    (((RS1 >> 24) & 0xFF) << 56) | (((RS2 >> 48) & 0xFF) << 48) | \
> > +    (((RS2 >> 8) & 0xFF) << 40) | (((RS1 >> 32) & 0xFF) << 32) | \
> > +    (((RS2 >> 56) & 0xFF) << 24) | (((RS2 >> 16) & 0xFF) << 16) | \
> > +    (((RS1 >> 40) & 0xFF) << 8) | (((RS1 >> 0) & 0xFF) << 0))
> > +
> > +#define AES_INVSHIFROWS_LO(RS1, RS2) ( \
> > +    (((RS2 >> 24) & 0xFF) << 56) | (((RS2 >> 48) & 0xFF) << 48) | \
> > +    (((RS1 >> 8) & 0xFF) << 40) | (((RS1 >> 32) & 0xFF) << 32) | \
> > +    (((RS1 >> 56) & 0xFF) << 24) | (((RS2 >> 16) & 0xFF) << 16) | \
> > +    (((RS2 >> 40) & 0xFF) << 8) | (((RS1 >> 0) & 0xFF) << 0))
>
> Unused.  Whcih is good, because it shifts an unknown type by 56.
It's used by aes64 operation. I'll move them to next patch.
> > +DEF_HELPER_3(aes32esmi, tl, tl, tl, tl)
> > +DEF_HELPER_3(aes32esi, tl, tl, tl, tl)
> > +DEF_HELPER_3(aes32dsmi, tl, tl, tl, tl)
> > +DEF_HELPER_3(aes32dsi, tl, tl, tl, tl)
>
> DEF_HELPER_FLAGS_3, with TCG_CALL_NO_RWG_SE.
OK. I'll fix it.
> > +static inline target_ulong aes32_operation(target_ulong bs, 
> > target_ulong rs1,
> > +                                           target_ulong rs2, bool enc,
> > +                                           bool mix)
> > +{
> > +    uint8_t shamt = bs << 3;
>
> Just thinking it would be better to pass in shamt, since bs is 
> otherwise unused.
OK. I'll fix it.
> > +    mixed = (mixed << shamt) | (mixed >> (32 - shamt));
>
> This is broken for shamt == 0.  This is intending a rol32(), I assume?
OK. I'll fix it.
> > +static bool trans_aes32esmi(DisasContext *ctx, arg_aes32esmi *a)
> > +{
> > +    REQUIRE_ZKNE(ctx);
> > +
> > +    TCGv bs = tcg_const_tl(a->bs);
> > +    TCGv dest = dest_gpr(ctx, a->rd);
> > +    TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> > +    TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> > +
> > +    gen_helper_aes32esmi(dest, src1, src2, bs);
> > +    gen_set_gpr(ctx, a->rd, dest);
> > +
> > +    tcg_temp_free(bs);
> > +    return true;
> > +}
> > +
>
> tcg_constant_tl, which then need not be freed.
> You should create a common helper for these 4 functions, so that you 
> do not replicate so much code between them.
OK. I'll fix them.

Regards,

Weiwei Li

> r~