Re: [Xen-devel] [PATCH v3 03/12] tools/libxenforeignmemory: add support for resource mapping

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 31, 2017 at 10:35:56AM +0100, Paul Durrant wrote:
> A previous patch introduced a new HYPERVISOR_memory_op to acquire guest
> resources for direct priv-mapping.
> 
> This patch adds new functionality into libxenforeignmemory to make use
> of a new privcmd ioctl [1] that uses the new memory op to make such
> resources available via mmap(2).
> 
> [1] 
> http://xenbits.xen.org/gitweb/?p=people/pauldu/linux.git;a=commit;h=ce59a05e6712
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just some very minor nits.

[...]

> diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
> index a6897dc561..838640aa84 100644
> --- a/tools/libs/foreignmemory/core.c
> +++ b/tools/libs/foreignmemory/core.c
> @@ -17,6 +17,8 @@
>  #include <assert.h>
>  #include <errno.h>
>  
> +#include <sys/mman.h>
> +
>  #include "private.h"
>  
>  xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
> @@ -120,6 +122,55 @@ int xenforeignmemory_restrict(xenforeignmemory_handle 
> *fmem,
>      return osdep_xenforeignmemory_restrict(fmem, domid);
>  }
>  
> +xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long frame, unsigned long nr_frames,
> +    void **paddr, int prot, int flags)
> +{
> +    xenforeignmemory_resource_handle *fres;
> +    int rc;
> +
> +    /* Check flags only contains POSIX defined values */
> +    if ( flags & ~(MAP_SHARED | MAP_PRIVATE) )
> +    {
> +        errno = EINVAL;
> +        return NULL;
> +    }
> +
> +    fres = calloc(1, sizeof(*fres));
> +    if ( !fres )

errno = ENOMEM?

> +        return NULL;
> +
> +    fres->domid = domid;
> +    fres->type = type;
> +    fres->id = id;
> +    fres->frame = frame;
> +    fres->nr_frames = nr_frames;
> +    fres->addr = *paddr;
> +    fres->prot = prot;
> +    fres->flags = flags;
> +
> +    rc = osdep_xenforeignmemory_map_resource(fmem, fres);
> +    if ( rc )
> +        goto fail;
> +
> +    *paddr = fres->addr;
> +    return fres;
> +
> +fail:

Not really sure you need a label, this error path is used only once.

> +    free(fres);
> +
> +    return NULL;
> +}
> +
> +void xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    osdep_xenforeignmemory_unmap_resource(fmem, fres);
> + 

Spurious newline?

> +    free(fres);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C

[...]

> diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map 
> b/tools/libs/foreignmemory/libxenforeignmemory.map
> index 716ecaf15c..d5323c87d9 100644
> --- a/tools/libs/foreignmemory/libxenforeignmemory.map
> +++ b/tools/libs/foreignmemory/libxenforeignmemory.map
> @@ -14,3 +14,8 @@ VERS_1.2 {
>       global:
>               xenforeignmemory_map2;
>  } VERS_1.1;
> +VERS_1.3 {
> +     global:
> +             xenforeignmemory_map_resource;
> +             xenforeignmemory_unmap_resource;
> +} VERS_1.2;
> diff --git a/tools/libs/foreignmemory/linux.c 
> b/tools/libs/foreignmemory/linux.c
> index 374e45aed5..4447723cb1 100644
> --- a/tools/libs/foreignmemory/linux.c
> +++ b/tools/libs/foreignmemory/linux.c
> @@ -277,6 +277,51 @@ int 
> osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
>      return ioctl(fmem->fd, IOCTL_PRIVCMD_RESTRICT, &domid);
>  }
>  
> +void osdep_xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    (void) munmap(fres->addr, fres->nr_frames << PAGE_SHIFT);

Do you really need the void cast?

Or should this really be a void function? Other unmap functions
(osdep_xenforeignmemory_unmap) return the error to the caller.

> +}
> +
> +int osdep_xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    privcmd_mmap_resource_t mr;
> +    int rc;
> +
> +    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> +                      fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
> +    if ( fres->addr == MAP_FAILED )
> +        return -1;
> +
> +    memset(&mr, 0, sizeof(mr));

No need for the memset, you are setting all fields below anyway.

> +    mr.dom = fres->domid;
> +    mr.type = fres->type;
> +    mr.id = fres->id;
> +    mr.idx = fres->frame;
> +    mr.num = fres->nr_frames;
> +    mr.addr = (uintptr_t)fres->addr;

[...]

> diff --git a/tools/libs/foreignmemory/private.h 
> b/tools/libs/foreignmemory/private.h
> index c5c07cc4c4..9ff94b724d 100644
> --- a/tools/libs/foreignmemory/private.h
> +++ b/tools/libs/foreignmemory/private.h
> @@ -42,6 +42,36 @@ void *compat_mapforeign_batch(xenforeignmem_handle *fmem, 
> uint32_t dom,
>                                xen_pfn_t *arr, int num);
>  #endif
>  
> +struct xenforeignmemory_resource_handle {
> +    domid_t domid;
> +    unsigned int type;
> +    unsigned int id;
> +    unsigned long frame;
> +    unsigned long nr_frames;
> +    void *addr;
> +    int prot;
> +    int flags;
> +};
> +
> +#ifndef __linux__
> +static inline int osdep_xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    errno = EOPNOTSUPP;
> +    return -1;
> +}
> +
> +static inline void osdep_xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +}
> +#else
> +int osdep_xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
> +void osdep_xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
> +#endif
> +

I would like to see osdep_xenforeignmemory_restrict moved here in a
pre-patch if possible :).

Thanks, Roger.

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