From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 2/18 V2]: PVH xen: add XENMEM_add_to_physmap_range Date: Mon, 18 Mar 2013 16:15:32 -0400 Message-ID: <20130318201532.GI27433@phenom.dumpdata.com> References: <20130315172018.395bbbe3@mantra.us.oracle.com> <51470ACC02000078000C64CB@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <51470ACC02000078000C64CB@nat28.tlf.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: xen-devel List-Id: xen-devel@lists.xenproject.org On Mon, Mar 18, 2013 at 11:38:35AM +0000, Jan Beulich wrote: > >>> On 16.03.13 at 01:20, Mukesh Rathor wrote: > > In this patch we add a new function xenmem_add_to_physmap_range(), and > > change xenmem_add_to_physmap_once parameters so it can be called from > > xenmem_add_to_physmap_range. There is no PVH specific change here. > > > > Changes in V2: > > - Do not break parameter so xenmem_add_to_physmap_once() but pass in > > struct xen_add_to_physmap. > > > > Signed-off-by: Mukesh Rathor > > --- > > xen/arch/x86/mm.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 files changed, 79 insertions(+), 3 deletions(-) > > This continued to lack compat mode support (i.e. modification to > xen/arch/x86/x86_64/compat/mm.c:compat_arch_memory_op()). Do we need it? Only 64-bit kernels can use PVH - and that was from the start the idea. > > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -4268,7 +4268,8 @@ static int handle_iomem_range(unsigned long s, unsigned long e, void *p) > > > > static int xenmem_add_to_physmap_once( > > struct domain *d, > > - const struct xen_add_to_physmap *xatp) > > + const struct xen_add_to_physmap *xatp, > > + domid_t foreign_domid) > > So you add this new parameter but don't use it? > > > { > > struct page_info *page = NULL; > > unsigned long gfn = 0; /* gcc ... */ > > @@ -4395,7 +4396,7 @@ static int xenmem_add_to_physmap(struct domain *d, > > start_xatp = *xatp; > > while ( xatp->size > 0 ) > > { > > - rc = xenmem_add_to_physmap_once(d, xatp); > > + rc = xenmem_add_to_physmap_once(d, xatp, -1); > > And if it indeed is being used, please use a proper DOMID_* value > here. > > > if ( rc < 0 ) > > return rc; > > > > @@ -4421,7 +4422,52 @@ static int xenmem_add_to_physmap(struct domain *d, > > return rc; > > } > > > > - return xenmem_add_to_physmap_once(d, xatp); > > + return xenmem_add_to_physmap_once(d, xatp, -1); > > And here. > > > +} > > + > > +static noinline int xenmem_add_to_physmap_range(struct domain *d, > > + struct xen_add_to_physmap_range *xatpr) > > +{ > > + int rc; > > + > > + /* Process entries in reverse order to allow continuations */ > > + while ( xatpr->size > 0 ) > > + { > > + xen_ulong_t idx; > > + xen_pfn_t gpfn; > > + struct xen_add_to_physmap xatp; > > + > > + rc = copy_from_guest_offset(&idx, xatpr->idxs, xatpr->size-1, 1); > > + if ( rc < 0 ) > > + goto out; > > + > > + rc = copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1); > > + if ( rc < 0 ) > > + goto out; > > + > > + xatp.space = xatpr->space; > > + xatp.idx = idx; > > + xatp.gpfn = gpfn; > > + rc = xenmem_add_to_physmap_once(d, &xatp, xatpr->foreign_domid); > > + > > + if (rc) > > + goto out; > > That doesn't seem right, together with you apparently not using > the "errs" array altogether. > > > + > > + xatpr->size--; > > + > > + /* Check for continuation if it's not the last interation */ > > + if ( xatpr->size > 0 && hypercall_preempt_check() ) > > + { > > + rc = -EAGAIN; > > + goto out; > > + } > > + } > > + > > + rc = 0; > > + > > +out: > > + return rc; > > + > > } > > > > long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) > > @@ -4438,6 +4484,10 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) > > if ( copy_from_guest(&xatp, arg, 1) ) > > return -EFAULT; > > > > + /* This one is only supported for add_to_physmap_range */ > > + if ( xatp.space == XENMAPSPACE_gmfn_foreign ) > > + return -EINVAL; > > + > > d = rcu_lock_domain_by_any_id(xatp.domid); > > if ( d == NULL ) > > return -ESRCH; > > @@ -4465,6 +4515,32 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) > > return rc; > > } > > > > + case XENMEM_add_to_physmap_range: > > + { > > + struct xen_add_to_physmap_range xatpr; > > + struct domain *d; > > + > > + if ( copy_from_guest(&xatpr, arg, 1) ) > > + return -EFAULT; > > + > > + rc = rcu_lock_target_domain_by_id(xatpr.domid, &d); > > + if ( rc != 0 ) > > + return rc; > > + > > + rc = xenmem_add_to_physmap_range(d, &xatpr); > > + > > + rcu_unlock_domain(d); > > + > > + if ( rc && copy_to_guest(arg, &xatpr, 1) ) > > For one, shouldn't this be "!rc"? > > And then you update ->size, but that one is specified to be only > and IN field. And considering that "errs" is the only OUT one, yet > that isn't even formally correct (because the field itself is an IN, > its what it points to where the output goes), I don't see why you The 'err' is not formally correct? The memory.h says: 258 /* OUT */ 259 260 /* Per index error code. */ 261 XEN_GUEST_HANDLE(int) errs; 262 }; or are you referring to 'size' which I agree with - it is part of 'IN'. > would need to copy back any part of the structure. > > Jan > > > + rc = -EFAULT; > > + > > + if ( rc == -EAGAIN ) > > + rc = hypercall_create_continuation( > > + __HYPERVISOR_memory_op, "ih", op, arg); > > + > > + return rc; > > + } > > + > > case XENMEM_set_memory_map: > > { > > struct xen_foreign_memory_map fmap; > > -- > > 1.7.2.3 > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >