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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/09/2017 05:12 PM, Jan Beulich wrote:
>>>> 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?

Well the question isn't whether I suspect there's something wrong; it's
checking to see if the author haven't missed something.  In order to
know that a particular instruction will DTRT when going through the
common code, I need to have an idea not only what the instruction needs,
but what the common code is doing.

And no, this process isn't different than what a reviewer has to do for
any patch; it's just particularly difficult for this patch.

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

No, but it would, for instance, have been easier to know which bits of
the patch went with which instruction.

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

Right, and I'm aware that my mail was unfortunately full of problems and
not solutions; which is why I said I'd try to re-work this patch as an
exercise for myself / example of what I mean.  (The end result might
even be that I come back and say that the patch you've posted is the
optimum from a comprehension / review perspective, although at the
moment I think that's unlikely.)  And I certainly don't think that
quoting large sections of the reference manual is useful; it is
reasonable to expect people reviewing this to have read the introduction
to the instruction format, and be able to look up the instruction name
in the Intel manual.

I won't write about things in detail now, but the basic idea comes down
to how much the reviewer has to *figure out* vs how much the reviewer is
*verifying*.  Verifying claims about what the code does before or after
the patch is a lot easier than figuring out important aspects about what
the code does before and after a patch.

And I want to emphasize: I'm not saying there was something wrong with
the patches; I'm just saying, as they are they it would be very unlikely
that someone not already very familiar with the code would review them.
And so if you want other people to review it, you may need to put in
some extra effort to make that possible.  (And hopefully I'll have
something useful to say about what effort would be useful in a little bit.)

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

Right; I'll take a look.

 -George

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