Re: [Xen-devel] [PATCH RFC v2] x86/emul: Fix the handling of unimplemented Grp7 instructions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/09/2017 10:43, Jan Beulich wrote:
>>>> On 05.09.17 at 09:34, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 05/09/2017 07:57, Jan Beulich wrote:
>>>>>> Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 09/04/17 7:35 PM >>>
>>>> Grp7 is abnormally complicated to decode, even by x86's standards, with
>>>> {s,l}msw being the problematic cases.
>>>>
>>>> Previously, any value which fell through the first switch statement 
>>>> (looking
>>>> for instructions with entirely implicit operands) would be interpreted by 
>> the
>>>> second switch statement (handling instructions with memory operands).
>>>>
>>>> Unimplemented instructions would then hit the #UD case for having a 
>> non-memory
>>>> operand, rather than taking the cannot_emulate path.
>>>>
>>>> Place a big if/else around the two switch statements (accounting for 
>> {s,l}msw
>>>> which need handling in the else clause), so both switch statments can have 
>>>> a
>>>> default goto cannot_emulate path.
>>>>
>>>> This fixes the emulation of xend, which would hit the #UD path when it 
>> should
>>>> complete with no side effects.
>>> This could be had with a single line change. And while I can see this 
>> mistake
>>> of mine alone to be justification for the restructuring, it's still rather 
>> big a change
>>> due to all the re-indentation. Did you instead consider simply combining the
>>> two switch() statements (retaining present indentation), by using range case
>>> labels for the opcodes permitting operands?
>> That was my first idea, but the cases are not adjacent.  You need 3
>> ranges for the mod != 11 instructions, and 4 for {s,l}msw, and there was
>> no clean way I could find to express that.
> I see you've found one (which is largely what I was going to suggest).
>
>>>  That would have the added benefit
>>> of no longer producing #UD for things like VMCALL, but instead having those
>>> go to cannot_emulate too.
>> This is the behaviour the patch is intended to introduce.  What's broken
>> with the logic?
> I guess you've realized meanwhile that it was the
>
>                 generate_exception_if(ea.type != OP_MEM, EXC_UD);
>
> that were left in place, which were causing the sub-optimal
> behavior.

VMCALL is encoded with mod == 11, so now doesn't fall into the sgdt case.

> Speaking of which - do we want to go farther and
> convert further similar #UD raising into cannot_emulate (or
> with Petre's unimplemented_insn) goto-s?

In this specific case, I think the generate_exception_if(ea.type !=
OP_MEM, EXC_UD); can be converted to asserts, because the only way to
violate that condition is with an earlier error calculating ea or modrm.

In the general case, I think we should prefer the cannot_emulate path. 
Are there any uses of this label which aren't due to having no
implementation?  If so, we probably want to introduce a new label so the
two cases can be distinguished.

~Andrew

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