Re: [Xen-devel] [PATCH RFC] tools/libxc, xen/x86: Added xc_set_mem_access_sparse()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/30/2016 09:39 AM, Jan Beulich wrote:
>>>> On 29.08.16 at 18:42, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 08/29/16 19:20, Jan Beulich wrote:
>>>>>> On 29.08.16 at 18:02, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 08/29/16 18:42, Jan Beulich wrote:
>>>>>>>> On 29.08.16 at 17:29, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>> On 08/26/2016 11:14 AM, Jan Beulich wrote:
>>>>>>>>>> On 26.08.16 at 09:40, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>>> On 08/26/16 10:18, Jan Beulich wrote:
>>>>>>>>>>>> On 26.08.16 at 08:11, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>>>>> @@ -76,6 +76,17 @@ int mem_access_memop(unsigned long cmd,
>>>>>>>>>>          }
>>>>>>>>>>          break;
>>>>>>>>>>  
>>>>>>>>>> +    case XENMEM_access_op_set_access_sparse:
>>>>>>>>>> +    {
>>>>>>>>>> +        xen_pfn_t *arr = xmalloc_bytes(sizeof(xen_pfn_t) * mao.nr);
>>>>>>>>>> +
>>>>>>>>>> +        // copy_from_guest(arr, mao.pfn_list, mao.nr);
>>>>>>>>>
>>>>>>>>> What is this (wrongly C++ style) comment about? I think this really
>>>>>>>>> wasn't meant to be a comment, so RFC or not - how do things work
>>>>>>>>> with this commented out? And where is the error checking for the
>>>>>>>>> allocation (which btw should be xmalloc_array(), but the need for
>>>>>>>>> an allocation here is questionable - the called function would better
>>>>>>>>> retrieve the GFNs one by one).
>>>>>>>>
>>>>>>>> They don't work, I forgot that comment in (the line should not have 
>>>>>>>> been
>>>>>>>> commented). I first wrote the patch on Xen 4.6 and there there was no
>>>>>>>> CHECK_mem_access_op, so I was just trying to figure out what went wrong
>>>>>>>> when I first saw the compile errors and tried this, then left it in
>>>>>>>> accidentally.
>>>>>>>>
>>>>>>>> Indeed, there should also be a check for allocation failure.
>>>>>>>>
>>>>>>>> Do you mean that I would do better to just copy_from_guest(&gfn,
>>>>>>>> mao.pfn_list + index, 1) in a for loop that sets mem_access 
>>>>>>>> restrictions?
>>>>>>>
>>>>>>> Yes, albeit it is copy_from_guest_offset(&gfn, mao.pfn_list, index, 1).
>>>>>>
>>>>>> Avoiding translation, to the best of my understanding (and tested with
>>>>>> the latest version of the patch I'm working on) would then require
>>>>>> replacing copy_from_guest() with copy_from_user(), which does not have a
>>>>>> copy_from_user_offset() alternative.
>>>>>
>>>>> I don't follow - where did you see copy_from_user() used in such a
>>>>> case? If you go through the existing examples, you'll find that with
>>>>> some #define-s (re-vectoring to copy_from_compat_offset()) this
>>>>> can easily be taken care of.
>>>>
>>>> I was looking at xc_mem_paging_memop(), where the buffer parameter is
>>>> being sent via mpo.buffer, which is an uint64_aligned_t, which I thought
>>>> was what you meant. On the HV side, it's being copied_from_user().
>>>>
>>>> In the interest of preventing furher misunderstanding, could you please
>>>> point out a specific example you have in mind?
>>>
>>> Actually, having looked again more closely, I'm not sure you need
>>> to use the compat version of the copying routine; you may need a
>>> thin handler in compat/memory.c. Before going to further search
>>> for a suitable example, would you mind pointing out what it is that
>>> does not work with copy_from_guest_offset()?
>>
>> With this change:
>>
>> @@ -452,6 +453,11 @@ struct xen_mem_access_op {
>>       * ~0ull is used to set and get the default access for pages
>>       */
>>      uint64_aligned_t pfn;
>> +    /*
>> +     * List of pfns to set access for
>> +     * Used only with XENMEM_access_op_set_access_multi
>> +     */
>> +    uint64_aligned_t pfn_list;
>>  };
> 
> Well, this can't possibly be right. You absolutely need to retain
> the handle here that you had in the patch; to avoid the need for
> compat translation you will want to make this a 64-bit handle,
> though.

That was my first thought as well, but any combination:

XEN_GUEST_HANDLE(uint64_aligned_t) pfn_list;
XEN_GUEST_HANDLE_64(uint64_aligned_t) pfn_list;
XEN_GUEST_HANDLE_64(xen_pfn_t) pfn_list;

triggers this error:

~/work/xen.git/xen/include/xen/compat.h:134:32: error: size of array
‘__checkSstruct_mem_access_op’ is negative
 #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
                                ^
~/work/xen.git/xen/include/xen/compat.h:155:17: note: in expansion of
macro ‘CHECK_NAME_’
     typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
                 ^
~/work/xen.git/xen/include/compat/xlat.h:672:5: note: in expansion of
macro ‘CHECK_SIZE_’
     CHECK_SIZE_(struct, mem_access_op); \
     ^
compat/memory.c:18:1: note: in expansion of macro ‘CHECK_mem_access_op’
 CHECK_mem_access_op;
 ^
compat/memory.c: In function ‘__checkFstruct_mem_access_op__pfn_list’:
~/work/xen.git/xen/include/xen/compat.h:170:18: error: comparison of
distinct pointer types lacks a cast [-Werror]
     return &x->f == &c->f; \
                  ^
~/work/xen.git/xen/include/xen/compat.h:176:5: note: in expansion of
macro ‘CHECK_FIELD_COMMON_’
     CHECK_FIELD_COMMON_(k, CHECK_NAME_(k, n ## __ ## f, F), n, f)
     ^
~/work/xen.git/xen/include/compat/xlat.h:678:5: note: in expansion of
macro ‘CHECK_FIELD_’
     CHECK_FIELD_(struct, mem_access_op, pfn_list)
     ^
compat/memory.c:18:1: note: in expansion of macro ‘CHECK_mem_access_op’
 CHECK_mem_access_op;
 ^
cc1: all warnings being treated as errors

It would appear that either I'm missing something, or the only way to
continue is to manually add the required translation considering the
modifications.


Thanks,
Razvan

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