Re: [Xen-devel] [PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.18 at 18:07, <andrew.cooper3@xxxxxxxxxx> wrote:
> Also, I'm not entirely convinced that making modrm an annonymous union is
> going to work with older CentOS compilers,

It certainly won't.

> and therefore am not sure whether
> that part of the change is worth it.  The instruction in question can be
> obtained from the printed INSN_ constant alone.
> ---
>  xen/arch/x86/hvm/svm/emulate.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index 3d04af0..71a1b6e 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -56,11 +56,14 @@ static unsigned long svm_nextrip_insn_length(struct vcpu 
> *v)
>  
>  static const struct {
>      unsigned int opcode;
> -    struct {
> -        unsigned int rm:3;
> -        unsigned int reg:3;
> -        unsigned int mod:2;
> -#define MODRM(mod, reg, rm) { rm, reg, mod }
> +    union {
> +        struct {
> +            unsigned int rm:3;
> +            unsigned int reg:3;
> +            unsigned int mod:2;
> +        };
> +        unsigned int raw;

Why unsigned int instead of uint8_t?

> @@ -152,8 +155,17 @@ int __get_instruction_length_from_list(struct vcpu *v,
>      }
>  
>      gdprintk(XENLOG_WARNING,
> -             "%s: Mismatch between expected and actual instruction: "
> -             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
> +             "%s: Mismatch between expected and actual instruction:\n",
> +             __func__);
> +    gdprintk(XENLOG_WARNING,
> +             "  list[0] val %d, { opc %#x, modrm %#x }, list entries: %u\n",
> +             list[0], opc_tab[list[0]].opcode, opc_tab[list[0]].modrm.raw,
> +             list_count);
> +    gdprintk(XENLOG_WARNING, "  rip 0x%lx, nextrip 0x%lx, len %lu\n",
> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
> +    hvm_dump_emulation_state(XENLOG_G_WARNING, "Insn_len",
> +                             &ctxt, X86EMUL_UNHANDLEABLE);
> +
>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
>      return 0;
>  }

The gdprintk()s all expanding to nothing in release builds I'm
not fully convinced the added verbosity is worth it. In debug
builds adding some debugging code like this shouldn't be a
big hurdle.

In any event %#lx instead of 0x%lx please.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel