Re: [Xen-devel] [PATCH v3 2/5] x86emul: don't assume a memory operand

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/16 13:14, Jan Beulich wrote:
>>>> On 07.12.16 at 14:05, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 07/12/16 08:22, Jan Beulich wrote:
>>>>>> On 06.12.16 at 17:49, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 06/12/16 11:13, Jan Beulich wrote:
>>>>> @@ -2359,7 +2360,7 @@ x86_decode(
>>>>>          }
>>>>>      }
>>>>>  
>>>>> -    if ( override_seg != -1 && ea.type == OP_MEM )
>>>>> +    if ( override_seg != x86_seg_none )
>>>>>          ea.mem.seg = override_seg;
>>>> Could we get away with asserting ea.type == OP_MEM if override_seg is
>>>> set, to help validate our assumptions about state?  (Possibly even
>>>> passing #UD back in the non-debug case)
>>> That would be wrong - we'd be asserting guest controlled state.
>>> There's nothing preventing a segment override to be present on
>>> instructions without memory operands.
>> True, but such overrides should have no effect on the instruction.
> Correct. But we can't ASSERT() anything for that reason.
>
>>> And for example string
>>> insns don't have OP_MEM set despite having (implicit) memory
>>> operands (after all that's the hole reason for the change here
>>> [but not the patch as a hole], as the following PV priv-op patch
>> Do you mean whole instead of hole here?
> Yes - shame on me, even twice the same mistake.
>
>>> requires the segment override to take effect on OUTS). Nor
>>> would such be correct for conditional branches, where some of
>>> the segment overrides have a different meaning (necessarily
>>> ignored by the emulator).
>> The point I am trying to address is that it feels wrong to be
>> referencing ea.mem for non OP_MEM types.  I accept that, no longer being
>> a union, this use should be safe.
>>
>> The string instructions are certainly a spanner in the works, but the
>> jump instructions need not care about likely/unlikely prefixes.  They
>> were purely advisory to start with, and are ignored on all modern hardware.
>>
>> Another spanner in the works is the upcoming meaning of %ds overrides
>> for jump instructions as the no-track hint from the forthcoming
>> Control-flow Enforcement extensions.
>>
>> Given that there can only ever be one active segment override, does it
>> need to be stored in ea.mem in the first place, or could it live
>> somewhere else which is mem-neutral?
> It could, but that wouldn't make ea.mem.seg go away, and it's a
> convenient place to put it. Also if we put it elsewhere we'd have
> to (a) change all existing uses (including all places structure-
> assigning dst or src from ea), and be rather careful with future
> changes.

Ok.  For now, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>,
but I would like to see if we can improve this in the future.

~Andrew

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