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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/12/2018 10:32, Jan Beulich wrote:
>>>> 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?

Because to being with, this was a diagnostic patch and copied the type
above without thinking too much.

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

You and I know what diagnostics to put here, but I think Pauls reaction
to finding that message demonstrates that most others don't.  Nor should
they - the peculiarities of first-gen AMD hardware needn't be mandatory
knowledge for most contributors.

For these diagnostics, they are only reachable in debug builds, and this
codepath is only even reachable in release builds on first-gen hardware.

However, the difference between what is currently present and this is
enough information to actually diagnose the problem.

~Andrew

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