Re: [PATCH RFC V2 2/6] xen: Optimize introspection access to guest state

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/07/14 16:43, Razvan Cojocaru wrote:
> Speed optimization for introspection purposes: a handful of registers
> are sent along with each mem_event. This requires enlargement of the
> mem_event_request / mem_event_response stuctures, and additional code
> to fill in relevant values. Since the EPT event processing code needs
> more data than CR3 or MSR event processors, hvm_mem_event_fill_regs()
> fills in less data than p2m_mem_event_fill_regs(), in order to avoid
> overhead. Struct hvm_hw_cpu has been considered instead of the custom
> struct mem_event_regs_st, but its size would cause quick filling up
> of the mem_event ring buffer.
>
> Changes since V1:
>  - Replaced guest_x86_mode with cs_arbytes in the mem_event_regs_st
>    structure.
>  - Removed superfluous preprocessor check for __x86_64__ in
>    p2m_mem_event_fill_regs().
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/x86/hvm/hvm.c         |   33 ++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c          |   60 ++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/mem_event.h |   38 +++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef2411c..89a0382 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6085,6 +6085,38 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>      return rc;
>  }
>  
> +static inline void hvm_mem_event_fill_regs(mem_event_request_t *req)

For C functions like this, the compiler is far better at guessing the
inlineabilty of the function than the programmer.  You don't need to
call it out explicitly.

> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    struct vcpu *v = current;
> +
> +    req->regs.rax = regs->eax;
> +    req->regs.rcx = regs->ecx;
> +    req->regs.rdx = regs->edx;
> +    req->regs.rbx = regs->ebx;
> +    req->regs.rsp = regs->esp;
> +    req->regs.rbp = regs->ebp;
> +    req->regs.rsi = regs->esi;
> +    req->regs.rdi = regs->edi;
> +
> +    req->regs.r8  = regs->r8;
> +    req->regs.r9  = regs->r9;
> +    req->regs.r10 = regs->r10;
> +    req->regs.r11 = regs->r11;
> +    req->regs.r12 = regs->r12;
> +    req->regs.r13 = regs->r13;
> +    req->regs.r14 = regs->r14;
> +    req->regs.r15 = regs->r15;
> +
> +    req->regs.rflags = regs->eflags;
> +    req->regs.rip    = regs->eip;
> +
> +    req->regs.msr_efer = v->arch.hvm_vcpu.guest_efer;
> +    req->regs.cr0 = v->arch.hvm_vcpu.guest_cr[0];
> +    req->regs.cr3 = v->arch.hvm_vcpu.guest_cr[3];
> +    req->regs.cr4 = v->arch.hvm_vcpu.guest_cr[4];
> +}
> +
>  static int hvm_memory_event_traps(long p, uint32_t reason,
>                                    unsigned long value, unsigned long old, 
>                                    bool_t gla_valid, unsigned long gla) 
> @@ -6129,6 +6161,7 @@ static int hvm_memory_event_traps(long p, uint32_t reason,
>          req.gla = old;
>      }
>      
> +    hvm_mem_event_fill_regs(&req);
>      mem_event_put_request(d, &d->mem_event->access, &req);
>      
>      return 1;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 642ec28..13fdf78 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1314,6 +1314,63 @@ void p2m_mem_paging_resume(struct domain *d)
>      }
>  }
>  
> +static inline void p2m_mem_event_fill_regs(mem_event_request_t *req)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    struct segment_register seg;
> +    struct hvm_hw_cpu ctxt;
> +    struct vcpu *v = current;
> +
> +    memset(&ctxt, 0, sizeof(struct hvm_hw_cpu));

You don't need to zero ctxt if you are about to save into it.

> +
> +    /* Architecture-specific vmcs/vmcb bits */
> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
> +
> +    req->regs.rax = regs->eax;
> +    req->regs.rcx = regs->ecx;
> +    req->regs.rdx = regs->edx;
> +    req->regs.rbx = regs->ebx;
> +    req->regs.rsp = regs->esp;
> +    req->regs.rbp = regs->ebp;
> +    req->regs.rsi = regs->esi;
> +    req->regs.rdi = regs->edi;
> +
> +    req->regs.r8  = regs->r8;
> +    req->regs.r9  = regs->r9;
> +    req->regs.r10 = regs->r10;
> +    req->regs.r11 = regs->r11;
> +    req->regs.r12 = regs->r12;
> +    req->regs.r13 = regs->r13;
> +    req->regs.r14 = regs->r14;
> +    req->regs.r15 = regs->r15;
> +
> +    req->regs.rflags = regs->eflags;
> +    req->regs.rip    = regs->eip;
> +
> +    req->regs.dr7 = v->arch.debugreg[7];
> +    req->regs.cr0 = ctxt.cr0;
> +    req->regs.cr2 = ctxt.cr2;
> +    req->regs.cr3 = ctxt.cr3;
> +    req->regs.cr4 = ctxt.cr4;
> +
> +    req->regs.sysenter_cs = ctxt.sysenter_cs;
> +    req->regs.sysenter_esp = ctxt.sysenter_esp;
> +    req->regs.sysenter_eip = ctxt.sysenter_eip;
> +
> +    req->regs.msr_efer = ctxt.msr_efer;
> +    req->regs.msr_star = ctxt.msr_star;
> +    req->regs.msr_lstar = ctxt.msr_lstar;
> +
> +    hvm_get_segment_register(v, x86_seg_fs, &seg);
> +    req->regs.fs_base = seg.base;
> +
> +    hvm_get_segment_register(v, x86_seg_gs, &seg);
> +    req->regs.gs_base = seg.base;
> +
> +    hvm_get_segment_register(v, x86_seg_cs, &seg);
> +    req->regs.cs_arbytes = seg.attr.bytes;;

Stray double semicolon.

> +}
> +
>  bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla, 
>                            bool_t access_r, bool_t access_w, bool_t access_x,
>                            mem_event_request_t **req_ptr)
> @@ -1407,6 +1464,9 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,
>      if ( p2ma != p2m_access_n2rwx )
>          vcpu_pause_nosync(v);
>  
> +    if ( req )
> +        p2m_mem_event_fill_regs(req);

Should this not be part of the if ( req ) just above the pause_nosync() ?

> +
>      /* VCPU may be paused, return whether we promoted automatically */
>      return (p2ma == p2m_access_n2rwx);
>  }
> diff --git a/xen/include/public/mem_event.h b/xen/include/public/mem_event.h
> index 3831b41..b9af728 100644
> --- a/xen/include/public/mem_event.h
> +++ b/xen/include/public/mem_event.h
> @@ -48,6 +48,43 @@
>  #define MEM_EVENT_REASON_MSR         7    /* MSR was hit: gfn is MSR value, gla is MSR address;
>                                               does NOT honour HVMPME_onchangeonly */
>  
> +/* Using a custom struct (not hvm_hw_cpu) so as to not fill
> + * the mem_event ring buffer too quickly. */
> +typedef struct mem_event_regs_st {
> +    uint64_t rax;
> +    uint64_t rcx;
> +    uint64_t rdx;
> +    uint64_t rbx;
> +    uint64_t rsp;
> +    uint64_t rbp;
> +    uint64_t rsi;
> +    uint64_t rdi;
> +    uint64_t r8;
> +    uint64_t r9;
> +    uint64_t r10;
> +    uint64_t r11;
> +    uint64_t r12;
> +    uint64_t r13;
> +    uint64_t r14;
> +    uint64_t r15;
> +    uint64_t rflags;
> +    uint64_t dr7;
> +    uint64_t rip;
> +    uint64_t cr0;
> +    uint64_t cr2;
> +    uint64_t cr3;
> +    uint64_t cr4;
> +    uint64_t sysenter_cs;
> +    uint64_t sysenter_esp;
> +    uint64_t sysenter_eip;
> +    uint64_t msr_efer;
> +    uint64_t msr_star;
> +    uint64_t msr_lstar;
> +    uint64_t fs_base;
> +    uint64_t gs_base;
> +    uint32_t cs_arbytes;

This trailing uint32_t means that sizeof(mem_event_regs_t) is different
between 32 and 64 bit builds, breaking compatibility between a 64bit xen
and 32bit dom0.

The easiest fix is to add an explicit uint32_t _pad field at the end.

~Andrew

> +} mem_event_regs_t;
> +
>  typedef struct mem_event_st {
>      uint32_t flags;
>      uint32_t vcpu_id;
> @@ -65,6 +102,7 @@ typedef struct mem_event_st {
>      uint16_t available:12;
>  
>      uint16_t reason;
> +    mem_event_regs_t regs;
>  } mem_event_request_t, mem_event_response_t;
>  
>  DEFINE_RING_TYPES(mem_event, mem_event_request_t, mem_event_response_t);