Re: VMX status report. Xen:26323 & Dom0:3.7.1

[David Vrabel <david.vrabel@citrix.com>]

On 14/01/13 15:06, Andres Lagar-Cavilla wrote:
> On Jan 14, 2013, at 8:59 AM, David Vrabel <david.vrabel@citrix.com> 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