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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

>          }
> +    }
> +    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?

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

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

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

Jan

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