From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 2/2] xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl Date: Wed, 29 Aug 2012 17:36:42 +0100 Message-ID: <503E451A.20107@citrix.com> References: <1346246116-29999-1-git-send-email-david.vrabel@citrix.com> <1346246116-29999-3-git-send-email-david.vrabel@citrix.com> <7392D0E0-02A4-48D7-8B16-4F93EA01F3AF@gridcentric.ca> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <7392D0E0-02A4-48D7-8B16-4F93EA01F3AF@gridcentric.ca> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar-Cavilla Cc: "xen-devel@lists.xensource.com" , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On 29/08/12 17:14, Andres Lagar-Cavilla wrote: > = > On Aug 29, 2012, at 9:15 AM, David Vrabel wrote: > = >> From: David Vrabel >> >> PRIVCMD_MMAPBATCH_V2 extends PRIVCMD_MMAPBATCH with an additional >> field for reporting the error code for every frame that could not be >> mapped. libxc prefers PRIVCMD_MMAPBATCH_V2 over PRIVCMD_MMAPBATCH. [...] >> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >> index ccee0f1..ddd32cf 100644 >> --- a/drivers/xen/privcmd.c >> +++ b/drivers/xen/privcmd.c >> @@ -248,18 +248,23 @@ struct mmap_batch_state { >> struct vm_area_struct *vma; >> int err; >> >> - xen_pfn_t __user *user; >> + xen_pfn_t __user *user_mfn; >> + int __user *user_err; >> }; >> >> static int mmap_batch_fn(void *data, void *state) >> { >> xen_pfn_t *mfnp =3D data; >> + int *err =3D data; > Am I missing something or is there an aliasing here? Both mfnp and err po= int to data? There is deliberate aliasing here. We use the mfn array to save the error codes for later processing. >> struct mmap_batch_state *st =3D state; >> + int ret; >> >> - if (xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1, >> - st->vma->vm_page_prot, st->domain) < 0) { >> - *mfnp |=3D 0xf0000000U; >> - st->err++; >> + ret =3D xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp,= 1, >> + st->vma->vm_page_prot, st->domain); >> + if (ret < 0) { >> + *err =3D ret; >> + if (st->err =3D=3D 0 || st->err =3D=3D -ENOENT) >> + st->err =3D ret; > This will unset -ENOENT if a frame after an ENOENT error fails differentl= y. I thought that was what the original implementation did but it seems it does not. >> @@ -325,12 +359,16 @@ static long privcmd_ioctl_mmap_batch(void __user *= udata) >> >> up_write(&mm->mmap_sem); >> >> - if (state.err > 0) { >> - state.user =3D m.arr; >> + if (state.err) { >> + state.user_mfn =3D (xen_pfn_t *)m.arr; >> + state.user_err =3D m.err; >> ret =3D traverse_pages(m.num, sizeof(xen_pfn_t), >> - &pagelist, >> - mmap_return_errors, &state); >> - } >> + &pagelist, >> + mmap_return_errors, &state); > The callback now maps data to err (instead of mfnp =85 but I see no > change to the gather_array other than a cast =85am I missing something? The kernel mfn and the err array are aliased. I could have made gather_array() allow the kernel array to have larger elements than the user array but that looked like too much work. David