Re: [Xen-devel] [PATCH 1/3] tools/libxencall: use hypercall buffer device if available

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Juergen Gross writes ("[PATCH 1/3] tools/libxencall: use hypercall buffer 
device if available"):
> Instead of using anonymous memory for hypercall buffers which is then
> locked into memory, use the hypercall buffer device of the Linux
> privcmd driver if available.
> 
> This has the advantage of needing just a single mmap() for allocating
> the buffer and page migration or compaction can't make the buffer
> unaccessible for the hypervisor.

This code looks reasonable to me (making some assumptions about the
behaviour of /dev/xen/privcmd-buf).  However, I find myself quibbling
with the flow control style.  And I have some other comments:

> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> index 533f0c4a8b..06d159cfb8 100644
> --- a/tools/libs/call/private.h
> +++ b/tools/libs/call/private.h
> @@ -21,6 +21,7 @@ struct xencall_handle {
>      xentoollog_logger *logger, *logger_tofree;
>      unsigned flags;
>      int fd;
> +    int buf_fd;

I think this deserves a comment, along the following lines:

                     /* partially     with         no          */
                     /* initialised   privcmd-buf  privcmd-buf */
       int fd;       /*      any        >=0          -1        */
  +    int buf_fd;   /*      any        >=0          >=0       */

or some such.

>  static int all_restrict_cb(Xentoolcore__Active_Handle *ah, domid_t domid) {
>      xencall_handle *xcall = CONTAINER_OF(ah, *xcall, tc_ah);
> -    return xentoolcore__restrict_by_dup2_null(xcall->fd);
> +    int rc;
> +
> +    rc = xentoolcore__restrict_by_dup2_null(xcall->buf_fd);
> +    if ( !rc )
> +        rc = xentoolcore__restrict_by_dup2_null(xcall->fd);
> +    return rc;
>  }

Would a `goto out' approach not be clearer here ?

>      xcall->fd = fd;
> +
> +    /*
> +     * Try the same for the hypercall buffer device.
> +     */
> +    fd = open("/dev/xen/privcmd-buf", O_RDWR|O_CLOEXEC);
> +    if ( fd == -1 && ( errno == ENOENT || errno == ENXIO || errno == ENODEV 
> ) )
> +    {
> +        /* Fallback to /proc/xen/privcmd-buf */
> +        fd = open("/proc/xen/privcmd-buf", O_RDWR|O_CLOEXEC);

Firstly, is it necessary to try both /proc/xen and /dev/xen ?  Surely
nowadays only /dev/xen is relevant.  Unless we intend to backport this
new driver to 2.6.18-based Classic Xen Linux kernels which are
probably not affected by the bug anyway ?

Secondly, please treat errors other than ENOENT on opening
/dev/xen/privcmd-buf as fatal (ie, make osdep_xencall_open return -1
in those cases).

>  int osdep_xencall_close(xencall_handle *xcall)
>  {
>      int fd = xcall->fd;
> +
> +    if ( xcall->buf_fd >= 0 )
> +        close(xcall->buf_fd);
>      if (fd == -1)
>          return 0;
>      return close(fd);

This now looks quite clumsy.  I would do this:

  -    int fd = xcall->fd;
  -
  -    if (fd == -1)
  -        return 0;

  +    if ( xcall->fd >= 0 )
  +        close(xcall->fd);
> +    if ( xcall->buf_fd >= 0 )
> +        close(xcall->buf_fd);
  +    return 0;

which is equivalent but makes the symmetry and idempotency much
clearer.

> @@ -78,6 +93,14 @@ void *osdep_alloc_pages(xencall_handle *xcall, size_t 
> npages)
>      void *p;
>      int rc, i, saved_errno;
>  
> +    if ( xcall->buf_fd >= 0 )
> +    {
> +        p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, 
> xcall->buf_fd, 0);
> +        if ( p == MAP_FAILED )
> +            PERROR("alloc_pages: mmap failed");
> +        return p;
> +    }
> +

I find this early exit approach a bit clumsy, but maybe putting all
the rest in an else branch would be worse.

If you do decide to lift the rest into an else branch, I think you
should keep the `out' clause outside it.  (It's a shame we don't have
the libxl-style correct error handling approach here, ie: initialise
p=NULL at the top; always `goto out' rather than `return NULL' on
error; and have the out section check p before calling munmap.

> @@ -119,8 +142,10 @@ out:
>  void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
>  {
>      int saved_errno = errno;
> -    /* Recover the VMA flags. Maybe it's not necessary */
> -    madvise(ptr, npages * PAGE_SIZE, MADV_DOFORK);
> +
> +    if ( xcall->buf_fd < 0 )
> +        /* Recover the VMA flags. Maybe it's not necessary */
> +        madvise(ptr, npages * PAGE_SIZE, MADV_DOFORK);

This part LGTM but given the multiple lines inside the if, maybe { }
would be warranted.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel