Re: [Xen-devel] [PATCH v4 06/44] x86emul: test for correct EVEX Disp8 scaling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/09/18 14:29, Jan Beulich wrote:
> Besides the already existing tests (which are going to be extended once
> respective ISA extension support is complete), let's also ensure for
> every individual insn that their Disp8 scaling (and memory access width)
> are correct.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

I can see what you're attempting to do, but you now have two
implementations of the EVEX disp8 logic written by yourself.  AFAICT,
this doesn't actually check that the behaviour of the instruction in
hardware matches your model of the instruction - it checks that two of
your models are the same.

The only way I can think of testing the emulator model against hardware
is to start with two memory area poisoned with a non-repeating pattern,
and a src/dst register poisoned with a different non-repeating pattern. 
Then, execute a real instruction stub, emulate the other and memcmp()
the two memory regions.

That way, a systematic error in the two models won't cancel out to "all ok".

Also, some observations about the code as presented.

> --- /dev/null
> +++ b/tools/tests/x86_emulator/evex-disp8.c
> @@ -0,0 +1,452 @@
> +#include <stdarg.h>
> +#include <stdio.h>
> +
> +#include "x86-emulate.h"

This now needs rearranging to avoid:

x86-emulate.h:30:3: error: #error "Must not include <stdio.h> before
x86-emulate.h"
 # error "Must not include <stdio.h> before x86-emulate.h"

> +
> +struct test {
> +    const char *mnemonic;
> +    unsigned int opc:8;
> +    unsigned int spc:2;
> +    unsigned int pfx:2;
> +    unsigned int vsz:3;
> +    unsigned int esz:4;
> +    unsigned int scale:1;
> +    unsigned int ext:3;
> +};
> +
> +enum spc {
> +    SPC_invalid,
> +    SPC_0f,
> +    SPC_0f38,
> +    SPC_0f3a,
> +};
> +
> +enum pfx {
> +    PFX_,
> +    PFX_66,
> +    PFX_f3,
> +    PFX_f2
> +};
> +
> +enum vl {
> +    VL_128,
> +    VL_256,
> +    VL_512,
> +};
> +
> +enum scale {
> +    SC_vl,
> +    SC_el,
> +};
> +
> +enum vsz {
> +    VSZ_vl,
> +    VSZ_vl_2, /* VL / 2 */
> +    VSZ_vl_4, /* VL / 4 */
> +    VSZ_vl_8, /* VL / 8 */
> +    /* "no broadcast" implied from here on. */
> +    VSZ_el,
> +    VSZ_el_2, /* EL * 2 */
> +    VSZ_el_4, /* EL * 4 */
> +    VSZ_el_8, /* EL * 8 */
> +};
> +

These acronyms get increasingly difficult to follow.  What is el in this
context?

> +enum esz {
> +    ESZ_d,
> +    ESZ_q,
> +    ESZ_dq,
> +    ESZ_sd,
> +    ESZ_d_nb,
> +    ESZ_q_nb,
> +    /* "no broadcast" implied from here on. */
> +#ifdef __i386__
> +    ESZ_d_WIG,
> +#endif
> +    ESZ_b,
> +    ESZ_w,
> +    ESZ_bw,
> +};
> +
> +#ifndef __i386__
> +# define ESZ_dq64 ESZ_dq
> +#else
> +# define ESZ_dq64 ESZ_d_WIG
> +#endif
> +
> +#define INSNX(m, p, sp, o, e, vs, es, sc) { \
> +    .mnemonic = #m, .opc = 0x##o, .spc = SPC_##sp, .pfx = PFX_##p, \
> +    .vsz = VSZ_##vs, .esz = ESZ_##es, .scale = SC_##sc, .ext = 0##e \
> +}
> +#define INSN(m, p, sp, o, vs, es, sc) INSNX(m, p, sp, o, 0, vs, es, sc)
> +#define INSN_PFP(m, sp, o) \
> +    INSN(m##pd, 66, sp, o, vl, q, vl), \
> +    INSN(m##ps,   , sp, o, vl, d, vl)
> +#define INSN_PFP_NB(m, sp, o) \
> +    INSN(m##pd, 66, sp, o, vl, q_nb, vl), \
> +    INSN(m##ps,   , sp, o, vl, d_nb, vl)
> +#define INSN_SFP(m, sp, o) \
> +    INSN(m##sd, f2, sp, o, el, q, el), \
> +    INSN(m##ss, f3, sp, o, el, d, el)
> +
> +#define INSN_FP(m, sp, o) \
> +    INSN_PFP(m, sp, o), \
> +    INSN_SFP(m, sp, o)
> +
> +static const struct test avx512f_all[] = {
> +    INSN_SFP(mov,            0f, 10),
> +    INSN_SFP(mov,            0f, 11),
> +    INSN_PFP_NB(mova,        0f, 28),
> +    INSN_PFP_NB(mova,        0f, 29),
> +    INSN(movdqa32,     66,   0f, 6f,    vl,   d_nb, vl),
> +    INSN(movdqa32,     66,   0f, 7f,    vl,   d_nb, vl),
> +    INSN(movdqa64,     66,   0f, 6f,    vl,   q_nb, vl),
> +    INSN(movdqa64,     66,   0f, 7f,    vl,   q_nb, vl),
> +    INSN(movdqu32,     f3,   0f, 6f,    vl,   d_nb, vl),
> +    INSN(movdqu32,     f3,   0f, 7f,    vl,   d_nb, vl),
> +    INSN(movdqu64,     f3,   0f, 6f,    vl,   q_nb, vl),
> +    INSN(movdqu64,     f3,   0f, 7f,    vl,   q_nb, vl),
> +    INSN(movntdq,      66,   0f, e7,    vl,   d_nb, vl),
> +    INSN(movntdqa,     66, 0f38, 2a,    vl,   d_nb, vl),
> +    INSN_PFP_NB(movnt,       0f, 2b),
> +    INSN_PFP_NB(movu,        0f, 10),
> +    INSN_PFP_NB(movu,        0f, 11),
> +};
> +
> +static const struct test avx512f_128[] = {
> +    INSN(mov,       66,   0f, 6e, el, dq64, el),
> +    INSN(mov,       66,   0f, 7e, el, dq64, el),
> +    INSN(movq,      f3,   0f, 7e, el,    q, el),
> +    INSN(movq,      66,   0f, d6, el,    q, el),
> +};
> +
> +static const struct test avx512bw_all[] = {
> +    INSN(movdqu8,     f2,   0f, 6f,    vl,    b, vl),
> +    INSN(movdqu8,     f2,   0f, 7f,    vl,    b, vl),
> +    INSN(movdqu16,    f2,   0f, 6f,    vl,    w, vl),
> +    INSN(movdqu16,    f2,   0f, 7f,    vl,    w, vl),
> +};
> +
> +static const unsigned char vl_all[] = { VL_512, VL_128, VL_256 };
> +static const unsigned char vl_128[] = { VL_128 };

What are these for, and why is vl_all[]'s VL_128 out of order?

> +
> +/*
> + * This table, indicating the presence of an immediate (byte) for an opcode
> + * space 0f major opcode, is indexed by high major opcode byte nibble, with
> + * each table element then bit-indexed by low major opcode byte nibble.
> + */
> +static const uint16_t imm0f[16] = {
> +    [0x7] = (1 << 0x0) /* vpshuf* */ |
> +            (1 << 0x1) /* vps{ll,ra,rl}w */ |
> +            (1 << 0x2) /* vps{l,r}ld, vp{rol,ror,sra}{d,q} */ |
> +            (1 << 0x3) /* vps{l,r}l{,d}q */,
> +    [0xc] = (1 << 0x2) /* vcmp{p,s}{d,s} */ |
> +            (1 << 0x4) /* vpinsrw */ |
> +            (1 << 0x5) /* vpextrw */ |
> +            (1 << 0x6) /* vshufp{d,s} */,
> +};
> +
> +static struct x86_emulate_ops emulops;
> +
> +static unsigned int accessed[3 * 64];

What are the expected properties?  Why 3 * ?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel