Re: [PATCH v9 01/13] hvm: Move MAX_INST_LEN into x86_emulate.h

[Don Slutz <dslutz@verizon.com>]

On 02/17/15 04:52, Andrew Cooper wrote:
> On 16/02/15 23:05, Don Slutz wrote:
>> Change some hard coded 15 into MAX_INST_LEN
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> 
> One formatting suggestion.
> 
>> ---
...
>> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
>> index f13f07d..42e2588 100644
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -579,8 +579,8 @@ do{ asm volatile (                                                      \
>>  ({ 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 */           \
>> -   generate_exception_if((uint8_t)(_regs.eip - ctxt->regs->eip) > 15,   \
>> -                         EXC_GP, 0);                                    \
>> +   generate_exception_if((uint8_t)(_regs.eip - ctxt->regs->eip) >       \
>> +			 MAX_INST_LEN, EXC_GP, 0);                       \
> 
> This would probably be better with the uint8_t cast on starting on the
> next line, which would avoid the need to put a linebreak in the middle
> of the comparison.
> 

If I am parsing this correctly, you feel that:

   generate_exception_if(                                               \
           (uint8_t)(_regs.eip - ctxt->regs->eip) > MAX_INST_LEN,       \
           EXC_GP, 0);                                                  \
   rc = ops->insn_fetch(x86_seg_cs, _eip, &_x, (_size), ctxt);          \

is the way to go.  I will plan to change it this way for v10 unless
otherwise notified.

    -Don Slutz

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>