[virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

This patch adds a new virtio feature for shared resources.

Shared resources are allocated by the guest from normal ram, like
traditional resources.  They can be used by the guest almost like
traditional resources, the only exception is that shared resources can
only be used with backing storage attached (the linux driver does that
anyway so the workflow doesn't change).  The host can map these
resources and use them directly (using udmabuf), so any guest changes to
these resources can be visible on the host without explicit transfer
request.  Transfer requests are still needed though as the host might
still have syncronize the resource, for example in case importing the
udmabuf failed for some reason.

guest/host interface: Two new commands are added to create 2D and 3D
shared resources.  They work exactly like the non-shared counterparts.

qemu patches:
  https://git.kraxel.org/cgit/qemu/log/?h=sirius/virtio-gpu-feature-shared

linux patches:
  https://git.kraxel.org/cgit/linux/log/?h=drm-virtio-feature-shared

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 virtio-gpu.tex | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/virtio-gpu.tex b/virtio-gpu.tex
index 15dbf9f2ec82..ca27b3d5fa45 100644
--- a/virtio-gpu.tex
+++ b/virtio-gpu.tex
@@ -35,6 +35,7 @@ \subsection{Feature bits}\label{sec:Device Types / GPU Device / Feature bits}
 \begin{description}
 \item[VIRTIO_GPU_F_VIRGL (0)] virgl 3D mode is supported.
 \item[VIRTIO_GPU_F_EDID  (1)] EDID is supported.
+\item[VIRTIO_GPU_F_RESOURCE_SHARED  2)] shared resources are supported.
 \end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / GPU Device / Device configuration layout}
@@ -103,6 +104,8 @@ \subsubsection{Device Operation: Create a framebuffer and configure scanout}
 
 \begin{itemize*}
 \item Create a host resource using VIRTIO_GPU_CMD_RESOURCE_CREATE_2D.
+  In case VIRTIO_GPU_F_RESOURCE_SHARED is negotiated the driver can
+  also use VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED.
 \item Allocate a framebuffer from guest ram, and attach it as backing
   storage to the resource just created, using
   VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING.  Scatter lists are
@@ -166,6 +169,28 @@ \subsubsection{Device Operation: Configure mouse cursor}
 the pointer shape and position.  To move the pointer without updating
 the shape use VIRTIO_GPU_CMD_MOVE_CURSOR instead.
 
+\subsubsection{Device Operation: Shared resources}
+
+In case VIRTIO_GPU_F_RESOURCE_SHARED is negotiated the driver can use
+the VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED command to create shared
+resources.  Normal resources have both a guest buffer and host buffer
+buffer for the resource and the VIRTIO_GPU_CMD_TRANSFER_* commands are
+used to transfer data between host and guest.  Shared (guest
+allocated) buffers CAN be used directly by the host, to remove or
+reduce the data copies needed.
+
+The driver MUST attach backing storage to a shared resource before
+using it.  Any changes on the shared resource MAY be instantly visible
+on the host.
+
+Otherwise the shared resources are used like normal resources.
+Especially the driver must send explicit VIRTIO_GPU_CMD_TRANSFER_*
+commands to the device for both normal and shared resources.  Reason:
+The device might have to flush caches.  The device MAY also choose to
+not create mapping for the shared resource.  Especially for small
+resources it might be more efficient to just copy the data instead of
+establishing a shared mapping.
+
 \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: Request header}
 
 All requests and responses on the virt queues have a fixed header
@@ -186,6 +211,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
         VIRTIO_GPU_CMD_GET_CAPSET_INFO,
         VIRTIO_GPU_CMD_GET_CAPSET,
         VIRTIO_GPU_CMD_GET_EDID,
+        VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED,
 
         /* cursor commands */
         VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
@@ -205,6 +231,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
         VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID,
         VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID,
         VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER,
+        VIRTIO_GPU_RESP_ERR_NO_BACKING_STORAGE,
 };
 
 #define VIRTIO_GPU_FLAG_FENCE (1 << 0)
-- 
2.18.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Matti Moell]

Hi Gerd,

I really like this!

