Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

From: Oleksandr Andrushchenko
Date: Tue Apr 24 2018 - 04:07:39 EST


On 04/24/2018 10:51 AM, Juergen Gross wrote:
On 24/04/18 07:43, Oleksandr Andrushchenko wrote:
On 04/24/2018 01:41 AM, Boris Ostrovsky wrote:
On 04/23/2018 08:10 AM, Oleksandr Andrushchenko wrote:
On 04/23/2018 02:52 PM, Wei Liu wrote:
On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr Andrushchenko
wrote:
ÂÂÂÂÂ the gntdev.

I think this is generic enough that it could be implemented by a
device not tied to Xen. AFAICT the hyper_dma guys also wanted
something similar to this.
You can't just wrap random userspace memory into a dma-buf. We've
just had
this discussion with kvm/qemu folks, who proposed just that, and
after a
bit of discussion they'll now try to have a driver which just wraps a
memfd into a dma-buf.
So, we have to decide either we introduce a new driver
(say, under drivers/xen/xen-dma-buf) or extend the existing
gntdev/balloon to support dma-buf use-cases.

Can anybody from Xen community express their preference here?

Oleksandr talked to me on IRC about this, he said a few IOCTLs need to
be added to either existing drivers or a new driver.

I went through this thread twice and skimmed through the relevant
documents, but I couldn't see any obvious pros and cons for either
approach. So I don't really have an opinion on this.

But, assuming if implemented in existing drivers, those IOCTLs need to
be added to different drivers, which means userspace program needs to
write more code and get more handles, it would be slightly better to
implement a new driver from that perspective.
If gntdev/balloon extension is still considered:

All the IOCTLs will be in gntdev driver (in current xen-zcopy
terminology):
ÂÂ- DRM_ICOTL_XEN_ZCOPY_DUMB_FROM_REFS
ÂÂ- DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
ÂÂ- DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE

Balloon driver extension, which is needed for contiguous/DMA
buffers, will be to provide new *kernel API*, no UAPI is needed.

So I am obviously a bit late to this thread, but why do you need to add
new ioctls to gntdev and balloon? Doesn't this driver manage to do what
you want without any extensions?
1. I only (may) need to add IOCTLs to gntdev
2. balloon driver needs to be extended, so it can allocate
contiguous (DMA) memory, not IOCTLs/UAPI here, all lives
in the kernel.
3. The reason I need to extend gnttab with new IOCTLs is to
provide new functionality to create a dma-buf from grant references
and to produce grant references for a dma-buf. This is what I have as UAPI
description for xen-zcopy driver:

1. DRM_IOCTL_XEN_ZCOPY_DUMB_FROM_REFS
This will create a DRM dumb buffer from grant references provided
by the frontend. The intended usage is:
 - Frontend
ÂÂÂ - creates a dumb/display buffer and allocates memory
ÂÂÂ - grants foreign access to the buffer pages
ÂÂÂ - passes granted references to the backend
 - Backend
ÂÂÂ - issues DRM_XEN_ZCOPY_DUMB_FROM_REFS ioctl to map
ÂÂÂÂÂ granted references and create a dumb buffer
ÂÂÂ - requests handle to fd conversion via DRM_IOCTL_PRIME_HANDLE_TO_FD
ÂÂÂ - requests real HW driver/consumer to import the PRIME buffer with
ÂÂÂÂÂ DRM_IOCTL_PRIME_FD_TO_HANDLE
ÂÂÂ - uses handle returned by the real HW driver
 - at the end:
ÂÂÂ o closes real HW driver's handle with DRM_IOCTL_GEM_CLOSE
ÂÂÂ o closes zero-copy driver's handle with DRM_IOCTL_GEM_CLOSE
ÂÂÂ o closes file descriptor of the exported buffer

2. DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
This will grant references to a dumb/display buffer's memory provided by
the
backend. The intended usage is:
 - Frontend
ÂÂÂ - requests backend to allocate dumb/display buffer and grant references
ÂÂÂÂÂ to its pages
 - Backend
ÂÂÂ - requests real HW driver to create a dumb with
DRM_IOCTL_MODE_CREATE_DUMB
ÂÂÂ - requests handle to fd conversion via DRM_IOCTL_PRIME_HANDLE_TO_FD
ÂÂÂ - requests zero-copy driver to import the PRIME buffer with
ÂÂÂÂÂ DRM_IOCTL_PRIME_FD_TO_HANDLE
ÂÂÂ - issues DRM_XEN_ZCOPY_DUMB_TO_REFS ioctl to
ÂÂÂÂÂ grant references to the buffer's memory.
ÂÂÂ - passes grant references to the frontend
Â- at the end:
ÂÂÂ - closes zero-copy driver's handle with DRM_IOCTL_GEM_CLOSE
ÂÂÂ - closes real HW driver's handle with DRM_IOCTL_GEM_CLOSE
ÂÂÂ - closes file descriptor of the imported buffer

3. DRM_XEN_ZCOPY_DUMB_WAIT_FREE
This will block until the dumb buffer with the wait handle provided be
freed:
this is needed for synchronization between frontend and backend in case
frontend provides grant references of the buffer via
DRM_XEN_ZCOPY_DUMB_FROM_REFS IOCTL and which must be released before
backend replies with XENDISPL_OP_DBUF_DESTROY response.
wait_handle must be the same value returned while calling
DRM_XEN_ZCOPY_DUMB_FROM_REFS IOCTL.

So, as you can see the above functionality is not covered by the
existing UAPI
of the gntdev driver.
Now, if we change dumb -> dma-buf and remove DRM code (which is only a
wrapper
here on top of dma-buf) we get new driver for dma-buf for Xen.

This is why I have 2 options here: either create a dedicated driver for
this
(e.g. re-work xen-zcopy to be DRM independent and put it under
drivers/xen/xen-dma-buf, for example) or extend the existing gntdev driver
with the above UAPI + make changes to the balloon driver to provide kernel
API for DMA buffer allocations.
Which user component would use the new ioctls?
It is currently used by the display backend [1] and will
probably be used by the hyper-dmabuf frontend/backend
(Dongwon from Intel can provide more info on this).

I'm asking because I'm not very fond of adding more linux specific
functions to libgnttab which are not related to a specific Xen version,
but to a kernel version.
Hm, I was not thinking about this UAPI to be added to libgnttab.
It seems it can be used directly w/o wrappers in user-space

So doing this in a separate driver seems to be the better option in
this regard.
Well, from maintenance POV it is easier for me to have it all in
a separate driver as all dma-buf related functionality will
reside at one place. This also means that no changes to existing
drivers will be needed (if it is ok to have ballooning in/out
code for DMA buffers (allocated with dma_alloc_xxx) not in the balloon driver)

Juergen
[1] https://github.com/xen-troops/displ_be