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

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Aug 29, 2012, at 12:36 PM, David Vrabel wrote:

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

May I suggest a comment to clarify this here? Are xen_pfn_t and int the same 
size in both bitnesses? The very fact that I raise the question is an argument 
against this black-magic aliasing. Imho.

A explicit union for each slot in the *data, or passing both arrays to the 
callback looks better to me.

> 
>>>     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
I think the best way to do this is:

if ((ret == -ENOENT) && (st->err == 0))
        st->err = -ENOENT;

Then st->err is -ENOENT if at least there was one individual -ENOENT or zero 
otherwise. Which is the expectation of libxc (barring an EFAULT or some other 
higher-level whole-operation error).

Andres

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