On 28.11.19 14:19, Gerd Hoffmann wrote:
> @@ -186,6 +211,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
>           VIRTIO_GPU_CMD_GET_CAPSET_INFO,
>           VIRTIO_GPU_CMD_GET_CAPSET,
>           VIRTIO_GPU_CMD_GET_EDID,
> +        VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED,
Shouldn't there also the RESOURCE_CREATE_3D_SHARED? I know that 3D isn't 
speced here but it looks awkward not to reserve it.

>   
>           /* cursor commands */
>           VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
> @@ -205,6 +231,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
>           VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID,
>           VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID,
>           VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER,
> +        VIRTIO_GPU_RESP_ERR_NO_BACKING_STORAGE,
>   };
>   
>   #define VIRTIO_GPU_FLAG_FENCE (1 << 0)
> 

Cheers,

		Matti

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Laszlo Ersek]

On 11/28/19 14:19, Gerd Hoffmann wrote:

> +resources.  Normal resources have both a guest buffer and host buffer
> +buffer for the resource and the VIRTIO_GPU_CMD_TRANSFER_* commands are

typo: "buffer\nbuffer"

Thanks
Laszlo


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

On Thu, Nov 28, 2019 at 07:04:23PM +0100, Matti Moell wrote:
> Hi Gerd,
> 
> I really like this!
> 
> On 28.11.19 14:19, Gerd Hoffmann wrote:
> > @@ -186,6 +211,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
> >           VIRTIO_GPU_CMD_GET_CAPSET_INFO,
> >           VIRTIO_GPU_CMD_GET_CAPSET,
> >           VIRTIO_GPU_CMD_GET_EDID,
> > +        VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED,
> Shouldn't there also the RESOURCE_CREATE_3D_SHARED? I know that 3D isn't
> speced here but it looks awkward not to reserve it.

Yep, none of the 3D commands is in the specs, thats why
RESOURCE_CREATE_3D_SHARED isn't there too (but, yes, it exists).

Guess it might be a good idea to at least list the commands,
but that is something for a separate patch ...

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Frank Yang]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

Interesting; what's the use case for this? Do we already have a mechanism
to import memory to virtio-gpu via importing a bo handle from somewhere
else?

On Thu, Nov 28, 2019 at 9:37 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Thu, Nov 28, 2019 at 07:04:23PM +0100, Matti Moell wrote:
> > Hi Gerd,
> >
> > I really like this!
> >
> > On 28.11.19 14:19, Gerd Hoffmann wrote:
> > > @@ -186,6 +211,7 @@ \subsubsection{Device Operation: Request
> header}\label{sec:Device Types / GPU De
> > >           VIRTIO_GPU_CMD_GET_CAPSET_INFO,
> > >           VIRTIO_GPU_CMD_GET_CAPSET,
> > >           VIRTIO_GPU_CMD_GET_EDID,
> > > +        VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED,
> > Shouldn't there also the RESOURCE_CREATE_3D_SHARED? I know that 3D isn't
> > speced here but it looks awkward not to reserve it.
>
> Yep, none of the 3D commands is in the specs, thats why
> RESOURCE_CREATE_3D_SHARED isn't there too (but, yes, it exists).
>
> Guess it might be a good idea to at least list the commands,
> but that is something for a separate patch ...
>
> cheers,
>   Gerd
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

[-- Attachment #2: Type: text/html, Size: 1904 bytes --]

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

On Mon, Dec 02, 2019 at 09:24:31AM -0800, Frank Yang wrote:
> Interesting; what's the use case for this?

Mostly reduce data copying.

> Do we already have a mechanism
> to import memory to virtio-gpu via importing a bo handle from somewhere
> else?

Inside the guest?  Import is not working yet, that needs another update
(blob resources) due to current virtio-gpu resources requiring a format
at creation time and dmabufs not having one.

See https://www.kraxel.org/blog/2019/11/virtio-gpu-status-and-plans/ for
some background.

Exporting virtio-gpu resources works in the drm-misc-next branch, should
land in mainline this merge window (i.e. 5.5-rc1).

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Frank Yang]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On Tue, Dec 3, 2019 at 1:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mon, Dec 02, 2019 at 09:24:31AM -0800, Frank Yang wrote:
> > Interesting; what's the use case for this?
>
> Mostly reduce data copying.
>
> > Do we already have a mechanism
> > to import memory to virtio-gpu via importing a bo handle from somewhere
> > else?
>
> Inside the guest?  Import is not working yet, that needs another update
> (blob resources) due to current virtio-gpu resources requiring a format
> at creation time and dmabufs not having one.
>
>
Gotcha, I was just poking around related code and it looked like I could
use a resource w/ R8 format and width = bytes. Blob resources would be a
more proper way then :)


