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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
