Re: [PATCH v3] i386/cpu_dump: support AVX512 ZMM regs dump

[Richard Henderson]

On 3/24/21 2:00 AM, Robert Hoo wrote:
> +        if ((env->xcr0 & XFEATURE_AVX512) == XFEATURE_AVX512) {
> +            /* XSAVE enabled AVX512 */
> +            nb = (env->hflags & HF_CS64_MASK) ? 32 : 8;
> +            for (i = 0; i < nb; i++) {
> +                qemu_fprintf(f, "ZMM%02d=0x%016lx %016lx %016lx %016lx %016lx "
> +                                "%016lx %016lx %016lx\n",
> +                             i,
> +                             env->xmm_regs[i].ZMM_Q(7),
> +                             env->xmm_regs[i].ZMM_Q(6),
> +                             env->xmm_regs[i].ZMM_Q(5),
> +                             env->xmm_regs[i].ZMM_Q(4),
> +                             env->xmm_regs[i].ZMM_Q(3),
> +                             env->xmm_regs[i].ZMM_Q(2),
> +                             env->xmm_regs[i].ZMM_Q(1),
> +                             env->xmm_regs[i].ZMM_Q(0));
> +            }

Dump opmask regs?

> +        } else if (env->xcr0 & XFEATURE_AVX) {

This is normally a 2-bit test.

> +            /* XSAVE enabled AVX */
> +            nb = env->hflags & HF_CS64_MASK ? 16 : 8;
> +            for (i = 0; i < nb; i++) {
> +                qemu_fprintf(f, "YMM%02d=0x%016lx %016lx %016lx %016lx\n",
> +                             i,
> +                             env->xmm_regs[i].ZMM_Q(3),
> +                             env->xmm_regs[i].ZMM_Q(2),
> +                             env->xmm_regs[i].ZMM_Q(1),
> +                             env->xmm_regs[i].ZMM_Q(0));
> +            }
> +        } else { /* SSE and below cases */
> +            nb = env->hflags & HF_CS64_MASK ? 16 : 8;
> +            for (i = 0; i < nb; i++) {
> +                qemu_fprintf(f, "XMM%02d=0x%016lx %016lx",
> +                             i,
> +                             env->xmm_regs[i].ZMM_Q(1),
> +                             env->xmm_regs[i].ZMM_Q(0));
> +                if ((i & 1) == 1)
> +                    qemu_fprintf(f, "\n");
> +                else
> +                    qemu_fprintf(f, " ");

I'd be tempted to merge that second printf into the first, with "%s" and (i & 1 
? "\n" : " ").  Otherwise you'll need to add braces to that IF to satisfy 
checkpatch.

> +#define XFEATURE_X87        (1UL << 0)
> +#define XFEATURE_SSE        (1UL << 1)
> +#define XFEATURE_AVX        (1UL << 2)
> +#define XFEATURE_AVX512_OPMASK          (1UL << 5)
> +#define XFEATURE_AVX512_ZMM_Hi256       (1UL << 6)
> +#define XFEATURE_AVX512_Hi16_ZMM        (1UL << 7)
> +#define XFEATURE_AVX512     (XFEATURE_AVX512_OPMASK | \
> +                             XFEATURE_AVX512_ZMM_Hi256 | \
> +                             XFEATURE_AVX512_Hi16_ZMM)

Except for the last, these already exist under the name XSTATE_*_MASK.

I think you can just as well declare local variables to hold the 3 bits for the 
avx512 test and the 2 bits for the avx test.


r~