> See https://www.kraxel.org/blog/2019/11/virtio-gpu-status-and-plans/ for
> some background.
>
> Exporting virtio-gpu resources works in the drm-misc-next branch, should
> land in mainline this merge window (i.e. 5.5-rc1).
>
> cheers,
>   Gerd
>
>

[-- Attachment #2: Type: text/html, Size: 1632 bytes --]

Re: [virtio-dev] [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

  Hi,

> > > Do we already have a mechanism
> > > to import memory to virtio-gpu via importing a bo handle from somewhere
> > > else?
> >
> > Inside the guest?  Import is not working yet, that needs another update
> > (blob resources) due to current virtio-gpu resources requiring a format
> > at creation time and dmabufs not having one.
> >
> >
> Gotcha, I was just poking around related code and it looked like I could
> use a resource w/ R8 format and width = bytes.

Yep, that trick might work for some use cases.  But mapping such an
imported dma-buf to a scanout isn't going to work for example ...

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH] virtio-gpu: add shared resource feature

[Gurchetan Singh]

On Thu, Nov 28, 2019 at 5:20 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> This patch adds a new virtio feature for shared resources.
>
> Shared resources are allocated by the guest from normal ram, like
> traditional resources.  They can be used by the guest almost like
> traditional resources, the only exception is that shared resources can
> only be used with backing storage attached (the linux driver does that
> anyway so the workflow doesn't change).  The host can map these
> resources and use them directly (using udmabuf), so any guest changes to
> these resources can be visible on the host without explicit transfer
> request.  Transfer requests are still needed though as the host might
> still have syncronize the resource, for example in case importing the
> udmabuf failed for some reason.
>
> guest/host interface: Two new commands are added to create 2D and 3D
> shared resources.  They work exactly like the non-shared counterparts.
>
> qemu patches:
>   https://git.kraxel.org/cgit/qemu/log/?h=sirius/virtio-gpu-feature-shared
>
> linux patches:
>   https://git.kraxel.org/cgit/linux/log/?h=drm-virtio-feature-shared
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  virtio-gpu.tex | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/virtio-gpu.tex b/virtio-gpu.tex
> index 15dbf9f2ec82..ca27b3d5fa45 100644
> --- a/virtio-gpu.tex
> +++ b/virtio-gpu.tex
> @@ -35,6 +35,7 @@ \subsection{Feature bits}\label{sec:Device Types / GPU Device / Feature bits}
>  \begin{description}
>  \item[VIRTIO_GPU_F_VIRGL (0)] virgl 3D mode is supported.
>  \item[VIRTIO_GPU_F_EDID  (1)] EDID is supported.
> +\item[VIRTIO_GPU_F_RESOURCE_SHARED  2)] shared resources are supported.
>  \end{description}
>
>  \subsection{Device configuration layout}\label{sec:Device Types / GPU Device / Device configuration layout}
> @@ -103,6 +104,8 @@ \subsubsection{Device Operation: Create a framebuffer and configure scanout}
>
>  \begin{itemize*}
>  \item Create a host resource using VIRTIO_GPU_CMD_RESOURCE_CREATE_2D.
> +  In case VIRTIO_GPU_F_RESOURCE_SHARED is negotiated the driver can
> +  also use VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED.
>  \item Allocate a framebuffer from guest ram, and attach it as backing
>    storage to the resource just created, using
>    VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING.  Scatter lists are
> @@ -166,6 +169,28 @@ \subsubsection{Device Operation: Configure mouse cursor}
>  the pointer shape and position.  To move the pointer without updating
>  the shape use VIRTIO_GPU_CMD_MOVE_CURSOR instead.
>
> +\subsubsection{Device Operation: Shared resources}
> +
> +In case VIRTIO_GPU_F_RESOURCE_SHARED is negotiated the driver can use
> +the VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED command to create shared
> +resources.  Normal resources have both a guest buffer and host buffer
> +buffer for the resource and the VIRTIO_GPU_CMD_TRANSFER_* commands are
> +used to transfer data between host and guest.  Shared (guest
> +allocated) buffers CAN be used directly by the host, to remove or
> +reduce the data copies needed.
> +
> +The driver MUST attach backing storage to a shared resource before
> +using it.  Any changes on the shared resource MAY be instantly visible
> +on the host.

If the following scenario happens:

1) Driver sends RESOURCE_CREATE_2D shared request
2) Driver sends ATTACH_BACKING request
3) Device creates a shared resource
4) Driver sends SET_SCANOUT request
5) Device sends shared resource to display
6) Driver sends DETACH_BACKING request

How do we synchronize between (5) and (6)?  Is there a guarantee the
shared pages won't go back to guest RAM, and will be owned by the
display (like traditional dma-bufs)?

> +
> +Otherwise the shared resources are used like normal resources.
> +Especially the driver must send explicit VIRTIO_GPU_CMD_TRANSFER_*
> +commands to the device for both normal and shared resources.  Reason:
> +The device might have to flush caches.

Can we make this spec stronger to avoid to avoid transfers all the
time (i.e, in virtio_gpu_update_dumb_bo)?

drm_gem_shmem_* helpers seem to use WC buffers, and dumb buffers are
traditionally WC as well.  If we mandate the host ("device?" here)
must properly clean caches at creation time, it may be possible to
avoid hypercalls for 2D_SHARED resources.

> The device MAY also choose to
> +not create mapping for the shared resource.  Especially for small
> +resources it might be more efficient to just copy the data instead of
> +establishing a shared mapping.

Should the device send a response code  (OK_SHARED} to inform the
driver that the resource is shared?  If the device can choose not to
create shared mappings, would the response code +
VIRTIO_GPU_CMD_RESOURCE_CREATE_2D + the feature flag suffice (i.e, no
VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED)?

> +
>  \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: Request header}
>
>  All requests and responses on the virt queues have a fixed header
> @@ -186,6 +211,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
>          VIRTIO_GPU_CMD_GET_CAPSET_INFO,
>          VIRTIO_GPU_CMD_GET_CAPSET,
>          VIRTIO_GPU_CMD_GET_EDID,
> +        VIRTIO_GPU_CMD_RESOURCE_CREATE_2D_SHARED,
>
>          /* cursor commands */
>          VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
> @@ -205,6 +231,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
>          VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID,
>          VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID,
>          VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER,
> +        VIRTIO_GPU_RESP_ERR_NO_BACKING_STORAGE,
>  };
>
>  #define VIRTIO_GPU_FLAG_FENCE (1 << 0)
> --
> 2.18.1
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

  Hi,

> If the following scenario happens:
> 
> 1) Driver sends RESOURCE_CREATE_2D shared request
> 2) Driver sends ATTACH_BACKING request
> 3) Device creates a shared resource
> 4) Driver sends SET_SCANOUT request
> 5) Device sends shared resource to display
> 6) Driver sends DETACH_BACKING request

Hmm, I think you can crash the current qemu code that way (didn't test
though).  I think the proper solution would be to return -EBUSY on the
DETACH_BACKING request.  Guess I'll add a new error code for that.

> > +Otherwise the shared resources are used like normal resources.
> > +Especially the driver must send explicit VIRTIO_GPU_CMD_TRANSFER_*
> > +commands to the device for both normal and shared resources.  Reason:
> > +The device might have to flush caches.
> 
> Can we make this spec stronger to avoid to avoid transfers all the
> time (i.e, in virtio_gpu_update_dumb_bo)?
> 
> drm_gem_shmem_* helpers seem to use WC buffers, and dumb buffers are
> traditionally WC as well.  If we mandate the host ("device?" here)
> must properly clean caches at creation time, it may be possible to
> avoid hypercalls for 2D_SHARED resources.

I think we can do 90% of that optimization without changing the
protocol.  Display updates typically involve multiple commands.  A
transfer, possibly a set-scanout (when page-flipping), finally flush.
The driver can first queue all commands, then notify the host, so we
will have only one vmexit per display update instead of one vmexit per
command.

