Re: [RFC PATCH v2 01/44] target/loongarch: Add LSX data type VReg

[Richard Henderson]

On 3/27/23 20:05, Song Gao wrote:
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   linux-user/loongarch64/signal.c |  4 ++--
>   target/loongarch/cpu.c          |  2 +-
>   target/loongarch/cpu.h          | 31 +++++++++++++++++++++++++++++-
>   target/loongarch/gdbstub.c      |  4 ++--
>   target/loongarch/machine.c      | 34 ++++++++++++++++++++++++++++++++-
>   5 files changed, 68 insertions(+), 7 deletions(-)
>
> diff --git a/linux-user/loongarch64/signal.c b/linux-user/loongarch64/signal.c
> index 7c7afb652e..bb8efb1172 100644
> --- a/linux-user/loongarch64/signal.c
> +++ b/linux-user/loongarch64/signal.c
> @@ -128,7 +128,7 @@ static void setup_sigframe(CPULoongArchState *env,
>   
>       fpu_ctx = (struct target_fpu_context *)(info + 1);
>       for (i = 0; i < 32; ++i) {
> -        __put_user(env->fpr[i], &fpu_ctx->regs[i]);
> +        __put_user(env->fpr[i].vreg.D(0), &fpu_ctx->regs[i]);
>       }
>       __put_user(read_fcc(env), &fpu_ctx->fcc);
>       __put_user(env->fcsr0, &fpu_ctx->fcsr);
> @@ -193,7 +193,7 @@ static void restore_sigframe(CPULoongArchState *env,
>           uint64_t fcc;
>   
>           for (i = 0; i < 32; ++i) {
> -            __get_user(env->fpr[i], &fpu_ctx->regs[i]);
> +            __get_user(env->fpr[i].vreg.D(0), &fpu_ctx->regs[i]);
>           }
>           __get_user(fcc, &fpu_ctx->fcc);
>           write_fcc(env, fcc);
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 97e6579f6a..18b41221a6 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -656,7 +656,7 @@ void loongarch_cpu_dump_state(CPUState *cs, FILE *f, int 
> flags)
>       /* fpr */
>       if (flags & CPU_DUMP_FPU) {
>           for (i = 0; i < 32; i++) {
> -            qemu_fprintf(f, " %s %016" PRIx64, fregnames[i], env->fpr[i]);
> +            qemu_fprintf(f, " %s %016" PRIx64, fregnames[i], 
> env->fpr[i].vreg.D(0));
>               if ((i & 3) == 3) {
>                   qemu_fprintf(f, "\n");
>               }
> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> index e11c875188..6e5fa6a01d 100644
> --- a/target/loongarch/cpu.h
> +++ b/target/loongarch/cpu.h
> @@ -8,6 +8,7 @@
>   #ifndef LOONGARCH_CPU_H
>   #define LOONGARCH_CPU_H
>   
> +#include "qemu/int128.h"
>   #include "exec/cpu-defs.h"
>   #include "fpu/softfloat-types.h"
>   #include "hw/registerfields.h"
> @@ -241,6 +242,34 @@ FIELD(TLB_MISC, ASID, 1, 10)
>   FIELD(TLB_MISC, VPPN, 13, 35)
>   FIELD(TLB_MISC, PS, 48, 6)
>   
> +#define LSX_LEN   (128)
> +typedef union VReg {
> +    int8_t   B[LSX_LEN / 8];
> +    int16_t  H[LSX_LEN / 16];
> +    int32_t  W[LSX_LEN / 32];
> +    int64_t  D[LSX_LEN / 64];
> +    Int128   Q[LSX_LEN / 128];
> +}VReg;
> +
> +typedef union fpr_t fpr_t;
> +union fpr_t {
> +    VReg  vreg;
> +};
> +
> +#if  HOST_BIG_ENDIAN
> +#define B(x)  B[15 - (x)]
> +#define H(x)  H[7 - (x)]
> +#define W(x)  W[3 - (x)]
> +#define D(x)  D[1 - (x)]
> +#define Q(x)  Q[x]
> +#else
> +#define B(x)  B[x]
> +#define H(x)  H[x]
> +#define W(x)  W[x]
> +#define D(x)  D[x]
> +#define Q(x)  Q[x]
> +#endif

It would probably be better to move these rather generically named macros outside of cpu.h 
(e.g. internals.h).

> @@ -33,7 +33,39 @@ const VMStateDescription vmstate_loongarch_cpu = {
>   
>           VMSTATE_UINTTL_ARRAY(env.gpr, LoongArchCPU, 32),
>           VMSTATE_UINTTL(env.pc, LoongArchCPU),
> -        VMSTATE_UINT64_ARRAY(env.fpr, LoongArchCPU, 32),
> +        VMSTATE_INT64(env.fpr[0].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[1].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[2].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[3].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[4].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[5].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[6].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[7].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[8].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[9].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[10].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[11].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[12].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[13].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[14].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[15].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[16].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[17].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[18].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[19].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[20].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[21].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[22].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[23].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[24].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[25].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[26].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[27].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[28].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[29].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[30].vreg.D(0), LoongArchCPU),
> +        VMSTATE_INT64(env.fpr[31].vreg.D(0), LoongArchCPU),

Do you care about migration compatibility between qemu versions?
If not, it might be easier to handle the vector registers differently.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~