From mboxrd@z Thu Jan  1 00:00:00 1970
From: Don Slutz <dslutz@verizon.com>
Subject: Re: [PATCH v9 01/13] hvm: Move MAX_INST_LEN into
	x86_emulate.h
Date: Tue, 17 Feb 2015 16:31:35 -0500
Message-ID: <54E3B337.90905@terremark.com>
References: <1424127915-27004-1-git-send-email-dslutz@verizon.com>
	<1424127915-27004-2-git-send-email-dslutz@verizon.com>
	<54E30F7A.7050607@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <54E30F7A.7050607@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>, Don Slutz <dslutz@verizon.com>, xen-devel@lists.xen.org
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Jun Nakajima <jun.nakajima@intel.com>, Eddie Dong <eddie.dong@intel.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, George Dunlap <George.Dunlap@eu.citrix.com>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>, Jan Beulich <jbeulich@suse.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
List-Id: xen-devel@lists.xenproject.org

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