> > The device MAY also choose to
> > +not create mapping for the shared resource.  Especially for small
> > +resources it might be more efficient to just copy the data instead of
> > +establishing a shared mapping.
> 
> Should the device send a response code  (OK_SHARED} to inform the
> driver that the resource is shared?

Is there a good reason why the driver should be aware of that?

I'd prefer to make that an internal implementation detail of
the device and not inform the guest.

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH] virtio-gpu: add shared resource feature

[Gurchetan Singh]

On Wed, Dec 4, 2019 at 11:09 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > If the following scenario happens:
> >
> > 1) Driver sends RESOURCE_CREATE_2D shared request
> > 2) Driver sends ATTACH_BACKING request
> > 3) Device creates a shared resource
> > 4) Driver sends SET_SCANOUT request
> > 5) Device sends shared resource to display
> > 6) Driver sends DETACH_BACKING request
>
> Hmm, I think you can crash the current qemu code that way (didn't test
> though).  I think the proper solution would be to return -EBUSY on the
> DETACH_BACKING request.  Guess I'll add a new error code for that.

That would add the extra complication of a "delayed detach list" in
the guest kernel.  I don't know if it's absolutely necessary --
because the open source implementation (udmabuf) seems to take a
reference on all constituent pages like DRM gem does [a][b][c][d].  We
should figure out if the extra reference is sufficient to prevent the
pages from going back to guest RAM (the guest kernel takes references
on those pages too -- I don't know if guest references are treated
differently).

[a] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L578
[b] https://github.com/torvalds/linux/blob/master/drivers/dma-buf/udmabuf.c#L161
[c] https://github.com/torvalds/linux/blob/master/drivers/dma-buf/udmabuf.c#L203
[d] http://lkml.iu.edu/hypermail/linux/kernel/0907.3/02155.html

>
> > > +Otherwise the shared resources are used like normal resources.
> > > +Especially the driver must send explicit VIRTIO_GPU_CMD_TRANSFER_*
> > > +commands to the device for both normal and shared resources.  Reason:
> > > +The device might have to flush caches.
> >
> > Can we make this spec stronger to avoid to avoid transfers all the
> > time (i.e, in virtio_gpu_update_dumb_bo)?
> >
> > drm_gem_shmem_* helpers seem to use WC buffers, and dumb buffers are
> > traditionally WC as well.  If we mandate the host ("device?" here)
> > must properly clean caches at creation time, it may be possible to
> > avoid hypercalls for 2D_SHARED resources.
>
> I think we can do 90% of that optimization without changing the
> protocol.  Display updates typically involve multiple commands.  A
> transfer, possibly a set-scanout (when page-flipping), finally flush.
> The driver can first queue all commands, then notify the host, so we
> will have only one vmexit per display update instead of one vmexit per
> command.

We also want to prevent this scenario:

1) Guest creates a writecombine mapping (like drm_gem_shmem_* helpers
do), but the pages were previously accessed and thus some data is in
the cache.
2) Cache eviction
3) Non-zero data in buffer.

>
> > > The device MAY also choose to
> > > +not create mapping for the shared resource.  Especially for small
> > > +resources it might be more efficient to just copy the data instead of
> > > +establishing a shared mapping.
> >
> > Should the device send a response code  (OK_SHARED} to inform the
> > driver that the resource is shared?
>
> Is there a good reason why the driver should be aware of that?

Technically TRANSFER_TO_HOST is legal for 2D resources (though rarely
used).  I suppose we can modify RESOURCE_INFO to inform userspace the
resource is shared, and thus avoid hypercalls.

If we leave the decision to create the shared resource to the driver
and not the device, *_2D_SHARED makes sense.  Right now, it's
*_2D_MAYBE_SHARED.

>
> I'd prefer to make that an internal implementation detail of
> the device and not inform the guest.
>
> cheers,
>   Gerd
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

> > > 1) Driver sends RESOURCE_CREATE_2D shared request
> > > 2) Driver sends ATTACH_BACKING request
> > > 3) Device creates a shared resource
> > > 4) Driver sends SET_SCANOUT request
> > > 5) Device sends shared resource to display
> > > 6) Driver sends DETACH_BACKING request
> >
> > Hmm, I think you can crash the current qemu code that way (didn't test
> > though).  I think the proper solution would be to return -EBUSY on the
> > DETACH_BACKING request.  Guess I'll add a new error code for that.
> 
> That would add the extra complication of a "delayed detach list" in
> the guest kernel.

