Re: [Xen-devel] [PATCH] x86/monitor: Include EAX/ECX in CPUID monitor events

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 09/01/16 02:52, Tamas K Lengyel wrote:
> Extend the CPUID monitor event to include EAX and ECX values that were used
> when CPUID was executed. This is useful in identifying which leaf was queried.
> We also adjust the xen-access output format to more closely resemble the 
> output
> of the Linux cpuid tool's raw format.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> ---
> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  tools/tests/xen-access/xen-access.c | 4 +++-
>  xen/arch/x86/hvm/monitor.c          | 5 ++++-
>  xen/arch/x86/hvm/vmx/vmx.c          | 7 ++++++-
>  xen/include/asm-x86/hvm/monitor.h   | 3 ++-
>  xen/include/public/vm_event.h       | 7 +++++++
>  5 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/tests/xen-access/xen-access.c 
> b/tools/tests/xen-access/xen-access.c
> index ebb63b1..b51b62c 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -735,10 +735,12 @@ int main(int argc, char *argv[])
>                  break;
>              case VM_EVENT_REASON_CPUID:
>                  printf("CPUID executed: rip=%016"PRIx64", vcpu %d. Insn 
> length: %"PRIu32" " \
> -                       "EAX: 0x%"PRIx64" EBX: 0x%"PRIx64" ECX: 0x%"PRIx64" 
> EDX: 0x%"PRIx64"\n",
> +                       "0x%"PRIx32" 0x%"PRIx32": EAX=0x%"PRIx64" 
> EBX=0x%"PRIx64" ECX=0x%"PRIx64" EDX=0x%"PRIx64"\n",
>                         req.data.regs.x86.rip,
>                         req.vcpu_id,
>                         req.u.cpuid.insn_length,
> +                       req.u.cpuid.eax,
> +                       req.u.cpuid.ecx,
>                         req.data.regs.x86.rax,
>                         req.data.regs.x86.rbx,
>                         req.data.regs.x86.rcx,
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index 7277c12..6540e2b 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -136,7 +136,8 @@ int hvm_monitor_debug(unsigned long rip, enum 
> hvm_monitor_debug_type type,
>      return monitor_traps(curr, sync, &req);
>  }
>  
> -int hvm_monitor_cpuid(unsigned long insn_length)
> +int hvm_monitor_cpuid(unsigned long insn_length, unsigned int eax,
> +                      unsigned int ecx)
>  {
>      struct vcpu *curr = current;
>      struct arch_domain *ad = &curr->domain->arch;
> @@ -148,6 +149,8 @@ int hvm_monitor_cpuid(unsigned long insn_length)
>      req.reason = VM_EVENT_REASON_CPUID;
>      req.vcpu_id = curr->vcpu_id;
>      req.u.cpuid.insn_length = insn_length;
> +    req.u.cpuid.eax = eax;
> +    req.u.cpuid.ecx = ecx;
>  
>      return monitor_traps(curr, 1, &req);
>  }
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 3d330b6..6df7612 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2402,12 +2402,17 @@ static void vmx_cpuid_intercept(
>  static int vmx_do_cpuid(struct cpu_user_regs *regs)
>  {
>      unsigned int eax, ebx, ecx, edx;
> +    unsigned int _eax, _ecx;
>  
>      eax = regs->eax;
>      ebx = regs->ebx;
>      ecx = regs->ecx;
>      edx = regs->edx;
>  
> +    /* The CPUID monitor needs both the old and new values of EAX and ECX */
> +    _eax = regs->eax;
> +    _ecx = regs->ecx;
> +
>      vmx_cpuid_intercept(&eax, &ebx, &ecx, &edx);
>  
>      regs->eax = eax;
> @@ -2415,7 +2420,7 @@ static int vmx_do_cpuid(struct cpu_user_regs *regs)
>      regs->ecx = ecx;
>      regs->edx = edx;
>  
> -    return hvm_monitor_cpuid(get_instruction_length());
> +    return hvm_monitor_cpuid(get_instruction_length(), _eax, _ecx);;
>  }
>  
>  static void vmx_dr_access(unsigned long exit_qualification,
> diff --git a/xen/include/asm-x86/hvm/monitor.h 
> b/xen/include/asm-x86/hvm/monitor.h
> index a92f3fc..cdcbeca 100644
> --- a/xen/include/asm-x86/hvm/monitor.h
> +++ b/xen/include/asm-x86/hvm/monitor.h
> @@ -40,7 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long 
> value,
>  void hvm_monitor_msr(unsigned int msr, uint64_t value);
>  int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
>                        unsigned long trap_type, unsigned long insn_length);
> -int hvm_monitor_cpuid(unsigned long insn_length);
> +int hvm_monitor_cpuid(unsigned long insn_length, unsigned int eax,
> +                      unsigned int ecx);
>  
>  #endif /* __ASM_X86_HVM_MONITOR_H__ */
>  
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 64e6857..e0fee23 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -226,6 +226,13 @@ struct vm_event_mov_to_msr {
>  
>  struct vm_event_cpuid {
>      uint32_t insn_length;
> +    /*
> +     * Value of EAX and ECX when CPUID was executed.
> +     * Note that the resulting register values are accessible in
> +     * vm_event_regs_x86.
> +     */
> +    uint32_t eax;
> +    uint32_t ecx;
>      uint32_t _pad;
>  };

Would it not be clearer if you named these old_eax and old_ecx? In user
code it would be hard to choose between these and the values in
vm_event_regs_x86, and then hard to understand why the choice was made
reading the user code later on (unless a comment is added) and so on.

It doesn't have to be old_eax and old_ecx, any other naming that
prevents cofusion would be great.


Thanks,
Razvan

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