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

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

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.

> ---
>  xen/arch/x86/hvm/svm/emulate.c         | 2 --
>  xen/arch/x86/hvm/svm/svm.c             | 4 ++--
>  xen/arch/x86/hvm/vmx/vmx.c             | 2 +-
>  xen/arch/x86/x86_emulate/x86_emulate.c | 4 ++--
>  xen/arch/x86/x86_emulate/x86_emulate.h | 2 ++
>  5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index 37a1ece..6f5c8d3 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -27,8 +27,6 @@
>  #include <asm/hvm/svm/vmcb.h>
>  #include <asm/hvm/svm/emulate.h>
>  
> -#define MAX_INST_LEN 15
> -
>  static unsigned int is_prefix(u8 opc)
>  {
>      switch ( opc )
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index a7655bd..4b7b818 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -106,7 +106,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
>      if ( unlikely(inst_len == 0) )
>          return;
>  
> -    if ( unlikely(inst_len > 15) )
> +    if ( unlikely(inst_len > MAX_INST_LEN) )
>      {
>          gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
>          svm_crash_or_fault(curr);
> @@ -859,7 +859,7 @@ static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
>      if ( len != 0 )
>      {
>          /* Latch and clear the cached instruction. */
> -        memcpy(buf, vmcb->guest_ins, 15);
> +        memcpy(buf, vmcb->guest_ins, MAX_INST_LEN);
>          v->arch.hvm_svm.cached_insn_len = 0;
>      }
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 357ef6c..e1c55ce 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1854,7 +1854,7 @@ static int get_instruction_length(void)
>      unsigned long len;
>  
>      __vmread(VM_EXIT_INSTRUCTION_LEN, &len); /* Safe: callers audited */
> -    BUG_ON((len < 1) || (len > 15));
> +    BUG_ON((len < 1) || (len > MAX_INST_LEN));
>      return len;
>  }
>  
> 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.

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

>     rc = ops->insn_fetch(x86_seg_cs, _eip, &_x, (_size), ctxt);          \
>     if ( rc ) goto done;                                                 \
>     _x;                                                                  \
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
> index bdce861..593b31e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -24,6 +24,8 @@
>  #ifndef __X86_EMULATE_H__
>  #define __X86_EMULATE_H__
>  
> +#define MAX_INST_LEN 15
> +
>  struct x86_emulate_ctxt;
>  
>  /* Comprehensive enumeration of x86 segment registers. */