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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>
>> 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

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