From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH v14 01/10] x86: add generic resource (e.g. MSR) access hypercall Date: Tue, 2 Sep 2014 16:33:10 +0800 Message-ID: <20140902083310.GA15872@pengc-linux> References: <1409211839-21718-1-git-send-email-chao.p.peng@linux.intel.com> <1409211839-21718-2-git-send-email-chao.p.peng@linux.intel.com> <5400BB24020000780002F1BF@mail.emea.novell.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <5400BB24020000780002F1BF@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Fri, Aug 29, 2014 at 04:40:52PM +0100, Jan Beulich wrote: > >>> On 28.08.14 at 09:43, wrote: > > +static void resource_access_one(void *info) > > +{ > > + struct xen_resource_access *ra = info; > > + int ret = 0; > > + > > + switch ( ra->data.cmd ) > > + { > > + case XEN_RESOURCE_OP_MSR_READ: > > + case XEN_RESOURCE_OP_MSR_WRITE: > > + if ( ra->data.idx >> 32 ) > > + ret = -EINVAL; > > + if ( !allow_access_msr(ra->data.idx) ) > > + ret = -EACCES; > > + if ( ra->data.cmd == XEN_RESOURCE_OP_MSR_READ ) > > + ret = rdmsr_safe(ra->data.idx, ra->data.val); > > + else > > + ret = wrmsr_safe(ra->data.idx, ra->data.val); > > + break; > > Did you mean these latter tow if()-s perhaps be "else if"? It is true, thanks. > > > + case XENPF_resource_op: > > + { > > + struct xen_resource_access ra; > > + struct xenpf_resource_op *rsc_op = &op->u.resource_op; > > + unsigned int i, j = 0, cpu = smp_processor_id(); > > + > > + for ( i = 0; i < rsc_op->nr; i++ ) > > + { > > + if ( copy_from_guest_offset(&ra.data, rsc_op->data, i, 1) ) > > + { > > + ret = -EFAULT; > > + break; > > + } > > + > > + if ( ra.data.cpu == cpu ) > > + resource_access_one(&ra); > > + else if ( cpu_online(ra.data.cpu) ) > > + on_selected_cpus(cpumask_of(ra.data.cpu), > > + resource_access_one, &ra, 1); > > + else > > + { > > + ret = -ENODEV; > > + break; > > + } > > + > > + if ( ra.ret ) > > + { > > + ret = ra.ret; > > + break; > > + } > > + > > + if ( copy_to_guest_offset(rsc_op->data, i, &ra.data, 1) ) > > + { > > + ret = -EFAULT; > > + break; > > + } > > + > > + /* Find the start point that requires no preemption */ > > + if ( ra.data.flag && j == 0 ) > > + j = i; > > + /* Set j = 0 when walking out of the non-preemption area */ > > + if ( ra.data.flag == 0 ) > > + j = 0; > > + if ( hypercall_preempt_check() ) > > + { > > + ret = hypercall_create_continuation( > > + __HYPERVISOR_platform_op, "ih", > > + ra.data.flag ? j : i, u_xenpf_op); > > Which means everything starting from j will be re-executed > another time when continuing. That creates three problems: You > can't guarantee forwards progress, you may do something > having side effects more than once, and you break the operation > in a place that was requested to not be preemptible. I saw the problem here. Actually the j or i here will not be passed to next iteration successfully. Possibly a 'count' param is needed to be added to do_platform_op() for this purpose. > > > +struct xenpf_resource_data { > > + uint16_t cmd; /* XEN_RESOURCE_OP_* */ > > + uint16_t flag; /* avoid preemption between certain iterations */ > > You want to use just one bit out of this field (and verify all other bits > are zero, so they can get assigned a meaning later). OK, I will add a one-bit flag macro to indicate this. > > Jan > > > + uint32_t cpu; > > + uint64_t idx; > > + uint64_t val; > > +};