Re: [PATCH 2/3] tools/libxc: Make the mem_access APIs generic

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/04/14 04:49, Aravindh Puthiyaparambil wrote:
> This patch does the following:
> 1. Add new xc_[sg]et_mem_access APIs.
> 2. Remove xc_hvm_[sg]et_mem_access() APIs.
>
> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
>
> ---
> Changes from the RFC version of the patch:
> 1. Remove xc_mem_access_memop() wrapper.
> 2. Remove xc_hvm_[sg]et_mem_access() APIs.
>
> Thanks,
> Aravindh
>
> diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
> index a50c145..0277ab3 100644
> --- a/tools/libxc/xc_mem_access.c
> +++ b/tools/libxc/xc_mem_access.c
> @@ -22,7 +22,7 @@
>   */
>  
>  #include "xc_private.h"
> -
> +#include <xen/memory.h>
>  
>  int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
>                           uint32_t *port)
> @@ -47,12 +47,53 @@ int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
>                                  NULL);
>  }
>  
> +

Spurious whitespace change.

>  int xc_mem_access_resume(xc_interface *xch, domid_t domain_id, unsigned long gfn)

What is this gfn parameter doing?  It isn't used in the hypercall.

>  {
> -    return xc_mem_event_memop(xch, domain_id,
> -                                XENMEM_access_op_resume,
> -                                XENMEM_access_op,
> -                                gfn, NULL);
> +    xen_mem_access_op_t mao;
> +
> +    memset(&mao, 0, sizeof(mao));
> +    mao.op    = XENMEM_access_op_resume;
> +    mao.domid = domain_id;

Please use structure initialisation for this.

xen_mem_access_op_t mao =
{
    .op = XENMEM_access_op_resume,
    .domid = domain_id
};



> +
> +    return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
> +}
> +
> +int xc_set_mem_access(xc_interface *xch,
> +                      domid_t domain_id,
> +                      xenmem_access_t access,
> +                      uint64_t first_pfn,
> +                      uint32_t nr)
> +{
> +    xen_mem_access_op_t mao;
> +
> +    mao.op     = XENMEM_access_op_set_access;
> +    mao.domid  = domain_id;
> +    mao.access = access;
> +    mao.pfn    = first_pfn;
> +    mao.nr     = nr;
> +
> +    return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
> +}
> +
> +int xc_get_mem_access(xc_interface *xch,
> +                      domid_t domain_id,
> +                      uint64_t pfn,
> +                      xenmem_access_t *access)
> +{
> +    xen_mem_access_op_t mao;
> +    int rc;
> +
> +    mao.op    = XENMEM_access_op_get_access;
> +    mao.domid = domain_id;
> +    mao.pfn   = pfn;
> +
> +    rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
> +
> +    if ( !rc )
> +        *access = mao.access;

Surely you should only write access back in the case of success?

~Andrew