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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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:
>>>> --- a/xen/common/compat/memory.c
>>>> +++ b/xen/common/compat/memory.c
>>>> @@ -15,7 +15,6 @@ CHECK_TYPE(domid);
>>>>  #undef compat_domid_t
>>>>  #undef xen_domid_t
>>>>  
>>>> -CHECK_mem_access_op;
>>>>  CHECK_vmemrange;
>>>
>>> You can't just delete this line. Some form of replacement is needed:
>>> Either you need to introduce compat mode translation, or you need
>>> to keep the line and suitably adjust the interface structure (which
>>> might be possible considering there's a [tools interface only] use of
>>> uint64_aligned_t already).
>>
>> I'll look into this. I'm not sure how to go about introducing compat
>> mode translation, could you please tell me where to look for an example
>> of doing that? Thanks!
> 
> Well, the first option to consider should be avoiding translation (and
> hence keeping the check macro invocation in place), the more that
> this is a tools only interface (you're certainly aware that e.g domctl
> and sysctl also avoid translation). Examples of translation can be
> found (you could have guessed that) in xen/common/compat/memory.c.
> 
>>>> @@ -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. Would keeping the dynamic GFNs
buffer allocation be acceptable in this case?


Thanks,
Razvan

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