[v14,5/7] vfio: ABI for mdev display dma-buf operation

Commit Message

Zhang, Tina Sept. 27, 2017, 9:03 a.m. UTC

It's really tricky to handle the cached dmabuf_obj's life-cycle without touching other kernel modules (e.g. i915 or dmabuf).The proposed two ioctls will be helpful.

So, there is a problem about the releasing cached dmabuf_obj. We cannot rely on the drm_i915_gem_object_ops.release() to release the cached dmabuf_obj,
as this release operation is running in another thread, which leads to a racing condition and tricky to be solved without touching other modules.

So, in order to solve that kind of problem, I’d like to add one more ioctl, which is used for user mode to close the dmabuf_obj. 

Including the proposed two ioctls,  the incremental patch is like this:


2. VFIO_DEVICE_GET_GFX_DMABUF: is used for user mode to ask kernel part which interface it likes the buffer to be wrapped.
Actually, I think there could be a bunch of types, including DMBUF type.
So, maybe we can change the IOCTL's name to some generic name and use flags field to distinguish them.

In dmabuf case, a new dmabuf will be created each time with this ioctl invoked, and installed with a new fd.
The dmabuf is just a wrapper of kernel dmabuf_obj distinguished by dmabuf_id. So there could be more
than one dmabuf as the wrappers of the same dmabuf_obj, if this ioctl was invoked with the same dmabuf_id
many times. The dmabuf and its fd is fully controlled by user mode. And the VFIO_DEVICE_CLOSE_BUF can
guarantee the referenced dmabuf_obj will be released at last, after all the dmabufs are released.

3. VFIO_DEVICE_CLOSE_BUF: is used for user mode to tell kernel part to release that buffer.
In dmabuf case, this ioctl won't release the dmabuf_obj at once. It just decreases the reference count of the dmabuf_obj
and make sure this dmabuf_obj won't be reused through VFIO_DEVICE_QUERY_GFX_PLANE again. At last, after all the
referencing dmabuf are released by user mode, the dmabuf_obj will be released by kernel.

Thanks,

BR,
Tina

> -----Original Message-----
> From: Gerd Hoffmann [mailto:[e-mail]]
> Sent: Tuesday, September 26, 2017 3:12 PM
> To: Zhang, Tina <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; Alex
> Williamson <[e-mail]>; Daniel Vetter
> <[e-mail]>
> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> [ bringing a private discussion back to the list ]
> 
> > The dma-buf's life cycle is handled by user mode and tracked by
> > kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host user mode can check the
> > value of fd and to see if it needs to create new resource according to
> > the new fd or just use the existed resource related to the old fd.
> 
> Ok, this idea has a fundamental flaw:  The life cycle of the dma-buf and the file
> handle are not identical.  The dma-buf can exist longer than the file handle in
> case other references to the dma-buf exist.  So when trying to use the file handle
> as identifier for the dma-buf you'll end up with all sorts of strange effects.
> 
> So, I'd suggest to use a id instead, and add a ioctl to get a dmabuf for a given id
> (incremental patch):
> 
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -538,12 +538,22 @@ struct vfio_device_gfx_plane_info {
>         __u32 y_pos;    /* vertical position of cursor plane, upper left corner in
> lines*/
>         union {
>                 __u32 region_index;     /* region index */
> -               __s32 fd;       /* dma-buf fd */
> +               __u32 dmabuf_id;        /* dma-buf id */
>         };
>  };
> 
>  #define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> 
> +struct vfio_device_gfx_dmabuf_fd {
> +       __u32 argsz;
> +       __u32 flags;
> +       /* in */
> +       __u32 dmabuf_id;
> +       /* out */
> +       __s32 dmabuf_fd;
> +};
> +
> +#define VFIO_DEVICE_GET_GFX_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 15)
> 
>  /* -------- API for Type1 VFIO IOMMU -------- */
> 
> [ no changes for a region-based display ]
> 
> git branch, kernel, with updated dmabuf patch:
>   https://www.kraxel.org/cgit/linux/log/?h=gvt-dmabuf-v14
> 
> qemu branch:
>   https://www.kraxel.org/cgit/qemu/log/?h=work/intel-vgpu
> 
> cheers,
>   Gerd

Comments

Gerd Hoffmann Sept. 27, 2017, 10:11 a.m. UTC | #1

Hi,

> So, there is a problem about the releasing cached dmabuf_obj. We
> cannot rely on the drm_i915_gem_object_ops.release() to release the
> cached dmabuf_obj,
> as this release operation is running in another thread, which leads
> to a racing condition and tricky to be solved without touching other
> modules.

PLANE_INFO just creates a intel_vgpu_dmabuf_obj.

GET_DMABUF creates a fresh proxy gem object and dmabuf.

proxy gem object references intel_vgpu_dmabuf_obj but not the other way
around.  Then you can simply refcount intel_vgpu_dmabuf_obj and be done
with it.

https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14&id=350a0e834
971e6f53d7235d8b6167bed4dccf074

Note: Patch renamed intel_vgpu_dmabuf_obj to intel_vgpu_fb_obj, because
it doesn't refer to dmabufs any more.  It basically carries the guest
plane/framebuffer information and the ID associated with it.

> So, in order to solve that kind of problem, I’d like to add one more
> ioctl, which is used for user mode to close the dmabuf_obj. 

Depending on userspace notifying the kernel for that kind of cleanups
is a bad idea.  What happens in case userspace crashes?  Do you leak
dmabufs then?

cheers,
  Gerd

Zhang, Tina Sept. 28, 2017, 11:43 p.m. UTC | #2

Thanks for the patch. Actually, I did the same thing in my local repo and also, I have a patch for the local Qemu repo to test it. I will send them out later.

The reason why I want to propose the close IOCTL is because that the current lock (fb_obj_list_lock), cannot sync the intel_vgpu_fb_info releasing and reusing.
You see, the intel_vgpu_fb_info reusing and releasing are in different threads. There is a case that intel_vgpu_find_dmabuf can return a intel_vgpu_fb_obj, while the intel_vgpu_fb_obj
is on the way to be released. That's the problem.

The invoker of the close IOCTL is only Qemu. So, if the Qemu crashes, the whole vGPU's resource is going to be released. We can handle our dmabuf_obj to be released there.

Thanks.

BR,
Tina

> -----Original Message-----

> From: intel-gvt-dev [mailto:[e-mail]] On

> Behalf Of Gerd Hoffmann

> Sent: Wednesday, September 27, 2017 6:11 PM

> To: Zhang, Tina <[e-mail]>; [e-mail]; Wang, Zhi

> A <[e-mail]>; Tian, Kevin <[e-mail]>

> Cc: Daniel Vetter <[e-mail]>; [e-mail];

> [e-mail]; Alex Williamson

> <[e-mail]>; Lv, Zhiyuan <[e-mail]>

> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

> 

>   Hi,

> 

> > So, there is a problem about the releasing cached dmabuf_obj. We

> > cannot rely on the drm_i915_gem_object_ops.release() to release the

> > cached dmabuf_obj, as this release operation is running in another

> > thread, which leads to a racing condition and tricky to be solved

> > without touching other modules.

> 

> PLANE_INFO just creates a intel_vgpu_dmabuf_obj.

> 

> GET_DMABUF creates a fresh proxy gem object and dmabuf.

> 

> proxy gem object references intel_vgpu_dmabuf_obj but not the other way

> around.  Then you can simply refcount intel_vgpu_dmabuf_obj and be done

> with it.

> 

> https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14&id=350a0e834

> 971e6f53d7235d8b6167bed4dccf074

> 

> Note: Patch renamed intel_vgpu_dmabuf_obj to intel_vgpu_fb_obj, because it

> doesn't refer to dmabufs any more.  It basically carries the guest

> plane/framebuffer information and the ID associated with it.

> 

> > So, in order to solve that kind of problem, I’d like to add one more

> > ioctl, which is used for user mode to close the dmabuf_obj.

> 

> Depending on userspace notifying the kernel for that kind of cleanups is a bad

> idea.  What happens in case userspace crashes?  Do you leak dmabufs then?

> 

> cheers,

>   Gerd

> 

> _______________________________________________

> intel-gvt-dev mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Zhang, Tina Sept. 29, 2017, 7:04 a.m. UTC | #3

So, there won't be dmabuf leaking problem, as we release all the dmabuf_obj in the release ops when user space crashing.

Can we just stop considering the way to fix the dmabuf life-cycle issue and try to just consider the generic way to handle buffer exposing?
Does the generic way need the close ioctl?
In my opinion, it's like to build up a producer-consumer way to expose the buffer:

	                        Create buffer and return its info
Mdev devices  -----------------------------------------------------> User space
                           <----------------------------------------------------- 
(producer)                              Close it                                                 (consumer)

Alex and Gerd, can you share your thoughts?
Thanks.


BR,
Tina

> -----Original Message-----

> From: Zhang, Tina

> Sent: Friday, September 29, 2017 7:43 AM

> To: 'Gerd Hoffmann' <[e-mail]>; [e-mail]; Wang,

> Zhi A <[e-mail]>; Tian, Kevin <[e-mail]>

> Cc: Daniel Vetter <[e-mail]>; [e-mail];

> [e-mail]; Alex Williamson

> <[e-mail]>; Lv, Zhiyuan <[e-mail]>

> Subject: RE: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

> 

> Thanks for the patch. Actually, I did the same thing in my local repo and also, I

> have a patch for the local Qemu repo to test it. I will send them out later.

> 

> The reason why I want to propose the close IOCTL is because that the current

> lock (fb_obj_list_lock), cannot sync the intel_vgpu_fb_info releasing and

> reusing.

> You see, the intel_vgpu_fb_info reusing and releasing are in different threads.

> There is a case that intel_vgpu_find_dmabuf can return a intel_vgpu_fb_obj,

> while the intel_vgpu_fb_obj is on the way to be released. That's the problem.

> 

> The invoker of the close IOCTL is only Qemu. So, if the Qemu crashes, the whole

> vGPU's resource is going to be released. We can handle our dmabuf_obj to be

> released there.

> 

> Thanks.

> 

> BR,

> Tina

> 

> > -----Original Message-----

> > From: intel-gvt-dev

> > [mailto:[e-mail]] On Behalf Of Gerd

> > Hoffmann

> > Sent: Wednesday, September 27, 2017 6:11 PM

> > To: Zhang, Tina <[e-mail]>; [e-mail]; Wang,

> > Zhi A <[e-mail]>; Tian, Kevin <[e-mail]>

> > Cc: Daniel Vetter <[e-mail]>;

> > [e-mail]; [e-mail];

> > Alex Williamson <[e-mail]>; Lv, Zhiyuan

> > <[e-mail]>

> > Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf

> > operation

> >

> >   Hi,

> >

> > > So, there is a problem about the releasing cached dmabuf_obj. We

> > > cannot rely on the drm_i915_gem_object_ops.release() to release the

> > > cached dmabuf_obj, as this release operation is running in another

> > > thread, which leads to a racing condition and tricky to be solved

> > > without touching other modules.

> >

> > PLANE_INFO just creates a intel_vgpu_dmabuf_obj.

> >

> > GET_DMABUF creates a fresh proxy gem object and dmabuf.

> >

> > proxy gem object references intel_vgpu_dmabuf_obj but not the other

> > way around.  Then you can simply refcount intel_vgpu_dmabuf_obj and be

> > done with it.

> >

> > https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-

> v14&id=350a0e83

> > 4

> > 971e6f53d7235d8b6167bed4dccf074

> >

> > Note: Patch renamed intel_vgpu_dmabuf_obj to intel_vgpu_fb_obj,

> > because it doesn't refer to dmabufs any more.  It basically carries

> > the guest plane/framebuffer information and the ID associated with it.

> >

> > > So, in order to solve that kind of problem, I’d like to add one more

> > > ioctl, which is used for user mode to close the dmabuf_obj.

> >

> > Depending on userspace notifying the kernel for that kind of cleanups

> > is a bad idea.  What happens in case userspace crashes?  Do you leak dmabufs

> then?

> >

> > cheers,

> >   Gerd

> >

> > _______________________________________________

> > intel-gvt-dev mailing list

> > [e-mail]

> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Gerd Hoffmann Sept. 29, 2017, 7:28 a.m. UTC | #4

On Fri, 2017-09-29 at 07:04 +0000, Zhang, Tina wrote:
> So, there won't be dmabuf leaking problem, as we release all the
> dmabuf_obj in the release ops when user space crashing.
> 
> Can we just stop considering the way to fix the dmabuf life-cycle
> issue and try to just consider the generic way to handle buffer
> exposing?

Can you describe in more detail what you have in mind?

> Does the generic way need the close ioctl?

I think we don't need a close ioctl anyway.

cheers,
  Gerd

Zhang, Tina Sept. 29, 2017, 7:49 a.m. UTC | #5

> -----Original Message-----

> From: Gerd Hoffmann [mailto:[e-mail]]

> Sent: Friday, September 29, 2017 3:29 PM

> To: Zhang, Tina <[e-mail]>; [e-mail]; Wang, Zhi

> A <[e-mail]>; Tian, Kevin <[e-mail]>; Alex

> Williamson <[e-mail]>

> Cc: Daniel Vetter <[e-mail]>; [e-mail];

> [e-mail]; Lv, Zhiyuan <[e-mail]>

> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

> 

> On Fri, 2017-09-29 at 07:04 +0000, Zhang, Tina wrote:

> > So, there won't be dmabuf leaking problem, as we release all the

> > dmabuf_obj in the release ops when user space crashing.

> >

> > Can we just stop considering the way to fix the dmabuf life-cycle

> > issue and try to just consider the generic way to handle buffer

> > exposing?

> 

> Can you describe in more detail what you have in mind?

> 

> > Does the generic way need the close ioctl?

> 

> I think we don't need a close ioctl anyway.

Can you share your thoughts?
Do you think the fd interface is enough for all kinds of buffer exposed by Mdev?
Thanks.

BR,
Tina

> 

> cheers,

>   Gerd

>

Gerd Hoffmann Sept. 29, 2017, 8:03 a.m. UTC | #6

Hi,

> > > Does the generic way need the close ioctl?
> > 
> > I think we don't need a close ioctl anyway.
> 
> Can you share your thoughts?

See other mail.  I think the race can be fixed by changing the locking,
so a explicit close ioctl isn't needed.

> Do you think the fd interface is enough for all kinds of buffer
> exposed by Mdev?

What kind of buffers do you have in mind which might not be covered?

cheers,
  Gerd

Zhang, Tina Sept. 29, 2017, 9:08 a.m. UTC | #7

> -----Original Message-----

> From: Gerd Hoffmann [mailto:[e-mail]]

> Sent: Friday, September 29, 2017 4:03 PM

> To: Zhang, Tina <[e-mail]>; [e-mail]; Wang, Zhi

> A <[e-mail]>; Tian, Kevin <[e-mail]>; Alex

> Williamson <[e-mail]>

> Cc: Daniel Vetter <[e-mail]>; [e-mail];

> [e-mail]; Lv, Zhiyuan <[e-mail]>

> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

> 

>   Hi,

> 

> > > > Does the generic way need the close ioctl?

> > >

> > > I think we don't need a close ioctl anyway.

> >

> > Can you share your thoughts?

> 

> See other mail.  I think the race can be fixed by changing the locking, so a explicit

> close ioctl isn't needed.

Yeah, I understand your idea. But unfortunately, it cannot solve the current issue.
There will still be a racing condition between releasing dmabuf_obj and reusing it.
For example, if the old reused dmabuf_obj is released just after query ioctl return it,  the next get_fd ioctl would
return error as the dmabuf_obj has already been closed.

> 

> > Do you think the fd interface is enough for all kinds of buffer

> > exposed by Mdev?

> 

> What kind of buffers do you have in mind which might not be covered?

I thinking about the case that would like to postpone the buffers releasing operation, after user space has closed all the fd.
Later these buffers may be used to expose to other kinds of fd to user space.
Thanks.

BR,
Tina
> 

> cheers,

>   Gerd

Gerd Hoffmann Sept. 29, 2017, 10:20 a.m. UTC | #8

Hi,

> For example, if the old reused dmabuf_obj is released just after
> query ioctl return it,  the next get_fd ioctl would
> return error as the dmabuf_obj has already been closed.

My branch already grabs an extra reference when creating a new
dmabuf_obj, which will be dropped on GET_DMABUF ioctl, exactly to avoid
the dmabuf_obj disappear between QUERY_PLANE and GET_DMABUF ioctls.

Can easily be extended to handle the reuse case too.

https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14&id=9959109ae
52cf15e119715a6b7de080fb849e3d2

While being at it also cleanup properly on close (so we don't leak
structs in case userspace never calls GET_DMABUF for a plane).

https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14&id=c0b0c407e
33904e749dec1ef44ec01099c16d39f

> > > Do you think the fd interface is enough for all kinds of buffer
> > > exposed by Mdev?
> > 
> > What kind of buffers do you have in mind which might not be
> > covered?
> 
> I thinking about the case that would like to postpone the buffers
> releasing operation, after user space has closed all the fd.

Work fine.  qemu can import the dma-buf as opengl texture, which
creates a extra reference.  Then close the fd.  dma-buf continues to
exist as long as the texture referencing it exists.

> Later these buffers may be used to expose to other kinds of fd to
> user space.

Sorry, I don't understand that sentence.

cheers,
  Gerd

Zhang, Tina Sept. 29, 2017, 11:59 p.m. UTC | #9

> -----Original Message-----

> From: Gerd Hoffmann [mailto:[e-mail]]

> Sent: Friday, September 29, 2017 6:21 PM

> To: Zhang, Tina <[e-mail]>; [e-mail]; Wang, Zhi

> A <[e-mail]>; Tian, Kevin <[e-mail]>; Alex

> Williamson <[e-mail]>

> Cc: Daniel Vetter <[e-mail]>; [e-mail];

> [e-mail]; Lv, Zhiyuan <[e-mail]>

> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

> 

>   Hi,

> 

> > For example, if the old reused dmabuf_obj is released just after query

> > ioctl return it,  the next get_fd ioctl would return error as the

> > dmabuf_obj has already been closed.

> 

> My branch already grabs an extra reference when creating a new dmabuf_obj,

> which will be dropped on GET_DMABUF ioctl, exactly to avoid the dmabuf_obj

> disappear between QUERY_PLANE and GET_DMABUF ioctls.

Yeah, that could solve the problem. But I'm not sure if it could be acceptable.
Zhenyu, can you share your comments?
Thanks.

BR,
Tina

> 

> Can easily be extended to handle the reuse case too.

> 

> https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14&id=9959109ae

> 52cf15e119715a6b7de080fb849e3d2

> 

> While being at it also cleanup properly on close (so we don't leak structs in case

> userspace never calls GET_DMABUF for a plane).

> 

> https://www.kraxel.org/cgit/linux/commit/?h=gvt-dmabuf-v14&id=c0b0c407e

> 33904e749dec1ef44ec01099c16d39f

> 

> > > > Do you think the fd interface is enough for all kinds of buffer

> > > > exposed by Mdev?

> > >

> > > What kind of buffers do you have in mind which might not be covered?

> >

> > I thinking about the case that would like to postpone the buffers

> > releasing operation, after user space has closed all the fd.

> 

> Work fine.  qemu can import the dma-buf as opengl texture, which creates a

> extra reference.  Then close the fd.  dma-buf continues to exist as long as the

> texture referencing it exists.

> 

> > Later these buffers may be used to expose to other kinds of fd to user

> > space.

> 

> Sorry, I don't understand that sentence.

> 

> cheers,

>   Gerd

Gerd Hoffmann Oct. 6, 2017, 12:12 p.m. UTC | #10

Hi,

> Yeah, that could solve the problem. But I'm not sure if it could be
> acceptable.
> Zhenyu, can you share your comments?

Ping?  Any progress here?  We are at 4.14-rc3 already.  v15 is needed
really soon now otherwise the 4.15 merge window will be missed.

cheers,
  Gerd

Zhang, Tina Oct. 7, 2017, 7:39 a.m. UTC | #11

> -----Original Message-----

> From: Gerd Hoffmann [mailto:[e-mail]]

> Sent: Friday, October 6, 2017 8:13 PM

> To: Zhang, Tina <[e-mail]>; [e-mail]; Wang, Zhi

> A <[e-mail]>; Tian, Kevin <[e-mail]>; Alex

> Williamson <[e-mail]>

> Cc: Daniel Vetter <[e-mail]>; [e-mail];

> [e-mail]; Lv, Zhiyuan <[e-mail]>

> Subject: Re: [PATCH v14 5/7] vfio: ABI for mdev display dma-buf operation

> 

>   Hi,

> 

> > Yeah, that could solve the problem. But I'm not sure if it could be

> > acceptable.

> > Zhenyu, can you share your comments?

> 

> Ping?  Any progress here?  We are at 4.14-rc3 already.  v15 is needed really

> soon now otherwise the 4.15 merge window will be missed.

V15 will be submitted next week, after discussion with maintainers.
Thanks.

BR,
Tina
> 

> cheers,

>   Gerd

Patch

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index bf40f7b..6aa6860 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -538,12 +538,33 @@  struct vfio_device_gfx_plane_info {
        __u32 y_pos;    /* vertical position of cursor plane, upper left corner in lines*/
        union {
                __u32 region_index;     /* region index */
-               __s32 fd;       /* dma-buf fd */
+               __s32 dmabuf_id;        /* dma-buf fd */
        };
 };

 #define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)

+struct vfio_device_gfx_dmabuf_fd {
+         __u32 argsz;
+         __u32 flags;
+         /* in */
+        __u32 dmabuf_id;
+        /* out */
+        __s32 dmabuf_fd;
+};
+
+#define VFIO_DEVICE_GET_GFX_DMABUF _IO(VFIO_TYPE, VFIO_BASE + 15)
+
+
+struct vfio_device_gfx_buffer {
+        __u32 argsz;
+        __u32 flags;
+        /* in */
+        __u32 id;
+};
+
+#define VFIO_DEVICE_CLOSE_BUF _IO(VFIO_TYPE, VFIO_BASE + 16)

And here are some details:

1. VFIO_DEVICE_QUERY_GFX_PLANE: is used for user mode to ask kernel part to create a buffer (in dmabuf case is dmabuf_obj in kernel) and return the buffer info.