Why?  I can't see how that can happen with the linux guest driver.  You
need a drm framebuffer to map an gem object to a scanout.  The drm
framebuffer holds a reference of the gem object, so the linux kernel
wouldn't try to delete the gem object (-> DETACH_BACKING + UNREF) while
it is mapped to the scanout because the refcount wouldn't go down to
zero.

> I don't know if it's absolutely necessary --
> because the open source implementation (udmabuf) seems to take a
> reference on all constituent pages like DRM gem does [a][b][c][d].  We
> should figure out if the extra reference is sufficient to prevent the
> pages from going back to guest RAM (the guest kernel takes references
> on those pages too -- I don't know if guest references are treated
> differently).

They are never taken away from guest ram.  The guest is free to do
whatever it wants with the pages.  It's the job of the guest driver to
make sure the pages are not released until the host stopped using them.
So the host must drop all references before completing the
DETACH_BACKING command.

> > I think we can do 90% of that optimization without changing the
> > protocol.  Display updates typically involve multiple commands.  A
> > transfer, possibly a set-scanout (when page-flipping), finally flush.
> > The driver can first queue all commands, then notify the host, so we
> > will have only one vmexit per display update instead of one vmexit per
> > command.
> 
> We also want to prevent this scenario:
> 
> 1) Guest creates a writecombine mapping (like drm_gem_shmem_* helpers
> do), but the pages were previously accessed and thus some data is in
> the cache.
> 2) Cache eviction
> 3) Non-zero data in buffer.

This can happen on arm I guess?  I know caching is a more complicated on
arm than on x86.  Care to explaint this (or do you have a good link?).

Also: what is special in virtualization here?  There are arm drivers
using drm_gem_shmem_* helpers.  How do they manage this?

> > Is there a good reason why the driver should be aware of that?
> 
> Technically TRANSFER_TO_HOST is legal for 2D resources (though rarely
> used).

It's used for dumb gem objects.

> I suppose we can modify RESOURCE_INFO to inform userspace the
> resource is shared, and thus avoid hypercalls.
> 
> If we leave the decision to create the shared resource to the driver
> and not the device, *_2D_SHARED makes sense.  Right now, it's
> *_2D_MAYBE_SHARED.

Yes, the guest can't force it.

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH] virtio-gpu: add shared resource feature

[Gurchetan Singh]

On Mon, Dec 9, 2019 at 3:31 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > > 1) Driver sends RESOURCE_CREATE_2D shared request
> > > > 2) Driver sends ATTACH_BACKING request
> > > > 3) Device creates a shared resource
> > > > 4) Driver sends SET_SCANOUT request
> > > > 5) Device sends shared resource to display
> > > > 6) Driver sends DETACH_BACKING request
> > >
> > > Hmm, I think you can crash the current qemu code that way (didn't test
> > > though).  I think the proper solution would be to return -EBUSY on the
> > > DETACH_BACKING request.  Guess I'll add a new error code for that.
> >
> > That would add the extra complication of a "delayed detach list" in
> > the guest kernel.
>
> Why?  I can't see how that can happen with the linux guest driver.  You
> need a drm framebuffer to map an gem object to a scanout.  The drm
> framebuffer holds a reference of the gem object, so the linux kernel
> wouldn't try to delete the gem object (-> DETACH_BACKING + UNREF) while
> it is mapped to the scanout because the refcount wouldn't go down to
> zero.

The case I'm thinking about is:

1) Guest DRM framebuffer refcount goes to zero
2) Host DRM framebuffer refcount is non-zero

I prefer the guest kernel just complain loudly rather than doing
complicated tricks in such instance.

>
> > I don't know if it's absolutely necessary --
> > because the open source implementation (udmabuf) seems to take a
> > reference on all constituent pages like DRM gem does [a][b][c][d].  We
> > should figure out if the extra reference is sufficient to prevent the
> > pages from going back to guest RAM (the guest kernel takes references
> > on those pages too -- I don't know if guest references are treated
> > differently).
>
> They are never taken away from guest ram.  The guest is free to do
> whatever it wants with the pages.  It's the job of the guest driver to
> make sure the pages are not released until the host stopped using them.
> So the host must drop all references before completing the
> DETACH_BACKING command.

Let's write that in the spec.

>
> > > I think we can do 90% of that optimization without changing the
> > > protocol.  Display updates typically involve multiple commands.  A
> > > transfer, possibly a set-scanout (when page-flipping), finally flush.
> > > The driver can first queue all commands, then notify the host, so we
> > > will have only one vmexit per display update instead of one vmexit per
> > > command.
> >
> > We also want to prevent this scenario:
> >
> > 1) Guest creates a writecombine mapping (like drm_gem_shmem_* helpers
> > do), but the pages were previously accessed and thus some data is in
> > the cache.
> > 2) Cache eviction
> > 3) Non-zero data in buffer.
>
> This can happen on arm I guess?

Exactly.

> I know caching is a more complicated on
> arm than on x86.  Care to explaint this (or do you have a good link?).

Two differences:
- On x86, the guest can perform cache operations, while on ARM it's a
privileged operation [1].
- On ARM, the guest attribute seems to override the host attribute[1].
I've verified this.

Even on x86, it seems it's possible to get WC buffers by changing the
MTTR[2] (though I haven't verified this yet).

[1] https://events.static.linuxfound.org/sites/events/files/slides/slides_10.pdf
[2] https://lwn.net/Articles/646712/

We (you?) need to modify drm_gem_shmem_* helpers to always be cached
for virtgpu for now, since the TTM removal changed that behavior.

Then, we should expose host properties to the guest, like [1] suggests
(maybe something like VIRTIO_GPU_F_GUEST_ATTRIBUTE_OVERRIDE,
VIRTIO_GPU_F_GUEST_CACHE_OPS).  If there's already a method to do
this, that would be awesome.

Then we can optimize away.

>
> Also: what is special in virtualization here?  There are arm drivers
> using drm_gem_shmem_* helpers.  How do they manage this?

Panfrost, v3d.  I'm surprised they haven't hit this issue yet.

>
> > > Is there a good reason why the driver should be aware of that?
> >
> > Technically TRANSFER_TO_HOST is legal for 2D resources (though rarely
> > used).
>
> It's used for dumb gem objects.
>
> > I suppose we can modify RESOURCE_INFO to inform userspace the
> > resource is shared, and thus avoid hypercalls.
> >
> > If we leave the decision to create the shared resource to the driver
> > and not the device, *_2D_SHARED makes sense.  Right now, it's
> > *_2D_MAYBE_SHARED.
>
> Yes, the guest can't force it.
>
> cheers,
>   Gerd
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH] virtio-gpu: add shared resource feature

[Gerd Hoffmann]

  Hi,

> > They are never taken away from guest ram.  The guest is free to do
> > whatever it wants with the pages.  It's the job of the guest driver to
> > make sure the pages are not released until the host stopped using them.
> > So the host must drop all references before completing the
> > DETACH_BACKING command.
> 
> Let's write that in the spec.

Yes, saying so explicitly that totally makes sense.
I'll update the patch accordingly.

> We (you?) need to modify drm_gem_shmem_* helpers to always be cached
> for virtgpu for now, since the TTM removal changed that behavior.

We can just roll our own mmap function to handle that.
I'll prepare a patch (that is independent from F_SHARED anyway).

> Then, we should expose host properties to the guest, like [1] suggests
> (maybe something like VIRTIO_GPU_F_GUEST_ATTRIBUTE_OVERRIDE,
> VIRTIO_GPU_F_GUEST_CACHE_OPS).  If there's already a method to do
> this, that would be awesome.

We'll need that for sure when mapping host resources into the guest, but
we are not there yet with F_SHARED only.

> > Also: what is special in virtualization here?  There are arm drivers
> > using drm_gem_shmem_* helpers.  How do they manage this?
> 
> Panfrost, v3d.  I'm surprised they haven't hit this issue yet.

Maybe because all mappings are consistent.  With virtio-gpu you could
end up in a situation where the guest uses wc whereas the host uses
cached and things go boom from there.

cheers,
  Gerd


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org