|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V2 2/6] xen: Optimize introspection access to guest state
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@xxxxxxxxxxxxxxx>
> ---
> 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |