[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
>>> On 20.05.19 at 14:55, <aisaila@xxxxxxxxxxxxxxx> wrote: > This patch aims to have mem access vm events sent from the emulator. > This is useful in the case of emulated instructions that cause > page-walks on access protected pages. > > We use hvmemul_map_linear_addr() ro intercept r/w access and > hvmemul_insn_fetch() to intercept exec access. I'm afraid I don't understand this sentence. Or wait - is this a simple typo, and you mean "to" instead of "ro"? > First we try to send a vm event and if the event is sent then emulation > returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the > emulation goes on as expected. Perhaps it's obvious for a vm-event person why successful sending of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not to me, despite having looked at prior versions. Can this (odd at the first glance) behavior please be briefly explained here? > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -15,6 +15,7 @@ > #include <xen/paging.h> > #include <xen/trace.h> > #include <xen/vm_event.h> > +#include <xen/monitor.h> > #include <asm/event.h> > #include <asm/i387.h> > #include <asm/xstate.h> > @@ -26,6 +27,7 @@ > #include <asm/hvm/support.h> > #include <asm/hvm/svm/svm.h> > #include <asm/vm_event.h> > +#include <asm/altp2m.h> In both cases please try to insert at least half way alphabetically (I didn't check if the directives are fully sorted already), rather than blindly at the end. > @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys( > return X86EMUL_OKAY; > } > > +static bool hvmemul_send_vm_event(unsigned long gla, > + uint32_t pfec, unsigned int bytes, > + struct hvm_emulate_ctxt ctxt) > +{ > + xenmem_access_t access; > + vm_event_request_t req = {}; > + gfn_t gfn; > + paddr_t gpa; > + unsigned long reps = 1; > + int rc; > + > + if ( !ctxt.send_event || !pfec ) Why the !pfec part of the condition? > + return false; > + > + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt); As said before - I don't think it's a good idea to do the page walk twice: This and the pre-existing one can easily return different results. Additionally, as also said before (I think), the function may raise #PF, which you don't seem to deal with despite discarding the X86EMUL_EXCEPTION return value ... > + if ( rc != X86EMUL_OKAY ) > + return false; ... here. > + gfn = gaddr_to_gfn(gpa); > + > + if ( p2m_get_mem_access(current->domain, gfn, &access, > + altp2m_vcpu_idx(current)) != 0 ) > + return false; > + > + switch ( access ) { > + case XENMEM_access_x: > + case XENMEM_access_rx: > + if ( pfec & PFEC_write_access ) > + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W; > + break; > + > + case XENMEM_access_w: > + case XENMEM_access_rw: > + if ( pfec & PFEC_insn_fetch ) > + req.u.mem_access.flags = MEM_ACCESS_X; > + break; > + > + case XENMEM_access_r: > + case XENMEM_access_n: > + if ( pfec & PFEC_write_access ) > + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W; > + if ( pfec & PFEC_insn_fetch ) > + req.u.mem_access.flags |= MEM_ACCESS_X; > + break; > + > + default: > + return false; > + } Aren't you looking at the leaf page here, rather than at any of the involved page tables? Or am I misunderstanding the description saying "page-walks on access protected pages"? > @@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr( > unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) - > (linear >> PAGE_SHIFT) + 1; > unsigned int i; > + gfn_t gfn; > > /* > * mfn points to the next free slot. All used slots have a page > reference > @@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr( > ASSERT(mfn_x(*mfn) == 0); > > res = hvm_translate_get_page(curr, addr, true, pfec, > - &pfinfo, &page, NULL, &p2mt); > + &pfinfo, &page, &gfn, &p2mt); > > switch ( res ) > { Are these two hunks leftovers? You don't use "gfn" anywhere. > @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch( > container_of(ctxt, struct hvm_emulate_ctxt, ctxt); > /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */ > uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip; > + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch; > + unsigned long addr, reps = 1; > + int rc = 0; > + > + rc = hvmemul_virtual_to_linear( > + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, > &addr); > + > + if ( rc != X86EMUL_OKAY || !bytes ) > + return rc; > + > + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 ) > + pfec |= PFEC_user_mode; > > + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) ) > + return X86EMUL_ACCESS_EXCEPTION; > /* > * Fall back if requested bytes are not in the prefetch cache. > * But always perform the (fake) read when bytes == 0. Despite what was said before you're still doing things a 2nd time here just because of hvmemul_send_vm_event()'s needs, even if that function ends up bailing right away. Also please don't lose the blank line ahead of the comment you add code ahead of. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux { > #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED > /* (cmpxchg accessor): CMPXCHG failed. */ > #define X86EMUL_CMPXCHG_FAILED 7 > +/* Emulator tried to access a protected page. */ > +#define X86EMUL_ACCESS_EXCEPTION 8 This still doesn't make clear what the difference is to X86EMUL_EXCEPTION. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |