Re: [PATCH 2/2] xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl

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

On 29/08/12 17:14, Andres Lagar-Cavilla wrote:
> 
> On Aug 29, 2012, at 9:15 AM, David Vrabel wrote:
> 
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> 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 = data;
>> +	int *err = data;
> Am I missing something or is there an aliasing here? Both mfnp and err point 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 = 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 |= 0xf0000000U;
>> -		st->err++;
>> +	ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
>> +					 st->vma->vm_page_prot, st->domain);
>> +	if (ret < 0) {
>> +		*err = ret;
>> +		if (st->err == 0 || st->err == -ENOENT)
>> +			st->err = ret;
> This will unset -ENOENT if a frame after an ENOENT error fails differently.

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 = m.arr;
>> +	if (state.err) {
>> +		state.user_mfn = (xen_pfn_t *)m.arr;
>> +		state.user_err = m.err;
>> 		ret = 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 … but I see no
> change to the gather_array other than a cast …am 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