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

[Robert Hoo <robert.hu@linux.intel.com>]

On Wed, 2021-03-24 at 07:44 -0600, Richard Henderson wrote:
> 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?

OK
> 
> > +        } else if (env->xcr0 & XFEATURE_AVX) {
> 
> This is normally a 2-bit test.

I beg your pardon. What 2 bits?
> 
> > +            /* 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.

Sure. I just retained previous code.
BTW, checkpatch didn't warn me on this. It escaped.:)
> 
> > +#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.

Ah, my poor eye sight. They even exist in the same file. Thanks
pointing out.
> 
> 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.
> 
Sure.
> 
> r~