Re: [PATCH] x86_emulate: Always truncate %eip out of long mode

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/12/15 10:47, Jan Beulich wrote:
>>>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -570,8 +570,10 @@ do{ asm volatile (                                       
>>                \
>>  /* Fetch next part of the instruction being emulated. */
>>  #define insn_fetch_bytes(_size)                                         \
>>  ({ unsigned long _x = 0, _eip = _regs.eip;                              \
>> -   if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \
>> -   _regs.eip += (_size); /* real hardware doesn't truncate */           \
>> +   _regs.eip += (_size);                                                \
>> +   if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */  \
>> +       _eip      &= ~((1UL << (def_ad_bytes * 8)) - 1);                 \
> Okay, what we use for actual fetching gets truncated. I'm fine
> with that.
>
>> +       _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); };              \
> But I don't think it's correct to truncate what eventually gets
> written back.

The written-back value is the one which must be truncated, to avoid a
vmentry failure in the 32bit case.

The 16bit case is definitely less obvious.

> If we're in doubt what real hardware does, and if
> the manuals are of no help, perhaps we should ask the hardware
> folks?

In 16bit segments, the 32bit %eip register does exist, but the
instruction pointer is mapped to the first 16 bits of it.

The Intel manual warns: "The 8086 16-bit instruction pointer (IP) is
mapped to the lower 16-bits of the EIP register. Note this register is
a 32-bit register and unintentional address wrapping may occur."

I can't locate anything in particular in the AMD manuals which talks
specifically about 16bit semantics.

>
> AMD? Intel? Please clarify.
>
> Furthermore, doesn't this make the wrapping-inside-an-insn
> situation worse (i.e. what looks broken for 32- and 64-bit modes
> now gets broken also for 16-bit mode)?

I don't understand which "broken" you are referring to here.

~Andrew