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

[Juergen Gross <jgross@xxxxxxxx>]

On 15/06/18 16:48, Ian Jackson wrote:
> 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.

Okay.

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

Can do.

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

Hmm, yes.

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

Okay.

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

Right.

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

What about two sub-functions and osdep_alloc_pages() just deciding which
to call?

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

Okay.


Juergen


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