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 18:04:12 +0800 Message-ID: <20140902100412.GE15872@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> <20140902083310.GA15872@pengc-linux> <5405A165020000780002FBF9@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: <5405A165020000780002FBF9@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 Tue, Sep 02, 2014 at 09:52:21AM +0100, Jan Beulich wrote: > >>> On 02.09.14 at 10:33, wrote: > > On Fri, Aug 29, 2014 at 04:40:52PM +0100, Jan Beulich wrote: > >> >>> On 28.08.14 at 09:43, wrote: > >> > + 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. > > You can't add any parameter to do_platform_op(), and I also > don't see why you'd need to. > > Jan Let me shed more light on this. The 'i' we want to pass to next iteration is saved in guest_cpu_user_regs in hypercall_create_continuation(), which will be used as parameter for the re-execution of do_platform_op(). Take do_hvm_op() as example, 6199 rc = hypercall_create_continuation(__HYPERVISOR_hvm_op,"lh", 6120 op | start_iter, arg); It reuses several bits in existed param 'op' to pass the start_iter and then the start_iter is obtained in the second call of do_hvm_op(): 5478 unsigned long start_iter = op & ~HVMOP_op_mask; For our case we only save the 'i' into guest_cpu_user_regs but we lack of way to accept it. However, the method used in do_hvm_op() does not work for us as do_platform_op() only has one point type param which we can't safely reuse any bit. Thanks, Chao