Re: [Xen-devel] [PATCH v8 32/50] x86emul: support AVX512F gather insns

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/03/2019 10:58, Jan Beulich wrote:
> This requires getting modrm_reg and sib_index set correctly in the EVEX
> case, to account for the high 16 [XYZ]MM registers. Extend the
> adjustments to modrm_rm as well, such that x86_insn_modrm() would
> correctly report register numbers (this was a latent issue only as we
> don't currently have callers of that function which would care about an
> EVEX case). The adjustment in turn requires dropping the assertion from
> decode_gpr() as well as re-introducing the explicit masking, as we now
> need to actively mask off the high bit when a GPR is meant.
> _decode_gpr() invocations also need slight adjustments, when invoked in
> generic code ahead of the main switch(). All other uses of modrm_reg and
> modrm_rm already get suitably masked where necessary.
>
> There was also an encoding mistake in the EVEX Disp8 test code, which
> was benign (due to %rdx getting set to zero) to all non-vSIB tests as it
> mistakenly encoded <disp8>(%rdx,%rdx) instead of <disp8>(%rdx,%riz). In
> the vSIB case this meant <disp8>(%rdx,%zmm2) instead of the intended
> <disp8>(%rdx,%zmm4).
>
> Likewise the access count check wasn't entirely correct for the S/G
> case: In the quad-word-index but dword-data case only half the number
> of full vector elements get accessed.
>
> As an unrelated change in the main test harness source file distinguish
> the "n/a" messages by bitness.

There is a very large amount going on here, and too much for a single patch.

I think the modrm fixes want splitting out because those alone are
subtle.  Its reasonable to keep the emulator test fixes in with the new
functionality which notices the bug, and it is a one-liner.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -662,8 +662,6 @@ static inline unsigned long *decode_gpr(
>      BUILD_BUG_ON(ARRAY_SIZE(cpu_user_regs_gpr_offsets) &
>                   (ARRAY_SIZE(cpu_user_regs_gpr_offsets) - 1));
>  
> -    ASSERT(modrm < ARRAY_SIZE(cpu_user_regs_gpr_offsets));
> -
>      /* Note that this also acts as array_access_nospec() stand-in. */

I can't locate this comment anywhere in Xen's history.

The comment currently in tree is:

/* For safety in release builds.  Debug builds will hit the ASSERT() */

which will need adjusting to make it clear that we may modrm >
ARRAY_SIZE() and that this truncation operation is the correct action to
take.

~Andrew

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