Re: [Xen-devel] [PATCH for 4.5 v3 2/2] x86/hvm: Improve "Emulation failed @" error messages

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Monday, September 29, 2014 2:46 AM
> 
> * Introduce hvm_dump_emulation_state() to be a common implementation
> rather
>   than having the printk() open-coded slightly differently in 3 separate
>   places.
> * Identify the vcpu operating mode to allow for unambiguous decoding of the
>   instruction bytes.
> * A valid instruction can be up to 15 bytes long, but may also be shorter than
>   the current arbitrary 10 bytes.  Print only the fetched bytes, which could
>   include nothing if the emulation failed due to an inability to fetch the
>   instruction.
> 
> A sample new error message looks like:
> 
> (d1) MMIO emulation failed: d1v0 64bit @ 0008:ffff82d0802c4f20 -> 48 8b b8
> e8 7f 00 00
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Tim Deegan <tim@xxxxxxx>
> Release-acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Eddie Dong <eddie.dong@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>

Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> ---
> v3:
>  * Include real/v86 distinction to help identify the meaning of %cs
>  * Fix parameter mistake between the declaration and definition of
>    hvm_dump_emulation_state()
> v2: (Suggestions from Jan and Tim)
>  * Reduce content of message for clarity.
>  * Only identify 16/32/64 bit operating mode.  ???_guest_x86_mode() is
>    currently insufficiently expressive to cover all operating modes.
> 
> Ideally, hvm_dump_emulation_state() would have a const context pointer, but
> hvmemul_get_seg_reg() currently needs to set an accessed bit.  We should
> probably differencitate between get_seg_reg() for the purposes of emulating,
> and get_seg_reg() for the purpose of debug printing.
> ---
>  xen/arch/x86/hvm/emulate.c        |   36
> +++++++++++++++++++++++++++---------
>  xen/arch/x86/hvm/io.c             |   11 +----------
>  xen/arch/x86/hvm/vmx/realmode.c   |    9 +--------
>  xen/include/asm-x86/hvm/emulate.h |    3 +++
>  4 files changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 463ccfb..c0f47d2 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1420,15 +1420,7 @@ void hvm_mem_event_emulate_one(bool_t
> nowrite, unsigned int trapnr,
>           */
>          return;
>      case X86EMUL_UNHANDLEABLE:
> -        gdprintk(XENLOG_DEBUG, "Emulation failed @ %04x:%lx: "
> -
> "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
> -               hvmemul_get_seg_reg(x86_seg_cs, &ctx)->sel,
> -               ctx.insn_buf_eip,
> -               ctx.insn_buf[0], ctx.insn_buf[1],
> -               ctx.insn_buf[2], ctx.insn_buf[3],
> -               ctx.insn_buf[4], ctx.insn_buf[5],
> -               ctx.insn_buf[6], ctx.insn_buf[7],
> -               ctx.insn_buf[8], ctx.insn_buf[9]);
> +        hvm_dump_emulation_state(XENLOG_G_DEBUG "Mem event",
> &ctx);
>          hvm_inject_hw_exception(trapnr, errcode);
>          break;
>      case X86EMUL_EXCEPTION:
> @@ -1479,6 +1471,32 @@ struct segment_register *hvmemul_get_seg_reg(
>      return &hvmemul_ctxt->seg_reg[seg];
>  }
> 
> +static const char *guest_x86_mode_to_str(int mode)
> +{
> +    switch ( mode )
> +    {
> +    case 0:  return "Real";
> +    case 1:  return "v86";
> +    case 2:  return "16bit";
> +    case 4:  return "32bit";
> +    case 8:  return "64bit";
> +    default: return "Unknown";
> +    }
> +}
> +
> +void hvm_dump_emulation_state(const char *prefix,
> +                              struct hvm_emulate_ctxt
> *hvmemul_ctxt)
> +{
> +    struct vcpu *curr = current;
> +    const char *mode_str =
> guest_x86_mode_to_str(hvm_guest_x86_mode(curr));
> +    const struct segment_register *cs =
> +        hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
> +
> +    printk("%s emulation failed: %pv %s @ %04x:%08lx -> %*ph\n",
> +           prefix, curr, mode_str, cs->sel, hvmemul_ctxt->insn_buf_eip,
> +           hvmemul_ctxt->insn_buf_bytes, hvmemul_ctxt->insn_buf);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index e5d5e79..68fb890 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -100,16 +100,7 @@ int handle_mmio(void)
>      switch ( rc )
>      {
>      case X86EMUL_UNHANDLEABLE:
> -        gdprintk(XENLOG_WARNING,
> -                 "MMIO emulation failed @ %04x:%lx: "
> -
> "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
> -                 hvmemul_get_seg_reg(x86_seg_cs, &ctxt)->sel,
> -                 ctxt.insn_buf_eip,
> -                 ctxt.insn_buf[0], ctxt.insn_buf[1],
> -                 ctxt.insn_buf[2], ctxt.insn_buf[3],
> -                 ctxt.insn_buf[4], ctxt.insn_buf[5],
> -                 ctxt.insn_buf[6], ctxt.insn_buf[7],
> -                 ctxt.insn_buf[8], ctxt.insn_buf[9]);
> +        hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO",
> &ctxt);
>          return 0;
>      case X86EMUL_EXCEPTION:
>          if ( ctxt.exn_pending )
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c
> b/xen/arch/x86/hvm/vmx/realmode.c
> index 9a6de6c..fe8b4a0 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -157,14 +157,7 @@ static void realmode_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt)
>      return;
> 
>   fail:
> -    gdprintk(XENLOG_ERR,
> -             "Real-mode emulation failed @ %04x:%08lx: "
> -             "%02x %02x %02x %02x %02x %02x\n",
> -             hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt)->sel,
> -             hvmemul_ctxt->insn_buf_eip,
> -             hvmemul_ctxt->insn_buf[0], hvmemul_ctxt->insn_buf[1],
> -             hvmemul_ctxt->insn_buf[2], hvmemul_ctxt->insn_buf[3],
> -             hvmemul_ctxt->insn_buf[4], hvmemul_ctxt->insn_buf[5]);
> +    hvm_dump_emulation_state(XENLOG_G_ERR "Real-mode",
> hvmemul_ctxt);
>      domain_crash(curr->domain);
>  }
> 
> diff --git a/xen/include/asm-x86/hvm/emulate.h
> b/xen/include/asm-x86/hvm/emulate.h
> index 6cdc57b..5411302 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -54,4 +54,7 @@ int hvmemul_do_pio(
>      unsigned long port, unsigned long *reps, int size,
>      paddr_t ram_gpa, int dir, int df, void *p_data);
> 
> +void hvm_dump_emulation_state(const char *prefix,
> +                              struct hvm_emulate_ctxt
> *hvmemul_ctxt);
> +
>  #endif /* __ASM_X86_HVM_EMULATE_H__ */
> --
> 1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel