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

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

[-- Attachment #1.1: Type: text/plain, Size: 2807 bytes --]

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

> 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.

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
> 


[-- Attachment #1.2: Type: text/html, Size: 5905 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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