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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.06.19 at 14:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Splitting out the ModR/M handling changes is certainly possible. I'll
do so since you ask for it, but I had deliberately not done so
because to me they looked "out of context" without the actual uses.

>> --- 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.

See "x86emul: avoid speculative out of bounds accesses" which, together
with 3 other patches in the same original series, is still awaiting your review.

Jan



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