[RFC PATCH v2 0/3] Deliver vGPU page flip events to userspace

[RFC PATCH v2 0/3] Deliver vGPU page flip events to userspace

[Tina Zhang]

This series tries to send the vGPU page flip events to userspace, which
can be used by QEMU UI for rendering and display with the latest guest
framebuffers.

v2: Use VFIO irq chain to get eventfds from userspace instead of adding
a new ABI. (Alex)

v1: https://patchwork.kernel.org/cover/10962341/


Tina Zhang (3):
  vfio: Use capability chains to handle device specific irq
  drm/i915/gvt: Leverage irq capability chain to get eventfd
  drm/i915/gvt: Send plane flip events to user space

 drivers/gpu/drm/i915/gvt/display.c   |  10 +-
 drivers/gpu/drm/i915/gvt/gvt.h       |   4 +
 drivers/gpu/drm/i915/gvt/handlers.c  |  20 ++-
 drivers/gpu/drm/i915/gvt/hypercall.h |   1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 208 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/gvt/mpt.h       |  16 +++
 include/uapi/linux/vfio.h            |  23 ++-
 7 files changed, 268 insertions(+), 14 deletions(-)

-- 
2.17.1

[RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Tina Zhang]

Caps the number of irqs with fixed indexes and uses capability chains
to chain device specific irqs.

VFIO vGPU leverages this mechanism to trigger primary plane and cursor
plane page flip event to the user space.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/uapi/linux/vfio.h | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 02bb7ad6e986..9b5e25937c7d 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -444,11 +444,31 @@ struct vfio_irq_info {
 #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
 #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
 #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
+#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps */
 	__u32	index;		/* IRQ index */
+	__u32	cap_offset;	/* Offset within info struct of first cap */
 	__u32	count;		/* Number of IRQs within this index */
 };
 #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
 
