Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[kbuild test robot]

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

In-Reply-To: <20200526105811.30784-2-stevensd@chromium.org>
References: <20200526105811.30784-2-stevensd@chromium.org>
TO: David Stevens <stevensd@chromium.org>
TO: Gerd Hoffmann <kraxel@redhat.com>
TO: David Airlie <airlied@linux.ie>
TO: Daniel Vetter <daniel@ffwll.ch>
TO: Sumit Semwal <sumit.semwal@linaro.org>
CC: "Michael S . Tsirkin" <mst@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
CC: Maxime Ripard <mripard@kernel.org>
CC: Thomas Zimmermann <tzimmermann@suse.de>
CC: David Stevens <stevensd@chromium.org>

Hi David,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200525]
[also build test WARNING on v5.7-rc7]
[cannot apply to linus/master v5.7-rc7 v5.7-rc6 v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/David-Stevens/Support-virtio-cross-device-resources/20200526-190644
base:    7b4cb0a48db03a67d1ce21c4f97b1508a241a2e7
config: x86_64-randconfig-a002-20200531 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 2388a096e7865c043e83ece4e26654bd3d1a20d5)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/virtio/virtio_dma_buf.o: warning: objtool: __llvm_gcov_writeout()+0x0: call without frame pointer save/setup
>> drivers/virtio/virtio_dma_buf.o: warning: objtool: __llvm_gcov_reset()+0x0: call without frame pointer save/setup
>> drivers/virtio/virtio_dma_buf.o: warning: objtool: __llvm_gcov_flush()+0x0: call without frame pointer save/setup
>> drivers/virtio/virtio_dma_buf.o: warning: objtool: __llvm_gcov_init()+0x0: call without frame pointer save/setup

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32603 bytes --]

[virtio-dev] [PATCH v4 0/3] Support virtio cross-device resources

[David Stevens]

This patchset implements the current proposal for virtio cross-device
resource sharing [1]. It will be used to import virtio resources into
the virtio-video driver currently under discussion [2]. The patch
under consideration to add support in the virtio-video driver is [3].
It uses the APIs from v3 of this series, but the changes to update it
are relatively minor.

This patchset adds a new flavor of dma-bufs that supports querying the
underlying virtio object UUID, as well as adding support for exporting
resources from virtgpu.

[1] https://markmail.org/thread/2ypjt5cfeu3m6lxu
[2] https://markmail.org/thread/p5d3k566srtdtute
[3] https://markmail.org/thread/j4xlqaaim266qpks

v3 -> v4 changes:
 - Replace dma-buf hooks with virtio dma-buf from v1.
 - Remove virtio_attach callback, as the work that had been done
   in that callback is now done on dma-buf export. The documented
   requirement that get_uuid only be called on attached virtio
   dma-bufs is also removed.
 - Rebase and add call to virtio_gpu_notify for ASSIGN_UUID.

David Stevens (3):
  virtio: add dma-buf support for exported objects
  virtio-gpu: add VIRTIO_GPU_F_RESOURCE_UUID feature
  drm/virtio: Support virtgpu exported resources

 drivers/gpu/drm/virtio/virtgpu_drv.c   |  3 +
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 20 ++++++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  4 ++
 drivers/gpu/drm/virtio/virtgpu_prime.c | 98 +++++++++++++++++++++++++-
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 55 +++++++++++++++
 drivers/virtio/Makefile                |  2 +-
 drivers/virtio/virtio.c                |  6 ++
 drivers/virtio/virtio_dma_buf.c        | 91 ++++++++++++++++++++++++
 include/linux/virtio.h                 |  1 +
 include/linux/virtio_dma_buf.h         | 58 +++++++++++++++
 include/uapi/linux/virtio_gpu.h        | 19 +++++
 11 files changed, 353 insertions(+), 4 deletions(-)
 create mode 100644 drivers/virtio/virtio_dma_buf.c
 create mode 100644 include/linux/virtio_dma_buf.h

-- 
2.27.0.rc0.183.gde8f92d652-goog


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

[PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

This change adds a new flavor of dma-bufs that can be used by virtio
drivers to share exported objects. A virtio dma-buf can be queried by
virtio drivers to obtain the UUID which identifies the underlying
exported object.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/virtio/Makefile         |  2 +-
 drivers/virtio/virtio.c         |  6 +++
 drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
 include/linux/virtio.h          |  1 +
 include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
 5 files changed, 155 insertions(+), 1 deletion(-)
 create mode 100644 drivers/virtio/virtio_dma_buf.c
 create mode 100644 include/linux/virtio_dma_buf.h

diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 29a1386ecc03..ecdae5b596de 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
+obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
 obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
 obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
 virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..5d46f0ded92d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(register_virtio_device);
 
+bool is_virtio_device(struct device *dev)
+{
+	return dev->bus == &virtio_bus;
+}
+EXPORT_SYMBOL_GPL(is_virtio_device);
+
 void unregister_virtio_device(struct virtio_device *dev)
 {
 	int index = dev->index; /* save for after device release */
diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
new file mode 100644
index 000000000000..23e3399b11ed
--- /dev/null
+++ b/drivers/virtio/virtio_dma_buf.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * dma-bufs for virtio exported objects
+ *
+ * Copyright (C) 2020 Google, Inc.
+ */
+
+#include <linux/virtio_dma_buf.h>
+
+/**
+ * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
+ *
+ * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
+ * for an virtio exported object that can be queried by other virtio drivers
+ * for the object's UUID.
+ */
+struct dma_buf *virtio_dma_buf_export(
+		const struct virtio_dma_buf_export_info *virtio_exp_info)
+{
+	struct dma_buf_export_info exp_info;
+
+	if (!virtio_exp_info->ops
+		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
+		|| !virtio_exp_info->ops->get_uuid) {
+		return ERR_PTR(-EINVAL);
+	}
+
+	exp_info.exp_name = virtio_exp_info->exp_name;
+	exp_info.owner = virtio_exp_info->owner;
+	exp_info.ops = &virtio_exp_info->ops->ops;
+	exp_info.size = virtio_exp_info->size;
+	exp_info.flags = virtio_exp_info->flags;
+	exp_info.resv = virtio_exp_info->resv;
+	exp_info.priv = virtio_exp_info->priv;
+	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
+		     != sizeof(struct dma_buf_export_info));
+
+	return dma_buf_export(&exp_info);
+}
+EXPORT_SYMBOL(virtio_dma_buf_export);
+
+/**
+ * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
+ */
+int virtio_dma_buf_attach(struct dma_buf *dma_buf,
+			  struct dma_buf_attachment *attach)
+{
+	int ret;
+	const struct virtio_dma_buf_ops *ops = container_of(
+			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+
+	if (ops->device_attach) {
+		ret = ops->device_attach(dma_buf, attach);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(virtio_dma_buf_attach);
+
+/**
+ * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
+ * @dma_buf: buffer to query
+ */
+bool is_virtio_dma_buf(struct dma_buf *dma_buf)
+{
+	return dma_buf->ops->attach == &virtio_dma_buf_attach;
+}
+EXPORT_SYMBOL(is_virtio_dma_buf);
+
+/**
+ * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
+ * @dma_buf: [in] buffer to query
+ * @uuid: [out] the uuid
+ *
+ * Returns: 0 on success, negative on failure.
+ */
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
+			    uuid_t *uuid)
+{
+	const struct virtio_dma_buf_ops *ops = container_of(
+			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+
+	if (!is_virtio_dma_buf(dma_buf))
+		return -EINVAL;
+
+	return ops->get_uuid(dma_buf, uuid);
+}
+EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 15f906e4a748..9397e25616c4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
 void virtio_add_status(struct virtio_device *dev, unsigned int status);
 int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
+bool is_virtio_device(struct device *dev);
 
 void virtio_break_device(struct virtio_device *dev);
 
diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
new file mode 100644
index 000000000000..29fee167afbd
--- /dev/null
+++ b/include/linux/virtio_dma_buf.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * dma-bufs for virtio exported objects
+ *
+ * Copyright (C) 2020 Google, Inc.
+ */
+
+#ifndef _LINUX_VIRTIO_DMA_BUF_H
+#define _LINUX_VIRTIO_DMA_BUF_H
+
+#include <linux/dma-buf.h>
+#include <linux/uuid.h>
+#include <linux/virtio.h>
+
+/**
+ * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
+ * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
+ * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
+ *		   all attach operations.
+ * @get_uid: [required] callback to get the uuid of the exported object.
+ */
+struct virtio_dma_buf_ops {
+	struct dma_buf_ops ops;
+	int (*device_attach)(struct dma_buf *dma_buf,
+			     struct dma_buf_attachment *attach);
+	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
+};
+
+/**
+ * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
+ */
+struct virtio_dma_buf_export_info {
+	const char *exp_name;
+	struct module *owner;
+	const struct virtio_dma_buf_ops *ops;
+	size_t size;
+	int flags;
+	struct dma_resv *resv;
+	void *priv;
+};
+
+/**
+ * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
+ */
+#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
+	struct virtio_dma_buf_export_info name = { \
+		.exp_name = KBUILD_MODNAME, \
+		.owner = THIS_MODULE }
+
+int virtio_dma_buf_attach(struct dma_buf *dma_buf,
+			  struct dma_buf_attachment *attach);
+
+struct dma_buf *virtio_dma_buf_export(
+		const struct virtio_dma_buf_export_info *virtio_exp_info);
+bool is_virtio_dma_buf(struct dma_buf *dma_buf);
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
+
+#endif /* _LINUX_VIRTIO_DMA_BUF_H */
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

This change adds a new flavor of dma-bufs that can be used by virtio
drivers to share exported objects. A virtio dma-buf can be queried by
virtio drivers to obtain the UUID which identifies the underlying
exported object.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/virtio/Makefile         |  2 +-
 drivers/virtio/virtio.c         |  6 +++
 drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
 include/linux/virtio.h          |  1 +
 include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
 5 files changed, 155 insertions(+), 1 deletion(-)
 create mode 100644 drivers/virtio/virtio_dma_buf.c
 create mode 100644 include/linux/virtio_dma_buf.h

diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 29a1386ecc03..ecdae5b596de 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
+obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
 obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
 obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
 virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..5d46f0ded92d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(register_virtio_device);
 
+bool is_virtio_device(struct device *dev)
+{
+	return dev->bus == &virtio_bus;
+}
+EXPORT_SYMBOL_GPL(is_virtio_device);
+
 void unregister_virtio_device(struct virtio_device *dev)
 {
 	int index = dev->index; /* save for after device release */
diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
new file mode 100644
index 000000000000..23e3399b11ed
--- /dev/null
+++ b/drivers/virtio/virtio_dma_buf.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * dma-bufs for virtio exported objects
+ *
+ * Copyright (C) 2020 Google, Inc.
+ */
+
+#include <linux/virtio_dma_buf.h>
+
+/**
+ * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
+ *
+ * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
+ * for an virtio exported object that can be queried by other virtio drivers
+ * for the object's UUID.
+ */
+struct dma_buf *virtio_dma_buf_export(
+		const struct virtio_dma_buf_export_info *virtio_exp_info)
+{
+	struct dma_buf_export_info exp_info;
+
+	if (!virtio_exp_info->ops
+		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
+		|| !virtio_exp_info->ops->get_uuid) {
+		return ERR_PTR(-EINVAL);
+	}
+
+	exp_info.exp_name = virtio_exp_info->exp_name;
+	exp_info.owner = virtio_exp_info->owner;
+	exp_info.ops = &virtio_exp_info->ops->ops;
+	exp_info.size = virtio_exp_info->size;
+	exp_info.flags = virtio_exp_info->flags;
+	exp_info.resv = virtio_exp_info->resv;
+	exp_info.priv = virtio_exp_info->priv;
+	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
+		     != sizeof(struct dma_buf_export_info));
+
+	return dma_buf_export(&exp_info);
+}
+EXPORT_SYMBOL(virtio_dma_buf_export);
+
+/**
+ * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
+ */
+int virtio_dma_buf_attach(struct dma_buf *dma_buf,
+			  struct dma_buf_attachment *attach)
+{
+	int ret;
+	const struct virtio_dma_buf_ops *ops = container_of(
+			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+
+	if (ops->device_attach) {
+		ret = ops->device_attach(dma_buf, attach);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(virtio_dma_buf_attach);
+
+/**
+ * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
+ * @dma_buf: buffer to query
+ */
+bool is_virtio_dma_buf(struct dma_buf *dma_buf)
+{
+	return dma_buf->ops->attach == &virtio_dma_buf_attach;
+}
+EXPORT_SYMBOL(is_virtio_dma_buf);
+
+/**
+ * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
+ * @dma_buf: [in] buffer to query
+ * @uuid: [out] the uuid
+ *
+ * Returns: 0 on success, negative on failure.
+ */
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
+			    uuid_t *uuid)
+{
+	const struct virtio_dma_buf_ops *ops = container_of(
+			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+
+	if (!is_virtio_dma_buf(dma_buf))
+		return -EINVAL;
+
+	return ops->get_uuid(dma_buf, uuid);
+}
+EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 15f906e4a748..9397e25616c4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
 void virtio_add_status(struct virtio_device *dev, unsigned int status);
 int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
+bool is_virtio_device(struct device *dev);
 
 void virtio_break_device(struct virtio_device *dev);
 
diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
new file mode 100644
index 000000000000..29fee167afbd
--- /dev/null
+++ b/include/linux/virtio_dma_buf.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * dma-bufs for virtio exported objects
+ *
+ * Copyright (C) 2020 Google, Inc.
+ */
+
+#ifndef _LINUX_VIRTIO_DMA_BUF_H
+#define _LINUX_VIRTIO_DMA_BUF_H
+
+#include <linux/dma-buf.h>
+#include <linux/uuid.h>
+#include <linux/virtio.h>
+
+/**
+ * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
+ * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
+ * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
+ *		   all attach operations.
+ * @get_uid: [required] callback to get the uuid of the exported object.
+ */
+struct virtio_dma_buf_ops {
+	struct dma_buf_ops ops;
+	int (*device_attach)(struct dma_buf *dma_buf,
+			     struct dma_buf_attachment *attach);
+	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
+};
+
+/**
+ * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
+ */
+struct virtio_dma_buf_export_info {
+	const char *exp_name;
+	struct module *owner;
+	const struct virtio_dma_buf_ops *ops;
+	size_t size;
+	int flags;
+	struct dma_resv *resv;
+	void *priv;
+};
+
+/**
+ * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
+ */
+#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
+	struct virtio_dma_buf_export_info name = { \
+		.exp_name = KBUILD_MODNAME, \
+		.owner = THIS_MODULE }
+
+int virtio_dma_buf_attach(struct dma_buf *dma_buf,
+			  struct dma_buf_attachment *attach);
+
+struct dma_buf *virtio_dma_buf_export(
+		const struct virtio_dma_buf_export_info *virtio_exp_info);
+bool is_virtio_dma_buf(struct dma_buf *dma_buf);
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
+
+#endif /* _LINUX_VIRTIO_DMA_BUF_H */
-- 
2.27.0.rc0.183.gde8f92d652-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> This change adds a new flavor of dma-bufs that can be used by virtio
> drivers to share exported objects. A virtio dma-buf can be queried by
> virtio drivers to obtain the UUID which identifies the underlying
> exported object.
> 
> Signed-off-by: David Stevens <stevensd@chromium.org>

Is this just for graphics? If yes I'd rather we put it in the graphics
driver. We can always move it later ...

> ---
>  drivers/virtio/Makefile         |  2 +-
>  drivers/virtio/virtio.c         |  6 +++
>  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
>  include/linux/virtio.h          |  1 +
>  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
>  5 files changed, 155 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/virtio/virtio_dma_buf.c
>  create mode 100644 include/linux/virtio_dma_buf.h
> 
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 29a1386ecc03..ecdae5b596de 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
>  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
>  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..5d46f0ded92d 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(register_virtio_device);
>  
> +bool is_virtio_device(struct device *dev)
> +{
> +	return dev->bus == &virtio_bus;
> +}
> +EXPORT_SYMBOL_GPL(is_virtio_device);
> +
>  void unregister_virtio_device(struct virtio_device *dev)
>  {
>  	int index = dev->index; /* save for after device release */
> diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> new file mode 100644
> index 000000000000..23e3399b11ed
> --- /dev/null
> +++ b/drivers/virtio/virtio_dma_buf.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * dma-bufs for virtio exported objects
> + *
> + * Copyright (C) 2020 Google, Inc.
> + */
> +
> +#include <linux/virtio_dma_buf.h>
> +
> +/**
> + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> + *
> + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> + * for an virtio exported object that can be queried by other virtio drivers
> + * for the object's UUID.
> + */
> +struct dma_buf *virtio_dma_buf_export(
> +		const struct virtio_dma_buf_export_info *virtio_exp_info)
> +{
> +	struct dma_buf_export_info exp_info;
> +
> +	if (!virtio_exp_info->ops
> +		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> +		|| !virtio_exp_info->ops->get_uuid) {
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	exp_info.exp_name = virtio_exp_info->exp_name;
> +	exp_info.owner = virtio_exp_info->owner;
> +	exp_info.ops = &virtio_exp_info->ops->ops;
> +	exp_info.size = virtio_exp_info->size;
> +	exp_info.flags = virtio_exp_info->flags;
> +	exp_info.resv = virtio_exp_info->resv;
> +	exp_info.priv = virtio_exp_info->priv;
> +	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> +		     != sizeof(struct dma_buf_export_info));

This is the only part that gives me pause. Why do we need this hack?
What's wrong with just using dma_buf_export_info directly,
and if you want the virtio ops, just using container_off?



> +
> +	return dma_buf_export(&exp_info);
> +}
> +EXPORT_SYMBOL(virtio_dma_buf_export);
> +
> +/**
> + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> + */
> +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> +			  struct dma_buf_attachment *attach)
> +{
> +	int ret;
> +	const struct virtio_dma_buf_ops *ops = container_of(
> +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> +	if (ops->device_attach) {
> +		ret = ops->device_attach(dma_buf, attach);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(virtio_dma_buf_attach);
> +
> +/**
> + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> + * @dma_buf: buffer to query
> + */
> +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> +{
> +	return dma_buf->ops->attach == &virtio_dma_buf_attach;
> +}
> +EXPORT_SYMBOL(is_virtio_dma_buf);
> +
> +/**
> + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> + * @dma_buf: [in] buffer to query
> + * @uuid: [out] the uuid
> + *
> + * Returns: 0 on success, negative on failure.
> + */
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> +			    uuid_t *uuid)
> +{
> +	const struct virtio_dma_buf_ops *ops = container_of(
> +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> +	if (!is_virtio_dma_buf(dma_buf))
> +		return -EINVAL;
> +
> +	return ops->get_uuid(dma_buf, uuid);
> +}
> +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 15f906e4a748..9397e25616c4 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
>  void virtio_add_status(struct virtio_device *dev, unsigned int status);
>  int register_virtio_device(struct virtio_device *dev);
>  void unregister_virtio_device(struct virtio_device *dev);
> +bool is_virtio_device(struct device *dev);
>  
>  void virtio_break_device(struct virtio_device *dev);
>  
> diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> new file mode 100644
> index 000000000000..29fee167afbd
> --- /dev/null
> +++ b/include/linux/virtio_dma_buf.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * dma-bufs for virtio exported objects
> + *
> + * Copyright (C) 2020 Google, Inc.
> + */
> +
> +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> +#define _LINUX_VIRTIO_DMA_BUF_H
> +
> +#include <linux/dma-buf.h>
> +#include <linux/uuid.h>
> +#include <linux/virtio.h>
> +
> +/**
> + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> + *		   all attach operations.
> + * @get_uid: [required] callback to get the uuid of the exported object.
> + */
> +struct virtio_dma_buf_ops {
> +	struct dma_buf_ops ops;
> +	int (*device_attach)(struct dma_buf *dma_buf,
> +			     struct dma_buf_attachment *attach);
> +	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> +};
> +
> +/**
> + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> + */
> +struct virtio_dma_buf_export_info {
> +	const char *exp_name;
> +	struct module *owner;
> +	const struct virtio_dma_buf_ops *ops;
> +	size_t size;
> +	int flags;
> +	struct dma_resv *resv;
> +	void *priv;
> +};
> +
> +/**
> + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> + */
> +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
> +	struct virtio_dma_buf_export_info name = { \
> +		.exp_name = KBUILD_MODNAME, \
> +		.owner = THIS_MODULE }
> +
> +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> +			  struct dma_buf_attachment *attach);
> +
> +struct dma_buf *virtio_dma_buf_export(
> +		const struct virtio_dma_buf_export_info *virtio_exp_info);
> +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> +
> +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> -- 
> 2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> This change adds a new flavor of dma-bufs that can be used by virtio
> drivers to share exported objects. A virtio dma-buf can be queried by
> virtio drivers to obtain the UUID which identifies the underlying
> exported object.
> 
> Signed-off-by: David Stevens <stevensd@chromium.org>

Is this just for graphics? If yes I'd rather we put it in the graphics
driver. We can always move it later ...

> ---
>  drivers/virtio/Makefile         |  2 +-
>  drivers/virtio/virtio.c         |  6 +++
>  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
>  include/linux/virtio.h          |  1 +
>  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
>  5 files changed, 155 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/virtio/virtio_dma_buf.c
>  create mode 100644 include/linux/virtio_dma_buf.h
> 
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 29a1386ecc03..ecdae5b596de 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
>  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
>  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..5d46f0ded92d 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(register_virtio_device);
>  
> +bool is_virtio_device(struct device *dev)
> +{
> +	return dev->bus == &virtio_bus;
> +}
> +EXPORT_SYMBOL_GPL(is_virtio_device);
> +
>  void unregister_virtio_device(struct virtio_device *dev)
>  {
>  	int index = dev->index; /* save for after device release */
> diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> new file mode 100644
> index 000000000000..23e3399b11ed
> --- /dev/null
> +++ b/drivers/virtio/virtio_dma_buf.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * dma-bufs for virtio exported objects
> + *
> + * Copyright (C) 2020 Google, Inc.
> + */
> +
> +#include <linux/virtio_dma_buf.h>
> +
> +/**
> + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> + *
> + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> + * for an virtio exported object that can be queried by other virtio drivers
> + * for the object's UUID.
> + */
> +struct dma_buf *virtio_dma_buf_export(
> +		const struct virtio_dma_buf_export_info *virtio_exp_info)
> +{
> +	struct dma_buf_export_info exp_info;
> +
> +	if (!virtio_exp_info->ops
> +		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> +		|| !virtio_exp_info->ops->get_uuid) {
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	exp_info.exp_name = virtio_exp_info->exp_name;
> +	exp_info.owner = virtio_exp_info->owner;
> +	exp_info.ops = &virtio_exp_info->ops->ops;
> +	exp_info.size = virtio_exp_info->size;
> +	exp_info.flags = virtio_exp_info->flags;
> +	exp_info.resv = virtio_exp_info->resv;
> +	exp_info.priv = virtio_exp_info->priv;
> +	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> +		     != sizeof(struct dma_buf_export_info));

This is the only part that gives me pause. Why do we need this hack?
What's wrong with just using dma_buf_export_info directly,
and if you want the virtio ops, just using container_off?



> +
> +	return dma_buf_export(&exp_info);
> +}
> +EXPORT_SYMBOL(virtio_dma_buf_export);
> +
> +/**
> + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> + */
> +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> +			  struct dma_buf_attachment *attach)
> +{
> +	int ret;
> +	const struct virtio_dma_buf_ops *ops = container_of(
> +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> +	if (ops->device_attach) {
> +		ret = ops->device_attach(dma_buf, attach);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(virtio_dma_buf_attach);
> +
> +/**
> + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> + * @dma_buf: buffer to query
> + */
> +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> +{
> +	return dma_buf->ops->attach == &virtio_dma_buf_attach;
> +}
> +EXPORT_SYMBOL(is_virtio_dma_buf);
> +
> +/**
> + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> + * @dma_buf: [in] buffer to query
> + * @uuid: [out] the uuid
> + *
> + * Returns: 0 on success, negative on failure.
> + */
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> +			    uuid_t *uuid)
> +{
> +	const struct virtio_dma_buf_ops *ops = container_of(
> +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> +	if (!is_virtio_dma_buf(dma_buf))
> +		return -EINVAL;
> +
> +	return ops->get_uuid(dma_buf, uuid);
> +}
> +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 15f906e4a748..9397e25616c4 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
>  void virtio_add_status(struct virtio_device *dev, unsigned int status);
>  int register_virtio_device(struct virtio_device *dev);
>  void unregister_virtio_device(struct virtio_device *dev);
> +bool is_virtio_device(struct device *dev);
>  
>  void virtio_break_device(struct virtio_device *dev);
>  
> diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> new file mode 100644
> index 000000000000..29fee167afbd
> --- /dev/null
> +++ b/include/linux/virtio_dma_buf.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * dma-bufs for virtio exported objects
> + *
> + * Copyright (C) 2020 Google, Inc.
> + */
> +
> +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> +#define _LINUX_VIRTIO_DMA_BUF_H
> +
> +#include <linux/dma-buf.h>
> +#include <linux/uuid.h>
> +#include <linux/virtio.h>
> +
> +/**
> + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> + *		   all attach operations.
> + * @get_uid: [required] callback to get the uuid of the exported object.
> + */
> +struct virtio_dma_buf_ops {
> +	struct dma_buf_ops ops;
> +	int (*device_attach)(struct dma_buf *dma_buf,
> +			     struct dma_buf_attachment *attach);
> +	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> +};
> +
> +/**
> + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> + */
> +struct virtio_dma_buf_export_info {
> +	const char *exp_name;
> +	struct module *owner;
> +	const struct virtio_dma_buf_ops *ops;
> +	size_t size;
> +	int flags;
> +	struct dma_resv *resv;
> +	void *priv;
> +};
> +
> +/**
> + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> + */
> +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
> +	struct virtio_dma_buf_export_info name = { \
> +		.exp_name = KBUILD_MODNAME, \
> +		.owner = THIS_MODULE }
> +
> +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> +			  struct dma_buf_attachment *attach);
> +
> +struct dma_buf *virtio_dma_buf_export(
> +		const struct virtio_dma_buf_export_info *virtio_exp_info);
> +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> +
> +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> -- 
> 2.27.0.rc0.183.gde8f92d652-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > This change adds a new flavor of dma-bufs that can be used by virtio
> > drivers to share exported objects. A virtio dma-buf can be queried by
> > virtio drivers to obtain the UUID which identifies the underlying
> > exported object.
> >
> > Signed-off-by: David Stevens <stevensd@chromium.org>
>
> Is this just for graphics? If yes I'd rather we put it in the graphics
> driver. We can always move it later ...

As stated in the cover letter, this will be used by virtio-video.

The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
The patch which imports these dma-bufs (slightly out of data, uses v3
of this patch set): https://markmail.org/thread/j4xlqaaim266qpks

> > ---
> >  drivers/virtio/Makefile         |  2 +-
> >  drivers/virtio/virtio.c         |  6 +++
> >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h          |  1 +
> >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> >  5 files changed, 155 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> >  create mode 100644 include/linux/virtio_dma_buf.h
> >
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 29a1386ecc03..ecdae5b596de 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a977e32a88f2..5d46f0ded92d 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(register_virtio_device);
> >
> > +bool is_virtio_device(struct device *dev)
> > +{
> > +     return dev->bus == &virtio_bus;
> > +}
> > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > +
> >  void unregister_virtio_device(struct virtio_device *dev)
> >  {
> >       int index = dev->index; /* save for after device release */
> > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > new file mode 100644
> > index 000000000000..23e3399b11ed
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_dma_buf.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#include <linux/virtio_dma_buf.h>
> > +
> > +/**
> > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > + *
> > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > + * for an virtio exported object that can be queried by other virtio drivers
> > + * for the object's UUID.
> > + */
> > +struct dma_buf *virtio_dma_buf_export(
> > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > +{
> > +     struct dma_buf_export_info exp_info;
> > +
> > +     if (!virtio_exp_info->ops
> > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > +             || !virtio_exp_info->ops->get_uuid) {
> > +             return ERR_PTR(-EINVAL);
> > +     }
> > +
> > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > +     exp_info.owner = virtio_exp_info->owner;
> > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > +     exp_info.size = virtio_exp_info->size;
> > +     exp_info.flags = virtio_exp_info->flags;
> > +     exp_info.resv = virtio_exp_info->resv;
> > +     exp_info.priv = virtio_exp_info->priv;
> > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > +                  != sizeof(struct dma_buf_export_info));
>
> This is the only part that gives me pause. Why do we need this hack?
> What's wrong with just using dma_buf_export_info directly,
> and if you want the virtio ops, just using container_off?

This approach provides a more explicit type signature and a little
more type safety, I think. If others don't think it's a worthwhile
tradeoff, I can remove it.

-David

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > This change adds a new flavor of dma-bufs that can be used by virtio
> > drivers to share exported objects. A virtio dma-buf can be queried by
> > virtio drivers to obtain the UUID which identifies the underlying
> > exported object.
> >
> > Signed-off-by: David Stevens <stevensd@chromium.org>
>
> Is this just for graphics? If yes I'd rather we put it in the graphics
> driver. We can always move it later ...

As stated in the cover letter, this will be used by virtio-video.

The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
The patch which imports these dma-bufs (slightly out of data, uses v3
of this patch set): https://markmail.org/thread/j4xlqaaim266qpks

> > ---
> >  drivers/virtio/Makefile         |  2 +-
> >  drivers/virtio/virtio.c         |  6 +++
> >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h          |  1 +
> >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> >  5 files changed, 155 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> >  create mode 100644 include/linux/virtio_dma_buf.h
> >
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 29a1386ecc03..ecdae5b596de 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a977e32a88f2..5d46f0ded92d 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(register_virtio_device);
> >
> > +bool is_virtio_device(struct device *dev)
> > +{
> > +     return dev->bus == &virtio_bus;
> > +}
> > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > +
> >  void unregister_virtio_device(struct virtio_device *dev)
> >  {
> >       int index = dev->index; /* save for after device release */
> > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > new file mode 100644
> > index 000000000000..23e3399b11ed
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_dma_buf.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#include <linux/virtio_dma_buf.h>
> > +
> > +/**
> > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > + *
> > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > + * for an virtio exported object that can be queried by other virtio drivers
> > + * for the object's UUID.
> > + */
> > +struct dma_buf *virtio_dma_buf_export(
> > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > +{
> > +     struct dma_buf_export_info exp_info;
> > +
> > +     if (!virtio_exp_info->ops
> > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > +             || !virtio_exp_info->ops->get_uuid) {
> > +             return ERR_PTR(-EINVAL);
> > +     }
> > +
> > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > +     exp_info.owner = virtio_exp_info->owner;
> > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > +     exp_info.size = virtio_exp_info->size;
> > +     exp_info.flags = virtio_exp_info->flags;
> > +     exp_info.resv = virtio_exp_info->resv;
> > +     exp_info.priv = virtio_exp_info->priv;
> > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > +                  != sizeof(struct dma_buf_export_info));
>
> This is the only part that gives me pause. Why do we need this hack?
> What's wrong with just using dma_buf_export_info directly,
> and if you want the virtio ops, just using container_off?

This approach provides a more explicit type signature and a little
more type safety, I think. If others don't think it's a worthwhile
tradeoff, I can remove it.

-David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > virtio drivers to obtain the UUID which identifies the underlying
> > > exported object.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> >
> > Is this just for graphics? If yes I'd rather we put it in the graphics
> > driver. We can always move it later ...
> 
> As stated in the cover letter, this will be used by virtio-video.
> 
> The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> The patch which imports these dma-bufs (slightly out of data, uses v3
> of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> 
> > > ---
> > >  drivers/virtio/Makefile         |  2 +-
> > >  drivers/virtio/virtio.c         |  6 +++
> > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > >  include/linux/virtio.h          |  1 +
> > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > >  create mode 100644 include/linux/virtio_dma_buf.h
> > >
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 29a1386ecc03..ecdae5b596de 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -1,5 +1,5 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index a977e32a88f2..5d46f0ded92d 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > >
> > > +bool is_virtio_device(struct device *dev)
> > > +{
> > > +     return dev->bus == &virtio_bus;
> > > +}
> > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > +
> > >  void unregister_virtio_device(struct virtio_device *dev)
> > >  {
> > >       int index = dev->index; /* save for after device release */
> > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > new file mode 100644
> > > index 000000000000..23e3399b11ed
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > @@ -0,0 +1,89 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * dma-bufs for virtio exported objects
> > > + *
> > > + * Copyright (C) 2020 Google, Inc.
> > > + */
> > > +
> > > +#include <linux/virtio_dma_buf.h>
> > > +
> > > +/**
> > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > + *
> > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > + * for an virtio exported object that can be queried by other virtio drivers
> > > + * for the object's UUID.
> > > + */
> > > +struct dma_buf *virtio_dma_buf_export(
> > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > +{
> > > +     struct dma_buf_export_info exp_info;
> > > +
> > > +     if (!virtio_exp_info->ops
> > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > +             || !virtio_exp_info->ops->get_uuid) {
> > > +             return ERR_PTR(-EINVAL);
> > > +     }
> > > +
> > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > +     exp_info.owner = virtio_exp_info->owner;
> > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > +     exp_info.size = virtio_exp_info->size;
> > > +     exp_info.flags = virtio_exp_info->flags;
> > > +     exp_info.resv = virtio_exp_info->resv;
> > > +     exp_info.priv = virtio_exp_info->priv;
> > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > +                  != sizeof(struct dma_buf_export_info));
> >
> > This is the only part that gives me pause. Why do we need this hack?
> > What's wrong with just using dma_buf_export_info directly,
> > and if you want the virtio ops, just using container_off?
> 
> This approach provides a more explicit type signature and a little
> more type safety, I think. If others don't think it's a worthwhile
> tradeoff, I can remove it.
> 
> -David

The cost is that if dma_buf_export_info changes even slightly, we get
weird crashes.

-- 
MST

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > virtio drivers to obtain the UUID which identifies the underlying
> > > exported object.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> >
> > Is this just for graphics? If yes I'd rather we put it in the graphics
> > driver. We can always move it later ...
> 
> As stated in the cover letter, this will be used by virtio-video.
> 
> The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> The patch which imports these dma-bufs (slightly out of data, uses v3
> of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> 
> > > ---
> > >  drivers/virtio/Makefile         |  2 +-
> > >  drivers/virtio/virtio.c         |  6 +++
> > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > >  include/linux/virtio.h          |  1 +
> > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > >  create mode 100644 include/linux/virtio_dma_buf.h
> > >
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 29a1386ecc03..ecdae5b596de 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -1,5 +1,5 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index a977e32a88f2..5d46f0ded92d 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > >
> > > +bool is_virtio_device(struct device *dev)
> > > +{
> > > +     return dev->bus == &virtio_bus;
> > > +}
> > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > +
> > >  void unregister_virtio_device(struct virtio_device *dev)
> > >  {
> > >       int index = dev->index; /* save for after device release */
> > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > new file mode 100644
> > > index 000000000000..23e3399b11ed
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > @@ -0,0 +1,89 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * dma-bufs for virtio exported objects
> > > + *
> > > + * Copyright (C) 2020 Google, Inc.
> > > + */
> > > +
> > > +#include <linux/virtio_dma_buf.h>
> > > +
> > > +/**
> > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > + *
> > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > + * for an virtio exported object that can be queried by other virtio drivers
> > > + * for the object's UUID.
> > > + */
> > > +struct dma_buf *virtio_dma_buf_export(
> > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > +{
> > > +     struct dma_buf_export_info exp_info;
> > > +
> > > +     if (!virtio_exp_info->ops
> > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > +             || !virtio_exp_info->ops->get_uuid) {
> > > +             return ERR_PTR(-EINVAL);
> > > +     }
> > > +
> > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > +     exp_info.owner = virtio_exp_info->owner;
> > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > +     exp_info.size = virtio_exp_info->size;
> > > +     exp_info.flags = virtio_exp_info->flags;
> > > +     exp_info.resv = virtio_exp_info->resv;
> > > +     exp_info.priv = virtio_exp_info->priv;
> > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > +                  != sizeof(struct dma_buf_export_info));
> >
> > This is the only part that gives me pause. Why do we need this hack?
> > What's wrong with just using dma_buf_export_info directly,
> > and if you want the virtio ops, just using container_off?
> 
> This approach provides a more explicit type signature and a little
> more type safety, I think. If others don't think it's a worthwhile
> tradeoff, I can remove it.
> 
> -David

The cost is that if dma_buf_export_info changes even slightly, we get
weird crashes.

-- 
MST

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > exported object.
> > > >
> > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > >
> > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > driver. We can always move it later ...
> >
> > As stated in the cover letter, this will be used by virtio-video.
> >
> > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > The patch which imports these dma-bufs (slightly out of data, uses v3
> > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> >
> > > > ---
> > > >  drivers/virtio/Makefile         |  2 +-
> > > >  drivers/virtio/virtio.c         |  6 +++
> > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > >  include/linux/virtio.h          |  1 +
> > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > >
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -1,5 +1,5 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > --- a/drivers/virtio/virtio.c
> > > > +++ b/drivers/virtio/virtio.c
> > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > >
> > > > +bool is_virtio_device(struct device *dev)
> > > > +{
> > > > +     return dev->bus == &virtio_bus;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > +
> > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > >  {
> > > >       int index = dev->index; /* save for after device release */
> > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > new file mode 100644
> > > > index 000000000000..23e3399b11ed
> > > > --- /dev/null
> > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > @@ -0,0 +1,89 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * dma-bufs for virtio exported objects
> > > > + *
> > > > + * Copyright (C) 2020 Google, Inc.
> > > > + */
> > > > +
> > > > +#include <linux/virtio_dma_buf.h>
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > + *
> > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > + * for the object's UUID.
> > > > + */
> > > > +struct dma_buf *virtio_dma_buf_export(
> > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > +{
> > > > +     struct dma_buf_export_info exp_info;
> > > > +
> > > > +     if (!virtio_exp_info->ops
> > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > +             return ERR_PTR(-EINVAL);
> > > > +     }
> > > > +
> > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > +     exp_info.size = virtio_exp_info->size;
> > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > +                  != sizeof(struct dma_buf_export_info));
> > >
> > > This is the only part that gives me pause. Why do we need this hack?
> > > What's wrong with just using dma_buf_export_info directly,
> > > and if you want the virtio ops, just using container_off?
> >
> > This approach provides a more explicit type signature and a little
> > more type safety, I think. If others don't think it's a worthwhile
> > tradeoff, I can remove it.
> >
> > -David
>
> The cost is that if dma_buf_export_info changes even slightly, we get
> weird crashes.

I'm not sure I understand what types of changes you're referring to.
As this is written, virtio-dma-buf is just another client of the
dma-buf API. If this were rewritten to use dma-buf directly, then
whatever code calls virtio_dma_buf_export would become a client of the
dma-buf API. If the semantics of existing fields in the dma-buf API
were changed and virtio-dma-buf wasn't updated, then yes, you could
get weird crashes from virtio-dma-buf. However, the same problem would
exist if virtio_dma_buf_export used dma-buf directly - changes to
dma-buf's semantics could cause weird crashes if the caller of
virtio_dma_buf_export wasn't updated properly. The only potential
source of problems I see is if virtio_dma_buf_export_info wasn't
updated properly, but virtio_dma_buf_export_info is dead simple, so I
don't know if that's really a problem.

-David

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > exported object.
> > > >
> > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > >
> > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > driver. We can always move it later ...
> >
> > As stated in the cover letter, this will be used by virtio-video.
> >
> > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > The patch which imports these dma-bufs (slightly out of data, uses v3
> > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> >
> > > > ---
> > > >  drivers/virtio/Makefile         |  2 +-
> > > >  drivers/virtio/virtio.c         |  6 +++
> > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > >  include/linux/virtio.h          |  1 +
> > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > >
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -1,5 +1,5 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > --- a/drivers/virtio/virtio.c
> > > > +++ b/drivers/virtio/virtio.c
> > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > >
> > > > +bool is_virtio_device(struct device *dev)
> > > > +{
> > > > +     return dev->bus == &virtio_bus;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > +
> > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > >  {
> > > >       int index = dev->index; /* save for after device release */
> > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > new file mode 100644
> > > > index 000000000000..23e3399b11ed
> > > > --- /dev/null
> > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > @@ -0,0 +1,89 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * dma-bufs for virtio exported objects
> > > > + *
> > > > + * Copyright (C) 2020 Google, Inc.
> > > > + */
> > > > +
> > > > +#include <linux/virtio_dma_buf.h>
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > + *
> > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > + * for the object's UUID.
> > > > + */
> > > > +struct dma_buf *virtio_dma_buf_export(
> > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > +{
> > > > +     struct dma_buf_export_info exp_info;
> > > > +
> > > > +     if (!virtio_exp_info->ops
> > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > +             return ERR_PTR(-EINVAL);
> > > > +     }
> > > > +
> > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > +     exp_info.size = virtio_exp_info->size;
> > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > +                  != sizeof(struct dma_buf_export_info));
> > >
> > > This is the only part that gives me pause. Why do we need this hack?
> > > What's wrong with just using dma_buf_export_info directly,
> > > and if you want the virtio ops, just using container_off?
> >
> > This approach provides a more explicit type signature and a little
> > more type safety, I think. If others don't think it's a worthwhile
> > tradeoff, I can remove it.
> >
> > -David
>
> The cost is that if dma_buf_export_info changes even slightly, we get
> weird crashes.

I'm not sure I understand what types of changes you're referring to.
As this is written, virtio-dma-buf is just another client of the
dma-buf API. If this were rewritten to use dma-buf directly, then
whatever code calls virtio_dma_buf_export would become a client of the
dma-buf API. If the semantics of existing fields in the dma-buf API
were changed and virtio-dma-buf wasn't updated, then yes, you could
get weird crashes from virtio-dma-buf. However, the same problem would
exist if virtio_dma_buf_export used dma-buf directly - changes to
dma-buf's semantics could cause weird crashes if the caller of
virtio_dma_buf_export wasn't updated properly. The only potential
source of problems I see is if virtio_dma_buf_export_info wasn't
updated properly, but virtio_dma_buf_export_info is dead simple, so I
don't know if that's really a problem.

-David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > exported object.
> > > > >
> > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > >
> > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > driver. We can always move it later ...
> > >
> > > As stated in the cover letter, this will be used by virtio-video.
> > >
> > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > >
> > > > > ---
> > > > >  drivers/virtio/Makefile         |  2 +-
> > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > >  include/linux/virtio.h          |  1 +
> > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > >
> > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > --- a/drivers/virtio/Makefile
> > > > > +++ b/drivers/virtio/Makefile
> > > > > @@ -1,5 +1,5 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > --- a/drivers/virtio/virtio.c
> > > > > +++ b/drivers/virtio/virtio.c
> > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > >
> > > > > +bool is_virtio_device(struct device *dev)
> > > > > +{
> > > > > +     return dev->bus == &virtio_bus;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > +
> > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > >  {
> > > > >       int index = dev->index; /* save for after device release */
> > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > new file mode 100644
> > > > > index 000000000000..23e3399b11ed
> > > > > --- /dev/null
> > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > @@ -0,0 +1,89 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/*
> > > > > + * dma-bufs for virtio exported objects
> > > > > + *
> > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/virtio_dma_buf.h>
> > > > > +
> > > > > +/**
> > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > + *
> > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > + * for the object's UUID.
> > > > > + */
> > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > +{
> > > > > +     struct dma_buf_export_info exp_info;
> > > > > +
> > > > > +     if (!virtio_exp_info->ops
> > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > +             return ERR_PTR(-EINVAL);
> > > > > +     }
> > > > > +
> > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > +                  != sizeof(struct dma_buf_export_info));
> > > >
> > > > This is the only part that gives me pause. Why do we need this hack?
> > > > What's wrong with just using dma_buf_export_info directly,
> > > > and if you want the virtio ops, just using container_off?
> > >
> > > This approach provides a more explicit type signature and a little
> > > more type safety, I think. If others don't think it's a worthwhile
> > > tradeoff, I can remove it.
> > >
> > > -David
> >
> > The cost is that if dma_buf_export_info changes even slightly, we get
> > weird crashes.
> 
> I'm not sure I understand what types of changes you're referring to.
> As this is written, virtio-dma-buf is just another client of the
> dma-buf API. If this were rewritten to use dma-buf directly, then
> whatever code calls virtio_dma_buf_export would become a client of the
> dma-buf API. If the semantics of existing fields in the dma-buf API
> were changed and virtio-dma-buf wasn't updated, then yes, you could
> get weird crashes from virtio-dma-buf.
> However, the same problem would
> exist if virtio_dma_buf_export used dma-buf directly - changes to
> dma-buf's semantics could cause weird crashes if the caller of
> virtio_dma_buf_export wasn't updated properly. The only potential
> source of problems I see is if virtio_dma_buf_export_info wasn't
> updated properly, but virtio_dma_buf_export_info is dead simple, so I
> don't know if that's really a problem.
> 
> -David

I think you can get weird crashes if fields in dma buf are reordered, or
if a field size changes.  You have a build bug catching overall struct
size changes but that can remain the same due do compiler padding or
such.

-- 
MST

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > exported object.
> > > > >
> > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > >
> > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > driver. We can always move it later ...
> > >
> > > As stated in the cover letter, this will be used by virtio-video.
> > >
> > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > >
> > > > > ---
> > > > >  drivers/virtio/Makefile         |  2 +-
> > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > >  include/linux/virtio.h          |  1 +
> > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > >
> > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > --- a/drivers/virtio/Makefile
> > > > > +++ b/drivers/virtio/Makefile
> > > > > @@ -1,5 +1,5 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > --- a/drivers/virtio/virtio.c
> > > > > +++ b/drivers/virtio/virtio.c
> > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > >
> > > > > +bool is_virtio_device(struct device *dev)
> > > > > +{
> > > > > +     return dev->bus == &virtio_bus;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > +
> > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > >  {
> > > > >       int index = dev->index; /* save for after device release */
> > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > new file mode 100644
> > > > > index 000000000000..23e3399b11ed
> > > > > --- /dev/null
> > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > @@ -0,0 +1,89 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/*
> > > > > + * dma-bufs for virtio exported objects
> > > > > + *
> > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/virtio_dma_buf.h>
> > > > > +
> > > > > +/**
> > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > + *
> > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > + * for the object's UUID.
> > > > > + */
> > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > +{
> > > > > +     struct dma_buf_export_info exp_info;
> > > > > +
> > > > > +     if (!virtio_exp_info->ops
> > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > +             return ERR_PTR(-EINVAL);
> > > > > +     }
> > > > > +
> > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > +                  != sizeof(struct dma_buf_export_info));
> > > >
> > > > This is the only part that gives me pause. Why do we need this hack?
> > > > What's wrong with just using dma_buf_export_info directly,
> > > > and if you want the virtio ops, just using container_off?
> > >
> > > This approach provides a more explicit type signature and a little
> > > more type safety, I think. If others don't think it's a worthwhile
> > > tradeoff, I can remove it.
> > >
> > > -David
> >
> > The cost is that if dma_buf_export_info changes even slightly, we get
> > weird crashes.
> 
> I'm not sure I understand what types of changes you're referring to.
> As this is written, virtio-dma-buf is just another client of the
> dma-buf API. If this were rewritten to use dma-buf directly, then
> whatever code calls virtio_dma_buf_export would become a client of the
> dma-buf API. If the semantics of existing fields in the dma-buf API
> were changed and virtio-dma-buf wasn't updated, then yes, you could
> get weird crashes from virtio-dma-buf.
> However, the same problem would
> exist if virtio_dma_buf_export used dma-buf directly - changes to
> dma-buf's semantics could cause weird crashes if the caller of
> virtio_dma_buf_export wasn't updated properly. The only potential
> source of problems I see is if virtio_dma_buf_export_info wasn't
> updated properly, but virtio_dma_buf_export_info is dead simple, so I
> don't know if that's really a problem.
> 
> -David

I think you can get weird crashes if fields in dma buf are reordered, or
if a field size changes.  You have a build bug catching overall struct
size changes but that can remain the same due do compiler padding or
such.

-- 
MST

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > exported object.
> > > > > >
> > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > >
> > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > driver. We can always move it later ...
> > > >
> > > > As stated in the cover letter, this will be used by virtio-video.
> > > >
> > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > >
> > > > > > ---
> > > > > >  drivers/virtio/Makefile         |  2 +-
> > > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > >  include/linux/virtio.h          |  1 +
> > > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > > >
> > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > --- a/drivers/virtio/Makefile
> > > > > > +++ b/drivers/virtio/Makefile
> > > > > > @@ -1,5 +1,5 @@
> > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > --- a/drivers/virtio/virtio.c
> > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > >
> > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > +{
> > > > > > +     return dev->bus == &virtio_bus;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > +
> > > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > > >  {
> > > > > >       int index = dev->index; /* save for after device release */
> > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..23e3399b11ed
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > @@ -0,0 +1,89 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +/*
> > > > > > + * dma-bufs for virtio exported objects
> > > > > > + *
> > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > +
> > > > > > +/**
> > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > + *
> > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > + * for the object's UUID.
> > > > > > + */
> > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > +{
> > > > > > +     struct dma_buf_export_info exp_info;
> > > > > > +
> > > > > > +     if (!virtio_exp_info->ops
> > > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > > +             return ERR_PTR(-EINVAL);
> > > > > > +     }
> > > > > > +
> > > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > +                  != sizeof(struct dma_buf_export_info));
> > > > >
> > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > and if you want the virtio ops, just using container_off?
> > > >
> > > > This approach provides a more explicit type signature and a little
> > > > more type safety, I think. If others don't think it's a worthwhile
> > > > tradeoff, I can remove it.
> > > >
> > > > -David
> > >
> > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > weird crashes.
> >
> > I'm not sure I understand what types of changes you're referring to.
> > As this is written, virtio-dma-buf is just another client of the
> > dma-buf API. If this were rewritten to use dma-buf directly, then
> > whatever code calls virtio_dma_buf_export would become a client of the
> > dma-buf API. If the semantics of existing fields in the dma-buf API
> > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > get weird crashes from virtio-dma-buf.
> > However, the same problem would
> > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > dma-buf's semantics could cause weird crashes if the caller of
> > virtio_dma_buf_export wasn't updated properly. The only potential
> > source of problems I see is if virtio_dma_buf_export_info wasn't
> > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > don't know if that's really a problem.
> >
> > -David
>
> I think you can get weird crashes if fields in dma buf are reordered, or
> if a field size changes.  You have a build bug catching overall struct
> size changes but that can remain the same due do compiler padding or
> such.

Since it's manually copying the fields instead of trying something
clever like memcpy, I don't see how reordering the fields or changing
the size of the fields would cause problems. Right now,
virtio_dma_buf_export is just a regular client of dma_buf_export, no
different than any of the other call sites in the kernel.

Overall, I don't really think that this is a problem. If someone makes
breaking changes to the semantics of dma-buf, then they will need to
update this call site, just like they will need to update all of the
other call sites in the kernel. If someone adds new functionality to
dma-buf and adds another field to dma_buf_export_info, the build bug
is a reminder to add it to virtio_dma_buf_export_info. However, if the
struct padding happens to work out such that the build bug doesn't
trigger, that doesn't really matter - it just means that the new
dma-buf feature won't be exposed by virito-dma-buf until someone needs
it and notices that the new field is missing.

-David

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > exported object.
> > > > > >
> > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > >
> > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > driver. We can always move it later ...
> > > >
> > > > As stated in the cover letter, this will be used by virtio-video.
> > > >
> > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > >
> > > > > > ---
> > > > > >  drivers/virtio/Makefile         |  2 +-
> > > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > >  include/linux/virtio.h          |  1 +
> > > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > > >
> > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > --- a/drivers/virtio/Makefile
> > > > > > +++ b/drivers/virtio/Makefile
> > > > > > @@ -1,5 +1,5 @@
> > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > --- a/drivers/virtio/virtio.c
> > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > >
> > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > +{
> > > > > > +     return dev->bus == &virtio_bus;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > +
> > > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > > >  {
> > > > > >       int index = dev->index; /* save for after device release */
> > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..23e3399b11ed
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > @@ -0,0 +1,89 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +/*
> > > > > > + * dma-bufs for virtio exported objects
> > > > > > + *
> > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > +
> > > > > > +/**
> > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > + *
> > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > + * for the object's UUID.
> > > > > > + */
> > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > +{
> > > > > > +     struct dma_buf_export_info exp_info;
> > > > > > +
> > > > > > +     if (!virtio_exp_info->ops
> > > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > > +             return ERR_PTR(-EINVAL);
> > > > > > +     }
> > > > > > +
> > > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > +                  != sizeof(struct dma_buf_export_info));
> > > > >
> > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > and if you want the virtio ops, just using container_off?
> > > >
> > > > This approach provides a more explicit type signature and a little
> > > > more type safety, I think. If others don't think it's a worthwhile
> > > > tradeoff, I can remove it.
> > > >
> > > > -David
> > >
> > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > weird crashes.
> >
> > I'm not sure I understand what types of changes you're referring to.
> > As this is written, virtio-dma-buf is just another client of the
> > dma-buf API. If this were rewritten to use dma-buf directly, then
> > whatever code calls virtio_dma_buf_export would become a client of the
> > dma-buf API. If the semantics of existing fields in the dma-buf API
> > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > get weird crashes from virtio-dma-buf.
> > However, the same problem would
> > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > dma-buf's semantics could cause weird crashes if the caller of
> > virtio_dma_buf_export wasn't updated properly. The only potential
> > source of problems I see is if virtio_dma_buf_export_info wasn't
> > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > don't know if that's really a problem.
> >
> > -David
>
> I think you can get weird crashes if fields in dma buf are reordered, or
> if a field size changes.  You have a build bug catching overall struct
> size changes but that can remain the same due do compiler padding or
> such.

Since it's manually copying the fields instead of trying something
clever like memcpy, I don't see how reordering the fields or changing
the size of the fields would cause problems. Right now,
virtio_dma_buf_export is just a regular client of dma_buf_export, no
different than any of the other call sites in the kernel.

Overall, I don't really think that this is a problem. If someone makes
breaking changes to the semantics of dma-buf, then they will need to
update this call site, just like they will need to update all of the
other call sites in the kernel. If someone adds new functionality to
dma-buf and adds another field to dma_buf_export_info, the build bug
is a reminder to add it to virtio_dma_buf_export_info. However, if the
struct padding happens to work out such that the build bug doesn't
trigger, that doesn't really matter - it just means that the new
dma-buf feature won't be exposed by virito-dma-buf until someone needs
it and notices that the new field is missing.

-David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 05:32:26PM +0900, David Stevens wrote:
> On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > > exported object.
> > > > > > >
> > > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > > >
> > > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > > driver. We can always move it later ...
> > > > >
> > > > > As stated in the cover letter, this will be used by virtio-video.
> > > > >
> > > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > > >
> > > > > > > ---
> > > > > > >  drivers/virtio/Makefile         |  2 +-
> > > > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > >  include/linux/virtio.h          |  1 +
> > > > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > > --- a/drivers/virtio/Makefile
> > > > > > > +++ b/drivers/virtio/Makefile
> > > > > > > @@ -1,5 +1,5 @@
> > > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > > >
> > > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > > +{
> > > > > > > +     return dev->bus == &virtio_bus;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > > +
> > > > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > > > >  {
> > > > > > >       int index = dev->index; /* save for after device release */
> > > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..23e3399b11ed
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > > @@ -0,0 +1,89 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > +/*
> > > > > > > + * dma-bufs for virtio exported objects
> > > > > > > + *
> > > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > > + *
> > > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > > + * for the object's UUID.
> > > > > > > + */
> > > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > > +{
> > > > > > > +     struct dma_buf_export_info exp_info;
> > > > > > > +
> > > > > > > +     if (!virtio_exp_info->ops
> > > > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > > > +             return ERR_PTR(-EINVAL);
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > > +                  != sizeof(struct dma_buf_export_info));
> > > > > >
> > > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > > and if you want the virtio ops, just using container_off?
> > > > >
> > > > > This approach provides a more explicit type signature and a little
> > > > > more type safety, I think. If others don't think it's a worthwhile
> > > > > tradeoff, I can remove it.
> > > > >
> > > > > -David
> > > >
> > > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > > weird crashes.
> > >
> > > I'm not sure I understand what types of changes you're referring to.
> > > As this is written, virtio-dma-buf is just another client of the
> > > dma-buf API. If this were rewritten to use dma-buf directly, then
> > > whatever code calls virtio_dma_buf_export would become a client of the
> > > dma-buf API. If the semantics of existing fields in the dma-buf API
> > > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > > get weird crashes from virtio-dma-buf.
> > > However, the same problem would
> > > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > > dma-buf's semantics could cause weird crashes if the caller of
> > > virtio_dma_buf_export wasn't updated properly. The only potential
> > > source of problems I see is if virtio_dma_buf_export_info wasn't
> > > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > > don't know if that's really a problem.
> > >
> > > -David
> >
> > I think you can get weird crashes if fields in dma buf are reordered, or
> > if a field size changes.  You have a build bug catching overall struct
> > size changes but that can remain the same due do compiler padding or
> > such.
> 
> Since it's manually copying the fields instead of trying something
> clever like memcpy, I don't see how reordering the fields or changing
> the size of the fields would cause problems. Right now,
> virtio_dma_buf_export is just a regular client of dma_buf_export, no
> different than any of the other call sites in the kernel.
> 
> Overall, I don't really think that this is a problem. If someone makes
> breaking changes to the semantics of dma-buf, then they will need to
> update this call site, just like they will need to update all of the
> other call sites in the kernel. If someone adds new functionality to
> dma-buf and adds another field to dma_buf_export_info, the build bug
> is a reminder to add it to virtio_dma_buf_export_info. However, if the
> struct padding happens to work out such that the build bug doesn't
> trigger, that doesn't really matter - it just means that the new
> dma-buf feature won't be exposed by virito-dma-buf until someone needs
> it and notices that the new field is missing.
> 
> -David

Think about the reasons for the BUILD_BUG_ON being there, checking
struct sizes like this is a clear sign of something strange going on.


But really this is just unnecessary complexity anyway.

The only difference with dma_buf is get_uuid and device_attacj, isn't it?

And they are called like this:



+ */
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
+                           uuid_t *uuid)
+{
+       const struct virtio_dma_buf_ops *ops = container_of(
+                       dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+       
+       if (!is_virtio_dma_buf(dma_buf))
+               return -EINVAL;
+
+       return ops->get_uuid(dma_buf, uuid);
+}


So you are doing the container_of trick anyway, the extra structure
did not give us any type safety.


-- 
MST

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 05:32:26PM +0900, David Stevens wrote:
> On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > > exported object.
> > > > > > >
> > > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > > >
> > > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > > driver. We can always move it later ...
> > > > >
> > > > > As stated in the cover letter, this will be used by virtio-video.
> > > > >
> > > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > > >
> > > > > > > ---
> > > > > > >  drivers/virtio/Makefile         |  2 +-
> > > > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > >  include/linux/virtio.h          |  1 +
> > > > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > > --- a/drivers/virtio/Makefile
> > > > > > > +++ b/drivers/virtio/Makefile
> > > > > > > @@ -1,5 +1,5 @@
> > > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > > >
> > > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > > +{
> > > > > > > +     return dev->bus == &virtio_bus;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > > +
> > > > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > > > >  {
> > > > > > >       int index = dev->index; /* save for after device release */
> > > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..23e3399b11ed
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > > @@ -0,0 +1,89 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > +/*
> > > > > > > + * dma-bufs for virtio exported objects
> > > > > > > + *
> > > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > > + *
> > > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > > + * for the object's UUID.
> > > > > > > + */
> > > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > > +{
> > > > > > > +     struct dma_buf_export_info exp_info;
> > > > > > > +
> > > > > > > +     if (!virtio_exp_info->ops
> > > > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > > > +             return ERR_PTR(-EINVAL);
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > > +                  != sizeof(struct dma_buf_export_info));
> > > > > >
> > > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > > and if you want the virtio ops, just using container_off?
> > > > >
> > > > > This approach provides a more explicit type signature and a little
> > > > > more type safety, I think. If others don't think it's a worthwhile
> > > > > tradeoff, I can remove it.
> > > > >
> > > > > -David
> > > >
> > > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > > weird crashes.
> > >
> > > I'm not sure I understand what types of changes you're referring to.
> > > As this is written, virtio-dma-buf is just another client of the
> > > dma-buf API. If this were rewritten to use dma-buf directly, then
> > > whatever code calls virtio_dma_buf_export would become a client of the
> > > dma-buf API. If the semantics of existing fields in the dma-buf API
> > > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > > get weird crashes from virtio-dma-buf.
> > > However, the same problem would
> > > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > > dma-buf's semantics could cause weird crashes if the caller of
> > > virtio_dma_buf_export wasn't updated properly. The only potential
> > > source of problems I see is if virtio_dma_buf_export_info wasn't
> > > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > > don't know if that's really a problem.
> > >
> > > -David
> >
> > I think you can get weird crashes if fields in dma buf are reordered, or
> > if a field size changes.  You have a build bug catching overall struct
> > size changes but that can remain the same due do compiler padding or
> > such.
> 
> Since it's manually copying the fields instead of trying something
> clever like memcpy, I don't see how reordering the fields or changing
> the size of the fields would cause problems. Right now,
> virtio_dma_buf_export is just a regular client of dma_buf_export, no
> different than any of the other call sites in the kernel.
> 
> Overall, I don't really think that this is a problem. If someone makes
> breaking changes to the semantics of dma-buf, then they will need to
> update this call site, just like they will need to update all of the
> other call sites in the kernel. If someone adds new functionality to
> dma-buf and adds another field to dma_buf_export_info, the build bug
> is a reminder to add it to virtio_dma_buf_export_info. However, if the
> struct padding happens to work out such that the build bug doesn't
> trigger, that doesn't really matter - it just means that the new
> dma-buf feature won't be exposed by virito-dma-buf until someone needs
> it and notices that the new field is missing.
> 
> -David

Think about the reasons for the BUILD_BUG_ON being there, checking
struct sizes like this is a clear sign of something strange going on.


But really this is just unnecessary complexity anyway.

The only difference with dma_buf is get_uuid and device_attacj, isn't it?

And they are called like this:



+ */
+int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
+                           uuid_t *uuid)
+{
+       const struct virtio_dma_buf_ops *ops = container_of(
+                       dma_buf->ops, const struct virtio_dma_buf_ops, ops);
+       
+       if (!is_virtio_dma_buf(dma_buf))
+               return -EINVAL;
+
+       return ops->get_uuid(dma_buf, uuid);
+}


So you are doing the container_of trick anyway, the extra structure
did not give us any type safety.


-- 
MST

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Mon, Jun 8, 2020 at 6:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 08, 2020 at 05:32:26PM +0900, David Stevens wrote:
> > On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > > > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > > > exported object.
> > > > > > > >
> > > > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > > > >
> > > > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > > > driver. We can always move it later ...
> > > > > >
> > > > > > As stated in the cover letter, this will be used by virtio-video.
> > > > > >
> > > > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/virtio/Makefile         |  2 +-
> > > > > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > > >  include/linux/virtio.h          |  1 +
> > > > > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > > > --- a/drivers/virtio/Makefile
> > > > > > > > +++ b/drivers/virtio/Makefile
> > > > > > > > @@ -1,5 +1,5 @@
> > > > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > > > >
> > > > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > > > +{
> > > > > > > > +     return dev->bus == &virtio_bus;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > > > +
> > > > > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > > > > >  {
> > > > > > > >       int index = dev->index; /* save for after device release */
> > > > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..23e3399b11ed
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > > > @@ -0,0 +1,89 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > > +/*
> > > > > > > > + * dma-bufs for virtio exported objects
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > > > + *
> > > > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > > > + * for the object's UUID.
> > > > > > > > + */
> > > > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > > > +{
> > > > > > > > +     struct dma_buf_export_info exp_info;
> > > > > > > > +
> > > > > > > > +     if (!virtio_exp_info->ops
> > > > > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > > > > +             return ERR_PTR(-EINVAL);
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > > > +                  != sizeof(struct dma_buf_export_info));
> > > > > > >
> > > > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > > > and if you want the virtio ops, just using container_off?
> > > > > >
> > > > > > This approach provides a more explicit type signature and a little
> > > > > > more type safety, I think. If others don't think it's a worthwhile
> > > > > > tradeoff, I can remove it.
> > > > > >
> > > > > > -David
> > > > >
> > > > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > > > weird crashes.
> > > >
> > > > I'm not sure I understand what types of changes you're referring to.
> > > > As this is written, virtio-dma-buf is just another client of the
> > > > dma-buf API. If this were rewritten to use dma-buf directly, then
> > > > whatever code calls virtio_dma_buf_export would become a client of the
> > > > dma-buf API. If the semantics of existing fields in the dma-buf API
> > > > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > > > get weird crashes from virtio-dma-buf.
> > > > However, the same problem would
> > > > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > > > dma-buf's semantics could cause weird crashes if the caller of
> > > > virtio_dma_buf_export wasn't updated properly. The only potential
> > > > source of problems I see is if virtio_dma_buf_export_info wasn't
> > > > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > > > don't know if that's really a problem.
> > > >
> > > > -David
> > >
> > > I think you can get weird crashes if fields in dma buf are reordered, or
> > > if a field size changes.  You have a build bug catching overall struct
> > > size changes but that can remain the same due do compiler padding or
> > > such.
> >
> > Since it's manually copying the fields instead of trying something
> > clever like memcpy, I don't see how reordering the fields or changing
> > the size of the fields would cause problems. Right now,
> > virtio_dma_buf_export is just a regular client of dma_buf_export, no
> > different than any of the other call sites in the kernel.
> >
> > Overall, I don't really think that this is a problem. If someone makes
> > breaking changes to the semantics of dma-buf, then they will need to
> > update this call site, just like they will need to update all of the
> > other call sites in the kernel. If someone adds new functionality to
> > dma-buf and adds another field to dma_buf_export_info, the build bug
> > is a reminder to add it to virtio_dma_buf_export_info. However, if the
> > struct padding happens to work out such that the build bug doesn't
> > trigger, that doesn't really matter - it just means that the new
> > dma-buf feature won't be exposed by virito-dma-buf until someone needs
> > it and notices that the new field is missing.
> >
> > -David
>
> Think about the reasons for the BUILD_BUG_ON being there, checking
> struct sizes like this is a clear sign of something strange going on.

Like I said, it's there as a reminder to update the virtio-dma-buf API
if the dma-buf API gets updated. Perhaps you could say it's a misuse
of BUILD_BUG_ON, since not updating virtio-dma-buf in such a situation
isn't a bug per se. If the BUILD_BUG_ON actually caught a real bug,
there would be a much more fundamental issue going on. The code is
doing nothing strange, nothing fundamentally different from any other
call such as in drm_gem_prime_export or udmabuf_create - it's just
setting fields in the dma_buf_export_info struct to values.

> But really this is just unnecessary complexity anyway.
>
> The only difference with dma_buf is get_uuid and device_attacj, isn't it?
>
> And they are called like this:
>
>
>
> + */
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> +                           uuid_t *uuid)
> +{
> +       const struct virtio_dma_buf_ops *ops = container_of(
> +                       dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> +       if (!is_virtio_dma_buf(dma_buf))
> +               return -EINVAL;
> +
> +       return ops->get_uuid(dma_buf, uuid);
> +}
>
>
> So you are doing the container_of trick anyway, the extra structure
> did not give us any type safety.

Lack of type safety in one situation doesn't mean that type safety in
another situation isn't worthwhile. If that were the case, then why
does C have typedef? Why does Linux bother with types at all, since
you can cast anything to anything with strict aliasing disabled? We
can still make the virtio_dma_buf_export function more type-safe and
readable while still having some unavoidable casting.

But that being said, it's not worth further bikeshedding. I'll send
out an updated patch set.

-David

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Mon, Jun 8, 2020 at 6:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 08, 2020 at 05:32:26PM +0900, David Stevens wrote:
> > On Mon, Jun 8, 2020 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jun 08, 2020 at 10:33:09AM +0900, David Stevens wrote:
> > > > On Sun, Jun 7, 2020 at 5:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jun 05, 2020 at 10:28:42AM +0900, David Stevens wrote:
> > > > > > On Fri, Jun 5, 2020 at 4:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > > > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > > > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > > > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > > > > > exported object.
> > > > > > > >
> > > > > > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > > > >
> > > > > > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > > > > > driver. We can always move it later ...
> > > > > >
> > > > > > As stated in the cover letter, this will be used by virtio-video.
> > > > > >
> > > > > > The proposed virtio-video patches: https://markmail.org/thread/p5d3k566srtdtute
> > > > > > The patch which imports these dma-bufs (slightly out of data, uses v3
> > > > > > of this patch set): https://markmail.org/thread/j4xlqaaim266qpks
> > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/virtio/Makefile         |  2 +-
> > > > > > > >  drivers/virtio/virtio.c         |  6 +++
> > > > > > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > > > > > >  include/linux/virtio.h          |  1 +
> > > > > > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > > > > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > > > > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > > > > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > > > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > > > > > --- a/drivers/virtio/Makefile
> > > > > > > > +++ b/drivers/virtio/Makefile
> > > > > > > > @@ -1,5 +1,5 @@
> > > > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > > > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > > > > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > > > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > > > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > > > > > --- a/drivers/virtio/virtio.c
> > > > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > > > > > >
> > > > > > > > +bool is_virtio_device(struct device *dev)
> > > > > > > > +{
> > > > > > > > +     return dev->bus == &virtio_bus;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > > > > > +
> > > > > > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > > > > > >  {
> > > > > > > >       int index = dev->index; /* save for after device release */
> > > > > > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..23e3399b11ed
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > > > > > @@ -0,0 +1,89 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > > +/*
> > > > > > > > + * dma-bufs for virtio exported objects
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2020 Google, Inc.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/virtio_dma_buf.h>
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > > > > > + *
> > > > > > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > > > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > > > > > + * for the object's UUID.
> > > > > > > > + */
> > > > > > > > +struct dma_buf *virtio_dma_buf_export(
> > > > > > > > +             const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > > > > > +{
> > > > > > > > +     struct dma_buf_export_info exp_info;
> > > > > > > > +
> > > > > > > > +     if (!virtio_exp_info->ops
> > > > > > > > +             || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > > > > > +             || !virtio_exp_info->ops->get_uuid) {
> > > > > > > > +             return ERR_PTR(-EINVAL);
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     exp_info.exp_name = virtio_exp_info->exp_name;
> > > > > > > > +     exp_info.owner = virtio_exp_info->owner;
> > > > > > > > +     exp_info.ops = &virtio_exp_info->ops->ops;
> > > > > > > > +     exp_info.size = virtio_exp_info->size;
> > > > > > > > +     exp_info.flags = virtio_exp_info->flags;
> > > > > > > > +     exp_info.resv = virtio_exp_info->resv;
> > > > > > > > +     exp_info.priv = virtio_exp_info->priv;
> > > > > > > > +     BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > > > > > +                  != sizeof(struct dma_buf_export_info));
> > > > > > >
> > > > > > > This is the only part that gives me pause. Why do we need this hack?
> > > > > > > What's wrong with just using dma_buf_export_info directly,
> > > > > > > and if you want the virtio ops, just using container_off?
> > > > > >
> > > > > > This approach provides a more explicit type signature and a little
> > > > > > more type safety, I think. If others don't think it's a worthwhile
> > > > > > tradeoff, I can remove it.
> > > > > >
> > > > > > -David
> > > > >
> > > > > The cost is that if dma_buf_export_info changes even slightly, we get
> > > > > weird crashes.
> > > >
> > > > I'm not sure I understand what types of changes you're referring to.
> > > > As this is written, virtio-dma-buf is just another client of the
> > > > dma-buf API. If this were rewritten to use dma-buf directly, then
> > > > whatever code calls virtio_dma_buf_export would become a client of the
> > > > dma-buf API. If the semantics of existing fields in the dma-buf API
> > > > were changed and virtio-dma-buf wasn't updated, then yes, you could
> > > > get weird crashes from virtio-dma-buf.
> > > > However, the same problem would
> > > > exist if virtio_dma_buf_export used dma-buf directly - changes to
> > > > dma-buf's semantics could cause weird crashes if the caller of
> > > > virtio_dma_buf_export wasn't updated properly. The only potential
> > > > source of problems I see is if virtio_dma_buf_export_info wasn't
> > > > updated properly, but virtio_dma_buf_export_info is dead simple, so I
> > > > don't know if that's really a problem.
> > > >
> > > > -David
> > >
> > > I think you can get weird crashes if fields in dma buf are reordered, or
> > > if a field size changes.  You have a build bug catching overall struct
> > > size changes but that can remain the same due do compiler padding or
> > > such.
> >
> > Since it's manually copying the fields instead of trying something
> > clever like memcpy, I don't see how reordering the fields or changing
> > the size of the fields would cause problems. Right now,
> > virtio_dma_buf_export is just a regular client of dma_buf_export, no
> > different than any of the other call sites in the kernel.
> >
> > Overall, I don't really think that this is a problem. If someone makes
> > breaking changes to the semantics of dma-buf, then they will need to
> > update this call site, just like they will need to update all of the
> > other call sites in the kernel. If someone adds new functionality to
> > dma-buf and adds another field to dma_buf_export_info, the build bug
> > is a reminder to add it to virtio_dma_buf_export_info. However, if the
> > struct padding happens to work out such that the build bug doesn't
> > trigger, that doesn't really matter - it just means that the new
> > dma-buf feature won't be exposed by virito-dma-buf until someone needs
> > it and notices that the new field is missing.
> >
> > -David
>
> Think about the reasons for the BUILD_BUG_ON being there, checking
> struct sizes like this is a clear sign of something strange going on.

Like I said, it's there as a reminder to update the virtio-dma-buf API
if the dma-buf API gets updated. Perhaps you could say it's a misuse
of BUILD_BUG_ON, since not updating virtio-dma-buf in such a situation
isn't a bug per se. If the BUILD_BUG_ON actually caught a real bug,
there would be a much more fundamental issue going on. The code is
doing nothing strange, nothing fundamentally different from any other
call such as in drm_gem_prime_export or udmabuf_create - it's just
setting fields in the dma_buf_export_info struct to values.

> But really this is just unnecessary complexity anyway.
>
> The only difference with dma_buf is get_uuid and device_attacj, isn't it?
>
> And they are called like this:
>
>
>
> + */
> +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> +                           uuid_t *uuid)
> +{
> +       const struct virtio_dma_buf_ops *ops = container_of(
> +                       dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> +
> +       if (!is_virtio_dma_buf(dma_buf))
> +               return -EINVAL;
> +
> +       return ops->get_uuid(dma_buf, uuid);
> +}
>
>
> So you are doing the container_of trick anyway, the extra structure
> did not give us any type safety.

Lack of type safety in one situation doesn't mean that type safety in
another situation isn't worthwhile. If that were the case, then why
does C have typedef? Why does Linux bother with types at all, since
you can cast anything to anything with strict aliasing disabled? We
can still make the virtio_dma_buf_export function more type-safe and
readable while still having some unavoidable casting.

But that being said, it's not worth further bikeshedding. I'll send
out an updated patch set.

-David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Guennadi Liakhovetski]

Hi Michael,

On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > This change adds a new flavor of dma-bufs that can be used by virtio
> > drivers to share exported objects. A virtio dma-buf can be queried by
> > virtio drivers to obtain the UUID which identifies the underlying
> > exported object.
> > 
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> 
> Is this just for graphics? If yes I'd rather we put it in the graphics
> driver. We can always move it later ...

Wouldn't this be the API that audio virtualisation will have to use to share 
buffers between the host and any guests?

Thanks
Guennadi

> > ---
> >  drivers/virtio/Makefile         |  2 +-
> >  drivers/virtio/virtio.c         |  6 +++
> >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h          |  1 +
> >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> >  5 files changed, 155 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> >  create mode 100644 include/linux/virtio_dma_buf.h
> > 
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 29a1386ecc03..ecdae5b596de 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a977e32a88f2..5d46f0ded92d 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(register_virtio_device);
> >  
> > +bool is_virtio_device(struct device *dev)
> > +{
> > +	return dev->bus == &virtio_bus;
> > +}
> > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > +
> >  void unregister_virtio_device(struct virtio_device *dev)
> >  {
> >  	int index = dev->index; /* save for after device release */
> > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > new file mode 100644
> > index 000000000000..23e3399b11ed
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_dma_buf.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#include <linux/virtio_dma_buf.h>
> > +
> > +/**
> > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > + *
> > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > + * for an virtio exported object that can be queried by other virtio drivers
> > + * for the object's UUID.
> > + */
> > +struct dma_buf *virtio_dma_buf_export(
> > +		const struct virtio_dma_buf_export_info *virtio_exp_info)
> > +{
> > +	struct dma_buf_export_info exp_info;
> > +
> > +	if (!virtio_exp_info->ops
> > +		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > +		|| !virtio_exp_info->ops->get_uuid) {
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	exp_info.exp_name = virtio_exp_info->exp_name;
> > +	exp_info.owner = virtio_exp_info->owner;
> > +	exp_info.ops = &virtio_exp_info->ops->ops;
> > +	exp_info.size = virtio_exp_info->size;
> > +	exp_info.flags = virtio_exp_info->flags;
> > +	exp_info.resv = virtio_exp_info->resv;
> > +	exp_info.priv = virtio_exp_info->priv;
> > +	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > +		     != sizeof(struct dma_buf_export_info));
> 
> This is the only part that gives me pause. Why do we need this hack?
> What's wrong with just using dma_buf_export_info directly,
> and if you want the virtio ops, just using container_off?
> 
> 
> 
> > +
> > +	return dma_buf_export(&exp_info);
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > +
> > +/**
> > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > + */
> > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > +			  struct dma_buf_attachment *attach)
> > +{
> > +	int ret;
> > +	const struct virtio_dma_buf_ops *ops = container_of(
> > +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > +
> > +	if (ops->device_attach) {
> > +		ret = ops->device_attach(dma_buf, attach);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > +
> > +/**
> > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > + * @dma_buf: buffer to query
> > + */
> > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > +{
> > +	return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > +}
> > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > +
> > +/**
> > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > + * @dma_buf: [in] buffer to query
> > + * @uuid: [out] the uuid
> > + *
> > + * Returns: 0 on success, negative on failure.
> > + */
> > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > +			    uuid_t *uuid)
> > +{
> > +	const struct virtio_dma_buf_ops *ops = container_of(
> > +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > +
> > +	if (!is_virtio_dma_buf(dma_buf))
> > +		return -EINVAL;
> > +
> > +	return ops->get_uuid(dma_buf, uuid);
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 15f906e4a748..9397e25616c4 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> >  int register_virtio_device(struct virtio_device *dev);
> >  void unregister_virtio_device(struct virtio_device *dev);
> > +bool is_virtio_device(struct device *dev);
> >  
> >  void virtio_break_device(struct virtio_device *dev);
> >  
> > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > new file mode 100644
> > index 000000000000..29fee167afbd
> > --- /dev/null
> > +++ b/include/linux/virtio_dma_buf.h
> > @@ -0,0 +1,58 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > +#define _LINUX_VIRTIO_DMA_BUF_H
> > +
> > +#include <linux/dma-buf.h>
> > +#include <linux/uuid.h>
> > +#include <linux/virtio.h>
> > +
> > +/**
> > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > + *		   all attach operations.
> > + * @get_uid: [required] callback to get the uuid of the exported object.
> > + */
> > +struct virtio_dma_buf_ops {
> > +	struct dma_buf_ops ops;
> > +	int (*device_attach)(struct dma_buf *dma_buf,
> > +			     struct dma_buf_attachment *attach);
> > +	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > +};
> > +
> > +/**
> > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > + */
> > +struct virtio_dma_buf_export_info {
> > +	const char *exp_name;
> > +	struct module *owner;
> > +	const struct virtio_dma_buf_ops *ops;
> > +	size_t size;
> > +	int flags;
> > +	struct dma_resv *resv;
> > +	void *priv;
> > +};
> > +
> > +/**
> > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > + */
> > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
> > +	struct virtio_dma_buf_export_info name = { \
> > +		.exp_name = KBUILD_MODNAME, \
> > +		.owner = THIS_MODULE }
> > +
> > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > +			  struct dma_buf_attachment *attach);
> > +
> > +struct dma_buf *virtio_dma_buf_export(
> > +		const struct virtio_dma_buf_export_info *virtio_exp_info);
> > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > +
> > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > -- 
> > 2.27.0.rc0.183.gde8f92d652-goog
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Guennadi Liakhovetski]

Hi Michael,

On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > This change adds a new flavor of dma-bufs that can be used by virtio
> > drivers to share exported objects. A virtio dma-buf can be queried by
> > virtio drivers to obtain the UUID which identifies the underlying
> > exported object.
> > 
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> 
> Is this just for graphics? If yes I'd rather we put it in the graphics
> driver. We can always move it later ...

Wouldn't this be the API that audio virtualisation will have to use to share 
buffers between the host and any guests?

Thanks
Guennadi

> > ---
> >  drivers/virtio/Makefile         |  2 +-
> >  drivers/virtio/virtio.c         |  6 +++
> >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h          |  1 +
> >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> >  5 files changed, 155 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> >  create mode 100644 include/linux/virtio_dma_buf.h
> > 
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 29a1386ecc03..ecdae5b596de 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a977e32a88f2..5d46f0ded92d 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(register_virtio_device);
> >  
> > +bool is_virtio_device(struct device *dev)
> > +{
> > +	return dev->bus == &virtio_bus;
> > +}
> > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > +
> >  void unregister_virtio_device(struct virtio_device *dev)
> >  {
> >  	int index = dev->index; /* save for after device release */
> > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > new file mode 100644
> > index 000000000000..23e3399b11ed
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_dma_buf.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#include <linux/virtio_dma_buf.h>
> > +
> > +/**
> > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > + *
> > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > + * for an virtio exported object that can be queried by other virtio drivers
> > + * for the object's UUID.
> > + */
> > +struct dma_buf *virtio_dma_buf_export(
> > +		const struct virtio_dma_buf_export_info *virtio_exp_info)
> > +{
> > +	struct dma_buf_export_info exp_info;
> > +
> > +	if (!virtio_exp_info->ops
> > +		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > +		|| !virtio_exp_info->ops->get_uuid) {
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	exp_info.exp_name = virtio_exp_info->exp_name;
> > +	exp_info.owner = virtio_exp_info->owner;
> > +	exp_info.ops = &virtio_exp_info->ops->ops;
> > +	exp_info.size = virtio_exp_info->size;
> > +	exp_info.flags = virtio_exp_info->flags;
> > +	exp_info.resv = virtio_exp_info->resv;
> > +	exp_info.priv = virtio_exp_info->priv;
> > +	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > +		     != sizeof(struct dma_buf_export_info));
> 
> This is the only part that gives me pause. Why do we need this hack?
> What's wrong with just using dma_buf_export_info directly,
> and if you want the virtio ops, just using container_off?
> 
> 
> 
> > +
> > +	return dma_buf_export(&exp_info);
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > +
> > +/**
> > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > + */
> > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > +			  struct dma_buf_attachment *attach)
> > +{
> > +	int ret;
> > +	const struct virtio_dma_buf_ops *ops = container_of(
> > +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > +
> > +	if (ops->device_attach) {
> > +		ret = ops->device_attach(dma_buf, attach);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > +
> > +/**
> > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > + * @dma_buf: buffer to query
> > + */
> > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > +{
> > +	return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > +}
> > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > +
> > +/**
> > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > + * @dma_buf: [in] buffer to query
> > + * @uuid: [out] the uuid
> > + *
> > + * Returns: 0 on success, negative on failure.
> > + */
> > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > +			    uuid_t *uuid)
> > +{
> > +	const struct virtio_dma_buf_ops *ops = container_of(
> > +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > +
> > +	if (!is_virtio_dma_buf(dma_buf))
> > +		return -EINVAL;
> > +
> > +	return ops->get_uuid(dma_buf, uuid);
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 15f906e4a748..9397e25616c4 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> >  int register_virtio_device(struct virtio_device *dev);
> >  void unregister_virtio_device(struct virtio_device *dev);
> > +bool is_virtio_device(struct device *dev);
> >  
> >  void virtio_break_device(struct virtio_device *dev);
> >  
> > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > new file mode 100644
> > index 000000000000..29fee167afbd
> > --- /dev/null
> > +++ b/include/linux/virtio_dma_buf.h
> > @@ -0,0 +1,58 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > +#define _LINUX_VIRTIO_DMA_BUF_H
> > +
> > +#include <linux/dma-buf.h>
> > +#include <linux/uuid.h>
> > +#include <linux/virtio.h>
> > +
> > +/**
> > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > + *		   all attach operations.
> > + * @get_uid: [required] callback to get the uuid of the exported object.
> > + */
> > +struct virtio_dma_buf_ops {
> > +	struct dma_buf_ops ops;
> > +	int (*device_attach)(struct dma_buf *dma_buf,
> > +			     struct dma_buf_attachment *attach);
> > +	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > +};
> > +
> > +/**
> > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > + */
> > +struct virtio_dma_buf_export_info {
> > +	const char *exp_name;
> > +	struct module *owner;
> > +	const struct virtio_dma_buf_ops *ops;
> > +	size_t size;
> > +	int flags;
> > +	struct dma_resv *resv;
> > +	void *priv;
> > +};
> > +
> > +/**
> > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > + */
> > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
> > +	struct virtio_dma_buf_export_info name = { \
> > +		.exp_name = KBUILD_MODNAME, \
> > +		.owner = THIS_MODULE }
> > +
> > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > +			  struct dma_buf_attachment *attach);
> > +
> > +struct dma_buf *virtio_dma_buf_export(
> > +		const struct virtio_dma_buf_export_info *virtio_exp_info);
> > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > +
> > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > -- 
> > 2.27.0.rc0.183.gde8f92d652-goog
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Guennadi Liakhovetski]

Hi Michael,

On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > This change adds a new flavor of dma-bufs that can be used by virtio
> > drivers to share exported objects. A virtio dma-buf can be queried by
> > virtio drivers to obtain the UUID which identifies the underlying
> > exported object.
> > 
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> 
> Is this just for graphics? If yes I'd rather we put it in the graphics
> driver. We can always move it later ...

Wouldn't this be the API that audio virtualisation will have to use to share 
buffers between the host and any guests?

Thanks
Guennadi

> > ---
> >  drivers/virtio/Makefile         |  2 +-
> >  drivers/virtio/virtio.c         |  6 +++
> >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h          |  1 +
> >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> >  5 files changed, 155 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> >  create mode 100644 include/linux/virtio_dma_buf.h
> > 
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 29a1386ecc03..ecdae5b596de 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a977e32a88f2..5d46f0ded92d 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(register_virtio_device);
> >  
> > +bool is_virtio_device(struct device *dev)
> > +{
> > +	return dev->bus == &virtio_bus;
> > +}
> > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > +
> >  void unregister_virtio_device(struct virtio_device *dev)
> >  {
> >  	int index = dev->index; /* save for after device release */
> > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > new file mode 100644
> > index 000000000000..23e3399b11ed
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_dma_buf.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#include <linux/virtio_dma_buf.h>
> > +
> > +/**
> > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > + *
> > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > + * for an virtio exported object that can be queried by other virtio drivers
> > + * for the object's UUID.
> > + */
> > +struct dma_buf *virtio_dma_buf_export(
> > +		const struct virtio_dma_buf_export_info *virtio_exp_info)
> > +{
> > +	struct dma_buf_export_info exp_info;
> > +
> > +	if (!virtio_exp_info->ops
> > +		|| virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > +		|| !virtio_exp_info->ops->get_uuid) {
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	exp_info.exp_name = virtio_exp_info->exp_name;
> > +	exp_info.owner = virtio_exp_info->owner;
> > +	exp_info.ops = &virtio_exp_info->ops->ops;
> > +	exp_info.size = virtio_exp_info->size;
> > +	exp_info.flags = virtio_exp_info->flags;
> > +	exp_info.resv = virtio_exp_info->resv;
> > +	exp_info.priv = virtio_exp_info->priv;
> > +	BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > +		     != sizeof(struct dma_buf_export_info));
> 
> This is the only part that gives me pause. Why do we need this hack?
> What's wrong with just using dma_buf_export_info directly,
> and if you want the virtio ops, just using container_off?
> 
> 
> 
> > +
> > +	return dma_buf_export(&exp_info);
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > +
> > +/**
> > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > + */
> > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > +			  struct dma_buf_attachment *attach)
> > +{
> > +	int ret;
> > +	const struct virtio_dma_buf_ops *ops = container_of(
> > +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > +
> > +	if (ops->device_attach) {
> > +		ret = ops->device_attach(dma_buf, attach);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > +
> > +/**
> > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > + * @dma_buf: buffer to query
> > + */
> > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > +{
> > +	return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > +}
> > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > +
> > +/**
> > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > + * @dma_buf: [in] buffer to query
> > + * @uuid: [out] the uuid
> > + *
> > + * Returns: 0 on success, negative on failure.
> > + */
> > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > +			    uuid_t *uuid)
> > +{
> > +	const struct virtio_dma_buf_ops *ops = container_of(
> > +			dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > +
> > +	if (!is_virtio_dma_buf(dma_buf))
> > +		return -EINVAL;
> > +
> > +	return ops->get_uuid(dma_buf, uuid);
> > +}
> > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 15f906e4a748..9397e25616c4 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> >  int register_virtio_device(struct virtio_device *dev);
> >  void unregister_virtio_device(struct virtio_device *dev);
> > +bool is_virtio_device(struct device *dev);
> >  
> >  void virtio_break_device(struct virtio_device *dev);
> >  
> > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > new file mode 100644
> > index 000000000000..29fee167afbd
> > --- /dev/null
> > +++ b/include/linux/virtio_dma_buf.h
> > @@ -0,0 +1,58 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * dma-bufs for virtio exported objects
> > + *
> > + * Copyright (C) 2020 Google, Inc.
> > + */
> > +
> > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > +#define _LINUX_VIRTIO_DMA_BUF_H
> > +
> > +#include <linux/dma-buf.h>
> > +#include <linux/uuid.h>
> > +#include <linux/virtio.h>
> > +
> > +/**
> > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > + *		   all attach operations.
> > + * @get_uid: [required] callback to get the uuid of the exported object.
> > + */
> > +struct virtio_dma_buf_ops {
> > +	struct dma_buf_ops ops;
> > +	int (*device_attach)(struct dma_buf *dma_buf,
> > +			     struct dma_buf_attachment *attach);
> > +	int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > +};
> > +
> > +/**
> > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > + */
> > +struct virtio_dma_buf_export_info {
> > +	const char *exp_name;
> > +	struct module *owner;
> > +	const struct virtio_dma_buf_ops *ops;
> > +	size_t size;
> > +	int flags;
> > +	struct dma_resv *resv;
> > +	void *priv;
> > +};
> > +
> > +/**
> > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > + */
> > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)	\
> > +	struct virtio_dma_buf_export_info name = { \
> > +		.exp_name = KBUILD_MODNAME, \
> > +		.owner = THIS_MODULE }
> > +
> > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > +			  struct dma_buf_attachment *attach);
> > +
> > +struct dma_buf *virtio_dma_buf_export(
> > +		const struct virtio_dma_buf_export_info *virtio_exp_info);
> > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > +
> > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > -- 
> > 2.27.0.rc0.183.gde8f92d652-goog
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Thu, Jun 18, 2020 at 9:29 PM Guennadi Liakhovetski
<guennadi.liakhovetski@linux.intel.com> wrote:
>
> Hi Michael,
>
> On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > virtio drivers to obtain the UUID which identifies the underlying
> > > exported object.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> >
> > Is this just for graphics? If yes I'd rather we put it in the graphics
> > driver. We can always move it later ...
>
> Wouldn't this be the API that audio virtualisation will have to use to share
> buffers between the host and any guests?

The new flavor of dma-buf isn't directly related to sharing buffers
between the host and the guest. The purpose of this API is to help
share buffers between multiple virtio devices - e.g. share buffers
created by a virito-gpu device with a virito-video device. In
particular, the new flavor of dma-buf provides a mechanism to attach
identifying metadata to a dma-buf object that is shared between
different virtio drivers in a single guest. This identifying metadata
can then be passed to the importing device and used to fetch some
resource from the exporting device. But the new flavor of dma-buf is
an abstraction within the guest kernel, independent of the host-guest
boundary, and it's definitely not necessary if we're only talking
about a single virtio subsystem.

-David

> Thanks
> Guennadi
>
> > > ---
> > >  drivers/virtio/Makefile         |  2 +-
> > >  drivers/virtio/virtio.c         |  6 +++
> > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > >  include/linux/virtio.h          |  1 +
> > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > >  create mode 100644 include/linux/virtio_dma_buf.h
> > >
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 29a1386ecc03..ecdae5b596de 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -1,5 +1,5 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index a977e32a88f2..5d46f0ded92d 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > >
> > > +bool is_virtio_device(struct device *dev)
> > > +{
> > > +   return dev->bus == &virtio_bus;
> > > +}
> > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > +
> > >  void unregister_virtio_device(struct virtio_device *dev)
> > >  {
> > >     int index = dev->index; /* save for after device release */
> > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > new file mode 100644
> > > index 000000000000..23e3399b11ed
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > @@ -0,0 +1,89 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * dma-bufs for virtio exported objects
> > > + *
> > > + * Copyright (C) 2020 Google, Inc.
> > > + */
> > > +
> > > +#include <linux/virtio_dma_buf.h>
> > > +
> > > +/**
> > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > + *
> > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > + * for an virtio exported object that can be queried by other virtio drivers
> > > + * for the object's UUID.
> > > + */
> > > +struct dma_buf *virtio_dma_buf_export(
> > > +           const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > +{
> > > +   struct dma_buf_export_info exp_info;
> > > +
> > > +   if (!virtio_exp_info->ops
> > > +           || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > +           || !virtio_exp_info->ops->get_uuid) {
> > > +           return ERR_PTR(-EINVAL);
> > > +   }
> > > +
> > > +   exp_info.exp_name = virtio_exp_info->exp_name;
> > > +   exp_info.owner = virtio_exp_info->owner;
> > > +   exp_info.ops = &virtio_exp_info->ops->ops;
> > > +   exp_info.size = virtio_exp_info->size;
> > > +   exp_info.flags = virtio_exp_info->flags;
> > > +   exp_info.resv = virtio_exp_info->resv;
> > > +   exp_info.priv = virtio_exp_info->priv;
> > > +   BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > +                != sizeof(struct dma_buf_export_info));
> >
> > This is the only part that gives me pause. Why do we need this hack?
> > What's wrong with just using dma_buf_export_info directly,
> > and if you want the virtio ops, just using container_off?
> >
> >
> >
> > > +
> > > +   return dma_buf_export(&exp_info);
> > > +}
> > > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > > +
> > > +/**
> > > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > > + */
> > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > +                     struct dma_buf_attachment *attach)
> > > +{
> > > +   int ret;
> > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > +
> > > +   if (ops->device_attach) {
> > > +           ret = ops->device_attach(dma_buf, attach);
> > > +           if (ret)
> > > +                   return ret;
> > > +   }
> > > +   return 0;
> > > +}
> > > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > > +
> > > +/**
> > > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > > + * @dma_buf: buffer to query
> > > + */
> > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > > +{
> > > +   return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > > +}
> > > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > > +
> > > +/**
> > > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > > + * @dma_buf: [in] buffer to query
> > > + * @uuid: [out] the uuid
> > > + *
> > > + * Returns: 0 on success, negative on failure.
> > > + */
> > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > > +                       uuid_t *uuid)
> > > +{
> > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > +
> > > +   if (!is_virtio_dma_buf(dma_buf))
> > > +           return -EINVAL;
> > > +
> > > +   return ops->get_uuid(dma_buf, uuid);
> > > +}
> > > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index 15f906e4a748..9397e25616c4 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> > >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> > >  int register_virtio_device(struct virtio_device *dev);
> > >  void unregister_virtio_device(struct virtio_device *dev);
> > > +bool is_virtio_device(struct device *dev);
> > >
> > >  void virtio_break_device(struct virtio_device *dev);
> > >
> > > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > > new file mode 100644
> > > index 000000000000..29fee167afbd
> > > --- /dev/null
> > > +++ b/include/linux/virtio_dma_buf.h
> > > @@ -0,0 +1,58 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * dma-bufs for virtio exported objects
> > > + *
> > > + * Copyright (C) 2020 Google, Inc.
> > > + */
> > > +
> > > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > > +#define _LINUX_VIRTIO_DMA_BUF_H
> > > +
> > > +#include <linux/dma-buf.h>
> > > +#include <linux/uuid.h>
> > > +#include <linux/virtio.h>
> > > +
> > > +/**
> > > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > > + *            all attach operations.
> > > + * @get_uid: [required] callback to get the uuid of the exported object.
> > > + */
> > > +struct virtio_dma_buf_ops {
> > > +   struct dma_buf_ops ops;
> > > +   int (*device_attach)(struct dma_buf *dma_buf,
> > > +                        struct dma_buf_attachment *attach);
> > > +   int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > > +};
> > > +
> > > +/**
> > > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > > + */
> > > +struct virtio_dma_buf_export_info {
> > > +   const char *exp_name;
> > > +   struct module *owner;
> > > +   const struct virtio_dma_buf_ops *ops;
> > > +   size_t size;
> > > +   int flags;
> > > +   struct dma_resv *resv;
> > > +   void *priv;
> > > +};
> > > +
> > > +/**
> > > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > > + */
> > > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)    \
> > > +   struct virtio_dma_buf_export_info name = { \
> > > +           .exp_name = KBUILD_MODNAME, \
> > > +           .owner = THIS_MODULE }
> > > +
> > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > +                     struct dma_buf_attachment *attach);
> > > +
> > > +struct dma_buf *virtio_dma_buf_export(
> > > +           const struct virtio_dma_buf_export_info *virtio_exp_info);
> > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > > +
> > > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > > --
> > > 2.27.0.rc0.183.gde8f92d652-goog
> >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[David Stevens]

On Thu, Jun 18, 2020 at 9:29 PM Guennadi Liakhovetski
<guennadi.liakhovetski@linux.intel.com> wrote:
>
> Hi Michael,
>
> On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > virtio drivers to obtain the UUID which identifies the underlying
> > > exported object.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> >
> > Is this just for graphics? If yes I'd rather we put it in the graphics
> > driver. We can always move it later ...
>
> Wouldn't this be the API that audio virtualisation will have to use to share
> buffers between the host and any guests?

The new flavor of dma-buf isn't directly related to sharing buffers
between the host and the guest. The purpose of this API is to help
share buffers between multiple virtio devices - e.g. share buffers
created by a virito-gpu device with a virito-video device. In
particular, the new flavor of dma-buf provides a mechanism to attach
identifying metadata to a dma-buf object that is shared between
different virtio drivers in a single guest. This identifying metadata
can then be passed to the importing device and used to fetch some
resource from the exporting device. But the new flavor of dma-buf is
an abstraction within the guest kernel, independent of the host-guest
boundary, and it's definitely not necessary if we're only talking
about a single virtio subsystem.

-David

> Thanks
> Guennadi
>
> > > ---
> > >  drivers/virtio/Makefile         |  2 +-
> > >  drivers/virtio/virtio.c         |  6 +++
> > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > >  include/linux/virtio.h          |  1 +
> > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > >  create mode 100644 include/linux/virtio_dma_buf.h
> > >
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 29a1386ecc03..ecdae5b596de 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -1,5 +1,5 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index a977e32a88f2..5d46f0ded92d 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > >
> > > +bool is_virtio_device(struct device *dev)
> > > +{
> > > +   return dev->bus == &virtio_bus;
> > > +}
> > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > +
> > >  void unregister_virtio_device(struct virtio_device *dev)
> > >  {
> > >     int index = dev->index; /* save for after device release */
> > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > new file mode 100644
> > > index 000000000000..23e3399b11ed
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > @@ -0,0 +1,89 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * dma-bufs for virtio exported objects
> > > + *
> > > + * Copyright (C) 2020 Google, Inc.
> > > + */
> > > +
> > > +#include <linux/virtio_dma_buf.h>
> > > +
> > > +/**
> > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > + *
> > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > + * for an virtio exported object that can be queried by other virtio drivers
> > > + * for the object's UUID.
> > > + */
> > > +struct dma_buf *virtio_dma_buf_export(
> > > +           const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > +{
> > > +   struct dma_buf_export_info exp_info;
> > > +
> > > +   if (!virtio_exp_info->ops
> > > +           || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > +           || !virtio_exp_info->ops->get_uuid) {
> > > +           return ERR_PTR(-EINVAL);
> > > +   }
> > > +
> > > +   exp_info.exp_name = virtio_exp_info->exp_name;
> > > +   exp_info.owner = virtio_exp_info->owner;
> > > +   exp_info.ops = &virtio_exp_info->ops->ops;
> > > +   exp_info.size = virtio_exp_info->size;
> > > +   exp_info.flags = virtio_exp_info->flags;
> > > +   exp_info.resv = virtio_exp_info->resv;
> > > +   exp_info.priv = virtio_exp_info->priv;
> > > +   BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > +                != sizeof(struct dma_buf_export_info));
> >
> > This is the only part that gives me pause. Why do we need this hack?
> > What's wrong with just using dma_buf_export_info directly,
> > and if you want the virtio ops, just using container_off?
> >
> >
> >
> > > +
> > > +   return dma_buf_export(&exp_info);
> > > +}
> > > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > > +
> > > +/**
> > > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > > + */
> > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > +                     struct dma_buf_attachment *attach)
> > > +{
> > > +   int ret;
> > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > +
> > > +   if (ops->device_attach) {
> > > +           ret = ops->device_attach(dma_buf, attach);
> > > +           if (ret)
> > > +                   return ret;
> > > +   }
> > > +   return 0;
> > > +}
> > > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > > +
> > > +/**
> > > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > > + * @dma_buf: buffer to query
> > > + */
> > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > > +{
> > > +   return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > > +}
> > > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > > +
> > > +/**
> > > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > > + * @dma_buf: [in] buffer to query
> > > + * @uuid: [out] the uuid
> > > + *
> > > + * Returns: 0 on success, negative on failure.
> > > + */
> > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > > +                       uuid_t *uuid)
> > > +{
> > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > +
> > > +   if (!is_virtio_dma_buf(dma_buf))
> > > +           return -EINVAL;
> > > +
> > > +   return ops->get_uuid(dma_buf, uuid);
> > > +}
> > > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index 15f906e4a748..9397e25616c4 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> > >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> > >  int register_virtio_device(struct virtio_device *dev);
> > >  void unregister_virtio_device(struct virtio_device *dev);
> > > +bool is_virtio_device(struct device *dev);
> > >
> > >  void virtio_break_device(struct virtio_device *dev);
> > >
> > > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > > new file mode 100644
> > > index 000000000000..29fee167afbd
> > > --- /dev/null
> > > +++ b/include/linux/virtio_dma_buf.h
> > > @@ -0,0 +1,58 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * dma-bufs for virtio exported objects
> > > + *
> > > + * Copyright (C) 2020 Google, Inc.
> > > + */
> > > +
> > > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > > +#define _LINUX_VIRTIO_DMA_BUF_H
> > > +
> > > +#include <linux/dma-buf.h>
> > > +#include <linux/uuid.h>
> > > +#include <linux/virtio.h>
> > > +
> > > +/**
> > > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > > + *            all attach operations.
> > > + * @get_uid: [required] callback to get the uuid of the exported object.
> > > + */
> > > +struct virtio_dma_buf_ops {
> > > +   struct dma_buf_ops ops;
> > > +   int (*device_attach)(struct dma_buf *dma_buf,
> > > +                        struct dma_buf_attachment *attach);
> > > +   int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > > +};
> > > +
> > > +/**
> > > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > > + */
> > > +struct virtio_dma_buf_export_info {
> > > +   const char *exp_name;
> > > +   struct module *owner;
> > > +   const struct virtio_dma_buf_ops *ops;
> > > +   size_t size;
> > > +   int flags;
> > > +   struct dma_resv *resv;
> > > +   void *priv;
> > > +};
> > > +
> > > +/**
> > > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > > + */
> > > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)    \
> > > +   struct virtio_dma_buf_export_info name = { \
> > > +           .exp_name = KBUILD_MODNAME, \
> > > +           .owner = THIS_MODULE }
> > > +
> > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > +                     struct dma_buf_attachment *attach);
> > > +
> > > +struct dma_buf *virtio_dma_buf_export(
> > > +           const struct virtio_dma_buf_export_info *virtio_exp_info);
> > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > > +
> > > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > > --
> > > 2.27.0.rc0.183.gde8f92d652-goog
> >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Guennadi Liakhovetski]

Hi David,

On Fri, Jun 19, 2020 at 10:57:37AM +0900, David Stevens wrote:
> On Thu, Jun 18, 2020 at 9:29 PM Guennadi Liakhovetski
> <guennadi.liakhovetski@linux.intel.com> wrote:
> >
> > Hi Michael,
> >
> > On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > exported object.
> > > >
> > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > >
> > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > driver. We can always move it later ...
> >
> > Wouldn't this be the API that audio virtualisation will have to use to share
> > buffers between the host and any guests?
> 
> The new flavor of dma-buf isn't directly related to sharing buffers
> between the host and the guest. The purpose of this API is to help
> share buffers between multiple virtio devices - e.g. share buffers
> created by a virito-gpu device with a virito-video device. In
> particular, the new flavor of dma-buf provides a mechanism to attach
> identifying metadata to a dma-buf object that is shared between
> different virtio drivers in a single guest. This identifying metadata
> can then be passed to the importing device and used to fetch some
> resource from the exporting device. But the new flavor of dma-buf is
> an abstraction within the guest kernel, independent of the host-guest
> boundary, and it's definitely not necessary if we're only talking
> about a single virtio subsystem.

Thanks for the explanation!

Regards
Guennadi

> > > > ---
> > > >  drivers/virtio/Makefile         |  2 +-
> > > >  drivers/virtio/virtio.c         |  6 +++
> > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > >  include/linux/virtio.h          |  1 +
> > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > >
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -1,5 +1,5 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > --- a/drivers/virtio/virtio.c
> > > > +++ b/drivers/virtio/virtio.c
> > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > >
> > > > +bool is_virtio_device(struct device *dev)
> > > > +{
> > > > +   return dev->bus == &virtio_bus;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > +
> > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > >  {
> > > >     int index = dev->index; /* save for after device release */
> > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > new file mode 100644
> > > > index 000000000000..23e3399b11ed
> > > > --- /dev/null
> > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > @@ -0,0 +1,89 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * dma-bufs for virtio exported objects
> > > > + *
> > > > + * Copyright (C) 2020 Google, Inc.
> > > > + */
> > > > +
> > > > +#include <linux/virtio_dma_buf.h>
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > + *
> > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > + * for the object's UUID.
> > > > + */
> > > > +struct dma_buf *virtio_dma_buf_export(
> > > > +           const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > +{
> > > > +   struct dma_buf_export_info exp_info;
> > > > +
> > > > +   if (!virtio_exp_info->ops
> > > > +           || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > +           || !virtio_exp_info->ops->get_uuid) {
> > > > +           return ERR_PTR(-EINVAL);
> > > > +   }
> > > > +
> > > > +   exp_info.exp_name = virtio_exp_info->exp_name;
> > > > +   exp_info.owner = virtio_exp_info->owner;
> > > > +   exp_info.ops = &virtio_exp_info->ops->ops;
> > > > +   exp_info.size = virtio_exp_info->size;
> > > > +   exp_info.flags = virtio_exp_info->flags;
> > > > +   exp_info.resv = virtio_exp_info->resv;
> > > > +   exp_info.priv = virtio_exp_info->priv;
> > > > +   BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > +                != sizeof(struct dma_buf_export_info));
> > >
> > > This is the only part that gives me pause. Why do we need this hack?
> > > What's wrong with just using dma_buf_export_info directly,
> > > and if you want the virtio ops, just using container_off?
> > >
> > >
> > >
> > > > +
> > > > +   return dma_buf_export(&exp_info);
> > > > +}
> > > > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > > > + */
> > > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > > +                     struct dma_buf_attachment *attach)
> > > > +{
> > > > +   int ret;
> > > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > > +
> > > > +   if (ops->device_attach) {
> > > > +           ret = ops->device_attach(dma_buf, attach);
> > > > +           if (ret)
> > > > +                   return ret;
> > > > +   }
> > > > +   return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > > > +
> > > > +/**
> > > > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > > > + * @dma_buf: buffer to query
> > > > + */
> > > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > > > +{
> > > > +   return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > > > +}
> > > > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > > > + * @dma_buf: [in] buffer to query
> > > > + * @uuid: [out] the uuid
> > > > + *
> > > > + * Returns: 0 on success, negative on failure.
> > > > + */
> > > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > > > +                       uuid_t *uuid)
> > > > +{
> > > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > > +
> > > > +   if (!is_virtio_dma_buf(dma_buf))
> > > > +           return -EINVAL;
> > > > +
> > > > +   return ops->get_uuid(dma_buf, uuid);
> > > > +}
> > > > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index 15f906e4a748..9397e25616c4 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> > > >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> > > >  int register_virtio_device(struct virtio_device *dev);
> > > >  void unregister_virtio_device(struct virtio_device *dev);
> > > > +bool is_virtio_device(struct device *dev);
> > > >
> > > >  void virtio_break_device(struct virtio_device *dev);
> > > >
> > > > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > > > new file mode 100644
> > > > index 000000000000..29fee167afbd
> > > > --- /dev/null
> > > > +++ b/include/linux/virtio_dma_buf.h
> > > > @@ -0,0 +1,58 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * dma-bufs for virtio exported objects
> > > > + *
> > > > + * Copyright (C) 2020 Google, Inc.
> > > > + */
> > > > +
> > > > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > > > +#define _LINUX_VIRTIO_DMA_BUF_H
> > > > +
> > > > +#include <linux/dma-buf.h>
> > > > +#include <linux/uuid.h>
> > > > +#include <linux/virtio.h>
> > > > +
> > > > +/**
> > > > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > > > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > > > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > > > + *            all attach operations.
> > > > + * @get_uid: [required] callback to get the uuid of the exported object.
> > > > + */
> > > > +struct virtio_dma_buf_ops {
> > > > +   struct dma_buf_ops ops;
> > > > +   int (*device_attach)(struct dma_buf *dma_buf,
> > > > +                        struct dma_buf_attachment *attach);
> > > > +   int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > > > + */
> > > > +struct virtio_dma_buf_export_info {
> > > > +   const char *exp_name;
> > > > +   struct module *owner;
> > > > +   const struct virtio_dma_buf_ops *ops;
> > > > +   size_t size;
> > > > +   int flags;
> > > > +   struct dma_resv *resv;
> > > > +   void *priv;
> > > > +};
> > > > +
> > > > +/**
> > > > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > > > + */
> > > > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)    \
> > > > +   struct virtio_dma_buf_export_info name = { \
> > > > +           .exp_name = KBUILD_MODNAME, \
> > > > +           .owner = THIS_MODULE }
> > > > +
> > > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > > +                     struct dma_buf_attachment *attach);
> > > > +
> > > > +struct dma_buf *virtio_dma_buf_export(
> > > > +           const struct virtio_dma_buf_export_info *virtio_exp_info);
> > > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > > > +
> > > > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > > > --
> > > > 2.27.0.rc0.183.gde8f92d652-goog
> > >
> > > _______________________________________________
> > > Virtualization mailing list
> > > Virtualization@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 1/3] virtio: add dma-buf support for exported objects

[Guennadi Liakhovetski]

Hi David,

On Fri, Jun 19, 2020 at 10:57:37AM +0900, David Stevens wrote:
> On Thu, Jun 18, 2020 at 9:29 PM Guennadi Liakhovetski
> <guennadi.liakhovetski@linux.intel.com> wrote:
> >
> > Hi Michael,
> >
> > On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote:
> > > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote:
> > > > This change adds a new flavor of dma-bufs that can be used by virtio
> > > > drivers to share exported objects. A virtio dma-buf can be queried by
> > > > virtio drivers to obtain the UUID which identifies the underlying
> > > > exported object.
> > > >
> > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > >
> > > Is this just for graphics? If yes I'd rather we put it in the graphics
> > > driver. We can always move it later ...
> >
> > Wouldn't this be the API that audio virtualisation will have to use to share
> > buffers between the host and any guests?
> 
> The new flavor of dma-buf isn't directly related to sharing buffers
> between the host and the guest. The purpose of this API is to help
> share buffers between multiple virtio devices - e.g. share buffers
> created by a virito-gpu device with a virito-video device. In
> particular, the new flavor of dma-buf provides a mechanism to attach
> identifying metadata to a dma-buf object that is shared between
> different virtio drivers in a single guest. This identifying metadata
> can then be passed to the importing device and used to fetch some
> resource from the exporting device. But the new flavor of dma-buf is
> an abstraction within the guest kernel, independent of the host-guest
> boundary, and it's definitely not necessary if we're only talking
> about a single virtio subsystem.

Thanks for the explanation!

Regards
Guennadi

> > > > ---
> > > >  drivers/virtio/Makefile         |  2 +-
> > > >  drivers/virtio/virtio.c         |  6 +++
> > > >  drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++
> > > >  include/linux/virtio.h          |  1 +
> > > >  include/linux/virtio_dma_buf.h  | 58 +++++++++++++++++++++
> > > >  5 files changed, 155 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/virtio/virtio_dma_buf.c
> > > >  create mode 100644 include/linux/virtio_dma_buf.h
> > > >
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index 29a1386ecc03..ecdae5b596de 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -1,5 +1,5 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> > > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o
> > > >  obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > >  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > index a977e32a88f2..5d46f0ded92d 100644
> > > > --- a/drivers/virtio/virtio.c
> > > > +++ b/drivers/virtio/virtio.c
> > > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(register_virtio_device);
> > > >
> > > > +bool is_virtio_device(struct device *dev)
> > > > +{
> > > > +   return dev->bus == &virtio_bus;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(is_virtio_device);
> > > > +
> > > >  void unregister_virtio_device(struct virtio_device *dev)
> > > >  {
> > > >     int index = dev->index; /* save for after device release */
> > > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c
> > > > new file mode 100644
> > > > index 000000000000..23e3399b11ed
> > > > --- /dev/null
> > > > +++ b/drivers/virtio/virtio_dma_buf.c
> > > > @@ -0,0 +1,89 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * dma-bufs for virtio exported objects
> > > > + *
> > > > + * Copyright (C) 2020 Google, Inc.
> > > > + */
> > > > +
> > > > +#include <linux/virtio_dma_buf.h>
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object
> > > > + *
> > > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf
> > > > + * for an virtio exported object that can be queried by other virtio drivers
> > > > + * for the object's UUID.
> > > > + */
> > > > +struct dma_buf *virtio_dma_buf_export(
> > > > +           const struct virtio_dma_buf_export_info *virtio_exp_info)
> > > > +{
> > > > +   struct dma_buf_export_info exp_info;
> > > > +
> > > > +   if (!virtio_exp_info->ops
> > > > +           || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach
> > > > +           || !virtio_exp_info->ops->get_uuid) {
> > > > +           return ERR_PTR(-EINVAL);
> > > > +   }
> > > > +
> > > > +   exp_info.exp_name = virtio_exp_info->exp_name;
> > > > +   exp_info.owner = virtio_exp_info->owner;
> > > > +   exp_info.ops = &virtio_exp_info->ops->ops;
> > > > +   exp_info.size = virtio_exp_info->size;
> > > > +   exp_info.flags = virtio_exp_info->flags;
> > > > +   exp_info.resv = virtio_exp_info->resv;
> > > > +   exp_info.priv = virtio_exp_info->priv;
> > > > +   BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info)
> > > > +                != sizeof(struct dma_buf_export_info));
> > >
> > > This is the only part that gives me pause. Why do we need this hack?
> > > What's wrong with just using dma_buf_export_info directly,
> > > and if you want the virtio ops, just using container_off?
> > >
> > >
> > >
> > > > +
> > > > +   return dma_buf_export(&exp_info);
> > > > +}
> > > > +EXPORT_SYMBOL(virtio_dma_buf_export);
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs
> > > > + */
> > > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > > +                     struct dma_buf_attachment *attach)
> > > > +{
> > > > +   int ret;
> > > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > > +
> > > > +   if (ops->device_attach) {
> > > > +           ret = ops->device_attach(dma_buf, attach);
> > > > +           if (ret)
> > > > +                   return ret;
> > > > +   }
> > > > +   return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(virtio_dma_buf_attach);
> > > > +
> > > > +/**
> > > > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf
> > > > + * @dma_buf: buffer to query
> > > > + */
> > > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf)
> > > > +{
> > > > +   return dma_buf->ops->attach == &virtio_dma_buf_attach;
> > > > +}
> > > > +EXPORT_SYMBOL(is_virtio_dma_buf);
> > > > +
> > > > +/**
> > > > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object
> > > > + * @dma_buf: [in] buffer to query
> > > > + * @uuid: [out] the uuid
> > > > + *
> > > > + * Returns: 0 on success, negative on failure.
> > > > + */
> > > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf,
> > > > +                       uuid_t *uuid)
> > > > +{
> > > > +   const struct virtio_dma_buf_ops *ops = container_of(
> > > > +                   dma_buf->ops, const struct virtio_dma_buf_ops, ops);
> > > > +
> > > > +   if (!is_virtio_dma_buf(dma_buf))
> > > > +           return -EINVAL;
> > > > +
> > > > +   return ops->get_uuid(dma_buf, uuid);
> > > > +}
> > > > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid);
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index 15f906e4a748..9397e25616c4 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> > > >  void virtio_add_status(struct virtio_device *dev, unsigned int status);
> > > >  int register_virtio_device(struct virtio_device *dev);
> > > >  void unregister_virtio_device(struct virtio_device *dev);
> > > > +bool is_virtio_device(struct device *dev);
> > > >
> > > >  void virtio_break_device(struct virtio_device *dev);
> > > >
> > > > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h
> > > > new file mode 100644
> > > > index 000000000000..29fee167afbd
> > > > --- /dev/null
> > > > +++ b/include/linux/virtio_dma_buf.h
> > > > @@ -0,0 +1,58 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * dma-bufs for virtio exported objects
> > > > + *
> > > > + * Copyright (C) 2020 Google, Inc.
> > > > + */
> > > > +
> > > > +#ifndef _LINUX_VIRTIO_DMA_BUF_H
> > > > +#define _LINUX_VIRTIO_DMA_BUF_H
> > > > +
> > > > +#include <linux/dma-buf.h>
> > > > +#include <linux/uuid.h>
> > > > +#include <linux/virtio.h>
> > > > +
> > > > +/**
> > > > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf
> > > > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach.
> > > > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during
> > > > + *            all attach operations.
> > > > + * @get_uid: [required] callback to get the uuid of the exported object.
> > > > + */
> > > > +struct virtio_dma_buf_ops {
> > > > +   struct dma_buf_ops ops;
> > > > +   int (*device_attach)(struct dma_buf *dma_buf,
> > > > +                        struct dma_buf_attachment *attach);
> > > > +   int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid);
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info
> > > > + */
> > > > +struct virtio_dma_buf_export_info {
> > > > +   const char *exp_name;
> > > > +   struct module *owner;
> > > > +   const struct virtio_dma_buf_ops *ops;
> > > > +   size_t size;
> > > > +   int flags;
> > > > +   struct dma_resv *resv;
> > > > +   void *priv;
> > > > +};
> > > > +
> > > > +/**
> > > > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters
> > > > + */
> > > > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name)    \
> > > > +   struct virtio_dma_buf_export_info name = { \
> > > > +           .exp_name = KBUILD_MODNAME, \
> > > > +           .owner = THIS_MODULE }
> > > > +
> > > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf,
> > > > +                     struct dma_buf_attachment *attach);
> > > > +
> > > > +struct dma_buf *virtio_dma_buf_export(
> > > > +           const struct virtio_dma_buf_export_info *virtio_exp_info);
> > > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf);
> > > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid);
> > > > +
> > > > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */
> > > > --
> > > > 2.27.0.rc0.183.gde8f92d652-goog
> > >
> > > _______________________________________________
> > > Virtualization mailing list
> > > Virtualization@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel