Re: [PATCH v11 9/9] Add xentrace to vmware_port

[Don Slutz <don.slutz@gmail.com>]

On 06/04/15 07:20, George Dunlap wrote:
> On 05/22/2015 04:50 PM, Don Slutz wrote:
>> Also added missing TRAP_DEBUG & VLAPIC.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> ---
>> v11:
>>   No change
>>
>> v10:
>>   Added Acked-by: Ian Campbell
>>   Added back in the trace point calls.
>>
>>     Why is cmd in this patch?
>>       Because the trace points use it.
>>
>> v9:
>>   Dropped unneed VMPORT_UNHANDLED, VMPORT_DECODE.
>>
>> v7:
>>       Dropped some of the new traces.
>>       Added HVMTRACE_ND7.
>>
>> v6:
>>       Dropped the attempt to use svm_nextrip_insn_length via
>>       __get_instruction_length (added in v2).  Just always look
>>       at upto 15 bytes on AMD.
>>
>> v5:
>>       exitinfo1 is used twice.
>>         Fixed.
>>
>>  tools/xentrace/formats           |  5 +++++
>>  xen/arch/x86/hvm/io.c            |  3 +++
>>  xen/arch/x86/hvm/vmware/vmport.c | 17 ++++++++++++++---
>>  xen/include/asm-x86/hvm/trace.h  | 22 ++++++++++++++++++++++
>>  xen/include/public/trace.h       |  3 +++
>>  5 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/xentrace/formats b/tools/xentrace/formats
>> index 5d7b72a..eec65f4 100644
>> --- a/tools/xentrace/formats
>> +++ b/tools/xentrace/formats
>> @@ -79,6 +79,11 @@
>>  0x00082020  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  INTR_WINDOW [ value = 0x%(1)08x ]
>>  0x00082021  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  NPF         [ gpa = 0x%(2)08x%(1)08x mfn = 0x%(4)08x%(3)08x qual = 0x%(5)04x p2mt = 0x%(6)04x ]
>>  0x00082023  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  TRAP        [ vector = 0x%(1)02x ]
>> +0x00082024  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  TRAP_DEBUG  [ exit_qualification = 0x%(1)08x ]
>> +0x00082025  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VLAPIC
>> +0x00082026  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_HANDLED   [ cmd = %(1)d eax = 0x%(2)08x ebx = 0x%(3)08x ecx = 0x%(4)08x edx = 0x%(5)08x esi = 0x%(6)08x edi = 0x%(7)08x ]
>> +0x00082027  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_IGNORED   [ port = %(1)d eax = 0x%(2)08x ebx = 0x%(3)08x ecx = 0x%(4)08x edx = 0x%(5)08x esi = 0x%(6)08x edi = 0x%(7)08x ]
>> +0x00082028  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_QEMU      [ eax = 0x%(1)08x ebx = 0x%(2)08x ecx = 0x%(3)08x edx = 0x%(4)08x esi = 0x%(5)08x edi = 0x%(6)08x ]
>>  
>>  0x0010f001  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_map      [ domid = %(1)d ]
>>  0x0010f002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_unmap    [ domid = %(1)d ]
>> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
>> index 7684cf0..6a9cfb0 100644
>> --- a/xen/arch/x86/hvm/io.c
>> +++ b/xen/arch/x86/hvm/io.c
>> @@ -206,6 +206,9 @@ void hvm_io_assist(ioreq_t *p)
>>                  regs->_edx = vr->edx;
>>                  regs->_esi = vr->esi;
>>                  regs->_edi = vr->edi;
>> +                HVMTRACE_ND(VMPORT_QEMU, 0, 1/*cycles*/, 6,
>> +                            p->data, regs->_ebx, regs->_ecx,
>> +                            regs->_edx, regs->_esi, regs->_edi);
>>              }
>>          }
>>          if ( vio->io_size == 4 ) /* Needs zero extension. */
>> diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
>> index 36e3f1b..3c3ccd4 100644
>> --- a/xen/arch/x86/hvm/vmware/vmport.c
>> +++ b/xen/arch/x86/hvm/vmware/vmport.c
>> @@ -16,6 +16,7 @@
>>  #include <xen/lib.h>
>>  #include <asm/hvm/hvm.h>
>>  #include <asm/hvm/support.h>
>> +#include <asm/hvm/trace.h>
>>  
>>  #include "backdoor_def.h"
>>  
>> @@ -35,6 +36,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>>      if ( port == BDOOR_PORT && regs->_eax == BDOOR_MAGIC )
>>      {
>>          uint32_t new_eax = ~0u;
>> +        uint16_t cmd = regs->_ecx;
>>          uint64_t value;
>>          struct vcpu *curr = current;
>>          struct domain *currd = curr->domain;
>> @@ -45,7 +47,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>>           * leaving the high 32-bits unchanged, unlike what one would
>>           * expect to happen.
>>           */
>> -        switch ( regs->_ecx & 0xffff )
>> +        switch ( cmd )
>>          {
>>          case BDOOR_CMD_GETMHZ:
>>              new_eax = currd->arch.tsc_khz / 1000;
>> @@ -123,11 +125,20 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>>              /* Let backing DM handle */
>>              return X86EMUL_UNHANDLEABLE;
>>          }
>> +        HVMTRACE_ND7(VMPORT_HANDLED, 0, 0/*cycles*/, 7,
>> +                     cmd, new_eax, regs->_ebx, regs->_ecx,
>> +                     regs->_edx, regs->_esi, regs->_edi);
> 
> Do you need to log edi as well? It looks like it's not used.


I guess not, but since there are VMware port commands that do use edi, a
future add might need it.  I find it simpler to have this and the QEMU
case above the same but will change it if you want.

> 
>>          if ( dir == IOREQ_READ )
>>              *val = new_eax;
>>      }
>> -    else if ( dir == IOREQ_READ )
>> -        *val = ~0u;
>> +    else
>> +    {
>> +        HVMTRACE_ND7(VMPORT_IGNORED, 0, 0/*cycles*/, 7,
>> +                     port, regs->_eax, regs->_ebx, regs->_ecx,
>> +                     regs->_edx, regs->_esi, regs->_edi);
> 
> And do you need to log all the registers here?  It seems like port +
> regs->_ecx would be enough to tell you why it got ignored.
> 

The min would be port and regs->_eax.  QEMU is the one that cares about
regs->_ecx.

Happy to make this change. Will wait until response from above.

>> +        if ( dir == IOREQ_READ )
>> +            *val = ~0u;
>> +    }
>>  
>>      return X86EMUL_OKAY;
>>  }
>> diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
>> index de802a6..0ad805f 100644
>> --- a/xen/include/asm-x86/hvm/trace.h
>> +++ b/xen/include/asm-x86/hvm/trace.h
>> @@ -54,6 +54,9 @@
>>  #define DO_TRC_HVM_TRAP             DEFAULT_HVM_MISC
>>  #define DO_TRC_HVM_TRAP_DEBUG       DEFAULT_HVM_MISC
>>  #define DO_TRC_HVM_VLAPIC           DEFAULT_HVM_MISC
>> +#define DO_TRC_HVM_VMPORT_HANDLED   DEFAULT_HVM_IO
>> +#define DO_TRC_HVM_VMPORT_IGNORED   DEFAULT_HVM_IO
>> +#define DO_TRC_HVM_VMPORT_QEMU      DEFAULT_HVM_IO
>>  
>>  
>>  #define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
>> @@ -83,6 +86,25 @@
>>          }                                                                 \
>>      } while(0)
>>  
>> +#define HVMTRACE_ND7(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6, d7) \
>> +    do {                                                                  \
>> +        if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt )               \
>> +        {                                                                 \
>> +            struct {                                                      \
>> +                u32 d[7];                                                 \
>> +            } _d;                                                         \
>> +            _d.d[0]=(d1);                                                 \
>> +            _d.d[1]=(d2);                                                 \
>> +            _d.d[2]=(d3);                                                 \
>> +            _d.d[3]=(d4);                                                 \
>> +            _d.d[4]=(d5);                                                 \
>> +            _d.d[5]=(d6);                                                 \
>> +            _d.d[6]=(d7);                                                 \
>> +            __trace_var(TRC_HVM_ ## evt | (modifier), cycles,             \
>> +                        sizeof(*_d.d) * count, &_d);                      \
>> +        }                                                                 \
>> +    } while(0)
> 
> If you reduced the registers as mentioned above, you wouldn't need this
> here either.
> 

Yes.  However with out this, I did not understand that a trace could
have 7 32bit arguments.  Happy to go either way depending on changes above.

   -Don Slutz

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