+/*
+ * The irq type capability allows irqs unique to a specific device or
+ * class of devices to be exposed.
+ *
+ * The structures below define version 1 of this capability.
+ */
+#define VFIO_IRQ_INFO_CAP_TYPE      3
+
+struct vfio_irq_info_cap_type {
+	struct vfio_info_cap_header header;
+	__u32 type;     /* global per bus driver */
+	__u32 subtype;  /* type specific */
+};
+
+#define VFIO_IRQ_TYPE_GFX				(1)
+#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP		(1)
+#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP		(2)
+
 /**
  * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
  *
@@ -550,7 +570,8 @@ enum {
 	VFIO_PCI_MSIX_IRQ_INDEX,
 	VFIO_PCI_ERR_IRQ_INDEX,
 	VFIO_PCI_REQ_IRQ_INDEX,
-	VFIO_PCI_NUM_IRQS
+	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use   */
+				/* device specific cap to define content */
 };
 
 /*
-- 
2.17.1

[RFC PATCH v2 2/3] drm/i915/gvt: Leverage irq capability chain to get eventfd

[Tina Zhang]

GVT-g display model leverages vfio irq capability chain to get eventfd
from the user space. With the eventfd, GVT-g display model in kernel
can deliver a plane update event to user space.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.c   |  10 +-
 drivers/gpu/drm/i915/gvt/gvt.h       |   4 +
 drivers/gpu/drm/i915/gvt/hypercall.h |   1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 208 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/gvt/mpt.h       |  16 +++
 5 files changed, 229 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index e1c313da6c00..1a0a4ae4826e 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -506,16 +506,22 @@ void intel_vgpu_clean_display(struct intel_vgpu *vgpu)
 int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int ret;
 
 	intel_vgpu_init_i2c_edid(vgpu);
 
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv) ||
 	    IS_COFFEELAKE(dev_priv))
-		return setup_virtual_dp_monitor(vgpu, PORT_D, GVT_DP_D,
+		ret = setup_virtual_dp_monitor(vgpu, PORT_D, GVT_DP_D,
 						resolution);
 	else
-		return setup_virtual_dp_monitor(vgpu, PORT_B, GVT_DP_B,
+		ret = setup_virtual_dp_monitor(vgpu, PORT_B, GVT_DP_B,
 						resolution);
+
+	if (ret == 0)
+		intel_gvt_hypervisor_register_display_irq(vgpu);
+
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index f5a328b5290a..1951fc6b029f 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -201,8 +201,12 @@ struct intel_vgpu {
 		struct mdev_device *mdev;
 		struct vfio_region *region;
 		int num_regions;
+		struct vfio_irq *irq;
+		int num_irqs;
 		struct eventfd_ctx *intx_trigger;
 		struct eventfd_ctx *msi_trigger;
+		struct eventfd_ctx *pri_flip_trigger;
+		struct eventfd_ctx *cur_flip_trigger;
 
 		/*
 		 * Two caches are used to avoid mapping duplicated pages (eg.
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index 4862fb12778e..be33f20f3bc1 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -68,6 +68,7 @@ struct intel_gvt_mpt {
 			     bool map);
 	int (*set_opregion)(void *vgpu);
 	int (*set_edid)(void *vgpu, int port_num);
+	int (*register_display_irq)(void *vgpu);
 	int (*get_vfio_device)(void *vgpu);
 	void (*put_vfio_device)(void *vgpu);
 	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index a68addf95c23..7d89d69fff20 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -78,6 +78,12 @@ struct vfio_region {
 	void				*data;
 };
 
+struct vfio_irq {
+	u32				type;
+	u32				subtype;
+	u32				flags;
+};
+
 struct vfio_edid_region {
 	struct vfio_region_gfx_edid vfio_edid_regs;
 	void *edid_blob;
@@ -635,6 +641,41 @@ static int kvmgt_set_edid(void *p_vgpu, int port_num)
 	return ret;
 }
 
+static int intel_vgpu_register_irq(struct intel_vgpu *vgpu,
+		unsigned int type, unsigned int subtype, u32 flags)
+{
+	struct vfio_irq *irq;
+
+	irq = krealloc(vgpu->vdev.irq,
+			(vgpu->vdev.num_irqs + 1) * sizeof(*irq),
+			GFP_KERNEL);
+	if (!irq)
+		return -ENOMEM;
+
+	vgpu->vdev.irq = irq;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].type = type;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].subtype = subtype;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].flags = flags;
+	vgpu->vdev.num_irqs++;
+	return 0;
+}
+
+static int kvmgt_register_display_irq(void *p_vgpu)
+{
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
+
+	intel_vgpu_register_irq(vgpu, VFIO_IRQ_TYPE_GFX,
+				VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP,
+				VFIO_IRQ_INFO_EVENTFD);
+
+	intel_vgpu_register_irq(vgpu, VFIO_IRQ_TYPE_GFX,
+				VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP,
+				VFIO_IRQ_INFO_EVENTFD);
+
+
+	return 0;
+}
+
 static void kvmgt_put_vfio_device(void *vgpu)
 {
 	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
@@ -1182,7 +1223,11 @@ static int intel_vgpu_mmap(struct mdev_device *mdev, struct vm_area_struct *vma)
 
 static int intel_vgpu_get_irq_count(struct intel_vgpu *vgpu, int type)
 {
-	if (type == VFIO_PCI_INTX_IRQ_INDEX || type == VFIO_PCI_MSI_IRQ_INDEX)
+	if (type == VFIO_PCI_INTX_IRQ_INDEX ||
+	    type == VFIO_PCI_MSI_IRQ_INDEX ||
+	    ((type >= VFIO_PCI_NUM_IRQS) &&
+	     (type < VFIO_PCI_NUM_IRQS +
+	      vgpu->vdev.num_irqs)))
 		return 1;
 
 	return 0;
@@ -1231,6 +1276,58 @@ static int intel_vgpu_set_msi_trigger(struct intel_vgpu *vgpu,
 	return 0;
 }
 
+static int intel_vgpu_set_pri_flip_trigger(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	struct eventfd_ctx *trigger;
+
+	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+		int fd = *(int *)data;
+
+		trigger = eventfd_ctx_fdget(fd);
+		if (IS_ERR(trigger)) {
+			gvt_vgpu_err("eventfd_ctx_fdget failed\n");
+			return PTR_ERR(trigger);
+		}
+		vgpu->vdev.pri_flip_trigger = trigger;
+	} else if ((flags & VFIO_IRQ_SET_DATA_NONE) && !count) {
+		trigger = vgpu->vdev.pri_flip_trigger;
+		if (trigger) {
+			eventfd_ctx_put(trigger);
+			vgpu->vdev.pri_flip_trigger = NULL;
+		}
+	}
+
+	return 0;
+}
+
+static int intel_vgpu_set_cur_flip_trigger(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	struct eventfd_ctx *trigger;
+
+	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+		int fd = *(int *)data;
+
+		trigger = eventfd_ctx_fdget(fd);
+		if (IS_ERR(trigger)) {
+			gvt_vgpu_err("eventfd_ctx_fdget failed\n");
+			return PTR_ERR(trigger);
+		}
+		vgpu->vdev.cur_flip_trigger = trigger;
+	} else if ((flags & VFIO_IRQ_SET_DATA_NONE) && !count) {
+		trigger = vgpu->vdev.cur_flip_trigger;
+		if (trigger) {
+			eventfd_ctx_put(trigger);
+			vgpu->vdev.cur_flip_trigger = NULL;
+		}
+	}
+
+	return 0;
+}
+
 static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
 		unsigned int index, unsigned int start, unsigned int count,
 		void *data)
@@ -1264,8 +1361,47 @@ static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
 			break;
 		}
 		break;
-	}
+	default:
+	{
+		int i;
 
+		if (index >= VFIO_PCI_NUM_IRQS +
+					vgpu->vdev.num_irqs)
+			return -EINVAL;
+		index =
+			array_index_nospec(index,
+						VFIO_PCI_NUM_IRQS +
+						vgpu->vdev.num_irqs);
+
+		i = index - VFIO_PCI_NUM_IRQS;
+		if (vgpu->vdev.irq[i].type == VFIO_IRQ_TYPE_GFX &&
+		    vgpu->vdev.irq[i].subtype ==
+		    VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP) {
+			switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+			case VFIO_IRQ_SET_ACTION_MASK:
+			case VFIO_IRQ_SET_ACTION_UNMASK:
+				/* XXX Need masking support exported */
+				break;
+			case VFIO_IRQ_SET_ACTION_TRIGGER:
+				func = intel_vgpu_set_pri_flip_trigger;
+				break;
+			}
+		} else if (vgpu->vdev.irq[i].type == VFIO_IRQ_TYPE_GFX &&
+		    vgpu->vdev.irq[i].subtype ==
+			   VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP) {
+			switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+			case VFIO_IRQ_SET_ACTION_MASK:
+			case VFIO_IRQ_SET_ACTION_UNMASK:
+				/* XXX Need masking support exported */
+				break;
+			case VFIO_IRQ_SET_ACTION_TRIGGER:
+				func = intel_vgpu_set_cur_flip_trigger;
+				break;
+			}
+		}
+	}
+	}
+	/* Add set_vgpu_irq here */
 	if (!func)
 		return -ENOTTY;
 
@@ -1295,7 +1431,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		info.flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.num_regions = VFIO_PCI_NUM_REGIONS +
 				vgpu->vdev.num_regions;
-		info.num_irqs = VFIO_PCI_NUM_IRQS;
+		info.num_irqs = VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs;
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
@@ -1455,24 +1591,55 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			-EFAULT : 0;
 	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
 		struct vfio_irq_info info;
+		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+		unsigned int i;
+		int ret;
 
 		minsz = offsetofend(struct vfio_irq_info, count);
 
 		if (copy_from_user(&info, (void __user *)arg, minsz))
 			return -EFAULT;
 
-		if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
+		if (info.argsz < minsz)
 			return -EINVAL;
 
 		switch (info.index) {
 		case VFIO_PCI_INTX_IRQ_INDEX:
 		case VFIO_PCI_MSI_IRQ_INDEX:
+			info.flags = VFIO_IRQ_INFO_EVENTFD;
 			break;
-		default:
+		case VFIO_PCI_MSIX_IRQ_INDEX:
+		case VFIO_PCI_ERR_IRQ_INDEX:
+		case VFIO_PCI_REQ_IRQ_INDEX:
 			return -EINVAL;
-		}
+		default:
+		{
+			struct vfio_irq_info_cap_type cap_type = {
+				.header.id = VFIO_IRQ_INFO_CAP_TYPE,
+				.header.version = 1 };
 
-		info.flags = VFIO_IRQ_INFO_EVENTFD;
+			if (info.index >= VFIO_PCI_NUM_IRQS +
+					vgpu->vdev.num_irqs)
+				return -EINVAL;
+			info.index =
+				array_index_nospec(info.index,
+						VFIO_PCI_NUM_IRQS +
+						vgpu->vdev.num_irqs);
+
+			i = info.index - VFIO_PCI_NUM_IRQS;
+
+			info.flags = vgpu->vdev.irq[i].flags;
+
+			cap_type.type = vgpu->vdev.irq[i].type;
+			cap_type.subtype = vgpu->vdev.irq[i].subtype;
+
+			ret = vfio_info_add_capability(&caps,
+						&cap_type.header,
+						sizeof(cap_type));
+			if (ret)
+				return ret;
+		}
+		}
 
 		info.count = intel_vgpu_get_irq_count(vgpu, info.index);
 
@@ -1482,6 +1649,25 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		else
 			info.flags |= VFIO_IRQ_INFO_NORESIZE;
 
+		if (caps.size) {
+			info.flags |= VFIO_IRQ_INFO_FLAG_CAPS;
+			if (info.argsz < sizeof(info) + caps.size) {
+				info.argsz = sizeof(info) + caps.size;
+				info.cap_offset = 0;
+			} else {
+				vfio_info_cap_shift(&caps, sizeof(info));
+				if (copy_to_user((void __user *)arg +
+						  sizeof(info), caps.buf,
+						  caps.size)) {
+					kfree(caps.buf);
+					return -EFAULT;
+				}
+				info.cap_offset = sizeof(info);
+			}
+
+			kfree(caps.buf);
+		}
+
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
 	} else if (cmd == VFIO_DEVICE_SET_IRQS) {
@@ -1499,7 +1685,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			int max = intel_vgpu_get_irq_count(vgpu, hdr.index);
 
 			ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
-						VFIO_PCI_NUM_IRQS, &data_size);
+					VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs,
+								 &data_size);
 			if (ret) {
 				gvt_vgpu_err("intel:vfio_set_irqs_validate_and_prepare failed\n");
 				return -EINVAL;
@@ -1838,6 +2025,10 @@ static void kvmgt_detach_vgpu(void *p_vgpu)
 	vgpu->vdev.num_regions = 0;
 	kfree(vgpu->vdev.region);
 	vgpu->vdev.region = NULL;
+
+	vgpu->vdev.num_irqs = 0;
+	kfree(vgpu->vdev.irq);
+	vgpu->vdev.irq = NULL;
 }
 
 static int kvmgt_inject_msi(unsigned long handle, u32 addr, u16 data)
@@ -2039,6 +2230,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
 	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
 	.set_opregion = kvmgt_set_opregion,
 	.set_edid = kvmgt_set_edid,
+	.register_display_irq = kvmgt_register_display_irq,
 	.get_vfio_device = kvmgt_get_vfio_device,
 	.put_vfio_device = kvmgt_put_vfio_device,
 	.is_valid_gfn = kvmgt_is_valid_gfn,
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 0f9440128123..03b31ce87ae1 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -330,6 +330,22 @@ static inline int intel_gvt_hypervisor_set_edid(struct intel_vgpu *vgpu,
 	return intel_gvt_host.mpt->set_edid(vgpu, port_num);
 }
 
+/**
+ * intel_gvt_hypervisor_set_irq - register vgpu specific irq
+ * @vgpu: a vGPU
+ * @port_num: display port number
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_register_display_irq(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->register_display_irq)
+		return 0;
+
+	return intel_gvt_host.mpt->register_display_irq(vgpu);
+}
+
 /**
  * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
  * @vgpu: a vGPU
-- 
2.17.1

[RFC PATCH v2 3/3] drm/i915/gvt: Send plane flip events to user space

[Tina Zhang]

Send the primary plane and the cursor plane flip events to user space.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/handlers.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index 18f01eeb2510..67129de8bc45 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -763,6 +763,20 @@ static int pri_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
 	else
 		set_bit(event, vgpu->irq.flip_done_event[pipe]);
 
+	if (vgpu->vdev.pri_flip_trigger)
+		eventfd_signal(vgpu->vdev.pri_flip_trigger, 1);
+
+	return 0;
+}
+
+static int cur_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
+		void *p_data, unsigned int bytes)
+{
+	write_vreg(vgpu, offset, p_data, bytes);
+
+	if (vgpu->vdev.cur_flip_trigger)
+		eventfd_signal(vgpu->vdev.cur_flip_trigger, 1);
+
 	return 0;
 }
 
@@ -1969,9 +1983,9 @@ static int init_generic_mmio_info(struct intel_gvt *gvt)
 	MMIO_D(CURPOS(PIPE_B), D_ALL);
 	MMIO_D(CURPOS(PIPE_C), D_ALL);
 
-	MMIO_D(CURBASE(PIPE_A), D_ALL);
-	MMIO_D(CURBASE(PIPE_B), D_ALL);
-	MMIO_D(CURBASE(PIPE_C), D_ALL);
+	MMIO_DH(CURBASE(PIPE_A), D_ALL, NULL, cur_surf_mmio_write);
+	MMIO_DH(CURBASE(PIPE_B), D_ALL, NULL, cur_surf_mmio_write);
+	MMIO_DH(CURBASE(PIPE_C), D_ALL, NULL, cur_surf_mmio_write);
 
 	MMIO_D(CUR_FBC_CTL(PIPE_A), D_ALL);
 	MMIO_D(CUR_FBC_CTL(PIPE_B), D_ALL);
-- 
2.17.1

Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Zhenyu Wang]

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

On 2019.06.04 17:55:32 +0800, Tina Zhang wrote:
> Caps the number of irqs with fixed indexes and uses capability chains
> to chain device specific irqs.
> 
> VFIO vGPU leverages this mechanism to trigger primary plane and cursor
> plane page flip event to the user space.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/linux/vfio.h | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 02bb7ad6e986..9b5e25937c7d 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -444,11 +444,31 @@ struct vfio_irq_info {
>  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
>  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
>  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps */
>  	__u32	index;		/* IRQ index */
> +	__u32	cap_offset;	/* Offset within info struct of first cap */
>  	__u32	count;		/* Number of IRQs within this index */

This would break ABI for get irq info. I think irq cap chain can just follow
vfio_irq_info.

>  };
>  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
>  
> +/*
> + * The irq type capability allows irqs unique to a specific device or
> + * class of devices to be exposed.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_TYPE      3
> +
> +struct vfio_irq_info_cap_type {
> +	struct vfio_info_cap_header header;
> +	__u32 type;     /* global per bus driver */
> +	__u32 subtype;  /* type specific */
> +};
> +
> +#define VFIO_IRQ_TYPE_GFX				(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP		(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP		(2)
> +

Really need to split for different planes? I'd like a VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
so user space can probe change for all.

>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
>   *
> @@ -550,7 +570,8 @@ enum {
>  	VFIO_PCI_MSIX_IRQ_INDEX,
>  	VFIO_PCI_ERR_IRQ_INDEX,
>  	VFIO_PCI_REQ_IRQ_INDEX,
> -	VFIO_PCI_NUM_IRQS
> +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use   */
> +				/* device specific cap to define content */
>  };
>  
>  /*
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Zhang, Tina]

> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Wednesday, June 5, 2019 12:05 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; kraxel@redhat.com; zhenyuw@linux.intel.com; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian,
> Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>;
> alex.williamson@redhat.com
> Subject: Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device
> specific irq
> 
> On 2019.06.04 17:55:32 +0800, Tina Zhang wrote:
> > Caps the number of irqs with fixed indexes and uses capability chains
> > to chain device specific irqs.
> >
> > VFIO vGPU leverages this mechanism to trigger primary plane and cursor
> > plane page flip event to the user space.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 02bb7ad6e986..9b5e25937c7d 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -444,11 +444,31 @@ struct vfio_irq_info {
> >  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
> >  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
> >  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> > +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info
> supports caps */
> >  	__u32	index;		/* IRQ index */
> > +	__u32	cap_offset;	/* Offset within info struct of first cap */
> >  	__u32	count;		/* Number of IRQs within this index */
> 
> This would break ABI for get irq info. I think irq cap chain can just follow
> vfio_irq_info.
> 
> >  };
> >  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE +
> 9)
> >
> > +/*
> > + * The irq type capability allows irqs unique to a specific device or
> > + * class of devices to be exposed.
> > + *
> > + * The structures below define version 1 of this capability.
> > + */
> > +#define VFIO_IRQ_INFO_CAP_TYPE      3
> > +
> > +struct vfio_irq_info_cap_type {
> > +	struct vfio_info_cap_header header;
> > +	__u32 type;     /* global per bus driver */
> > +	__u32 subtype;  /* type specific */
> > +};
> > +
> > +#define VFIO_IRQ_TYPE_GFX				(1)
> > +#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP		(1)
> > +#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP		(2)
> > +
> 
> Really need to split for different planes? I'd like a
> VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> so user space can probe change for all.
User space can choose to user different handlers according to the specific event. For example, user space might not want to handle every cursor event due to performance consideration. Besides, it can reduce the probe times, as we don't need to probe twice to make sure if both cursor plane and primary plane have been updated.
Thanks.

BR,
Tina

> 
> >  /**
> >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> vfio_irq_set)
> >   *
> > @@ -550,7 +570,8 @@ enum {
> >  	VFIO_PCI_MSIX_IRQ_INDEX,
> >  	VFIO_PCI_ERR_IRQ_INDEX,
> >  	VFIO_PCI_REQ_IRQ_INDEX,
> > -	VFIO_PCI_NUM_IRQS
> > +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5
> use   */
> > +				/* device specific cap to define content */
> >  };
> >
> >  /*
> > --
> > 2.17.1
> >
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[kraxel]

  Hi,

> > Really need to split for different planes? I'd like a
> > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > so user space can probe change for all.

> User space can choose to user different handlers according to the
> specific event. For example, user space might not want to handle every
> cursor event due to performance consideration. Besides, it can reduce
> the probe times, as we don't need to probe twice to make sure if both
> cursor plane and primary plane have been updated.

I'd suggest to use the value passed via eventfd for that, i.e. instead
of sending "1" unconditionally send a mask of changed planes.

cheers,
  Gerd

RE: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Tian, Kevin]

> From: kraxel@redhat.com
> Sent: Wednesday, June 5, 2019 6:10 PM
> 
>   Hi,
> 
> > > Really need to split for different planes? I'd like a
> > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > > so user space can probe change for all.
> 
> > User space can choose to user different handlers according to the
> > specific event. For example, user space might not want to handle every
> > cursor event due to performance consideration. Besides, it can reduce
> > the probe times, as we don't need to probe twice to make sure if both
> > cursor plane and primary plane have been updated.
> 
> I'd suggest to use the value passed via eventfd for that, i.e. instead
> of sending "1" unconditionally send a mask of changed planes.
> 

sounds reasonable.

RE: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of kraxel@redhat.com
> Sent: Wednesday, June 5, 2019 6:10 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Zhenyu Wang <zhenyuw@linux.intel.com>; Yuan,
> Hang <hang.yuan@intel.com>; alex.williamson@redhat.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> <zhi.a.wang@intel.com>
> Subject: Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device
> specific irq
> 
>   Hi,
> 
> > > Really need to split for different planes? I'd like a
> > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > > so user space can probe change for all.
> 
> > User space can choose to user different handlers according to the
> > specific event. For example, user space might not want to handle every
> > cursor event due to performance consideration. Besides, it can reduce
> > the probe times, as we don't need to probe twice to make sure if both
> > cursor plane and primary plane have been updated.
> 
> I'd suggest to use the value passed via eventfd for that, i.e. instead of
> sending "1" unconditionally send a mask of changed planes.
If there is only one eventfd working for GFX_DISPLAY, should it be  VFIO_IRQ_INFO_EVENTFD and VFIO_IRQ_INFO_AUTOMASKED? i.e. after signaling, the interrupt is automatically masked and the user space needs to unmask the line to receive new irq event.

BR,
Tina


> 
> cheers,
>   Gerd
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Alex Williamson]

On Thu, 6 Jun 2019 10:17:51 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> > Behalf Of kraxel@redhat.com
> > Sent: Wednesday, June 5, 2019 6:10 PM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Zhenyu Wang <zhenyuw@linux.intel.com>; Yuan,
> > Hang <hang.yuan@intel.com>; alex.williamson@redhat.com; Lv, Zhiyuan
> > <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> > <zhi.a.wang@intel.com>
> > Subject: Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device
> > specific irq
> > 
> >   Hi,
> >   
> > > > Really need to split for different planes? I'd like a
> > > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > > > so user space can probe change for all.  
> >   
> > > User space can choose to user different handlers according to the
> > > specific event. For example, user space might not want to handle every
> > > cursor event due to performance consideration. Besides, it can reduce
> > > the probe times, as we don't need to probe twice to make sure if both
> > > cursor plane and primary plane have been updated.  
> > 
> > I'd suggest to use the value passed via eventfd for that, i.e. instead of
> > sending "1" unconditionally send a mask of changed planes.  
> If there is only one eventfd working for GFX_DISPLAY, should it be
> VFIO_IRQ_INFO_EVENTFD and VFIO_IRQ_INFO_AUTOMASKED? i.e. after
> signaling, the interrupt is automatically masked and the user space
> needs to unmask the line to receive new irq event.

If there's any way at all the interrupt is rate limited already, I'd
suggest not to use automasked.  This flag is generally intended for
cases where we need to mask a host interrupt and don't have a generic
or efficient way to determine acknowledgement of the interrupt and
therefore require an explicit unmask.  If the events here are not at a
high frequency or you can tell by other interactions that they've been
acted upon, I'd suggest to handle these as an edge triggered interrupt
w/o automasked.  Thanks,

Alex