[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V3 4/5] xen, libxc: Request page fault injection via libxc
On 07/24/2014 06:27 PM, Jan Beulich wrote: >>>> On 23.07.14 at 14:34, <rcojocaru@xxxxxxxxxxxxxxx> wrote: >> +static void check_pf_injection(void) >> +{ >> + struct vcpu *curr = current; >> + struct domain *d = curr->domain; >> + struct hvm_hw_cpu ctxt; >> + struct segment_register seg; >> + int errcode = PFEC_user_mode; >> + >> + if ( !is_hvm_domain(d) >> + || d->arch.hvm_domain.fault_info.virtual_address == 0 ) >> + return; >> + >> + hvm_funcs.save_cpu_ctxt(curr, &ctxt); > > Isn't this a little heavy handed? It did seem that way to me too, but I couldn't find another way to get to ctxt.pending_event and CR3 at this point in the code. Is there an alternative I'm not aware of? >> + hvm_get_segment_register(curr, x86_seg_ss, &seg); >> + >> + if ( seg.attr.fields.dpl == 3 /* Guest is in user mode */ > > Did you verify that this covers VM86 mode too? Assuming CPL is SS.DPL (I've been previously been using CS.DPL), it did work in our tests, yes. >> + && !ctxt.pending_event >> + && ctxt.cr3 == d->arch.hvm_domain.fault_info.address_space ) >> + { >> + /* Cache */ > > Cache? Did you mean "Latch" or some such perhaps? > >> + uint64_t virtual_address = >> d->arch.hvm_domain.fault_info.virtual_address; >> + uint32_t write_access = d->arch.hvm_domain.fault_info.write_access; I meant "cache", as in "I'm caching d->arch.hvm_domain.fault_info.virtual_address into virtual_address" before resetting it, but the comment is not only unimportant but obviously confusing, so I'll take it out. >> + /* Reset */ >> + d->arch.hvm_domain.fault_info.address_space = 0; >> + d->arch.hvm_domain.fault_info.virtual_address = 0; >> + d->arch.hvm_domain.fault_info.write_access = 0; >> + >> + if ( write_access ) >> + errcode |= PFEC_write_access; >> + >> + hvm_inject_page_fault(errcode, virtual_address); >> + } >> +} > > Even together with its call site it's remaining unclear without knowing > almost all of the rest of your code why this function would inject > only two types of page faults (and I'm still not finally sure this is > intended/correct). A comment would certainly help, short of a > more descriptive name for the function (which I suppose would get > unreasonably long). The function, as previously suggested, will be split into the check part and the injection part. What our code does in this case has been briefly described here: http://lists.xen.org/archives/html/xen-devel/2014-07/msg03013.html >> @@ -1012,6 +1024,7 @@ struct xen_domctl { >> #define XEN_DOMCTL_gdbsx_pausevcpu 1001 >> #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 >> #define XEN_DOMCTL_gdbsx_domstatus 1003 >> +#define XEN_DOMCTL_set_pagefault_info 1004 > > That doesn't look like the range you want to extend. Could you please elaborate? Thank you. Thanks, Razvan Cojocaru _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |