Re: [Xen-devel] [PATCH v2 08/16] SVM: use generic instruction decoding

2016-09-30 Thread Jan Beulich 
>>> On 29.09.16 at 21:24,  wrote:
> On 28/09/16 09:13, Jan Beulich wrote:
>> ... instead of custom handling. To facilitate this break out init code
>> from _hvm_emulate_one() into the new hvm_emulate_init(), and make
>> hvmemul_insn_fetch( globally available.
> 
> )
> 
>>  int __get_instruction_length_from_list(struct vcpu *v,
>>  const enum instruction_index *list, unsigned int list_count)
>>  {
>>  struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>> -unsigned int i, j, inst_len = 0;
>> -enum instruction_index instr = 0;
>> -u8 buf[MAX_INST_LEN];
>> -const u8 *opcode = NULL;
>> -unsigned long fetch_addr, fetch_limit;
>> -unsigned int fetch_len, max_len;
>> +struct hvm_emulate_ctxt ctxt;
>> +struct x86_emulate_state *state;
>> +unsigned int inst_len, j, modrm_rm, modrm_reg;
>> +int modrm_mod;
>>  
> 
> Despite knowing how this works, it is still confusing to read.  Do you
> mind putting in a comment such as:
> 
> /* In a debug build, always use x86_decode_insn() and compare with
> hardware. */

Sure.

>>  for ( j = 0; j < list_count; j++ )
>>  {
>> -instr = list[j];
>> -opcode = opc_bytes[instr];
>> +enum instruction_index instr = list[j];
>>  
>> -for ( i = 0; (i < opcode[0]) && ((inst_len + i) < max_len); i++ )
>> +ASSERT(instr >= 0 && instr < ARRAY_SIZE(opc_tab));
> 
> This is another ASSERT() used as a bounds check, and will suffer a build
> failure on clang.
> 
> You need to use s/enum instruction_index/unsigned int/ to fix the build
> issue.

Oh, right. This predates us having become aware of that clang
issue.

>  Can I also request the use of
> 
> if ( instr >= ARRAY_SIZE(opc_tab) )
> {
> ASSERT_UNREACHABLE();
> return 0;
> }
> 
> instead?

Except that I prefer "break" over "return 0" here.

Jan


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

Re: [Xen-devel] [PATCH v2 08/16] SVM: use generic instruction decoding

2016-09-29 Thread Andrew Cooper 
On 28/09/16 09:13, Jan Beulich wrote:
> ... instead of custom handling. To facilitate this break out init code
> from _hvm_emulate_one() into the new hvm_emulate_init(), and make
> hvmemul_insn_fetch( globally available.

)

>  int __get_instruction_length_from_list(struct vcpu *v,
>  const enum instruction_index *list, unsigned int list_count)
>  {
>  struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> -unsigned int i, j, inst_len = 0;
> -enum instruction_index instr = 0;
> -u8 buf[MAX_INST_LEN];
> -const u8 *opcode = NULL;
> -unsigned long fetch_addr, fetch_limit;
> -unsigned int fetch_len, max_len;
> +struct hvm_emulate_ctxt ctxt;
> +struct x86_emulate_state *state;
> +unsigned int inst_len, j, modrm_rm, modrm_reg;
> +int modrm_mod;
>  

Despite knowing how this works, it is still confusing to read.  Do you
mind putting in a comment such as:

/* In a debug build, always use x86_decode_insn() and compare with
hardware. */

> +#ifdef NDEBUG
>  if ( (inst_len = svm_nextrip_insn_length(v)) != 0 )
>  return inst_len;
>  
>  if ( vmcb->exitcode == VMEXIT_IOIO )
>  return vmcb->exitinfo2 - vmcb->rip;
> +#endif
>  
> -/* Fetch up to the next page break; we'll fetch from the next page
> - * later if we have to. */
> -fetch_addr = svm_rip2pointer(v, _limit);
> -if ( vmcb->rip > fetch_limit )
> -return 0;
> -max_len = min(fetch_limit - vmcb->rip + 1, MAX_INST_LEN + 0UL);
> -fetch_len = min_t(unsigned int, max_len,
> -  PAGE_SIZE - (fetch_addr & ~PAGE_MASK));
> -if ( !fetch(vmcb, buf, fetch_addr, fetch_len) )
> +ASSERT(v == current);
> +hvm_emulate_prepare(, guest_cpu_user_regs());
> +hvm_emulate_init(, NULL, 0);
> +state = x86_decode_insn(, hvmemul_insn_fetch);
> +if ( IS_ERR_OR_NULL(state) )
>  return 0;
>  
> -while ( (inst_len < max_len) && is_prefix(buf[inst_len]) )
> -{
> -inst_len++;
> -if ( inst_len >= fetch_len )
> -{
> -if ( !fetch(vmcb, buf + fetch_len, fetch_addr + fetch_len,
> -max_len - fetch_len) )
> -return 0;
> -fetch_len = max_len;
> -}
> +inst_len = x86_insn_length(state, );
> +modrm_mod = x86_insn_modrm(state, _rm, _reg);
> +x86_emulate_free_state(state);
> +#ifndef NDEBUG
> +if ( vmcb->exitcode == VMEXIT_IOIO )
> +j = vmcb->exitinfo2 - vmcb->rip;
> +else
> +j = svm_nextrip_insn_length(v);
> +if ( j && j != inst_len )
> +{
> +gprintk(XENLOG_WARNING, "insn-len[%02x]=%u (exp %u)\n",
> +ctxt.ctxt.opcode, inst_len, j);
> +return j;
>  }
> +#endif
>  
>  for ( j = 0; j < list_count; j++ )
>  {
> -instr = list[j];
> -opcode = opc_bytes[instr];
> +enum instruction_index instr = list[j];
>  
> -for ( i = 0; (i < opcode[0]) && ((inst_len + i) < max_len); i++ )
> +ASSERT(instr >= 0 && instr < ARRAY_SIZE(opc_tab));

This is another ASSERT() used as a bounds check, and will suffer a build
failure on clang.

You need to use s/enum instruction_index/unsigned int/ to fix the build
issue.  Can I also request the use of

if ( instr >= ARRAY_SIZE(opc_tab) )
{
ASSERT_UNREACHABLE();
return 0;
}

instead?

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -5200,3 +5214,89 @@ x86_emulate(
>  #undef vex
>  #undef override_seg
>  #undef ea
> +
> +#ifdef __XEN__
> +
> +#include 
> +
> +struct x86_emulate_state *
> +x86_decode_insn(
> +struct x86_emulate_ctxt *ctxt,
> +int (*insn_fetch)(
> +enum x86_segment seg, unsigned long offset,
> +void *p_data, unsigned int bytes,
> +struct x86_emulate_ctxt *ctxt))
> +{
> +static DEFINE_PER_CPU(struct x86_emulate_state, state);
> +struct x86_emulate_state *state = _cpu(state);
> +const struct x86_emulate_ops ops = {

This can be static, to avoid having it reconstructed on the stack each
function call.

Otherwise, Reviewed-by: Andrew Cooper 


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

[Xen-devel] [PATCH v2 08/16] SVM: use generic instruction decoding

2016-09-28 Thread Jan Beulich 
... instead of custom handling. To facilitate this break out init code
from _hvm_emulate_one() into the new hvm_emulate_init(), and make
hvmemul_insn_fetch( globally available.

Signed-off-by: Jan Beulich 
---
v2: Add comment to caller field. Rename REG_POISON to PTR_POISON. Align
opc_tab[] initializer lines.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -835,7 +835,7 @@ static int hvmemul_read(
 container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
 }
 
-static int hvmemul_insn_fetch(
+int hvmemul_insn_fetch(
 enum x86_segment seg,
 unsigned long offset,
 void *p_data,
@@ -1765,15 +1765,14 @@ static const struct x86_emulate_ops hvm_
 .vmfunc= hvmemul_vmfunc,
 };
 
-static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
-const struct x86_emulate_ops *ops)
+void hvm_emulate_init(
+struct hvm_emulate_ctxt *hvmemul_ctxt,
+const unsigned char *insn_buf,
+unsigned int insn_bytes)
 {
-struct cpu_user_regs *regs = hvmemul_ctxt->ctxt.regs;
 struct vcpu *curr = current;
-uint32_t new_intr_shadow, pfec = PFEC_page_present;
-struct hvm_vcpu_io *vio = >arch.hvm_vcpu.hvm_io;
+unsigned int pfec = PFEC_page_present;
 unsigned long addr;
-int rc;
 
 if ( hvm_long_mode_enabled(curr) &&
  hvmemul_ctxt->seg_reg[x86_seg_cs].attr.fields.l )
@@ -1791,14 +1790,14 @@ static int _hvm_emulate_one(struct hvm_e
 if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
 pfec |= PFEC_user_mode;
 
-hvmemul_ctxt->insn_buf_eip = regs->eip;
-if ( !vio->mmio_insn_bytes )
+hvmemul_ctxt->insn_buf_eip = hvmemul_ctxt->ctxt.regs->eip;
+if ( !insn_bytes )
 {
 hvmemul_ctxt->insn_buf_bytes =
 hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?:
 (hvm_virtual_to_linear_addr(x86_seg_cs,
 _ctxt->seg_reg[x86_seg_cs],
-regs->eip,
+hvmemul_ctxt->insn_buf_eip,
 sizeof(hvmemul_ctxt->insn_buf),
 hvm_access_insn_fetch,
 hvmemul_ctxt->ctxt.addr_size,
@@ -1810,11 +1809,24 @@ static int _hvm_emulate_one(struct hvm_e
 }
 else
 {
-hvmemul_ctxt->insn_buf_bytes = vio->mmio_insn_bytes;
-memcpy(hvmemul_ctxt->insn_buf, vio->mmio_insn, vio->mmio_insn_bytes);
+hvmemul_ctxt->insn_buf_bytes = insn_bytes;
+memcpy(hvmemul_ctxt->insn_buf, insn_buf, insn_bytes);
 }
 
 hvmemul_ctxt->exn_pending = 0;
+}
+
+static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
+const struct x86_emulate_ops *ops)
+{
+const struct cpu_user_regs *regs = hvmemul_ctxt->ctxt.regs;
+struct vcpu *curr = current;
+uint32_t new_intr_shadow;
+struct hvm_vcpu_io *vio = >arch.hvm_vcpu.hvm_io;
+int rc;
+
+hvm_emulate_init(hvmemul_ctxt, vio->mmio_insn, vio->mmio_insn_bytes);
+
 vio->mmio_retry = 0;
 
 if ( cpu_has_vmx )
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -15,7 +15,7 @@
  * this program; If not, see .
  */
 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -26,41 +26,6 @@
 #include 
 #include 
 
-static unsigned int is_prefix(u8 opc)
-{
-switch ( opc )
-{
-case 0x66:
-case 0x67:
-case 0x2E:
-case 0x3E:
-case 0x26:
-case 0x64:
-case 0x65:
-case 0x36:
-case 0xF0:
-case 0xF3:
-case 0xF2:
-case 0x40 ... 0x4f:
-return 1;
-}
-return 0;
-}
-
-static unsigned long svm_rip2pointer(struct vcpu *v, unsigned long *limit)
-{
-struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-unsigned long p = vmcb->cs.base + vmcb->rip;
-
-if ( !(vmcb->cs.attr.fields.l && hvm_long_mode_enabled(v)) )
-{
-*limit = vmcb->cs.limit;
-return (u32)p; /* mask to 32 bits */
-}
-*limit = ~0UL;
-return p;
-}
-
 static unsigned long svm_nextrip_insn_length(struct vcpu *v)
 {
 struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -89,141 +54,96 @@ static unsigned long svm_nextrip_insn_le
 return vmcb->nextrip - vmcb->rip;
 }
 
-/* First byte: Length. Following bytes: Opcode bytes. */
-#define MAKE_INSTR(nm, ...) static const u8 OPCODE_##nm[] = { __VA_ARGS__ }
-MAKE_INSTR(INVD,   2, 0x0f, 0x08);
-MAKE_INSTR(WBINVD, 2, 0x0f, 0x09);
-MAKE_INSTR(CPUID,  2, 0x0f, 0xa2);
-MAKE_INSTR(RDMSR,  2, 0x0f, 0x32);
-MAKE_INSTR(WRMSR,  2, 0x0f, 0x30);
-MAKE_INSTR(VMCALL, 3, 0x0f, 0x01, 0xd9);
-MAKE_INSTR(HLT,1, 0xf4);
-MAKE_INSTR(INT3,   1, 0xcc);
-MAKE_INSTR(RDTSC,  2, 0x0f, 0x31);
-MAKE_INSTR(PAUSE,  1, 0x90);
-MAKE_INSTR(XSETBV, 3, 0x0f, 0x01, 0xd1);
-MAKE_INSTR(VMRUN,  3, 0x0f, 0x01, 0xd8);
-MAKE_INSTR(VMLOAD, 3, 0x0f, 0x01, 0xda);
-MAKE_INSTR(VMSAVE, 3, 0x0f, 0x01, 0xdb);
-MAKE_INSTR(STGI,   3, 0x0f, 0x01, 0xdc);