Re: [Xen-devel] [PATCH RFC 2/2] tools/libxc: Make the mem_access APIs generic

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-08 at 22:50 -0700, Aravindh Puthiyaparambil wrote:
> @@ -47,12 +47,59 @@ int xc_mem_access_disable(xc_interface *xch, domid_t 
> domain_id)
>                                  NULL);
>  }
>  
> +static int xc_mem_access_memop(xc_interface *xch, domid_t domid,
> +                               xen_mem_access_op_t *mao)
> +{
> +    mao->domid = domid;
> +
> +    return do_memory_op(xch, XENMEM_access_op, mao, sizeof(*mao));

I'm not sure what this wrapper is bringing to the table over just making
the call in the callers. Also you inconsistently do and don't set
mao->domid in the callers. If you want to keep the wrapper then I'd
suggest making the callers set domid along with all the other fields and
dropping the domid argument here.

> @@ -594,64 +596,18 @@ int xc_hvm_set_mem_type(
>  }
>  
>  int xc_hvm_set_mem_access(
> -    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access, uint64_t 
> first_pfn, uint64_t nr)
> +    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access,
> +    uint64_t first_pfn, uint64_t nr)
>  {
> -    DECLARE_HYPERCALL;
> -    DECLARE_HYPERCALL_BUFFER(struct xen_hvm_set_mem_access, arg);
> -    int rc;
> -
> -    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
> -    if ( arg == NULL )
> -    {
> -        PERROR("Could not allocate memory for xc_hvm_set_mem_access 
> hypercall");
> -        return -1;
> -    }
> -
> -    arg->domid         = dom;
> -    arg->hvmmem_access = mem_access;
> -    arg->first_pfn     = first_pfn;
> -    arg->nr            = nr;
> -
> -    hypercall.op     = __HYPERVISOR_hvm_op;
> -    hypercall.arg[0] = HVMOP_set_mem_access;
> -    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> -
> -    rc = do_xen_hypercall(xch, &hypercall);
> -
> -    xc_hypercall_buffer_free(xch, arg);
> -
> -    return rc;
> +    if ( nr > UINT_MAX )
> +        return -EINVAL;

I guess the hypervisor must also make a similar check?

> +    return xc_set_mem_access(xch, dom, mem_access, first_pfn, nr);

This will return -1 and set errno, while the EINVAL above returns a
-errno value. 

Personally I would just drop the limit check and rely on the hypervisor
to return an error, but if you want to keep the check then please make
it use the -1 and set errno pattern.

Is this now just a wrapper for xc_set_mem_access? Can we drop the now
pointless xc_hvm_*_mem_access wrappers? We don't guarantee API stability
for libxc.

Speaking of wrappers, how come the UINT_MAX limit check is only done in
this wrapper?

> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index e3a32f2..2fc6743 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1730,14 +1730,17 @@ int xc_hvm_set_mem_type(
>  
>  /*
>   * Set a range of memory to a specific access.
> + * Maximum value of nr is UINT_MAX
>   * Allowed types are HVMMEM_access_default, HVMMEM_access_n, any combination 
> of 
>   * HVM_access_ + (rwx), and HVM_access_rx2rw
> + * Please note that this API has been deprecated by xc_set_mem_access()

Aha, lets nuke it then!

Ian.


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