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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 04 September 2017 11:44
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu <wei.liu2@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v3 03/12] tools/libxenforeignmemory: add
> support for resource mapping
> 
> 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=ce5
> 9a05e6712
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> LGTM:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 

Thanks.

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

Yes. It should be.

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

That's true. In previous incarnations it was used more than once. I'll blow it 
away now.

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

I like newlines :-) This one probably is overkill though.

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

Yeah, I'll follow suit and let the callers ignore the error if they want to.

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

Actually I should probably be doing C99 style initialization 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 :).
> 

It's a post-patch (as you noticed :-))

  Paul

> Thanks, Roger.

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