Re: [Xen-devel] VMX status report. Xen:26323 & Dom0:3.7.1

[Andres Lagar-Cavilla <andres.lagarcavilla@xxxxxxxxx>]

On Jan 14, 2013, at 11:03 AM, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:

On 14/01/13 15:06, Andres Lagar-Cavilla wrote:

On Jan 14, 2013, at 8:59 AM, David Vrabel <david.vrabel@xxxxxxxxxx> 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.

But classic kernels don't do this either, afaict (e.g. http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/file/c340a22a3a63/drivers/xen/privcmd/privcmd.c#l282 or XenServer's 6.1 kernel).

The key reason for V2 is to get rid of all these limitations and stop trying to fix them in V1.

I'm open to whichever fix, though. It'd be just one line for the else case. I'd just like some feedback before "officially" submitting a patch.

Thanks!

Andres

David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel