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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.17 at 11:53, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Oh, right.

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

Yes, I was about to ask you to do that (in reply to v3).

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

That's what Petre's patch does. But my question was about
questionable uses of generate_exception_if(..., EXC_UD).

Jan


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