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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/26/16 10:18, Jan Beulich wrote:
>>>> On 26.08.16 at 08:11, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 
> One general note first: I don't really like the term "sparse" here,
> as that suggests to me you want to skip address space holes.
> How about calling it "multi", "array", or some such?

Fair enough, will rename it.

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1815,7 +1815,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct 
>> p2m_domain *hp2m,
>>   * Set access type for a region of gfns.
>>   * If gfn == INVALID_GFN, sets the default access type.
>>   */
>> -long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>> +long p2m_set_mem_access(struct domain *d, gfn_t gfn, xen_pfn_t *arr, 
>> uint32_t nr,
> 
> const

Will do.

>> @@ -1874,28 +1874,53 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>      if ( ap2m )
>>          p2m_lock(ap2m);
>>  
>> -    for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
>> +    if ( !arr )
>>      {
>> -        if ( ap2m )
>> +        for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
>>          {
>> -            rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l));
>> -            /* If the corresponding mfn is invalid we will just skip it */
>> -            if ( rc && rc != -ESRCH )
>> -                break;
>> -        }
>> -        else
>> -        {
>> -            mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
>> -            rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
>> -            if ( rc )
>> +            if ( ap2m )
>> +            {
>> +                rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, 
>> _gfn(gfn_l));
>> +                /* If the corresponding mfn is invalid we will just skip it 
>> */
>> +                if ( rc && rc != -ESRCH )
>> +                    break;
>> +            }
>> +            else
>> +            {
>> +                mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
>> +                rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, 
>> -1);
>> +                if ( rc )
>> +                    break;
>> +            }
>> +
>> +            /* Check for continuation if it's not the last iteration. */
>> +            if ( nr > ++start && !(start & mask) && 
>> hypercall_preempt_check() )
>> +            {
>> +                rc = start;
>>                  break;
>> +            }
> 
> I think it would help if you broke out some of the loop body into a
> helper function.

I'll refactor it.

>>          }
>> +    }
>> +    else
>> +    {
>> +        uint32_t i;
>>  
>> -        /* Check for continuation if it's not the last iteration. */
>> -        if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
>> +        for ( i = 0; i < nr; ++i )
>>          {
>> -            rc = start;
>> -            break;
>> +            if ( ap2m )
>> +            {
>> +                rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, 
>> _gfn(arr[i]));
>> +                /* If the corresponding mfn is invalid we will just skip it 
>> */
>> +                if ( rc && rc != -ESRCH )
>> +                    break;
>> +            }
>> +            else
>> +            {
>> +                mfn = p2m->get_entry(p2m, arr[i], &t, &_a, 0, NULL, NULL);
>> +                rc = p2m->set_entry(p2m, arr[i], mfn, PAGE_ORDER_4K, t, a, 
>> -1);
>> +                if ( rc )
>> +                    break;
>> +            }
>>          }
> 
> Where is the preemption handling?

It's missing. I'll add it back in.

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

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

>> +        rc = p2m_set_mem_access(d, _gfn(mao.pfn), arr, mao.nr, start_iter,
>> +                                MEMOP_CMD_MASK, mao.access, 0);
> 
> Please don't pass mao.pfn here when it's meant to be ignore by
> the caller. Use GFN_INVALID instead. And for future extensibility
> please check that the caller put some pre-defined pattern here
> (not sure whether zero is appropriate in this case).

Will do.


Thanks for the review,
Razvan


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