From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: VMX status report. Xen:26323 & Dom0:3.7.1 Date: Mon, 14 Jan 2013 16:03:54 +0000 Message-ID: <50F42C6A.2080702@citrix.com> References: <1B4B44D9196EFF41AE41FDA404FC0A1024486E@SHSMSX101.ccr.corp.intel.com> <50EE908602000078000B44CE@nat28.tlf.novell.com> <50EFDC8802000078000B4AC2@nat28.tlf.novell.com> <750FD2DB-E7A5-4038-9274-2CBAF2B4027C@gridcentric.ca> <50F40F42.5020807@citrix.com> <65B8802C-FE47-43CB-87EF-B168F57FF6DA@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <65B8802C-FE47-43CB-87EF-B168F57FF6DA@gmail.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: Andres Lagar-Cavilla Cc: Yongjie Ren , Ian Campbell , Konrad Wilk , Andres Lagar-Cavilla , xen-devel , Chao Zhou , Jan Beulich , Mats Petersson , Yan Dai , YongweiX Xu , SongtaoX Liu , Andres Lagar-Cavilla List-Id: xen-devel@lists.xenproject.org On 14/01/13 15:06, Andres Lagar-Cavilla wrote: > On Jan 14, 2013, at 8:59 AM, David Vrabel wrote: > >> On 14/01/13 04:29, Andres Lagar-Cavilla wrote: >>> >>> Below you'll find pasted an RFC patch to fix this. I've expanded the >>> cc line to add Mats Peterson, who is also looking into some improvements >>> to privcmd (and IanC for general feedback). >>> >>> The RFC patch cuts down code overall and cleans up logic too. I did >>> change the behavior wrt classic implementations when it comes to >>> handling errors & EFAULT. Instead of doing all the mapping work and then >>> copying back to user, I copy back each individual mapping error as soon >>> as it arises. And short-circuit and quit the whole operation as soon as >>> the first EFAULT arises. >> >> Which is broken. > Certainly due to copy_on_write within mmap semaphore. Unfortunately I didn't have time last night to post the fix, pardon for the noise. >> Please just look at my v3 patch and implement that method. ... but be aware that I messed up mmap_return_errors() for V1 and set all MFNs as having errors. Oops. > The one nit I have about that is that it does an unnecessary get_user of the mfn on the second pass for V1. HOw about this? __get_user() and __put_user() are actually cheap (provided they don't fault). This looks ok except for one thing. > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index 3421f0d..fc4952d 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c [...] > @@ -288,7 +289,19 @@ static int mmap_batch_fn(void *data, void *state) > &cur_page); > > /* Store error code for second pass. */ > - *(st->err++) = ret; > + if (st->version == 1) { > + if (ret < 0) { > + /* > + * V1 encodes the error codes in the 32bit top nibble of the > + * mfn (with its known limitations vis-a-vis 64 bit callers). > + */ > + *mfnp |= (ret == -ENOENT) ? > + PRIVCMD_MMAPBATCH_PAGED_ERROR : > + PRIVCMD_MMAPBATCH_MFN_ERROR; You also need to clear the top nibble on success (ret >= 0) so large PFNs with the top nibble already set don't give false positives of errors. David