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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/11/2018 11:12, Jan Beulich wrote:
>>>> On 12.11.18 at 18:42, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> Correct, but I've specifically tried to make the two models sufficiently
> different.
>
>> 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's what some of the tests added right in patch 5 do. Did you
> intentionally skip that patch while reviewing?

I intentionally wanted to understand this patch first.

>
>> That way, a systematic error in the two models won't cancel out to "all ok".
> Hence the two different models. I certainly realize the risk you
> name.
>
>>> --- /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"
> Yes, I've already re-based over that other change.
>
>>> +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?
> VL -> vector length
> EL -> element length

Can you at least leave trailing comments after the identifiers for the
benefit of people other than you reading the code?

>
>>> +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?
> The RUN() macro invocations (further down) reference one them
> each, to indicate what vector lengths to test. The first array
> entry does always get used, while subsequent entries (if any)
> require AVX512VL to be available. See the conditional at the top
> of the inner loop in test_group().

After re-reading the apparently relevant bits of Vol 1, 2 and 3, I'm
still actually none the wiser as to which AVX512 feature bits mean what.

Is there a chapter with an overview that I've overlooked, or if not, can
we see about putting one together?

>
>>> +
>>> +/*
>>> + * 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 * ?
> See record_access(): The instructions under test all get a Disp8 value
> of 1 encoded. In order to be able to sensibly see how exactly things
> go wrong (during debugging), it simply helps to cover the entire range
> from zero to 3 times the (maximum) vector length. All accesses farther
> out of bounds than by vector length will not be recorded here, and
> hence fail "silently".

Please can you put a short description in a comment somewhere around
about here.

~Andrew

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