Re: [Xen-devel] [PATCH] mem_event: Allow emulating an instruction that caused a page fault

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 16:32 +0200 on 14 Jan (1358181126), Razvan Cojocaru wrote:
> This patch makes it possible to emulate an instruction that triggered
> a page fault (received via the mem_event API). This is done by setting
> the MEM_EVENT_FLAG_EMULATE in mem_event_response_t.flags. The purpose
> of this is to be able to receive several distinct page fault mem_events
> for the same address, and choose which ones are allowed to go through
> from dom0 userspace.

I think there ought to be some other control to this: what if a single
instruction accesses multiple pages, each of which would cause an access
fault?  You only get a notification of the first one, so short of
emulating the instruction yourself in userspace I don't know how you can
decide that it's safe.

I've a few comments on implementation below:

> diff -r 35a0556a7f76 -r a22fe4e2bc32 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c   Thu Jan 10 17:32:10 2013 +0000
> +++ b/xen/arch/x86/mm/p2m.c   Mon Jan 14 16:31:29 2013 +0200
> @@ -1205,7 +1205,7 @@ void p2m_mem_paging_resume(struct domain
>  
>  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)
> +                          mem_event_request_t **req_ptr, struct 
> cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
>      unsigned long gfn = gpa >> PAGE_SHIFT;
> @@ -1258,6 +1258,17 @@ bool_t p2m_mem_access_check(paddr_t gpa,
>          }
>      }
>  
> +    if ( v->arch.hvm_vmx.mem_event_emulate )
> +    {
> +        struct hvm_emulate_ctxt ctx[1] = {};
> +
> +        v->arch.hvm_vmx.mem_event_emulate = 0;
> +        hvm_emulate_prepare(ctx, regs);

This function always operates on the currently scheduled vcpu, so you
don't need to pass a cpu-user-regs struct all the way down the stack --
you can just use guest_cpu_user_regs() here.

> +        hvm_emulate_one(ctx);
> +
> +        return 1;
> +    }
> +
>      *req_ptr = NULL;
>      req = xzalloc(mem_event_request_t);
>      if ( req )
5A> diff -r 35a0556a7f76 -r a22fe4e2bc32 xen/arch/x86/x86_64/asm-offsets.c
> --- a/xen/arch/x86/x86_64/asm-offsets.c       Thu Jan 10 17:32:10 2013 +0000
> +++ b/xen/arch/x86/x86_64/asm-offsets.c       Mon Jan 14 16:31:29 2013 +0200
> @@ -113,6 +113,7 @@ void __dummy__(void)
>      OFFSET(VCPU_vmx_emulate, struct vcpu, arch.hvm_vmx.vmx_emulate);
>      OFFSET(VCPU_vm86_seg_mask, struct vcpu, arch.hvm_vmx.vm86_segment_mask);
>      OFFSET(VCPU_hvm_guest_cr2, struct vcpu, arch.hvm_vcpu.guest_cr[2]);
> +    OFFSET(VCPU_mem_event_emulate, struct vcpu, 
> arch.hvm_vmx.mem_event_emulate);

I don't think this is necessary: you only need to add a field to thuiis
file if you'll be using it from assembly code. 

>      BLANK();
>  
>      OFFSET(VCPU_nhvm_guestmode, struct vcpu, 
> arch.hvm_vcpu.nvcpu.nv_guestmode);
> diff -r 35a0556a7f76 -r a22fe4e2bc32 xen/include/public/mem_event.h
> --- a/xen/include/public/mem_event.h  Thu Jan 10 17:32:10 2013 +0000
> +++ b/xen/include/public/mem_event.h  Mon Jan 14 16:31:29 2013 +0200
> @@ -36,6 +36,7 @@
>  #define MEM_EVENT_FLAG_EVICT_FAIL   (1 << 2)
>  #define MEM_EVENT_FLAG_FOREIGN      (1 << 3)
>  #define MEM_EVENT_FLAG_DUMMY        (1 << 4)
> +#define MEM_EVENT_FLAG_EMULATE      (1 << 5)

Please add a comment saying what this flag does.  I know the rest of
this code is poorly commented, but let's try to make thing better as we
go. :)

Tim.

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