[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: Friday, July 04, 2014 6:44 PM > To: Xu, Dongxiao > Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx; > George.Dunlap@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx; > stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; > konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx > Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access > hypercall > > >>> On 04.07.14 at 10:34, <dongxiao.xu@xxxxxxxxx> wrote: > > Add a generic resource access hypercall for tool stack or other > > components, e.g., accessing MSR, port I/O, etc. > > Sigh - you're still allocating an unbounded amount of memory for > passing around the input arguments, despite it being possible (and > having been suggested) to read these from the original buffer on > each iteration. You're still not properly checking for preemption > between iterations. And you're still not making use of > continue_hypercall_on_cpu(). Plus you now silently ignore the > upper 32-bits of the passing in "idx" value as well as not > understood XEN_RESOURCE_OP_* values. continue_hypercall_on_cpu() is asynchronized, which requires the "data" field always points to the right place before the hypercall returns. However in our function, we have a "for" loop to cover multiple operations, so the "data" field will be modified in each iteration, which cannot meet the continue_hypercall_on_cpu() requirements... For the preemption check, what about the following? Here the preemption is checked within each resource_access_one() function. static int resource_access_one(uint32_t type, uint32_t cmd, uint64_t idx, uint64_t *val) { int ret; if ( !is_idle_vcpu(current) && hypercall_preempt_check() ) { ret = -ERESTART; break; } /* Handle the resource access code. */ ... return ret; } int resource_access_helper(struct xenpf_resource_op *op) { ... for ( i = 0; i < op->nr; i++ ) { ... if ( data.cpu == smp_processor_id() ) resource_access_one(&data); else on_selected_cpus(cpumask_of(last_cpu), resource_access_one, &data, 1); } ... } > > I also doubt that this warrants a new source file to be introduced, > the helper functions (if indeed needed) can very well live in > platform_hypercall.c. Okay, I will put the resource related function in platform_hypercall.c. Thanks, Dongxiao > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |