Re: [Xen-devel] [PATCH v2 01/17] x86emul: support remaining AVX insns

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 17:36, <george.dunlap@xxxxxxxxxx> wrote:
> On 09/14/2017 04:12 PM, Jan Beulich wrote:
>> @@ -7119,6 +7142,18 @@ x86_emulate(
>>          fic.insn_bytes = PFX_BYTES + 3;
>>          break;
>>  
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x19): /* vbroadcastsd m64,ymm */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x1a): /* vbroadcastf128 m128,ymm */
>> +        generate_exception_if(!vex.l, EXC_UD);
>> +        /* fall through */
>> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x18): /* vbroadcastss m32,{x,y}mm */
>> +        generate_exception_if(ea.type != OP_MEM, EXC_UD);
> 
> Just checking my understanding here: The reason for this check is that
> as of this patch, we still only support AVX instructions, not AVX2 (and
> later) variants, which have non-memory-source variants?

That's a good point actually. Yes, for this patch alone it's fine to
handle just memory operands. But the later AVX2 patch doesn't
generalize this - I've managed to overlook this aspect. Based on
how other support has been added, it could have been done the
other way as well (using suitable conditionals), but now that this
patch has gone in, I'll have to do it in the AVX2 patch.

> I'm not trying to complain, but I want to reflect back some of what I'm
> experiencing trying to review this series.  After having gotten somewhat
> up to speed on the instructions and terminology, and the general layout
> of the existing code, I understand that the basic method for adding a
> new instruction of this type is:
> 
> 1. Add instruction re-execution support
>   a. Add decode information into the appropriate tables (in this case
> ext0f38_table[] and ext0f3a_table[]
>   b. Add case statements which
>    - Check for the appropriate conditions
>    - Set up anything that needs setting up for the instruction
> re-execution (in the "if (state->simd_size)" clause).
> 
> 2. Add testing

Whatever that means. Generally I'm trying to add tests for new
code paths, or for insns using unusual operand sizes. But I'm not
going to claim the testing being added is always completely
covering all new insns.

> And of course 1b may involve extending functionality, such as adding
> simd_128 or handling new source / destination modes or combinations.
> 
> So a proper review would involve making sure that all the above had been
> done properly: That the right instruction was decoded, the right tables
> set up, the right conditions checked, the right inputs made to the
> re-execution code further down; and then of course making sure that
> these instructions were properly added to the testing matrix.
> 
> So I look up the instructions named above (vbroadcast*) and verified
> that they matched the codes listed.  The manual says first two generate
> an exception if vex.l == 0; so far so good.
> 
> But what's the ea.type != MEM thing?  The manual certainly says there
> are instructions that don't reference memory.  A bit of looking and
> poking around, and I formulated the question above.  The test for AVX
> support is made in a completely different part of the file, and no
> mention of AVX2 / whatever is done here.
> 
> OK, go up and check the tables -- additions to ext0f38 at 0x18, 0x19,
> and 0x1a seem to match the description.
> 
> Now what?  How do I verify that everything else on the path will work as
> it should?

Well, if we assume that the code paths outside the big switch
statements work, then it is really only the table addition and the
customization in the new case label which needs verifying. If otoh
you suspect that the glue between existing (common) and new
(per opcode) code isn't right, then going through the common
parts with the specifics of the new instruction in mind won't be
avoidable. But isn't that how you'd review other code as well?

> And I still don't have any idea how to start on verifying that these
> instructions have been added to the test framework properly.

Is seeing the respective compiler intrinsics being used (or, where
those aren't suitable, inline asm()-s with the very instructions) not
enough? Plus, as said above, don't expect every single insn to be
tested, or even every single flavor of one insn.

> I'm only 3 instructions in, with at least 20 more to go.
> 
> There is certainly something to be said for saying that if you're going
> to review a change to code you have to understand the underlying code
> itself; and with a piece of code as complicated as this, there's just no
> way around that being a big learning curve.
> 
> But as a practical matter, very few people are that familiar with this
> code.  Even without all the other random distractions with security
> issues and such, I doubt anyone who hadn't specifically been trying to
> put forward the effort to help you would have made it through reviewing
> this patch without deciding their time would be better spent elsewhere.
> 
> So it really seems to be that if you want someone to review this code --
> particularly anyone besides Andy, but probably even with Andy -- you
> have to go further into making it easier for someone able to read a
> manual and read code, but not intimately familiar with either the x86
> instruction set, the emulator, or the testing framework, to verify that
> the changes you're making are correct.

I can fully understand what you say, yet this doesn't help me see
how I could have helped the situation. I could have split the patch
into smaller pieces (one insn at a time, for example), but that wouldn't
have changed the amount of checking you'd have to do. It would
merely reduce the granularity at which you could send back comments
or an ack.

I specifically cannot see how I could have helped you with a more
verbose description, since everything the patch does that isn't
mentioned there is simply cloning existing code to support the new
insns, which the title names. In particular I don't think listing
individual instructions being added would be helpful. And in no
case do I think that reproducing any information the SDM has
would be useful.

> Maybe after the code freeze I'll see if I can re-work this patch (or
> portions of it) into something which I think would be easier to review;
> both as an exercise for myself (to make sure I understand what's going
> on), and an an example for what I'm talking about.  Given the rate this
> is going, there's no way I'm going to be able to give an R-b for this
> patch before the freeze.

Given this patch has gone in with Andrew's ack (which wasn't sent
on the list), perhaps it would be worthwhile instead doing any such
for one of the later patches (the F16C patch, adding just two insns,
may be a good candidate if you're not meaning to also split up
patches into smaller pieces, whereas e.g. the AVX2 one would be
about the same size and number of added insns as the one here is).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel