[PATCH v3 0/8] Add enlightenments for vGPU

[PATCH v3 0/8] Add enlightenments for vGPU

[Yu Zhang]

Intel GVT-g (previously known as XenGT), is a complete GPU
virtualization solution with mediated pass-through for 4th
generation Intel Core processors - Haswell platform. This
technology presents a virtual full-fledged GPU to each Virtual
Machine (VM). VMs can directly access performance-critical
resources, without intervention from the hypervisor in most
cases, while privileged operations from VMs are trap-and-emulated
at minimal cost. For detail, please refer to
https://01.org/xen/blogs/wangbo85/2014/intel-gvt-gxengt-pubic-release

This patch set includes necessary code changes when i915 driver
runs inside a VM. Though ideally we can run an unmodified i915
driver in VM, adding such enlightenments can greatly reduce the
virtualization complexity in orders of magnitude. Code changes
for the host side, which includes the actual Intel GVT-g
implementation, will be sent out in another patchset.

The primary change introduced here is to implement so-called
"address space ballooning" technique. XenGT partitions global
graphics memory among multiple VMs, so each VM can directly
access a portion of the memory w/o hypervisor's intervention,
e.g. filling textures or queuing commands. However w/ the
partitioning an unmodified i915 driver would assume a smaller
graphics memory starting from address ZERO, so requires XenGT
core module (vgt) to translate the graphics address between
'guest view' and 'host view', for all registers and command
opcodes which contain a graphics memory address. To reduce the
complexity, XenGT introduces "address space ballooning", by
telling the exact partitioning knowledge to each guest i915
driver, which then reserves and prevents non-allocated portions
from allocation. Then vgt module only needs to scan and validate
graphics addresses w/o complexity of translation.

Note: The partitioning of global graphics memory may break some
applications, with large objects in the aperture, because current
userspace assumes half of the aperture usable. That would need
separate fix either in user space (e.g. remove assumption in mesa)
or in kernel (w/ some faulting mechanism).

The partitioning knowledge is conveyed through a reserved MMIO
range, called PVINFO, which will be architecturally reserved in
future hardware generations. Another information carried through
PVINFO is about the number of fence registers. As a global resource
XenGT also partitions them among VMs.

Other changes are trivial as optimizations, to either reduce the
trap overhead or disable power management features which don't
make sense in a virtualized environment.


Yu Zhang (8):
  drm/i915: Introduce a PV INFO page structure for Intel GVT-g.
  drm/i915: Adds graphic address space ballooning logic
  drm/i915: Partition the fence registers for vGPU in i915 driver
  drm/i915: Disable framebuffer compression for i915 driver in VM
  drm/i915: Add the display switch logic for vGPU in i915 driver
  drm/i915: Disable power management for i915 driver in VM
  drm/i915: Create vGPU specific write MMIO to reduce traps
  drm/i915: Support alias ppgtt in VM if ppgtt is enabled

 Documentation/DocBook/drm.tmpl      |   5 +
 drivers/gpu/drm/i915/Makefile       |   3 +
 drivers/gpu/drm/i915/i915_dma.c     |  11 ++
 drivers/gpu/drm/i915/i915_drv.h     |  11 ++
 drivers/gpu/drm/i915/i915_gem.c     |   5 +
 drivers/gpu/drm/i915/i915_gem_gtt.c |  33 ++++-
 drivers/gpu/drm/i915/i915_vgpu.c    | 234 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_vgpu.h    |  94 +++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c     |   8 ++
 drivers/gpu/drm/i915/intel_uncore.c |  19 +++
 10 files changed, 420 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_vgpu.c
 create mode 100644 drivers/gpu/drm/i915/i915_vgpu.h

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Yu Zhang]

Introduce a PV INFO structure, to facilitate the Intel GVT-g
technology, which is a GPU virtualization solution with mediated
pass-through. This page contains the shared information between
i915 driver and the host emulator. For now, this structure utilizes
an area of 4K bypes on HSW GPU's unused MMIO space to support
existing production. Future hardware will have the reserved window
architecturally defined, and layout of the page will be added in
future BSpec.

The i915 driver load routine detects if it is running in a VM by
reading the contents of this PV INFO page. Thereafter a flag,
vgpu.active is set, and intel_vgpu_active() is used by checking
this flag to conclude if GPU is virtualized with Intel GVT-g. By
now, intel_vgpu_active() will return true, only when the driver
is running as a guest in the Intel GVT-g enhanced environment on
HSW platform.

v2:
take Chris' comments:
	- call the i915_check_vgpu() in intel_uncore_init()
	- sanitize i915_check_vgpu() by adding BUILD_BUG_ON() and debug info
take Daniel's comments:
	- put the definition of PV INFO into a new header - i915_vgt_if.h
other changes:
	- access mmio regs by readq/readw in i915_check_vgpu()

v3:
take Daniel's comments:
	- move the i915/vgt interfaces into a new i915_vgpu.c
	- update makefile
	- add kerneldoc to functions which are non-static
	- add a DOC: section describing some of the high-level design
	- update drm docbook
other changes:
	- rename i915_vgt_if.h to i915_vgpu.h

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Eddie Dong <eddie.dong@intel.com>
---
 Documentation/DocBook/drm.tmpl      |  5 +++
 drivers/gpu/drm/i915/Makefile       |  3 ++
 drivers/gpu/drm/i915/i915_drv.h     | 11 +++++
 drivers/gpu/drm/i915/i915_vgpu.c    | 85 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_vgpu.h    | 85 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c |  3 ++
 6 files changed, 192 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_vgpu.c
 create mode 100644 drivers/gpu/drm/i915/i915_vgpu.h

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 64d9c1e..e4db4cf 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3832,6 +3832,11 @@ int num_ioctls;</synopsis>
 !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
 !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
       </sect2>
+      <sect2>
+        <title>Intel GVT-g Guest Support(vGPU)</title>
+!Pdrivers/gpu/drm/i915/i915_vgpu.c Intel GVT-g guest support
+!Idrivers/gpu/drm/i915/i915_vgpu.c
+      </sect2>
     </sect1>
     <sect1>
       <title>Display Hardware Handling</title>
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 891e584..d9aa5ca 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -79,6 +79,9 @@ i915-y += dvo_ch7017.o \
 	  intel_sdvo.o \
 	  intel_tv.o
 
+# virtual gpu code
+i915-y += i915_vgpu.o
+
 # legacy horrors
 i915-y += i915_dma.o \
 	  i915_ums.o
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 54691bc..e1e221e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1470,6 +1470,10 @@ struct i915_workarounds {
 	u32 count;
 };
 
+struct i915_virtual_gpu {
+	bool active;
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1482,6 +1486,8 @@ struct drm_i915_private {
 
 	struct intel_uncore uncore;
 
+	struct i915_virtual_gpu vgpu;
+
 	struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
 
 
@@ -2311,6 +2317,11 @@ extern void intel_uncore_check_errors(struct drm_device *dev);
 extern void intel_uncore_fini(struct drm_device *dev);
 extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
 
+static inline bool intel_vgpu_active(struct drm_device *dev)
+{
+	return to_i915(dev)->vgpu.active;
+}
+
 void
 i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 		     u32 status_mask);
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
new file mode 100644
index 0000000..3f6b797
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include "intel_drv.h"
+#include "i915_vgpu.h"
+
+
+/**
+ * DOC: Intel GVT-g guest support
+ *
+ * Intel GVT-g is a graphics virtualization technology which shares the
+ * GPU among multiple virtual machines on a time-sharing basis. Each
+ * virtual machine is presented a virtual GPU (vGPU), which has equivalent
+ * features as the underlying physical GPU (pGPU), so i915 driver can run
+ * seamlessly in a virtual machine. This file provides vGPU specific
+ * optimizations when running in a virtual machine, to reduce the complexity
+ * of vGPU emulation and to improve the overall performance.
+ *
+ * A primary function introduced here is so-called "address space ballooning"
+ * technique. Intel GVT-g partitions global graphics memory among multiple VMs,
+ * so each VM can directly access a portion of the memory w/o hypervisor's
+ * intervention, e.g. filling textures or queuing commands. However w/ the
+ * partitioning an unmodified i915 driver would assume a smaller graphics
+ * memory starting from address ZERO, then requires vGPU emulation module to
+ * translate the graphics address between 'guest view' and 'host view', for
+ * all registers and command opcodes which contain a graphics memory address.
+ * To reduce the complexity, Intel GVT-g introduces "address space ballooning",
+ * by telling the exact partitioning knowledge to each guest i915 driver, which
+ * then reserves and prevents non-allocated portions from allocation. Thus vGPU
+ * emulation module module only needs to scan and validate graphics addresses
+ * w/o complexity of address translation.
+ *
+ */
+
+/**
+ * i915_check_vgpu - detect virtual GPU
+ * @dev: drm device *
+ *
+ * This function is called at the initialization stage, to detect whether
+ * running on a vGPU.
+ */
+void i915_check_vgpu(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	uint64_t magic;
+	uint32_t version;
+
+	BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
+
+	if (!IS_HASWELL(dev))
+		return;
+
+	magic = readq(dev_priv->regs + vgtif_reg(magic));
+	if (magic != VGT_MAGIC)
+		return;
+
+	version = INTEL_VGT_IF_VERSION_ENCODE(
+		readw(dev_priv->regs + vgtif_reg(version_major)),
+		readw(dev_priv->regs + vgtif_reg(version_minor)));
+	if (version != INTEL_VGT_IF_VERSION)
+		return;
+
+	dev_priv->vgpu.active = true;
+	DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
+}
diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
new file mode 100644
index 0000000..5f41d01c
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_vgpu.h
@@ -0,0 +1,85 @@
+/*
+ * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef _I915_VGPU_H_
+#define _I915_VGPU_H_
+
+/* The MMIO offset of the shared info between guest and host emulator */
+#define VGT_PVINFO_PAGE	0x78000
+#define VGT_PVINFO_SIZE	0x1000
+
+/*
+ * The following structure pages are defined in GEN MMIO space
+ * for virtualization. (One page for now)
+ */
+#define VGT_MAGIC         0x4776544776544776	/* 'vGTvGTvG' */
+#define VGT_VERSION_MAJOR 1
+#define VGT_VERSION_MINOR 0
+
+#define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 | (minor))
+#define INTEL_VGT_IF_VERSION \
+	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
+
+struct vgt_if {
+	uint64_t magic;		/* VGT_MAGIC */
+	uint16_t version_major;
+	uint16_t version_minor;
+	uint32_t vgt_id;	/* ID of vGT instance */
+	uint32_t rsv1[12];	/* pad to offset 0x40 */
+	/*
+	 *  Data structure to describe the balooning info of resources.
+	 *  Each VM can only have one portion of continuous area for now.
+	 *  (May support scattered resource in future)
+	 *  (starting from offset 0x40)
+	 */
+	struct {
+		/* Aperture register balooning */
+		struct {
+			uint32_t my_base;
+			uint32_t my_size;
+		} low_gmadr;	/* aperture */
+		/* GMADR register balooning */
+		struct {
+			uint32_t my_base;
+			uint32_t my_size;
+		} high_gmadr;	/* non aperture */
+		/* allowed fence registers */
+		uint32_t fence_num;
+		uint32_t rsv2[3];
+	} avail_rs;		/* available/assigned resource */
+	uint32_t rsv3[0x200 - 24];	/* pad to half page */
+	/*
+	 * The bottom half page is for response from Gfx driver to hypervisor.
+	 * Set to reserved fields temporarily by now.
+	 */
+	uint32_t rsv4;
+	uint32_t display_ready;	/* ready for display owner switch */
+	uint32_t rsv5[0x200 - 2];	/* pad to one page */
+} __packed;
+
+#define vgtif_reg(x) \
+	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
+
+extern void i915_check_vgpu(struct drm_device *dev);
+
+#endif /* _I915_VGPU_H_ */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 9427641..cae27bb 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -23,6 +23,7 @@
 
 #include "i915_drv.h"
 #include "intel_drv.h"
+#include "i915_vgpu.h"
 
 #define FORCEWAKE_ACK_TIMEOUT_MS 2
 
@@ -850,6 +851,8 @@ void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	i915_check_vgpu(dev);
+
 	setup_timer(&dev_priv->uncore.force_wake_timer,
 		    gen6_force_wake_timer, (unsigned long)dev_priv);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Yu Zhang]

With Intel GVT-g, the global graphic memory space is partitioned by
multiple vGPU instances in different VMs. The ballooning code is called
in i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
mark the graphic address space which are partitioned out to other vGPUs
as reserved.

v2:
take Chris and Daniel's comments:
	- no guard page between different VMs
	- use drm_mm_reserve_node() to do the reservation for ballooning,
	instead of the previous drm_mm_insert_node_in_range_generic()

v3:
take Daniel's comments:
	- move ballooning functions into i915_vgpu.c
	- add kerneldoc to ballooning functions

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Signed-off-by: Eddie Dong <eddie.dong@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |  17 +++-
 drivers/gpu/drm/i915/i915_vgpu.c    | 149 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_vgpu.h    |   2 +
 3 files changed, 165 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index de12017..2dfac13 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -27,6 +27,7 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_vgpu.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
 
@@ -1683,6 +1684,16 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 
 	/* Subtract the guard page ... */
 	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
+
+	dev_priv->gtt.base.start = start;
+	dev_priv->gtt.base.total = end - start;
+
+	if (intel_vgpu_active(dev)) {
+		ret = intel_vgt_balloon(dev);
+		if (ret)
+			return ret;
+	}
+
 	if (!HAS_LLC(dev))
 		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
 
@@ -1702,9 +1713,6 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 		vma->bound |= GLOBAL_BIND;
 	}
 
-	dev_priv->gtt.base.start = start;
-	dev_priv->gtt.base.total = end - start;
-
 	/* Clear any non-preallocated blocks */
 	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
 		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
@@ -1756,6 +1764,9 @@ void i915_global_gtt_cleanup(struct drm_device *dev)
 	}
 
 	if (drm_mm_initialized(&vm->mm)) {
+		if (intel_vgpu_active(dev))
+			intel_vgt_deballoon();
+
 		drm_mm_takedown(&vm->mm);
 		list_del(&vm->global_link);
 	}
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 3f6b797..ff5fba3 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -83,3 +83,152 @@ void i915_check_vgpu(struct drm_device *dev)
 	dev_priv->vgpu.active = true;
 	DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
 }
+
+struct _balloon_info_ {
+	/*
+	 * There are up to 2 regions per low/high graphic memory that
+	 * might be ballooned. Here, index 0/1 is for low
+	 * graphic memory, 2/3 for high graphic memory.
+	 */
+	struct drm_mm_node space[4];
+} bl_info;
+
+/**
+ * intel_vgt_deballoon - deballoon reserved graphics address trunks
+ *
+ * This function is called to deallocate the ballooned-out graphic memory, when
+ * driver is unloaded or when ballooning fails.
+ */
+void intel_vgt_deballoon(void)
+{
+	int i;
+
+	DRM_INFO("VGT deballoon.\n");
+
+	for (i = 0; i < 4; i++) {
+		if (bl_info.space[i].allocated)
+			drm_mm_remove_node(&bl_info.space[i]);
+	}
+
+	memset(&bl_info, 0, sizeof(bl_info));
+}
+
+static int vgt_balloon_space(struct drm_mm *mm,
+			     struct drm_mm_node *node,
+			     unsigned long start, unsigned long end)
+{
+	unsigned long size = end - start;
+
+	if (start == end)
+		return -EINVAL;
+
+	DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KB.\n",
+		 start, end, size / 1024);
+
+	node->start = start;
+	node->size = size;
+
+	return drm_mm_reserve_node(mm, node);
+}
+
+/**
+ * intel_vgt_balloon - balloon out reserved graphics address trunks
+ * @dev: drm device
+ *
+ * This function is called at the initialization stage, to balloon out the
+ * graphic address space allocated to other VMs, by marking these spaces as
+ * reserved.
+ *
+ * The ballooning related knowledges(starting address and size of the low/high
+ * graphic memory) are depicted in the vgt_if structure in a reserved MMIO
+ * range.
+ *
+ * Returns:
+ * zero on success, non-zero if configuration invalid or ballooning failed
+ */
+int intel_vgt_balloon(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
+	unsigned long ggtt_vm_end = ggtt_vm->start + ggtt_vm->total;
+
+	unsigned long low_gm_base, low_gm_size, low_gm_end;
+	unsigned long high_gm_base, high_gm_size, high_gm_end;
+	int ret;
+
+	low_gm_base = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_base));
+	low_gm_size = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_size));
+	high_gm_base = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_base));
+	high_gm_size = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_size));
+
+	low_gm_end = low_gm_base + low_gm_size;
+	high_gm_end = high_gm_base + high_gm_size;
+
+	DRM_INFO("VGT ballooning configuration:\n");
+	DRM_INFO("Low graphic memory: base 0x%lx size %ldKB\n",
+		 low_gm_base, low_gm_size / 1024);
+	DRM_INFO("High graphic memory: base 0x%lx size %ldKB\n",
+		 high_gm_base, high_gm_size / 1024);
+
+	if (low_gm_base < ggtt_vm->start
+	    || low_gm_end > dev_priv->gtt.mappable_end
+	    || high_gm_base < dev_priv->gtt.mappable_end
+	    || high_gm_end > ggtt_vm_end) {
+		DRM_ERROR("Invalid ballooning configuration!\n");
+		return -EINVAL;
+	}
+
+	memset(&bl_info, 0, sizeof(bl_info));
+
+	/* High graphic memory ballooning */
+	if (high_gm_base > dev_priv->gtt.mappable_end) {
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[2],
+					dev_priv->gtt.mappable_end,
+					high_gm_base);
+
+		if (ret)
+			goto err;
+	}
+
+	/*
+	 * No need to partition out the last physical page,
+	 * because it is reserved to the guard page.
+	 */
+	if (high_gm_end < ggtt_vm_end - PAGE_SIZE) {
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[3],
+					high_gm_end,
+					ggtt_vm_end - PAGE_SIZE);
+		if (ret)
+			goto err;
+	}
+
+	/* Low graphic memory ballooning */
+	if (low_gm_base > ggtt_vm->start) {
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[0],
+					ggtt_vm->start, low_gm_base);
+
+		if (ret)
+			goto err;
+	}
+
+	if (low_gm_end < dev_priv->gtt.mappable_end) {
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[1],
+					low_gm_end,
+					dev_priv->gtt.mappable_end);
+
+		if (ret)
+			goto err;
+	}
+
+	DRM_INFO("VGT balloon successfully\n");
+	return 0;
+
+err:
+	DRM_ERROR("VGT balloon fail\n");
+	intel_vgt_deballoon();
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
index 5f41d01c..f538b18 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.h
+++ b/drivers/gpu/drm/i915/i915_vgpu.h
@@ -81,5 +81,7 @@ struct vgt_if {
 	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
 
 extern void i915_check_vgpu(struct drm_device *dev);
+extern int intel_vgt_balloon(struct drm_device *dev);
+extern void intel_vgt_deballoon(void);
 
 #endif /* _I915_VGPU_H_ */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Yu Zhang]

With Intel GVT-g, the fence registers are partitioned by multiple
vGPU instances in different VMs. Routine i915_gem_load() is modified
to reset the num_fence_regs, when the driver detects it's running in
a VM. And the allocated fence number is provided in PV INFO page
structure.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Eddie Dong <eddie.dong@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1de94cc..0c8b32e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -29,6 +29,7 @@
 #include <drm/drm_vma_manager.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_vgpu.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
 #include <linux/oom.h>
@@ -5014,6 +5015,10 @@ i915_gem_load(struct drm_device *dev)
 	else
 		dev_priv->num_fence_regs = 8;
 
+	if (intel_vgpu_active(dev))
+		dev_priv->num_fence_regs =
+				I915_READ(vgtif_reg(avail_rs.fence_num));
+
 	/* Initialize fence registers to zero */
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 	i915_gem_restore_fences(dev);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Yu Zhang]

Framebuffer compression is disabled when driver detects it's
running in a Intel GVT-g enlightened VM, because FBC is not
emulated and there is no stolen memory for a vGPU.

v2:
take Chris' comments:
	- move the code into intel_update_fbc()

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7a69eba..3bc5d93 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -544,6 +544,10 @@ void intel_update_fbc(struct drm_device *dev)
 		return;
 	}
 
+	/* disable framebuffer compression in vGPU */
+	if (intel_vgpu_active(dev))
+		i915.enable_fbc = 0;
+
 	/*
 	 * If FBC is already on, we just have to verify that we can
 	 * keep it that way...
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 5/8] drm/i915: Add the display switch logic for vGPU in i915 driver

[Yu Zhang]

Display switch logic is added to notify the host side that
current vGPU have a valid surface to show. It does so by
writing the display_ready field in PV INFO page, and then
will be handled in the host side. This is useful to avoid
trickiness when the VM's framebuffer is being accessed in
the middle of VM modesetting, e.g. compositing the framebuffer
in the host side.

v2:
	- move the notification code outside the 'else' in load sequence
	- remove the notification code in intel_crtc_set_config()

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  | 11 +++++++++++
 drivers/gpu/drm/i915/i915_vgpu.h |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9a73533..06daa5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -36,6 +36,7 @@
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_vgpu.h"
 #include "i915_trace.h"
 #include <linux/pci.h>
 #include <linux/console.h>
@@ -1780,6 +1781,16 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		dev_priv->ums.mm_suspended = 1;
 	}
 
+	if (intel_vgpu_active(dev)) {
+		/*
+		 * Notify a valid surface after modesetting,
+		 * when running inside a VM.
+		 */
+		struct drm_i915_private *dev_priv = to_i915(dev);
+
+		I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
+	}
+
 	i915_setup_sysfs(dev);
 
 	if (INTEL_INFO(dev)->num_pipes) {
diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
index f538b18..9d35fb8 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.h
+++ b/drivers/gpu/drm/i915/i915_vgpu.h
@@ -80,6 +80,13 @@ struct vgt_if {
 #define vgtif_reg(x) \
 	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
 
+/* vGPU display status to be used by the host side */
+enum vgt_display_status {
+	VGT_DRV_DISPLAY_NOT_READY = 0,
+	VGT_DRV_DISPLAY_READY,  /* ready for display switch */
+};
+
+
 extern void i915_check_vgpu(struct drm_device *dev);
 extern int intel_vgt_balloon(struct drm_device *dev);
 extern void intel_vgt_deballoon(void);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 6/8] drm/i915: Disable power management for i915 driver in VM

[Yu Zhang]

With Intel GVT-g, GPU power management is controlled by
host driver, so there is no need to provide virtualized
GPU PM support. In the future it might be useful to gather
VM input for freq boost, but now let's disable it simply.

v2:
take Chris' comments:
        - do not special case this to gen6+

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3bc5d93..3722bd4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5314,6 +5314,10 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* Powersaving is controlled by the host when inside a VM */
+	if (intel_vgpu_active(dev))
+		return;
+
 	if (IS_IRONLAKE_M(dev)) {
 		mutex_lock(&dev->struct_mutex);
 		ironlake_enable_drps(dev);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 7/8] drm/i915: Create vGPU specific write MMIO to reduce traps

[Yu Zhang]

In the virtualized environment, forcewake operations are not
necessory for the driver, because mmio accesses will be trapped
and emulated by the host side, and real forcewake operations are
also done in the host. New mmio write handlers are added to directly
call the __raw_i915_write, therefore will reduce many traps and
increase the overall performance for drivers running in the VM
with Intel GVT-g enhancement.

v2:
take Chris' comments:
        - register the mmio hooks in intel_uncore_init()
v3:
take Daniel's comments:
        - use macros to assign mmio write functions for vGPU

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Kevin Tian <kevin.tian@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index cae27bb..b76c21d 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -727,6 +727,14 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
 	REG_WRITE_FOOTER; \
 }
 
+#define __vgpu_write(x) \
+static void \
+vgpu_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
+	REG_WRITE_HEADER; \
+	__raw_i915_write##x(dev_priv, reg, val); \
+	REG_WRITE_FOOTER; \
+}
+
 static const u32 gen8_shadowed_regs[] = {
 	FORCEWAKE_MT,
 	GEN6_RPNSWREQ,
@@ -821,6 +829,10 @@ __gen4_write(8)
 __gen4_write(16)
 __gen4_write(32)
 __gen4_write(64)
+__vgpu_write(8)
+__vgpu_write(16)
+__vgpu_write(32)
+__vgpu_write(64)
 
 #undef __chv_write
 #undef __gen8_write
@@ -828,6 +840,7 @@ __gen4_write(64)
 #undef __gen6_write
 #undef __gen5_write
 #undef __gen4_write
+#undef __vgpu_write
 #undef REG_WRITE_FOOTER
 #undef REG_WRITE_HEADER
 
@@ -939,6 +952,9 @@ void intel_uncore_init(struct drm_device *dev)
 		break;
 	}
 
+	if (intel_vgpu_active(dev))
+		ASSIGN_WRITE_MMIO_VFUNCS(vgpu);
+
 	i915_check_and_clear_faults(dev);
 }
 #undef ASSIGN_WRITE_MMIO_VFUNCS
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 8/8] drm/i915: Support alias ppgtt in VM if ppgtt is enabled

[Yu Zhang]

The current Intel GVT-g only supports alias ppgtt. And the
emulation is done in the host by first trapping PP_DIR_BASE
mmio accesses. Updating PP_DIR_BASE by using instructions such
as MI_LOAD_REGISTER_IMM are hard to detect and are not supported
in current code. Therefore this patch also added a new callback
routine - vgpu_mm_switch() to set the PP_DIR_BASE by mmio writes.

v2:
take Chris' comments:
        - move the code into sanitize_enable_ppgtt()

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2dfac13..55307eb 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -44,6 +44,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 	if (IS_GEN8(dev))
 		has_full_ppgtt = false; /* XXX why? */
 
+	if (intel_vgpu_active(dev))
+		has_full_ppgtt = false; /* emulation is too hard */
+
 	if (enable_ppgtt == 0 || !has_aliasing_ppgtt)
 		return 0;
 
@@ -733,6 +736,16 @@ static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	return 0;
 }
 
+static int vgpu_mm_switch(struct i915_hw_ppgtt *ppgtt,
+			 struct intel_engine_cs *ring)
+{
+	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
+
+	I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G);
+	I915_WRITE(RING_PP_DIR_BASE(ring), get_pd_offset(ppgtt));
+	return 0;
+}
+
 static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
 			  struct intel_engine_cs *ring)
 {
@@ -1059,6 +1072,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	} else
 		BUG();
 
+	if (intel_vgpu_active(dev))
+		ppgtt->switch_mm = vgpu_mm_switch;
+
 	ret = gen6_ppgtt_alloc(ppgtt);
 	if (ret)
 		return ret;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 8/8] drm/i915: Support alias ppgtt in VM if

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=291/291->290/291
PNV: pass/total=356/356->356/356
ILK: pass/total=372/372->371/372
IVB: pass/total=544/546->544/546
SNB: pass/total=423/425->424/425
HSW: pass/total=579/579->579/579
BDW: pass/total=421/423->422/423
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(15, M36M31)PASS(1, M38) -> TIMEOUT(4, M31)
ILK: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, FAIL(3, M26)DMESG_FAIL(1, M26)TIMEOUT(24, M37M6M26)PASS(1, M26) -> TIMEOUT(4, M37)
IVB: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(5, M4M21M34M45)PASS(8, M21M34M4M45) -> NSPT(1, M34)PASS(3, M34)
IVB: Intel_gpu_tools, igt_kms_pipe_crc_basic_hang-read-crc-pipe-B, PASS(4, M4M34) -> TIMEOUT(1, M34)PASS(3, M34)
SNB: Intel_gpu_tools, igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-forked, FAIL(1, M22)PASS(9, M35M22) -> FAIL(1, M22)PASS(3, M22)
BDW: Intel_gpu_tools, igt_gem_reset_stats_ban-bsd, DMESG_WARN(1, M30)PASS(9, M30M42) -> PASS(4, M42)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Daniel Vetter]

On Thu, Nov 13, 2014 at 08:02:43PM +0800, Yu Zhang wrote:
> +	if (low_gm_base < ggtt_vm->start
> +	    || low_gm_end > dev_priv->gtt.mappable_end
> +	    || high_gm_base < dev_priv->gtt.mappable_end
> +	    || high_gm_end > ggtt_vm_end) {

Nit: Logical operators like || or && should be at the end of the previous
line, not at the beginning of the new line. checkpatch would have noticed
this one.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/8] Add enlightenments for vGPU

[Daniel Vetter]

On Thu, Nov 13, 2014 at 08:02:41PM +0800, Yu Zhang wrote:
> Intel GVT-g (previously known as XenGT), is a complete GPU
> virtualization solution with mediated pass-through for 4th
> generation Intel Core processors - Haswell platform. This
> technology presents a virtual full-fledged GPU to each Virtual
> Machine (VM). VMs can directly access performance-critical
> resources, without intervention from the hypervisor in most
> cases, while privileged operations from VMs are trap-and-emulated
> at minimal cost. For detail, please refer to
> https://01.org/xen/blogs/wangbo85/2014/intel-gvt-gxengt-pubic-release
> 
> This patch set includes necessary code changes when i915 driver
> runs inside a VM. Though ideally we can run an unmodified i915
> driver in VM, adding such enlightenments can greatly reduce the
> virtualization complexity in orders of magnitude. Code changes
> for the host side, which includes the actual Intel GVT-g
> implementation, will be sent out in another patchset.
> 
> The primary change introduced here is to implement so-called
> "address space ballooning" technique. XenGT partitions global
> graphics memory among multiple VMs, so each VM can directly
> access a portion of the memory w/o hypervisor's intervention,
> e.g. filling textures or queuing commands. However w/ the
> partitioning an unmodified i915 driver would assume a smaller
> graphics memory starting from address ZERO, so requires XenGT
> core module (vgt) to translate the graphics address between
> 'guest view' and 'host view', for all registers and command
> opcodes which contain a graphics memory address. To reduce the
> complexity, XenGT introduces "address space ballooning", by
> telling the exact partitioning knowledge to each guest i915
> driver, which then reserves and prevents non-allocated portions
> from allocation. Then vgt module only needs to scan and validate
> graphics addresses w/o complexity of translation.
> 
> Note: The partitioning of global graphics memory may break some
> applications, with large objects in the aperture, because current
> userspace assumes half of the aperture usable. That would need
> separate fix either in user space (e.g. remove assumption in mesa)
> or in kernel (w/ some faulting mechanism).
> 
> The partitioning knowledge is conveyed through a reserved MMIO
> range, called PVINFO, which will be architecturally reserved in
> future hardware generations. Another information carried through
> PVINFO is about the number of fence registers. As a global resource
> XenGT also partitions them among VMs.
> 
> Other changes are trivial as optimizations, to either reduce the
> trap overhead or disable power management features which don't
> make sense in a virtualized environment.

I think this looks good now overall, so please find someone from the i915
kernel team to review the details. You might need to poke managers to make
that happen ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Yu, Zhang]

On 11/14/2014 6:16 PM, Daniel Vetter wrote:
> On Thu, Nov 13, 2014 at 08:02:43PM +0800, Yu Zhang wrote:
>> +	if (low_gm_base < ggtt_vm->start
>> +	    || low_gm_end > dev_priv->gtt.mappable_end
>> +	    || high_gm_base < dev_priv->gtt.mappable_end
>> +	    || high_gm_end > ggtt_vm_end) {
>
> Nit: Logical operators like || or && should be at the end of the previous
> line, not at the beginning of the new line. checkpatch would have noticed
> this one.
> -Daniel
>

Got it.
Thank you. :-)

Yu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/8] Add enlightenments for vGPU

[Yu, Zhang]

On 11/14/2014 6:17 PM, Daniel Vetter wrote:
> On Thu, Nov 13, 2014 at 08:02:41PM +0800, Yu Zhang wrote:
>> Intel GVT-g (previously known as XenGT), is a complete GPU
>> virtualization solution with mediated pass-through for 4th
>> generation Intel Core processors - Haswell platform. This
>> technology presents a virtual full-fledged GPU to each Virtual
>> Machine (VM). VMs can directly access performance-critical
>> resources, without intervention from the hypervisor in most
>> cases, while privileged operations from VMs are trap-and-emulated
>> at minimal cost. For detail, please refer to
>> https://01.org/xen/blogs/wangbo85/2014/intel-gvt-gxengt-pubic-release
>>
>> This patch set includes necessary code changes when i915 driver
>> runs inside a VM. Though ideally we can run an unmodified i915
>> driver in VM, adding such enlightenments can greatly reduce the
>> virtualization complexity in orders of magnitude. Code changes
>> for the host side, which includes the actual Intel GVT-g
>> implementation, will be sent out in another patchset.
>>
>> The primary change introduced here is to implement so-called
>> "address space ballooning" technique. XenGT partitions global
>> graphics memory among multiple VMs, so each VM can directly
>> access a portion of the memory w/o hypervisor's intervention,
>> e.g. filling textures or queuing commands. However w/ the
>> partitioning an unmodified i915 driver would assume a smaller
>> graphics memory starting from address ZERO, so requires XenGT
>> core module (vgt) to translate the graphics address between
>> 'guest view' and 'host view', for all registers and command
>> opcodes which contain a graphics memory address. To reduce the
>> complexity, XenGT introduces "address space ballooning", by
>> telling the exact partitioning knowledge to each guest i915
>> driver, which then reserves and prevents non-allocated portions
>> from allocation. Then vgt module only needs to scan and validate
>> graphics addresses w/o complexity of translation.
>>
>> Note: The partitioning of global graphics memory may break some
>> applications, with large objects in the aperture, because current
>> userspace assumes half of the aperture usable. That would need
>> separate fix either in user space (e.g. remove assumption in mesa)
>> or in kernel (w/ some faulting mechanism).
>>
>> The partitioning knowledge is conveyed through a reserved MMIO
>> range, called PVINFO, which will be architecturally reserved in
>> future hardware generations. Another information carried through
>> PVINFO is about the number of fence registers. As a global resource
>> XenGT also partitions them among VMs.
>>
>> Other changes are trivial as optimizations, to either reduce the
>> trap overhead or disable power management features which don't
>> make sense in a virtualized environment.
>
> I think this looks good now overall, so please find someone from the i915
> kernel team to review the details. You might need to poke managers to make
> that happen ;-)
> -Daniel
Thanks a lot, Daniel.
This is great. :-)

Yu
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/8] Add enlightenments for vGPU

[Tvrtko Ursulin]

Hi,

I'll try to do the detailed review of your series in the following few 
days. I might ask some questions on the design also to help me 
understand the bigger picture.

First thing, I see that patches are checkpatch.pl clean, apart when run 
in strict mode. I think Daniel prefers "--strict" nowadays, at least I 
needed to fix up those in my patches so you should probably do the same.

On 11/13/2014 12:02 PM, Yu Zhang wrote:
[snip]
> The primary change introduced here is to implement so-called
> "address space ballooning" technique. XenGT partitions global
> graphics memory among multiple VMs, so each VM can directly
> access a portion of the memory w/o hypervisor's intervention,
> e.g. filling textures or queuing commands. However w/ the
> partitioning an unmodified i915 driver would assume a smaller
> graphics memory starting from address ZERO, so requires XenGT
> core module (vgt) to translate the graphics address between
> 'guest view' and 'host view', for all registers and command
> opcodes which contain a graphics memory address. To reduce the
> complexity, XenGT introduces "address space ballooning", by
> telling the exact partitioning knowledge to each guest i915
> driver, which then reserves and prevents non-allocated portions
> from allocation. Then vgt module only needs to scan and validate
> graphics addresses w/o complexity of translation.

I couldn't figure this out - is there any hardware protection in there, 
or a virtual i915 instance could access memory outside it's area if 
there was a security bug/exploit in the driver, or the balloon was 
incorrectly set up?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> Introduce a PV INFO structure, to facilitate the Intel GVT-g
> technology, which is a GPU virtualization solution with mediated
> pass-through. This page contains the shared information between
> i915 driver and the host emulator. For now, this structure utilizes
> an area of 4K bypes on HSW GPU's unused MMIO space to support

bytes

> existing production. Future hardware will have the reserved window

It is slightly unclear to me what "existing production" means?

> architecturally defined, and layout of the page will be added in
> future BSpec.
>
> The i915 driver load routine detects if it is running in a VM by
> reading the contents of this PV INFO page. Thereafter a flag,
> vgpu.active is set, and intel_vgpu_active() is used by checking
> this flag to conclude if GPU is virtualized with Intel GVT-g. By
> now, intel_vgpu_active() will return true, only when the driver
> is running as a guest in the Intel GVT-g enhanced environment on
> HSW platform.
>
> v2:
> take Chris' comments:
> 	- call the i915_check_vgpu() in intel_uncore_init()
> 	- sanitize i915_check_vgpu() by adding BUILD_BUG_ON() and debug info
> take Daniel's comments:
> 	- put the definition of PV INFO into a new header - i915_vgt_if.h
> other changes:
> 	- access mmio regs by readq/readw in i915_check_vgpu()
>
> v3:
> take Daniel's comments:
> 	- move the i915/vgt interfaces into a new i915_vgpu.c
> 	- update makefile
> 	- add kerneldoc to functions which are non-static
> 	- add a DOC: section describing some of the high-level design
> 	- update drm docbook
> other changes:
> 	- rename i915_vgt_if.h to i915_vgpu.h
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> ---
>   Documentation/DocBook/drm.tmpl      |  5 +++
>   drivers/gpu/drm/i915/Makefile       |  3 ++
>   drivers/gpu/drm/i915/i915_drv.h     | 11 +++++
>   drivers/gpu/drm/i915/i915_vgpu.c    | 85 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_vgpu.h    | 85 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_uncore.c |  3 ++
>   6 files changed, 192 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.c
>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.h
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 64d9c1e..e4db4cf 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3832,6 +3832,11 @@ int num_ioctls;</synopsis>
>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
>         </sect2>
> +      <sect2>
> +        <title>Intel GVT-g Guest Support(vGPU)</title>
> +!Pdrivers/gpu/drm/i915/i915_vgpu.c Intel GVT-g guest support
> +!Idrivers/gpu/drm/i915/i915_vgpu.c
> +      </sect2>
>       </sect1>
>       <sect1>
>         <title>Display Hardware Handling</title>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 891e584..d9aa5ca 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -79,6 +79,9 @@ i915-y += dvo_ch7017.o \
>   	  intel_sdvo.o \
>   	  intel_tv.o
>
> +# virtual gpu code
> +i915-y += i915_vgpu.o
> +
>   # legacy horrors
>   i915-y += i915_dma.o \
>   	  i915_ums.o
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 54691bc..e1e221e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1470,6 +1470,10 @@ struct i915_workarounds {
>   	u32 count;
>   };
>
> +struct i915_virtual_gpu {
> +	bool active;
> +};
> +
>   struct drm_i915_private {
>   	struct drm_device *dev;
>   	struct kmem_cache *slab;
> @@ -1482,6 +1486,8 @@ struct drm_i915_private {
>
>   	struct intel_uncore uncore;
>
> +	struct i915_virtual_gpu vgpu;
> +
>   	struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>
>
> @@ -2311,6 +2317,11 @@ extern void intel_uncore_check_errors(struct drm_device *dev);
>   extern void intel_uncore_fini(struct drm_device *dev);
>   extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>
> +static inline bool intel_vgpu_active(struct drm_device *dev)
> +{
> +	return to_i915(dev)->vgpu.active;
> +}
> +
>   void
>   i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>   		     u32 status_mask);
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
> new file mode 100644
> index 0000000..3f6b797
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include "intel_drv.h"
> +#include "i915_vgpu.h"
> +
> +
> +/**
> + * DOC: Intel GVT-g guest support
> + *
> + * Intel GVT-g is a graphics virtualization technology which shares the
> + * GPU among multiple virtual machines on a time-sharing basis. Each
> + * virtual machine is presented a virtual GPU (vGPU), which has equivalent
> + * features as the underlying physical GPU (pGPU), so i915 driver can run
> + * seamlessly in a virtual machine. This file provides vGPU specific
> + * optimizations when running in a virtual machine, to reduce the complexity
> + * of vGPU emulation and to improve the overall performance.
> + *
> + * A primary function introduced here is so-called "address space ballooning"
> + * technique. Intel GVT-g partitions global graphics memory among multiple VMs,
> + * so each VM can directly access a portion of the memory w/o hypervisor's
> + * intervention, e.g. filling textures or queuing commands. However w/ the

Personal preference, but It may be nicer to avoid "w/o" and similar 
shorthand in documentation.

> + * partitioning an unmodified i915 driver would assume a smaller graphics
> + * memory starting from address ZERO, then requires vGPU emulation module to
> + * translate the graphics address between 'guest view' and 'host view', for
> + * all registers and command opcodes which contain a graphics memory address.
> + * To reduce the complexity, Intel GVT-g introduces "address space ballooning",
> + * by telling the exact partitioning knowledge to each guest i915 driver, which
> + * then reserves and prevents non-allocated portions from allocation. Thus vGPU
> + * emulation module module only needs to scan and validate graphics addresses
> + * w/o complexity of address translation.
> + *
> + */
> +
> +/**
> + * i915_check_vgpu - detect virtual GPU
> + * @dev: drm device *
> + *
> + * This function is called at the initialization stage, to detect whether
> + * running on a vGPU.
> + */
> +void i915_check_vgpu(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	uint64_t magic;
> +	uint32_t version;
> +
> +	BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
> +
> +	if (!IS_HASWELL(dev))
> +		return;
> +
> +	magic = readq(dev_priv->regs + vgtif_reg(magic));
> +	if (magic != VGT_MAGIC)
> +		return;
> +
> +	version = INTEL_VGT_IF_VERSION_ENCODE(
> +		readw(dev_priv->regs + vgtif_reg(version_major)),
> +		readw(dev_priv->regs + vgtif_reg(version_minor)));
> +	if (version != INTEL_VGT_IF_VERSION)
> +		return;

Would it be useful to something on version mismatch?

> +	dev_priv->vgpu.active = true;
> +	DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
> +}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
> new file mode 100644
> index 0000000..5f41d01c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _I915_VGPU_H_
> +#define _I915_VGPU_H_
> +
> +/* The MMIO offset of the shared info between guest and host emulator */
> +#define VGT_PVINFO_PAGE	0x78000
> +#define VGT_PVINFO_SIZE	0x1000
> +
> +/*
> + * The following structure pages are defined in GEN MMIO space
> + * for virtualization. (One page for now)
> + */
> +#define VGT_MAGIC         0x4776544776544776	/* 'vGTvGTvG' */
> +#define VGT_VERSION_MAJOR 1
> +#define VGT_VERSION_MINOR 0
> +
> +#define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 | (minor))
> +#define INTEL_VGT_IF_VERSION \
> +	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
> +
> +struct vgt_if {
> +	uint64_t magic;		/* VGT_MAGIC */
> +	uint16_t version_major;
> +	uint16_t version_minor;
> +	uint32_t vgt_id;	/* ID of vGT instance */
> +	uint32_t rsv1[12];	/* pad to offset 0x40 */

In general I thought stdint types should be avoided in the kernel in 
favour of u32 and company. But I see i915 is full of them already so 
maybe my information is outdated.

> +	/*
> +	 *  Data structure to describe the balooning info of resources.
> +	 *  Each VM can only have one portion of continuous area for now.
> +	 *  (May support scattered resource in future)
> +	 *  (starting from offset 0x40)
> +	 */
> +	struct {
> +		/* Aperture register balooning */
> +		struct {
> +			uint32_t my_base;
> +			uint32_t my_size;
> +		} low_gmadr;	/* aperture */
> +		/* GMADR register balooning */
> +		struct {
> +			uint32_t my_base;
> +			uint32_t my_size;
> +		} high_gmadr;	/* non aperture */
> +		/* allowed fence registers */
> +		uint32_t fence_num;
> +		uint32_t rsv2[3];
> +	} avail_rs;		/* available/assigned resource */

I didn't figure out yet why the concept is to pass in two ranges to mark 
as reserved, instead of one usable/allowed range and then implicitly all 
the rest as reserved?

> +	uint32_t rsv3[0x200 - 24];	/* pad to half page */
> +	/*
> +	 * The bottom half page is for response from Gfx driver to hypervisor.
> +	 * Set to reserved fields temporarily by now.
> +	 */
> +	uint32_t rsv4;
> +	uint32_t display_ready;	/* ready for display owner switch */
> +	uint32_t rsv5[0x200 - 2];	/* pad to one page */
> +} __packed;

You probably don't need __packed since you have BUILD_BUG_ON on size 
mismatch and pad to full size with reserved fields. I don't think it 
matters though.

> +#define vgtif_reg(x) \
> +	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> +
> +extern void i915_check_vgpu(struct drm_device *dev);
> +
> +#endif /* _I915_VGPU_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 9427641..cae27bb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -23,6 +23,7 @@
>
>   #include "i915_drv.h"
>   #include "intel_drv.h"
> +#include "i915_vgpu.h"
>
>   #define FORCEWAKE_ACK_TIMEOUT_MS 2
>
> @@ -850,6 +851,8 @@ void intel_uncore_init(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>
> +	i915_check_vgpu(dev);
> +
>   	setup_timer(&dev_priv->uncore.force_wake_timer,
>   		    gen6_force_wake_timer, (unsigned long)dev_priv);
>
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> With Intel GVT-g, the global graphic memory space is partitioned by
> multiple vGPU instances in different VMs. The ballooning code is called
> in i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
> mark the graphic address space which are partitioned out to other vGPUs
> as reserved.
>
> v2:
> take Chris and Daniel's comments:
> 	- no guard page between different VMs
> 	- use drm_mm_reserve_node() to do the reservation for ballooning,
> 	instead of the previous drm_mm_insert_node_in_range_generic()
>
> v3:
> take Daniel's comments:
> 	- move ballooning functions into i915_vgpu.c
> 	- add kerneldoc to ballooning functions
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c |  17 +++-
>   drivers/gpu/drm/i915/i915_vgpu.c    | 149 ++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_vgpu.h    |   2 +
>   3 files changed, 165 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index de12017..2dfac13 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -27,6 +27,7 @@
>   #include <drm/drmP.h>
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> +#include "i915_vgpu.h"
>   #include "i915_trace.h"
>   #include "intel_drv.h"
>
> @@ -1683,6 +1684,16 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>
>   	/* Subtract the guard page ... */
>   	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> +
> +	dev_priv->gtt.base.start = start;
> +	dev_priv->gtt.base.total = end - start;
> +
> +	if (intel_vgpu_active(dev)) {
> +		ret = intel_vgt_balloon(dev);
> +		if (ret)
> +			return ret;
> +	}
> +

Out of curiosity, what will be the mechanism to prevent a vGPU instance 
from ignoring the ballooning data? Must be something in the hypervisor 
blocking pass-through access to such domains?

And probably GPU reset should also be disallowed for vGPU instances?

>   	if (!HAS_LLC(dev))
>   		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
>
> @@ -1702,9 +1713,6 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>   		vma->bound |= GLOBAL_BIND;
>   	}
>
> -	dev_priv->gtt.base.start = start;
> -	dev_priv->gtt.base.total = end - start;
> -
>   	/* Clear any non-preallocated blocks */
>   	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
>   		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
> @@ -1756,6 +1764,9 @@ void i915_global_gtt_cleanup(struct drm_device *dev)
>   	}
>
>   	if (drm_mm_initialized(&vm->mm)) {
> +		if (intel_vgpu_active(dev))
> +			intel_vgt_deballoon();
> +
>   		drm_mm_takedown(&vm->mm);
>   		list_del(&vm->global_link);
>   	}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
> index 3f6b797..ff5fba3 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -83,3 +83,152 @@ void i915_check_vgpu(struct drm_device *dev)
>   	dev_priv->vgpu.active = true;
>   	DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>   }
> +
> +struct _balloon_info_ {
> +	/*
> +	 * There are up to 2 regions per low/high graphic memory that
> +	 * might be ballooned. Here, index 0/1 is for low
> +	 * graphic memory, 2/3 for high graphic memory.
> +	 */
> +	struct drm_mm_node space[4];
> +} bl_info;

This should be static I think.

> +/**
> + * intel_vgt_deballoon - deballoon reserved graphics address trunks
> + *
> + * This function is called to deallocate the ballooned-out graphic memory, when
> + * driver is unloaded or when ballooning fails.
> + */
> +void intel_vgt_deballoon(void)
> +{
> +	int i;
> +
> +	DRM_INFO("VGT deballoon.\n");

Would debug be more appropriate? I don't see much value of saying this 
on driver unload - it's not that it is optional at this point.

Also for all logging, is intended human readable name VGT or vGT? If the 
latter it would be nicer to log it in that form.

> +
> +	for (i = 0; i < 4; i++) {
> +		if (bl_info.space[i].allocated)
> +			drm_mm_remove_node(&bl_info.space[i]);
> +	}
> +
> +	memset(&bl_info, 0, sizeof(bl_info));
> +}
> +
> +static int vgt_balloon_space(struct drm_mm *mm,
> +			     struct drm_mm_node *node,
> +			     unsigned long start, unsigned long end)
> +{
> +	unsigned long size = end - start;
> +
> +	if (start == end)
> +		return -EINVAL;
> +
> +	DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KB.\n",
> +		 start, end, size / 1024);

KiB ?

> +	node->start = start;
> +	node->size = size;
> +
> +	return drm_mm_reserve_node(mm, node);
> +}
> +
> +/**
> + * intel_vgt_balloon - balloon out reserved graphics address trunks
> + * @dev: drm device
> + *
> + * This function is called at the initialization stage, to balloon out the
> + * graphic address space allocated to other VMs, by marking these spaces as
> + * reserved.
> + *
> + * The ballooning related knowledges(starting address and size of the low/high

s/knowledges\(/knowledge /

> + * graphic memory) are depicted in the vgt_if structure in a reserved MMIO
> + * range.
> + *
> + * Returns:
> + * zero on success, non-zero if configuration invalid or ballooning failed
> + */
> +int intel_vgt_balloon(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
> +	unsigned long ggtt_vm_end = ggtt_vm->start + ggtt_vm->total;
> +
> +	unsigned long low_gm_base, low_gm_size, low_gm_end;
> +	unsigned long high_gm_base, high_gm_size, high_gm_end;
> +	int ret;
> +
> +	low_gm_base = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_base));
> +	low_gm_size = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_size));
> +	high_gm_base = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_base));
> +	high_gm_size = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_size));

Get rid of my_ prefix ?

> +
> +	low_gm_end = low_gm_base + low_gm_size;
> +	high_gm_end = high_gm_base + high_gm_size;
> +
> +	DRM_INFO("VGT ballooning configuration:\n");
> +	DRM_INFO("Low graphic memory: base 0x%lx size %ldKB\n",
> +		 low_gm_base, low_gm_size / 1024);
> +	DRM_INFO("High graphic memory: base 0x%lx size %ldKB\n",
> +		 high_gm_base, high_gm_size / 1024);
> +
> +	if (low_gm_base < ggtt_vm->start
> +	    || low_gm_end > dev_priv->gtt.mappable_end
> +	    || high_gm_base < dev_priv->gtt.mappable_end
> +	    || high_gm_end > ggtt_vm_end) {
> +		DRM_ERROR("Invalid ballooning configuration!\n");
> +		return -EINVAL;
> +	}
> +
> +	memset(&bl_info, 0, sizeof(bl_info));

If bl_info is static then you don't need this memset?

> +	/* High graphic memory ballooning */
> +	if (high_gm_base > dev_priv->gtt.mappable_end) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[2],
> +					dev_priv->gtt.mappable_end,
> +					high_gm_base);
> +
> +		if (ret)
> +			goto err;
> +	}
> +
> +	/*
> +	 * No need to partition out the last physical page,
> +	 * because it is reserved to the guard page.
> +	 */
> +	if (high_gm_end < ggtt_vm_end - PAGE_SIZE) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[3],
> +					high_gm_end,
> +					ggtt_vm_end - PAGE_SIZE);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	/* Low graphic memory ballooning */
> +	if (low_gm_base > ggtt_vm->start) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[0],
> +					ggtt_vm->start, low_gm_base);
> +
> +		if (ret)
> +			goto err;
> +	}
> +
> +	if (low_gm_end < dev_priv->gtt.mappable_end) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[1],
> +					low_gm_end,
> +					dev_priv->gtt.mappable_end);
> +
> +		if (ret)
> +			goto err;
> +	}

Okay, I've figured it out. :) I suppose going back to patch 1, where it 
says "Each VM can only have one portion of continuous area for now", 
with the emphasis on _one_. That threw me off thinking you have two 
"balloons" meaning forbidden areas. And then with low and high naming I 
got the wrong idea that one balloon marks the bottom inaccessible part, 
and the other top. I didn't figure out the whole low == mappable, high 
== non-mappable split. I suppose it was my inexperience, but if you can 
think of a way on how to improve the comment, even ASCII art would be 
very nice if possible.

Actually ballooning in the first place confuses me because I thought 
prior use for that in virtualisation was for memory which can shrink and 
grow at runtime. Did I get that wrong?

> +	DRM_INFO("VGT balloon successfully\n");
> +	return 0;
> +
> +err:
> +	DRM_ERROR("VGT balloon fail\n");
> +	intel_vgt_deballoon();
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
> index 5f41d01c..f538b18 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -81,5 +81,7 @@ struct vgt_if {
>   	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>
>   extern void i915_check_vgpu(struct drm_device *dev);
> +extern int intel_vgt_balloon(struct drm_device *dev);
> +extern void intel_vgt_deballoon(void);
>
>   #endif /* _I915_VGPU_H_ */
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> With Intel GVT-g, the fence registers are partitioned by multiple
> vGPU instances in different VMs. Routine i915_gem_load() is modified
> to reset the num_fence_regs, when the driver detects it's running in
> a VM. And the allocated fence number is provided in PV INFO page
> structure.
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1de94cc..0c8b32e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -29,6 +29,7 @@
>   #include <drm/drm_vma_manager.h>
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> +#include "i915_vgpu.h"
>   #include "i915_trace.h"
>   #include "intel_drv.h"
>   #include <linux/oom.h>
> @@ -5014,6 +5015,10 @@ i915_gem_load(struct drm_device *dev)
>   	else
>   		dev_priv->num_fence_regs = 8;
>
> +	if (intel_vgpu_active(dev))
> +		dev_priv->num_fence_regs =
> +				I915_READ(vgtif_reg(avail_rs.fence_num));
> +
>   	/* Initialize fence registers to zero */
>   	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
>   	i915_gem_restore_fences(dev);
>

You don't need a start offset and number of allocated fences per domain?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> Framebuffer compression is disabled when driver detects it's
> running in a Intel GVT-g enlightened VM, because FBC is not
> emulated and there is no stolen memory for a vGPU.
>
> v2:
> take Chris' comments:
> 	- move the code into intel_update_fbc()
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7a69eba..3bc5d93 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -544,6 +544,10 @@ void intel_update_fbc(struct drm_device *dev)
>   		return;
>   	}
>
> +	/* disable framebuffer compression in vGPU */
> +	if (intel_vgpu_active(dev))
> +		i915.enable_fbc = 0;
> +
>   	/*
>   	 * If FBC is already on, we just have to verify that we can
>   	 * keep it that way...
>

Looks like you'll need to rebase this one, I see no intel_update_fbc in 
my tree. :(

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/8] drm/i915: Add the display switch logic for vGPU in i915 driver

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> Display switch logic is added to notify the host side that
> current vGPU have a valid surface to show. It does so by
> writing the display_ready field in PV INFO page, and then
> will be handled in the host side. This is useful to avoid
> trickiness when the VM's framebuffer is being accessed in
> the middle of VM modesetting, e.g. compositing the framebuffer
> in the host side.
>
> v2:
> 	- move the notification code outside the 'else' in load sequence
> 	- remove the notification code in intel_crtc_set_config()
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_dma.c  | 11 +++++++++++
>   drivers/gpu/drm/i915/i915_vgpu.h |  7 +++++++
>   2 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9a73533..06daa5c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -36,6 +36,7 @@
>   #include "intel_drv.h"
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> +#include "i915_vgpu.h"
>   #include "i915_trace.h"
>   #include <linux/pci.h>
>   #include <linux/console.h>
> @@ -1780,6 +1781,16 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>   		dev_priv->ums.mm_suspended = 1;
>   	}
>
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +		I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
> +	}
> +

Current code base already has dev_priv so looks like another rebase. 
Otherwise pretty simple so:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

>   	i915_setup_sysfs(dev);
>
>   	if (INTEL_INFO(dev)->num_pipes) {
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
> index f538b18..9d35fb8 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -80,6 +80,13 @@ struct vgt_if {
>   #define vgtif_reg(x) \
>   	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>
> +/* vGPU display status to be used by the host side */
> +enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,  /* ready for display switch */
> +};
> +
> +
>   extern void i915_check_vgpu(struct drm_device *dev);
>   extern int intel_vgt_balloon(struct drm_device *dev);
>   extern void intel_vgt_deballoon(void);
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/8] drm/i915: Disable power management for i915 driver in VM

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> With Intel GVT-g, GPU power management is controlled by
> host driver, so there is no need to provide virtualized
> GPU PM support. In the future it might be useful to gather
> VM input for freq boost, but now let's disable it simply.
>
> v2:
> take Chris' comments:
>          - do not special case this to gen6+
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3bc5d93..3722bd4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5314,6 +5314,10 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>
> +	/* Powersaving is controlled by the host when inside a VM */
> +	if (intel_vgpu_active(dev))
> +		return;
> +
>   	if (IS_IRONLAKE_M(dev)) {
>   		mutex_lock(&dev->struct_mutex);
>   		ironlake_enable_drps(dev);
>

I was looking for potential call sites of this which come earlier than 
i915_check_vgpu. Didn't find any but found RPS (intel_pm_setup) - what's 
with that - should it be disabled as well?

Otherwise,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/8] drm/i915: Create vGPU specific write MMIO to reduce traps

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> In the virtualized environment, forcewake operations are not
> necessory for the driver, because mmio accesses will be trapped

necessary

> and emulated by the host side, and real forcewake operations are
> also done in the host. New mmio write handlers are added to directly
> call the __raw_i915_write, therefore will reduce many traps and
> increase the overall performance for drivers running in the VM
> with Intel GVT-g enhancement.
>
> v2:
> take Chris' comments:
>          - register the mmio hooks in intel_uncore_init()
> v3:
> take Daniel's comments:
>          - use macros to assign mmio write functions for vGPU
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index cae27bb..b76c21d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -727,6 +727,14 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
>   	REG_WRITE_FOOTER; \
>   }
>
> +#define __vgpu_write(x) \
> +static void \
> +vgpu_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> +	REG_WRITE_HEADER; \
> +	__raw_i915_write##x(dev_priv, reg, val); \
> +	REG_WRITE_FOOTER; \
> +}
> +
>   static const u32 gen8_shadowed_regs[] = {
>   	FORCEWAKE_MT,
>   	GEN6_RPNSWREQ,
> @@ -821,6 +829,10 @@ __gen4_write(8)
>   __gen4_write(16)
>   __gen4_write(32)
>   __gen4_write(64)
> +__vgpu_write(8)
> +__vgpu_write(16)
> +__vgpu_write(32)
> +__vgpu_write(64)
>
>   #undef __chv_write
>   #undef __gen8_write
> @@ -828,6 +840,7 @@ __gen4_write(64)
>   #undef __gen6_write
>   #undef __gen5_write
>   #undef __gen4_write
> +#undef __vgpu_write
>   #undef REG_WRITE_FOOTER
>   #undef REG_WRITE_HEADER
>
> @@ -939,6 +952,9 @@ void intel_uncore_init(struct drm_device *dev)
>   		break;
>   	}
>
> +	if (intel_vgpu_active(dev))
> +		ASSIGN_WRITE_MMIO_VFUNCS(vgpu);
> +
>   	i915_check_and_clear_faults(dev);
>   }
>   #undef ASSIGN_WRITE_MMIO_VFUNCS
>

Maybe I am missing something obvious, but why are read variants not needed?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 8/8] drm/i915: Support alias ppgtt in VM if ppgtt is enabled

[Tvrtko Ursulin]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> The current Intel GVT-g only supports alias ppgtt. And the
> emulation is done in the host by first trapping PP_DIR_BASE
> mmio accesses. Updating PP_DIR_BASE by using instructions such
> as MI_LOAD_REGISTER_IMM are hard to detect and are not supported
> in current code. Therefore this patch also added a new callback
> routine - vgpu_mm_switch() to set the PP_DIR_BASE by mmio writes.
>
> v2:
> take Chris' comments:
>          - move the code into sanitize_enable_ppgtt()
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 2dfac13..55307eb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -44,6 +44,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>   	if (IS_GEN8(dev))
>   		has_full_ppgtt = false; /* XXX why? */
>
> +	if (intel_vgpu_active(dev))
> +		has_full_ppgtt = false; /* emulation is too hard */
> +
>   	if (enable_ppgtt == 0 || !has_aliasing_ppgtt)
>   		return 0;
>
> @@ -733,6 +736,16 @@ static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
>   	return 0;
>   }
>
> +static int vgpu_mm_switch(struct i915_hw_ppgtt *ppgtt,
> +			 struct intel_engine_cs *ring)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
> +
> +	I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G);
> +	I915_WRITE(RING_PP_DIR_BASE(ring), get_pd_offset(ppgtt));
> +	return 0;
> +}
> +
>   static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
>   			  struct intel_engine_cs *ring)
>   {
> @@ -1059,6 +1072,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>   	} else
>   		BUG();
>
> +	if (intel_vgpu_active(dev))
> +		ppgtt->switch_mm = vgpu_mm_switch;
> +
>   	ret = gen6_ppgtt_alloc(ppgtt);
>   	if (ret)
>   		return ret;
>

Pending potential "checkpatch.pl --strict" fixes,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Daniel Vetter]

On Thu, Dec 11, 2014 at 05:33:00PM +0000, Tvrtko Ursulin wrote:
> >+struct vgt_if {
> >+	uint64_t magic;		/* VGT_MAGIC */
> >+	uint16_t version_major;
> >+	uint16_t version_minor;
> >+	uint32_t vgt_id;	/* ID of vGT instance */
> >+	uint32_t rsv1[12];	/* pad to offset 0x40 */
> 
> In general I thought stdint types should be avoided in the kernel in favour
> of u32 and company. But I see i915 is full of them already so maybe my
> information is outdated.

That was back when c99 types weren't a common thing, nowadays I think we
should prefer _t types for more uniformity with userspace. Also it makes
the __u32 and similar types which are required for ioctl structs stand out
much more. And yes we're totally inconsistent about it.

Just an aside.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/8] drm/i915: Add the display switch logic for vGPU in i915 driver

[Daniel Vetter]

On Thu, Nov 13, 2014 at 08:02:46PM +0800, Yu Zhang wrote:
> Display switch logic is added to notify the host side that
> current vGPU have a valid surface to show. It does so by
> writing the display_ready field in PV INFO page, and then
> will be handled in the host side. This is useful to avoid
> trickiness when the VM's framebuffer is being accessed in
> the middle of VM modesetting, e.g. compositing the framebuffer
> in the host side.
> 
> v2:
> 	- move the notification code outside the 'else' in load sequence
> 	- remove the notification code in intel_crtc_set_config()
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c  | 11 +++++++++++
>  drivers/gpu/drm/i915/i915_vgpu.h |  7 +++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9a73533..06daa5c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -36,6 +36,7 @@
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> +#include "i915_vgpu.h"
>  #include "i915_trace.h"
>  #include <linux/pci.h>
>  #include <linux/console.h>
> @@ -1780,6 +1781,16 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		dev_priv->ums.mm_suspended = 1;
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +		I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
> +	}
> +
>  	i915_setup_sysfs(dev);
>  
>  	if (INTEL_INFO(dev)->num_pipes) {
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
> index f538b18..9d35fb8 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -80,6 +80,13 @@ struct vgt_if {
>  #define vgtif_reg(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/* vGPU display status to be used by the host side */
> +enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,  /* ready for display switch */
> +};

Nitpick: Since this is a virtual register I prefer we use plain #defines
with explicit codes for everything instead of enums. Makes it clearer
which values are used and all that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/8] Add enlightenments for vGPU

[Daniel Vetter]

On Thu, Dec 11, 2014 at 05:03:46PM +0000, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> I'll try to do the detailed review of your series in the following few days.
> I might ask some questions on the design also to help me understand the
> bigger picture.
> 
> First thing, I see that patches are checkpatch.pl clean, apart when run in
> strict mode. I think Daniel prefers "--strict" nowadays, at least I needed
> to fix up those in my patches so you should probably do the same.

Well I mostly run --strict since that checks for the alignment of
continuation lines for function parameters. Imo not following the i915
coding style for that looks a bit too jarring ;-)

Still checkpatch is just a tool and occasionally seriously lacks taste, so
please use --strict wisely instead of strictly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/8] Add enlightenments for vGPU

[Jani Nikula]

On Mon, 15 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> Well I mostly run --strict since that checks for the alignment of
> continuation lines for function parameters. Imo not following the i915
> coding style for that looks a bit too jarring ;-)
>
> Still checkpatch is just a tool and occasionally seriously lacks taste, so
> please use --strict wisely instead of strictly.

Perhaps we should use the --subjective option instead. Not that it would
actually change anything, it's exactly the same as --strict. We'd still
require the same things for i915, but we'd be openly subjective about
it. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Yu, Zhang]

Thank you very much for your thorough review, Tvrtko. :)

On 12/12/2014 1:33 AM, Tvrtko Ursulin wrote:
>
> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>> Introduce a PV INFO structure, to facilitate the Intel GVT-g
>> technology, which is a GPU virtualization solution with mediated
>> pass-through. This page contains the shared information between
>> i915 driver and the host emulator. For now, this structure utilizes
>> an area of 4K bypes on HSW GPU's unused MMIO space to support
>
> bytes
Got it. Thanks.
>
>> existing production. Future hardware will have the reserved window
>
> It is slightly unclear to me what "existing production" means?
Our current gpu virtualization implementation for HSW. It's compared 
with future implematations for BDW etc.
>
>> architecturally defined, and layout of the page will be added in
>> future BSpec.
>>
>> The i915 driver load routine detects if it is running in a VM by
>> reading the contents of this PV INFO page. Thereafter a flag,
>> vgpu.active is set, and intel_vgpu_active() is used by checking
>> this flag to conclude if GPU is virtualized with Intel GVT-g. By
>> now, intel_vgpu_active() will return true, only when the driver
>> is running as a guest in the Intel GVT-g enhanced environment on
>> HSW platform.
>>
>> v2:
>> take Chris' comments:
>>     - call the i915_check_vgpu() in intel_uncore_init()
>>     - sanitize i915_check_vgpu() by adding BUILD_BUG_ON() and debug info
>> take Daniel's comments:
>>     - put the definition of PV INFO into a new header - i915_vgt_if.h
>> other changes:
>>     - access mmio regs by readq/readw in i915_check_vgpu()
>>
>> v3:
>> take Daniel's comments:
>>     - move the i915/vgt interfaces into a new i915_vgpu.c
>>     - update makefile
>>     - add kerneldoc to functions which are non-static
>>     - add a DOC: section describing some of the high-level design
>>     - update drm docbook
>> other changes:
>>     - rename i915_vgt_if.h to i915_vgpu.h
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>> ---
>>   Documentation/DocBook/drm.tmpl      |  5 +++
>>   drivers/gpu/drm/i915/Makefile       |  3 ++
>>   drivers/gpu/drm/i915/i915_drv.h     | 11 +++++
>>   drivers/gpu/drm/i915/i915_vgpu.c    | 85
>> +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_vgpu.h    | 85
>> +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_uncore.c |  3 ++
>>   6 files changed, 192 insertions(+)
>>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.c
>>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.h
>>
>> diff --git a/Documentation/DocBook/drm.tmpl
>> b/Documentation/DocBook/drm.tmpl
>> index 64d9c1e..e4db4cf 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -3832,6 +3832,11 @@ int num_ioctls;</synopsis>
>>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
>>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
>>         </sect2>
>> +      <sect2>
>> +        <title>Intel GVT-g Guest Support(vGPU)</title>
>> +!Pdrivers/gpu/drm/i915/i915_vgpu.c Intel GVT-g guest support
>> +!Idrivers/gpu/drm/i915/i915_vgpu.c
>> +      </sect2>
>>       </sect1>
>>       <sect1>
>>         <title>Display Hardware Handling</title>
>> diff --git a/drivers/gpu/drm/i915/Makefile
>> b/drivers/gpu/drm/i915/Makefile
>> index 891e584..d9aa5ca 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -79,6 +79,9 @@ i915-y += dvo_ch7017.o \
>>         intel_sdvo.o \
>>         intel_tv.o
>>
>> +# virtual gpu code
>> +i915-y += i915_vgpu.o
>> +
>>   # legacy horrors
>>   i915-y += i915_dma.o \
>>         i915_ums.o
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 54691bc..e1e221e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1470,6 +1470,10 @@ struct i915_workarounds {
>>       u32 count;
>>   };
>>
>> +struct i915_virtual_gpu {
>> +    bool active;
>> +};
>> +
>>   struct drm_i915_private {
>>       struct drm_device *dev;
>>       struct kmem_cache *slab;
>> @@ -1482,6 +1486,8 @@ struct drm_i915_private {
>>
>>       struct intel_uncore uncore;
>>
>> +    struct i915_virtual_gpu vgpu;
>> +
>>       struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>>
>>
>> @@ -2311,6 +2317,11 @@ extern void intel_uncore_check_errors(struct
>> drm_device *dev);
>>   extern void intel_uncore_fini(struct drm_device *dev);
>>   extern void intel_uncore_forcewake_reset(struct drm_device *dev,
>> bool restore);
>>
>> +static inline bool intel_vgpu_active(struct drm_device *dev)
>> +{
>> +    return to_i915(dev)->vgpu.active;
>> +}
>> +
>>   void
>>   i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>>                u32 status_mask);
>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
>> b/drivers/gpu/drm/i915/i915_vgpu.c
>> new file mode 100644
>> index 0000000..3f6b797
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
>> @@ -0,0 +1,85 @@
>> +/*
>> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> the next
>> + * paragraph) shall be included in all copies or substantial portions
>> of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> DEALINGS IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#include "intel_drv.h"
>> +#include "i915_vgpu.h"
>> +
>> +
>> +/**
>> + * DOC: Intel GVT-g guest support
>> + *
>> + * Intel GVT-g is a graphics virtualization technology which shares the
>> + * GPU among multiple virtual machines on a time-sharing basis. Each
>> + * virtual machine is presented a virtual GPU (vGPU), which has
>> equivalent
>> + * features as the underlying physical GPU (pGPU), so i915 driver can
>> run
>> + * seamlessly in a virtual machine. This file provides vGPU specific
>> + * optimizations when running in a virtual machine, to reduce the
>> complexity
>> + * of vGPU emulation and to improve the overall performance.
>> + *
>> + * A primary function introduced here is so-called "address space
>> ballooning"
>> + * technique. Intel GVT-g partitions global graphics memory among
>> multiple VMs,
>> + * so each VM can directly access a portion of the memory w/o
>> hypervisor's
>> + * intervention, e.g. filling textures or queuing commands. However
>> w/ the
>
> Personal preference, but It may be nicer to avoid "w/o" and similar
> shorthand in documentation.
OK. Will fix it. :)
>
>> + * partitioning an unmodified i915 driver would assume a smaller
>> graphics
>> + * memory starting from address ZERO, then requires vGPU emulation
>> module to
>> + * translate the graphics address between 'guest view' and 'host
>> view', for
>> + * all registers and command opcodes which contain a graphics memory
>> address.
>> + * To reduce the complexity, Intel GVT-g introduces "address space
>> ballooning",
>> + * by telling the exact partitioning knowledge to each guest i915
>> driver, which
>> + * then reserves and prevents non-allocated portions from allocation.
>> Thus vGPU
>> + * emulation module module only needs to scan and validate graphics
>> addresses
>> + * w/o complexity of address translation.
>> + *
>> + */
>> +
>> +/**
>> + * i915_check_vgpu - detect virtual GPU
>> + * @dev: drm device *
>> + *
>> + * This function is called at the initialization stage, to detect
>> whether
>> + * running on a vGPU.
>> + */
>> +void i915_check_vgpu(struct drm_device *dev)
>> +{
>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>> +    uint64_t magic;
>> +    uint32_t version;
>> +
>> +    BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
>> +
>> +    if (!IS_HASWELL(dev))
>> +        return;
>> +
>> +    magic = readq(dev_priv->regs + vgtif_reg(magic));
>> +    if (magic != VGT_MAGIC)
>> +        return;
>> +
>> +    version = INTEL_VGT_IF_VERSION_ENCODE(
>> +        readw(dev_priv->regs + vgtif_reg(version_major)),
>> +        readw(dev_priv->regs + vgtif_reg(version_minor)));
>> +    if (version != INTEL_VGT_IF_VERSION)
>> +        return;
>
> Would it be useful to something on version mismatch?
Well. Thanks for your notification. :)
In fact, this version judgement is just for future usages. By now, we 
have only one vgt interface version, 0x10000. But you are right, unlike 
the judgement for 'magic', which specifies the Intel GVT-g feature, and 
can be returned directly when false, failure for the 'version' may 
should trigger something like unload the driver. However, if so, this 
means we may need to:
1> return something in i915_check_vgpu();
2> change the prototype of i915_check_vgpu() and its caller 
intel_uncore_init();
3> check the return value of intel_uncore_init() in i915_driver_load()
Is this approach acceptable?

>
>> +    dev_priv->vgpu.active = true;
>> +    DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h
>> b/drivers/gpu/drm/i915/i915_vgpu.h
>> new file mode 100644
>> index 0000000..5f41d01c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
>> @@ -0,0 +1,85 @@
>> +/*
>> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> the next
>> + * paragraph) shall be included in all copies or substantial portions
>> of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> DEALINGS IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#ifndef _I915_VGPU_H_
>> +#define _I915_VGPU_H_
>> +
>> +/* The MMIO offset of the shared info between guest and host emulator */
>> +#define VGT_PVINFO_PAGE    0x78000
>> +#define VGT_PVINFO_SIZE    0x1000
>> +
>> +/*
>> + * The following structure pages are defined in GEN MMIO space
>> + * for virtualization. (One page for now)
>> + */
>> +#define VGT_MAGIC         0x4776544776544776    /* 'vGTvGTvG' */
>> +#define VGT_VERSION_MAJOR 1
>> +#define VGT_VERSION_MINOR 0
>> +
>> +#define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 |
>> (minor))
>> +#define INTEL_VGT_IF_VERSION \
>> +    INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
>> +
>> +struct vgt_if {
>> +    uint64_t magic;        /* VGT_MAGIC */
>> +    uint16_t version_major;
>> +    uint16_t version_minor;
>> +    uint32_t vgt_id;    /* ID of vGT instance */
>> +    uint32_t rsv1[12];    /* pad to offset 0x40 */
>
> In general I thought stdint types should be avoided in the kernel in
> favour of u32 and company. But I see i915 is full of them already so
> maybe my information is outdated.
>
Let's keep the _t types, like explanations in Daniel's mail. Is this OK?
>> +    /*
>> +     *  Data structure to describe the balooning info of resources.
>> +     *  Each VM can only have one portion of continuous area for now.
>> +     *  (May support scattered resource in future)
>> +     *  (starting from offset 0x40)
>> +     */
>> +    struct {
>> +        /* Aperture register balooning */
>> +        struct {
>> +            uint32_t my_base;
>> +            uint32_t my_size;
>> +        } low_gmadr;    /* aperture */
>> +        /* GMADR register balooning */
>> +        struct {
>> +            uint32_t my_base;
>> +            uint32_t my_size;
>> +        } high_gmadr;    /* non aperture */
>> +        /* allowed fence registers */
>> +        uint32_t fence_num;
>> +        uint32_t rsv2[3];
>> +    } avail_rs;        /* available/assigned resource */
>
> I didn't figure out yet why the concept is to pass in two ranges to mark
> as reserved, instead of one usable/allowed range and then implicitly all
> the rest as reserved?
>
Like you said in 2/3 comments, this 2 part is for mappable and 
non-mapplable respectively. :)

>> +    uint32_t rsv3[0x200 - 24];    /* pad to half page */
>> +    /*
>> +     * The bottom half page is for response from Gfx driver to
>> hypervisor.
>> +     * Set to reserved fields temporarily by now.
>> +     */
>> +    uint32_t rsv4;
>> +    uint32_t display_ready;    /* ready for display owner switch */
>> +    uint32_t rsv5[0x200 - 2];    /* pad to one page */
>> +} __packed;
>
> You probably don't need __packed since you have BUILD_BUG_ON on size
> mismatch and pad to full size with reserved fields. I don't think it
> matters though.
>
Well, IIRC, this __packed is added to follow Chris's comments. How about 
we keep it? Or will it hurt any place? :)

>> +#define vgtif_reg(x) \
>> +    (VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>> +
>> +extern void i915_check_vgpu(struct drm_device *dev);
>> +
>> +#endif /* _I915_VGPU_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index 9427641..cae27bb 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -23,6 +23,7 @@
>>
>>   #include "i915_drv.h"
>>   #include "intel_drv.h"
>> +#include "i915_vgpu.h"
>>
>>   #define FORCEWAKE_ACK_TIMEOUT_MS 2
>>
>> @@ -850,6 +851,8 @@ void intel_uncore_init(struct drm_device *dev)
>>   {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> +    i915_check_vgpu(dev);
>> +
>>       setup_timer(&dev_priv->uncore.force_wake_timer,
>>               gen6_force_wake_timer, (unsigned long)dev_priv);
>>
>>
>
> Regards,
>
> Tvrtko

B.R.
Yu
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Tvrtko Ursulin]

On 12/16/2014 12:51 PM, Yu, Zhang wrote:
> Thank you very much for your thorough review, Tvrtko. :)
>
> On 12/12/2014 1:33 AM, Tvrtko Ursulin wrote:
>>
>> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>>> Introduce a PV INFO structure, to facilitate the Intel GVT-g
>>> technology, which is a GPU virtualization solution with mediated
>>> pass-through. This page contains the shared information between
>>> i915 driver and the host emulator. For now, this structure utilizes
>>> an area of 4K bypes on HSW GPU's unused MMIO space to support
>>
>> bytes
> Got it. Thanks.
>>
>>> existing production. Future hardware will have the reserved window
>>
>> It is slightly unclear to me what "existing production" means?
> Our current gpu virtualization implementation for HSW. It's compared
> with future implematations for BDW etc.

So maybe instead of "existing production" say "currently supported 
hardware" or similar?

>>> architecturally defined, and layout of the page will be added in
>>> future BSpec.
>>>
>>> The i915 driver load routine detects if it is running in a VM by
>>> reading the contents of this PV INFO page. Thereafter a flag,
>>> vgpu.active is set, and intel_vgpu_active() is used by checking
>>> this flag to conclude if GPU is virtualized with Intel GVT-g. By
>>> now, intel_vgpu_active() will return true, only when the driver
>>> is running as a guest in the Intel GVT-g enhanced environment on
>>> HSW platform.
>>>
>>> v2:
>>> take Chris' comments:
>>>     - call the i915_check_vgpu() in intel_uncore_init()
>>>     - sanitize i915_check_vgpu() by adding BUILD_BUG_ON() and debug info
>>> take Daniel's comments:
>>>     - put the definition of PV INFO into a new header - i915_vgt_if.h
>>> other changes:
>>>     - access mmio regs by readq/readw in i915_check_vgpu()
>>>
>>> v3:
>>> take Daniel's comments:
>>>     - move the i915/vgt interfaces into a new i915_vgpu.c
>>>     - update makefile
>>>     - add kerneldoc to functions which are non-static
>>>     - add a DOC: section describing some of the high-level design
>>>     - update drm docbook
>>> other changes:
>>>     - rename i915_vgt_if.h to i915_vgpu.h
>>>
>>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>>> ---
>>>   Documentation/DocBook/drm.tmpl      |  5 +++
>>>   drivers/gpu/drm/i915/Makefile       |  3 ++
>>>   drivers/gpu/drm/i915/i915_drv.h     | 11 +++++
>>>   drivers/gpu/drm/i915/i915_vgpu.c    | 85
>>> +++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/i915_vgpu.h    | 85
>>> +++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_uncore.c |  3 ++
>>>   6 files changed, 192 insertions(+)
>>>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.c
>>>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.h
>>>
>>> diff --git a/Documentation/DocBook/drm.tmpl
>>> b/Documentation/DocBook/drm.tmpl
>>> index 64d9c1e..e4db4cf 100644
>>> --- a/Documentation/DocBook/drm.tmpl
>>> +++ b/Documentation/DocBook/drm.tmpl
>>> @@ -3832,6 +3832,11 @@ int num_ioctls;</synopsis>
>>>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
>>>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
>>>         </sect2>
>>> +      <sect2>
>>> +        <title>Intel GVT-g Guest Support(vGPU)</title>
>>> +!Pdrivers/gpu/drm/i915/i915_vgpu.c Intel GVT-g guest support
>>> +!Idrivers/gpu/drm/i915/i915_vgpu.c
>>> +      </sect2>
>>>       </sect1>
>>>       <sect1>
>>>         <title>Display Hardware Handling</title>
>>> diff --git a/drivers/gpu/drm/i915/Makefile
>>> b/drivers/gpu/drm/i915/Makefile
>>> index 891e584..d9aa5ca 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -79,6 +79,9 @@ i915-y += dvo_ch7017.o \
>>>         intel_sdvo.o \
>>>         intel_tv.o
>>>
>>> +# virtual gpu code
>>> +i915-y += i915_vgpu.o
>>> +
>>>   # legacy horrors
>>>   i915-y += i915_dma.o \
>>>         i915_ums.o
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 54691bc..e1e221e 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1470,6 +1470,10 @@ struct i915_workarounds {
>>>       u32 count;
>>>   };
>>>
>>> +struct i915_virtual_gpu {
>>> +    bool active;
>>> +};
>>> +
>>>   struct drm_i915_private {
>>>       struct drm_device *dev;
>>>       struct kmem_cache *slab;
>>> @@ -1482,6 +1486,8 @@ struct drm_i915_private {
>>>
>>>       struct intel_uncore uncore;
>>>
>>> +    struct i915_virtual_gpu vgpu;
>>> +
>>>       struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>>>
>>>
>>> @@ -2311,6 +2317,11 @@ extern void intel_uncore_check_errors(struct
>>> drm_device *dev);
>>>   extern void intel_uncore_fini(struct drm_device *dev);
>>>   extern void intel_uncore_forcewake_reset(struct drm_device *dev,
>>> bool restore);
>>>
>>> +static inline bool intel_vgpu_active(struct drm_device *dev)
>>> +{
>>> +    return to_i915(dev)->vgpu.active;
>>> +}
>>> +
>>>   void
>>>   i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe
>>> pipe,
>>>                u32 status_mask);
>>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
>>> b/drivers/gpu/drm/i915/i915_vgpu.c
>>> new file mode 100644
>>> index 0000000..3f6b797
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
>>> @@ -0,0 +1,85 @@
>>> +/*
>>> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> obtaining a
>>> + * copy of this software and associated documentation files (the
>>> "Software"),
>>> + * to deal in the Software without restriction, including without
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom
>>> the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including
>>> the next
>>> + * paragraph) shall be included in all copies or substantial portions
>>> of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>>> SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>> OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>> DEALINGS IN THE
>>> + * SOFTWARE.
>>> + */
>>> +
>>> +#include "intel_drv.h"
>>> +#include "i915_vgpu.h"
>>> +
>>> +
>>> +/**
>>> + * DOC: Intel GVT-g guest support
>>> + *
>>> + * Intel GVT-g is a graphics virtualization technology which shares the
>>> + * GPU among multiple virtual machines on a time-sharing basis. Each
>>> + * virtual machine is presented a virtual GPU (vGPU), which has
>>> equivalent
>>> + * features as the underlying physical GPU (pGPU), so i915 driver can
>>> run
>>> + * seamlessly in a virtual machine. This file provides vGPU specific
>>> + * optimizations when running in a virtual machine, to reduce the
>>> complexity
>>> + * of vGPU emulation and to improve the overall performance.
>>> + *
>>> + * A primary function introduced here is so-called "address space
>>> ballooning"
>>> + * technique. Intel GVT-g partitions global graphics memory among
>>> multiple VMs,
>>> + * so each VM can directly access a portion of the memory w/o
>>> hypervisor's
>>> + * intervention, e.g. filling textures or queuing commands. However
>>> w/ the
>>
>> Personal preference, but It may be nicer to avoid "w/o" and similar
>> shorthand in documentation.
> OK. Will fix it. :)
>>
>>> + * partitioning an unmodified i915 driver would assume a smaller
>>> graphics
>>> + * memory starting from address ZERO, then requires vGPU emulation
>>> module to
>>> + * translate the graphics address between 'guest view' and 'host
>>> view', for
>>> + * all registers and command opcodes which contain a graphics memory
>>> address.
>>> + * To reduce the complexity, Intel GVT-g introduces "address space
>>> ballooning",
>>> + * by telling the exact partitioning knowledge to each guest i915
>>> driver, which
>>> + * then reserves and prevents non-allocated portions from allocation.
>>> Thus vGPU
>>> + * emulation module module only needs to scan and validate graphics
>>> addresses
>>> + * w/o complexity of address translation.
>>> + *
>>> + */
>>> +
>>> +/**
>>> + * i915_check_vgpu - detect virtual GPU
>>> + * @dev: drm device *
>>> + *
>>> + * This function is called at the initialization stage, to detect
>>> whether
>>> + * running on a vGPU.
>>> + */
>>> +void i915_check_vgpu(struct drm_device *dev)
>>> +{
>>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>>> +    uint64_t magic;
>>> +    uint32_t version;
>>> +
>>> +    BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
>>> +
>>> +    if (!IS_HASWELL(dev))
>>> +        return;
>>> +
>>> +    magic = readq(dev_priv->regs + vgtif_reg(magic));
>>> +    if (magic != VGT_MAGIC)
>>> +        return;
>>> +
>>> +    version = INTEL_VGT_IF_VERSION_ENCODE(
>>> +        readw(dev_priv->regs + vgtif_reg(version_major)),
>>> +        readw(dev_priv->regs + vgtif_reg(version_minor)));
>>> +    if (version != INTEL_VGT_IF_VERSION)
>>> +        return;
>>
>> Would it be useful to something on version mismatch?
> Well. Thanks for your notification. :)
> In fact, this version judgement is just for future usages. By now, we
> have only one vgt interface version, 0x10000. But you are right, unlike
> the judgement for 'magic', which specifies the Intel GVT-g feature, and
> can be returned directly when false, failure for the 'version' may
> should trigger something like unload the driver. However, if so, this
> means we may need to:
> 1> return something in i915_check_vgpu();
> 2> change the prototype of i915_check_vgpu() and its caller
> intel_uncore_init();
> 3> check the return value of intel_uncore_init() in i915_driver_load()
> Is this approach acceptable?

I would leave this for the future version. I was simply suggesting 
logging something on version mismatch. Just because it is unexpected so 
there is a trace in the logs why feature is not available. It's up to you.

>>
>>> +    dev_priv->vgpu.active = true;
>>> +    DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h
>>> b/drivers/gpu/drm/i915/i915_vgpu.h
>>> new file mode 100644
>>> index 0000000..5f41d01c
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
>>> @@ -0,0 +1,85 @@
>>> +/*
>>> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> obtaining a
>>> + * copy of this software and associated documentation files (the
>>> "Software"),
>>> + * to deal in the Software without restriction, including without
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom
>>> the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including
>>> the next
>>> + * paragraph) shall be included in all copies or substantial portions
>>> of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>>> SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>> OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>> DEALINGS IN THE
>>> + * SOFTWARE.
>>> + */
>>> +
>>> +#ifndef _I915_VGPU_H_
>>> +#define _I915_VGPU_H_
>>> +
>>> +/* The MMIO offset of the shared info between guest and host
>>> emulator */
>>> +#define VGT_PVINFO_PAGE    0x78000
>>> +#define VGT_PVINFO_SIZE    0x1000
>>> +
>>> +/*
>>> + * The following structure pages are defined in GEN MMIO space
>>> + * for virtualization. (One page for now)
>>> + */
>>> +#define VGT_MAGIC         0x4776544776544776    /* 'vGTvGTvG' */
>>> +#define VGT_VERSION_MAJOR 1
>>> +#define VGT_VERSION_MINOR 0
>>> +
>>> +#define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 |
>>> (minor))
>>> +#define INTEL_VGT_IF_VERSION \
>>> +    INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
>>> +
>>> +struct vgt_if {
>>> +    uint64_t magic;        /* VGT_MAGIC */
>>> +    uint16_t version_major;
>>> +    uint16_t version_minor;
>>> +    uint32_t vgt_id;    /* ID of vGT instance */
>>> +    uint32_t rsv1[12];    /* pad to offset 0x40 */
>>
>> In general I thought stdint types should be avoided in the kernel in
>> favour of u32 and company. But I see i915 is full of them already so
>> maybe my information is outdated.
>>
> Let's keep the _t types, like explanations in Daniel's mail. Is this OK?

Yes.

>>> +    /*
>>> +     *  Data structure to describe the balooning info of resources.
>>> +     *  Each VM can only have one portion of continuous area for now.
>>> +     *  (May support scattered resource in future)
>>> +     *  (starting from offset 0x40)
>>> +     */
>>> +    struct {
>>> +        /* Aperture register balooning */
>>> +        struct {
>>> +            uint32_t my_base;
>>> +            uint32_t my_size;
>>> +        } low_gmadr;    /* aperture */
>>> +        /* GMADR register balooning */
>>> +        struct {
>>> +            uint32_t my_base;
>>> +            uint32_t my_size;
>>> +        } high_gmadr;    /* non aperture */
>>> +        /* allowed fence registers */
>>> +        uint32_t fence_num;
>>> +        uint32_t rsv2[3];
>>> +    } avail_rs;        /* available/assigned resource */
>>
>> I didn't figure out yet why the concept is to pass in two ranges to mark
>> as reserved, instead of one usable/allowed range and then implicitly all
>> the rest as reserved?
>>
> Like you said in 2/3 comments, this 2 part is for mappable and
> non-mapplable respectively. :)

Are high and low memory as terms known in the i915 space so far as 
synonyms for mappable and non-mappable?

>>> +    uint32_t rsv3[0x200 - 24];    /* pad to half page */
>>> +    /*
>>> +     * The bottom half page is for response from Gfx driver to
>>> hypervisor.
>>> +     * Set to reserved fields temporarily by now.
>>> +     */
>>> +    uint32_t rsv4;
>>> +    uint32_t display_ready;    /* ready for display owner switch */
>>> +    uint32_t rsv5[0x200 - 2];    /* pad to one page */
>>> +} __packed;
>>
>> You probably don't need __packed since you have BUILD_BUG_ON on size
>> mismatch and pad to full size with reserved fields. I don't think it
>> matters though.
>>
> Well, IIRC, this __packed is added to follow Chris's comments. How about
> we keep it? Or will it hurt any place? :)

The only purpose for packed here I can see is a double guard along the 
BUILD_BUG_ON. As I said I don't think it makes any difference so you can 
leave it in.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Yu, Zhang]

On 12/12/2014 9:00 PM, Tvrtko Ursulin wrote:
>
> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>> With Intel GVT-g, the global graphic memory space is partitioned by
>> multiple vGPU instances in different VMs. The ballooning code is called
>> in i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
>> mark the graphic address space which are partitioned out to other vGPUs
>> as reserved.
>>
>> v2:
>> take Chris and Daniel's comments:
>>     - no guard page between different VMs
>>     - use drm_mm_reserve_node() to do the reservation for ballooning,
>>     instead of the previous drm_mm_insert_node_in_range_generic()
>>
>> v3:
>> take Daniel's comments:
>>     - move ballooning functions into i915_vgpu.c
>>     - add kerneldoc to ballooning functions
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c |  17 +++-
>>   drivers/gpu/drm/i915/i915_vgpu.c    | 149
>> ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_vgpu.h    |   2 +
>>   3 files changed, 165 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index de12017..2dfac13 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -27,6 +27,7 @@
>>   #include <drm/drmP.h>
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>> +#include "i915_vgpu.h"
>>   #include "i915_trace.h"
>>   #include "intel_drv.h"
>>
>> @@ -1683,6 +1684,16 @@ int i915_gem_setup_global_gtt(struct drm_device
>> *dev,
>>
>>       /* Subtract the guard page ... */
>>       drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
>> +
>> +    dev_priv->gtt.base.start = start;
>> +    dev_priv->gtt.base.total = end - start;
>> +
>> +    if (intel_vgpu_active(dev)) {
>> +        ret = intel_vgt_balloon(dev);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>
> Out of curiosity, what will be the mechanism to prevent a vGPU instance
> from ignoring the ballooning data? Must be something in the hypervisor
> blocking pass-through access to such domains?
Well, although we have range check logic in the host side(which checks 
the legality of a GM address), the correctness of a GM from vGPU side 
are supposed to be guaranteed by the drm mm allocator - all those 
ballooned out spaces are marked as reserved.
>
> And probably GPU reset should also be disallowed for vGPU instances?
>
>>       if (!HAS_LLC(dev))
>>           dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
>>
>> @@ -1702,9 +1713,6 @@ int i915_gem_setup_global_gtt(struct drm_device
>> *dev,
>>           vma->bound |= GLOBAL_BIND;
>>       }
>>
>> -    dev_priv->gtt.base.start = start;
>> -    dev_priv->gtt.base.total = end - start;
>> -
>>       /* Clear any non-preallocated blocks */
>>       drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
>>           DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
>> @@ -1756,6 +1764,9 @@ void i915_global_gtt_cleanup(struct drm_device
>> *dev)
>>       }
>>
>>       if (drm_mm_initialized(&vm->mm)) {
>> +        if (intel_vgpu_active(dev))
>> +            intel_vgt_deballoon();
>> +
>>           drm_mm_takedown(&vm->mm);
>>           list_del(&vm->global_link);
>>       }
>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
>> b/drivers/gpu/drm/i915/i915_vgpu.c
>> index 3f6b797..ff5fba3 100644
>> --- a/drivers/gpu/drm/i915/i915_vgpu.c
>> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
>> @@ -83,3 +83,152 @@ void i915_check_vgpu(struct drm_device *dev)
>>       dev_priv->vgpu.active = true;
>>       DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>>   }
>> +
>> +struct _balloon_info_ {
>> +    /*
>> +     * There are up to 2 regions per low/high graphic memory that
>> +     * might be ballooned. Here, index 0/1 is for low
>> +     * graphic memory, 2/3 for high graphic memory.
>> +     */
>> +    struct drm_mm_node space[4];
>> +} bl_info;
>
> This should be static I think.
Yes, you are right.
>
>> +/**
>> + * intel_vgt_deballoon - deballoon reserved graphics address trunks
>> + *
>> + * This function is called to deallocate the ballooned-out graphic
>> memory, when
>> + * driver is unloaded or when ballooning fails.
>> + */
>> +void intel_vgt_deballoon(void)
>> +{
>> +    int i;
>> +
>> +    DRM_INFO("VGT deballoon.\n");
>
> Would debug be more appropriate? I don't see much value of saying this
> on driver unload - it's not that it is optional at this point.
OK. :)
>
> Also for all logging, is intended human readable name VGT or vGT? If the
> latter it would be nicer to log it in that form.
Well, I prefer VGT. :)
>
>> +
>> +    for (i = 0; i < 4; i++) {
>> +        if (bl_info.space[i].allocated)
>> +            drm_mm_remove_node(&bl_info.space[i]);
>> +    }
>> +
>> +    memset(&bl_info, 0, sizeof(bl_info));
>> +}
>> +
>> +static int vgt_balloon_space(struct drm_mm *mm,
>> +                 struct drm_mm_node *node,
>> +                 unsigned long start, unsigned long end)
>> +{
>> +    unsigned long size = end - start;
>> +
>> +    if (start == end)
>> +        return -EINVAL;
>> +
>> +    DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KB.\n",
>> +         start, end, size / 1024);
>
> KiB ?
Yes. Thanks. :)
>
>> +    node->start = start;
>> +    node->size = size;
>> +
>> +    return drm_mm_reserve_node(mm, node);
>> +}
>> +
>> +/**
>> + * intel_vgt_balloon - balloon out reserved graphics address trunks
>> + * @dev: drm device
>> + *
>> + * This function is called at the initialization stage, to balloon
>> out the
>> + * graphic address space allocated to other VMs, by marking these
>> spaces as
>> + * reserved.
>> + *
>> + * The ballooning related knowledges(starting address and size of the
>> low/high
>
> s/knowledges\(/knowledge /
Got it.
>
>> + * graphic memory) are depicted in the vgt_if structure in a reserved
>> MMIO
>> + * range.
>> + *
>> + * Returns:
>> + * zero on success, non-zero if configuration invalid or ballooning
>> failed
>> + */
>> +int intel_vgt_balloon(struct drm_device *dev)
>> +{
>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>> +    struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
>> +    unsigned long ggtt_vm_end = ggtt_vm->start + ggtt_vm->total;
>> +
>> +    unsigned long low_gm_base, low_gm_size, low_gm_end;
>> +    unsigned long high_gm_base, high_gm_size, high_gm_end;
>> +    int ret;
>> +
>> +    low_gm_base = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_base));
>> +    low_gm_size = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_size));
>> +    high_gm_base = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_base));
>> +    high_gm_size = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_size));
>
> Get rid of my_ prefix ?
OK. Will do. :)
>
>> +
>> +    low_gm_end = low_gm_base + low_gm_size;
>> +    high_gm_end = high_gm_base + high_gm_size;
>> +
>> +    DRM_INFO("VGT ballooning configuration:\n");
>> +    DRM_INFO("Low graphic memory: base 0x%lx size %ldKB\n",
>> +         low_gm_base, low_gm_size / 1024);
>> +    DRM_INFO("High graphic memory: base 0x%lx size %ldKB\n",
>> +         high_gm_base, high_gm_size / 1024);
>> +
>> +    if (low_gm_base < ggtt_vm->start
>> +        || low_gm_end > dev_priv->gtt.mappable_end
>> +        || high_gm_base < dev_priv->gtt.mappable_end
>> +        || high_gm_end > ggtt_vm_end) {
>> +        DRM_ERROR("Invalid ballooning configuration!\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    memset(&bl_info, 0, sizeof(bl_info));
>
> If bl_info is static then you don't need this memset?
Oh, yes. :)
>
>> +    /* High graphic memory ballooning */
>> +    if (high_gm_base > dev_priv->gtt.mappable_end) {
>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>> +                    &bl_info.space[2],
>> +                    dev_priv->gtt.mappable_end,
>> +                    high_gm_base);
>> +
>> +        if (ret)
>> +            goto err;
>> +    }
>> +
>> +    /*
>> +     * No need to partition out the last physical page,
>> +     * because it is reserved to the guard page.
>> +     */
>> +    if (high_gm_end < ggtt_vm_end - PAGE_SIZE) {
>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>> +                    &bl_info.space[3],
>> +                    high_gm_end,
>> +                    ggtt_vm_end - PAGE_SIZE);
>> +        if (ret)
>> +            goto err;
>> +    }
>> +
>> +    /* Low graphic memory ballooning */
>> +    if (low_gm_base > ggtt_vm->start) {
>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>> +                    &bl_info.space[0],
>> +                    ggtt_vm->start, low_gm_base);
>> +
>> +        if (ret)
>> +            goto err;
>> +    }
>> +
>> +    if (low_gm_end < dev_priv->gtt.mappable_end) {
>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>> +                    &bl_info.space[1],
>> +                    low_gm_end,
>> +                    dev_priv->gtt.mappable_end);
>> +
>> +        if (ret)
>> +            goto err;
>> +    }
>
> Okay, I've figured it out. :) I suppose going back to patch 1, where it
> says "Each VM can only have one portion of continuous area for now",
> with the emphasis on _one_. That threw me off thinking you have two
> "balloons" meaning forbidden areas. And then with low and high naming I
> got the wrong idea that one balloon marks the bottom inaccessible part,
> and the other top. I didn't figure out the whole low == mappable, high
> == non-mappable split. I suppose it was my inexperience, but if you can
> think of a way on how to improve the comment, even ASCII art would be
> very nice if possible.
Hah. Guess I need a better comments above. Let me think how to organize.  :)
>
> Actually ballooning in the first place confuses me because I thought
> prior use for that in virtualisation was for memory which can shrink and
> grow at runtime. Did I get that wrong?
>
Well, for memory virtualization, the word 'ballooning' does mean this - 
it can shrink and grow.
Yet for our Intel GVT-g, the 'ballooning' is used, to emphasis that we 
are not just partitioning the GM space to each vgpu, the vgpu can also 
perceive its GM address space as starting from zero(although the usable 
GM spaces may probably not).
By now, we do not grow/shrink. :)

>> +    DRM_INFO("VGT balloon successfully\n");
>> +    return 0;
>> +
>> +err:
>> +    DRM_ERROR("VGT balloon fail\n");
>> +    intel_vgt_deballoon();
>> +    return ret;
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h
>> b/drivers/gpu/drm/i915/i915_vgpu.h
>> index 5f41d01c..f538b18 100644
>> --- a/drivers/gpu/drm/i915/i915_vgpu.h
>> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
>> @@ -81,5 +81,7 @@ struct vgt_if {
>>       (VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>>
>>   extern void i915_check_vgpu(struct drm_device *dev);
>> +extern int intel_vgt_balloon(struct drm_device *dev);
>> +extern void intel_vgt_deballoon(void);
>>
>>   #endif /* _I915_VGPU_H_ */
>>
>
> Regards,
>
> Tvrtko
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Yu, Zhang]

On 12/12/2014 9:07 PM, Tvrtko Ursulin wrote:
>
>
> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>> With Intel GVT-g, the fence registers are partitioned by multiple
>> vGPU instances in different VMs. Routine i915_gem_load() is modified
>> to reset the num_fence_regs, when the driver detects it's running in
>> a VM. And the allocated fence number is provided in PV INFO page
>> structure.
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 1de94cc..0c8b32e 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -29,6 +29,7 @@
>>   #include <drm/drm_vma_manager.h>
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>> +#include "i915_vgpu.h"
>>   #include "i915_trace.h"
>>   #include "intel_drv.h"
>>   #include <linux/oom.h>
>> @@ -5014,6 +5015,10 @@ i915_gem_load(struct drm_device *dev)
>>       else
>>           dev_priv->num_fence_regs = 8;
>>
>> +    if (intel_vgpu_active(dev))
>> +        dev_priv->num_fence_regs =
>> +                I915_READ(vgtif_reg(avail_rs.fence_num));
>> +
>>       /* Initialize fence registers to zero */
>>       INIT_LIST_HEAD(&dev_priv->mm.fence_list);
>>       i915_gem_restore_fences(dev);
>>
>
> You don't need a start offset and number of allocated fences per domain?
The PV INFO structure is shared between each vgpu and host, so I guess 
this is per domain?
Not sure if I get your exact meaning. :)
>
> Regards,
>
B.R.
Yu
> Tvrtko
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tvrtko Ursulin]

On 12/16/2014 01:22 PM, Yu, Zhang wrote:
> On 12/12/2014 9:00 PM, Tvrtko Ursulin wrote:
>>
>> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>>> With Intel GVT-g, the global graphic memory space is partitioned by
>>> multiple vGPU instances in different VMs. The ballooning code is called
>>> in i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
>>> mark the graphic address space which are partitioned out to other vGPUs
>>> as reserved.
>>>
>>> v2:
>>> take Chris and Daniel's comments:
>>>     - no guard page between different VMs
>>>     - use drm_mm_reserve_node() to do the reservation for ballooning,
>>>     instead of the previous drm_mm_insert_node_in_range_generic()
>>>
>>> v3:
>>> take Daniel's comments:
>>>     - move ballooning functions into i915_vgpu.c
>>>     - add kerneldoc to ballooning functions
>>>
>>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
>>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c |  17 +++-
>>>   drivers/gpu/drm/i915/i915_vgpu.c    | 149
>>> ++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/i915_vgpu.h    |   2 +
>>>   3 files changed, 165 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index de12017..2dfac13 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -27,6 +27,7 @@
>>>   #include <drm/drmP.h>
>>>   #include <drm/i915_drm.h>
>>>   #include "i915_drv.h"
>>> +#include "i915_vgpu.h"
>>>   #include "i915_trace.h"
>>>   #include "intel_drv.h"
>>>
>>> @@ -1683,6 +1684,16 @@ int i915_gem_setup_global_gtt(struct drm_device
>>> *dev,
>>>
>>>       /* Subtract the guard page ... */
>>>       drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
>>> +
>>> +    dev_priv->gtt.base.start = start;
>>> +    dev_priv->gtt.base.total = end - start;
>>> +
>>> +    if (intel_vgpu_active(dev)) {
>>> +        ret = intel_vgt_balloon(dev);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>
>> Out of curiosity, what will be the mechanism to prevent a vGPU instance
>> from ignoring the ballooning data? Must be something in the hypervisor
>> blocking pass-through access to such domains?
> Well, although we have range check logic in the host side(which checks
> the legality of a GM address), the correctness of a GM from vGPU side

Oh, I thought that part is direct for performance reasons (avoiding 
translation). It's not then?

> are supposed to be guaranteed by the drm mm allocator - all those
> ballooned out spaces are marked as reserved.

I was thinking about a malicious i915 driver instance, or simply a bug 
in DRM or i915 which breaks the blocked ranges assumption.

If that is possible then it sounds pretty bad if one VM could snoop the 
surfaces from another.

>> And probably GPU reset should also be disallowed for vGPU instances?
>>
>>>       if (!HAS_LLC(dev))
>>>           dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
>>>
>>> @@ -1702,9 +1713,6 @@ int i915_gem_setup_global_gtt(struct drm_device
>>> *dev,
>>>           vma->bound |= GLOBAL_BIND;
>>>       }
>>>
>>> -    dev_priv->gtt.base.start = start;
>>> -    dev_priv->gtt.base.total = end - start;
>>> -
>>>       /* Clear any non-preallocated blocks */
>>>       drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
>>>           DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
>>> @@ -1756,6 +1764,9 @@ void i915_global_gtt_cleanup(struct drm_device
>>> *dev)
>>>       }
>>>
>>>       if (drm_mm_initialized(&vm->mm)) {
>>> +        if (intel_vgpu_active(dev))
>>> +            intel_vgt_deballoon();
>>> +
>>>           drm_mm_takedown(&vm->mm);
>>>           list_del(&vm->global_link);
>>>       }
>>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
>>> b/drivers/gpu/drm/i915/i915_vgpu.c
>>> index 3f6b797..ff5fba3 100644
>>> --- a/drivers/gpu/drm/i915/i915_vgpu.c
>>> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
>>> @@ -83,3 +83,152 @@ void i915_check_vgpu(struct drm_device *dev)
>>>       dev_priv->vgpu.active = true;
>>>       DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>>>   }
>>> +
>>> +struct _balloon_info_ {
>>> +    /*
>>> +     * There are up to 2 regions per low/high graphic memory that
>>> +     * might be ballooned. Here, index 0/1 is for low
>>> +     * graphic memory, 2/3 for high graphic memory.
>>> +     */
>>> +    struct drm_mm_node space[4];
>>> +} bl_info;
>>
>> This should be static I think.
> Yes, you are right.
>>
>>> +/**
>>> + * intel_vgt_deballoon - deballoon reserved graphics address trunks
>>> + *
>>> + * This function is called to deallocate the ballooned-out graphic
>>> memory, when
>>> + * driver is unloaded or when ballooning fails.
>>> + */
>>> +void intel_vgt_deballoon(void)
>>> +{
>>> +    int i;
>>> +
>>> +    DRM_INFO("VGT deballoon.\n");
>>
>> Would debug be more appropriate? I don't see much value of saying this
>> on driver unload - it's not that it is optional at this point.
> OK. :)
>>
>> Also for all logging, is intended human readable name VGT or vGT? If the
>> latter it would be nicer to log it in that form.
> Well, I prefer VGT. :)

So it's vGPU and VGT.. OK... :) (and GVT-d) :)

>>> +
>>> +    for (i = 0; i < 4; i++) {
>>> +        if (bl_info.space[i].allocated)
>>> +            drm_mm_remove_node(&bl_info.space[i]);
>>> +    }
>>> +
>>> +    memset(&bl_info, 0, sizeof(bl_info));
>>> +}
>>> +
>>> +static int vgt_balloon_space(struct drm_mm *mm,
>>> +                 struct drm_mm_node *node,
>>> +                 unsigned long start, unsigned long end)
>>> +{
>>> +    unsigned long size = end - start;
>>> +
>>> +    if (start == end)
>>> +        return -EINVAL;
>>> +
>>> +    DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KB.\n",
>>> +         start, end, size / 1024);
>>
>> KiB ?
> Yes. Thanks. :)
>>
>>> +    node->start = start;
>>> +    node->size = size;
>>> +
>>> +    return drm_mm_reserve_node(mm, node);
>>> +}
>>> +
>>> +/**
>>> + * intel_vgt_balloon - balloon out reserved graphics address trunks
>>> + * @dev: drm device
>>> + *
>>> + * This function is called at the initialization stage, to balloon
>>> out the
>>> + * graphic address space allocated to other VMs, by marking these
>>> spaces as
>>> + * reserved.
>>> + *
>>> + * The ballooning related knowledges(starting address and size of the
>>> low/high
>>
>> s/knowledges\(/knowledge /
> Got it.
>>
>>> + * graphic memory) are depicted in the vgt_if structure in a reserved
>>> MMIO
>>> + * range.
>>> + *
>>> + * Returns:
>>> + * zero on success, non-zero if configuration invalid or ballooning
>>> failed
>>> + */
>>> +int intel_vgt_balloon(struct drm_device *dev)
>>> +{
>>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>>> +    struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
>>> +    unsigned long ggtt_vm_end = ggtt_vm->start + ggtt_vm->total;
>>> +
>>> +    unsigned long low_gm_base, low_gm_size, low_gm_end;
>>> +    unsigned long high_gm_base, high_gm_size, high_gm_end;
>>> +    int ret;
>>> +
>>> +    low_gm_base = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_base));
>>> +    low_gm_size = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_size));
>>> +    high_gm_base = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_base));
>>> +    high_gm_size = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_size));
>>
>> Get rid of my_ prefix ?
> OK. Will do. :)
>>
>>> +
>>> +    low_gm_end = low_gm_base + low_gm_size;
>>> +    high_gm_end = high_gm_base + high_gm_size;
>>> +
>>> +    DRM_INFO("VGT ballooning configuration:\n");
>>> +    DRM_INFO("Low graphic memory: base 0x%lx size %ldKB\n",
>>> +         low_gm_base, low_gm_size / 1024);
>>> +    DRM_INFO("High graphic memory: base 0x%lx size %ldKB\n",
>>> +         high_gm_base, high_gm_size / 1024);
>>> +
>>> +    if (low_gm_base < ggtt_vm->start
>>> +        || low_gm_end > dev_priv->gtt.mappable_end
>>> +        || high_gm_base < dev_priv->gtt.mappable_end
>>> +        || high_gm_end > ggtt_vm_end) {
>>> +        DRM_ERROR("Invalid ballooning configuration!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    memset(&bl_info, 0, sizeof(bl_info));
>>
>> If bl_info is static then you don't need this memset?
> Oh, yes. :)
>>
>>> +    /* High graphic memory ballooning */
>>> +    if (high_gm_base > dev_priv->gtt.mappable_end) {
>>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>>> +                    &bl_info.space[2],
>>> +                    dev_priv->gtt.mappable_end,
>>> +                    high_gm_base);
>>> +
>>> +        if (ret)
>>> +            goto err;
>>> +    }
>>> +
>>> +    /*
>>> +     * No need to partition out the last physical page,
>>> +     * because it is reserved to the guard page.
>>> +     */
>>> +    if (high_gm_end < ggtt_vm_end - PAGE_SIZE) {
>>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>>> +                    &bl_info.space[3],
>>> +                    high_gm_end,
>>> +                    ggtt_vm_end - PAGE_SIZE);
>>> +        if (ret)
>>> +            goto err;
>>> +    }
>>> +
>>> +    /* Low graphic memory ballooning */
>>> +    if (low_gm_base > ggtt_vm->start) {
>>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>>> +                    &bl_info.space[0],
>>> +                    ggtt_vm->start, low_gm_base);
>>> +
>>> +        if (ret)
>>> +            goto err;
>>> +    }
>>> +
>>> +    if (low_gm_end < dev_priv->gtt.mappable_end) {
>>> +        ret = vgt_balloon_space(&ggtt_vm->mm,
>>> +                    &bl_info.space[1],
>>> +                    low_gm_end,
>>> +                    dev_priv->gtt.mappable_end);
>>> +
>>> +        if (ret)
>>> +            goto err;
>>> +    }
>>
>> Okay, I've figured it out. :) I suppose going back to patch 1, where it
>> says "Each VM can only have one portion of continuous area for now",
>> with the emphasis on _one_. That threw me off thinking you have two
>> "balloons" meaning forbidden areas. And then with low and high naming I
>> got the wrong idea that one balloon marks the bottom inaccessible part,
>> and the other top. I didn't figure out the whole low == mappable, high
>> == non-mappable split. I suppose it was my inexperience, but if you can
>> think of a way on how to improve the comment, even ASCII art would be
>> very nice if possible.
> Hah. Guess I need a better comments above. Let me think how to
> organize.  :)

Cool thanks!

>> Actually ballooning in the first place confuses me because I thought
>> prior use for that in virtualisation was for memory which can shrink and
>> grow at runtime. Did I get that wrong?
>>
> Well, for memory virtualization, the word 'ballooning' does mean this -
> it can shrink and grow.
> Yet for our Intel GVT-g, the 'ballooning' is used, to emphasis that we
> are not just partitioning the GM space to each vgpu, the vgpu can also
> perceive its GM address space as starting from zero(although the usable
> GM spaces may probably not).
> By now, we do not grow/shrink. :)

Unless there are plan to make it grow and shrink it seems that we agree 
it is confusing. But I suppose it is too late to change the term now?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tvrtko Ursulin]

On 12/16/2014 01:32 PM, Yu, Zhang wrote:
> On 12/12/2014 9:07 PM, Tvrtko Ursulin wrote:
>> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>>> With Intel GVT-g, the fence registers are partitioned by multiple
>>> vGPU instances in different VMs. Routine i915_gem_load() is modified
>>> to reset the num_fence_regs, when the driver detects it's running in
>>> a VM. And the allocated fence number is provided in PV INFO page
>>> structure.
>>>
>>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index 1de94cc..0c8b32e 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -29,6 +29,7 @@
>>>   #include <drm/drm_vma_manager.h>
>>>   #include <drm/i915_drm.h>
>>>   #include "i915_drv.h"
>>> +#include "i915_vgpu.h"
>>>   #include "i915_trace.h"
>>>   #include "intel_drv.h"
>>>   #include <linux/oom.h>
>>> @@ -5014,6 +5015,10 @@ i915_gem_load(struct drm_device *dev)
>>>       else
>>>           dev_priv->num_fence_regs = 8;
>>>
>>> +    if (intel_vgpu_active(dev))
>>> +        dev_priv->num_fence_regs =
>>> +                I915_READ(vgtif_reg(avail_rs.fence_num));
>>> +
>>>       /* Initialize fence registers to zero */
>>>       INIT_LIST_HEAD(&dev_priv->mm.fence_list);
>>>       i915_gem_restore_fences(dev);
>>>
>>
>> You don't need a start offset and number of allocated fences per domain?
> The PV INFO structure is shared between each vgpu and host, so I guess
> this is per domain?
> Not sure if I get your exact meaning. :)

I didn't figure out how each domain knowns which fences to use? They 
know how many, but which ones?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Gerd Hoffmann]

  Hi,

> >> Out of curiosity, what will be the mechanism to prevent a vGPU instance
> >> from ignoring the ballooning data? Must be something in the hypervisor
> >> blocking pass-through access to such domains?
> > Well, although we have range check logic in the host side(which checks
> > the legality of a GM address), the correctness of a GM from vGPU side
> 
> Oh, I thought that part is direct for performance reasons (avoiding 
> translation). It's not then?

> > are supposed to be guaranteed by the drm mm allocator - all those
> > ballooned out spaces are marked as reserved.
> 
> I was thinking about a malicious i915 driver instance, or simply a bug 
> in DRM or i915 which breaks the blocked ranges assumption.

Cover letter had a link to a longish paper explaining how all this works
in detail.

Short summary:
  * Direct access is denied by simply mapping only the guests piece
    of memory into the guest address space.
  * Indirect access (via exec buffer commands) is denied by verifying
    the exec buffer commands (map read-only + verify + hand over to
    the hardware when checks pass).
  * The ballooning basically makes the guest aware what the offset for
    its piece of memory has, so the references in the execbuffer don't
    need to be translated, only verified.

> Unless there are plan to make it grow and shrink it seems that we agree 
> it is confusing. But I suppose it is too late to change the term now?

It's confusing indeed (same question came up @ kvm forum after kvmgt
presentation).  So a better name would be good I think.

cheers,
  Gerd


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Gerd Hoffmann]

  Hi,

> I didn't figure out how each domain knowns which fences to use? They 
> know how many, but which ones?

I think the guest doesn't have to know because mmio access is trapped by
the hypervisor anyway.

cheers,
  Gerd


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tvrtko Ursulin]

Hi,

On 12/16/2014 02:41 PM, Gerd Hoffmann wrote:
>> I didn't figure out how each domain knowns which fences to use? They
>> know how many, but which ones?
>
> I think the guest doesn't have to know because mmio access is trapped by
> the hypervisor anyway.

Okay.. so the point is to ensure no domain can hog all fences, but if 
the hypervisor is already tracking everything why is this limiting 
required then in the guest? Hypervisor could just say all fences are 
allocated since it knows how much it gave out per guest.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tvrtko Ursulin]

Hi,

On 12/16/2014 02:39 PM, Gerd Hoffmann wrote:
>>>> Out of curiosity, what will be the mechanism to prevent a vGPU instance
>>>> from ignoring the ballooning data? Must be something in the hypervisor
>>>> blocking pass-through access to such domains?
>>> Well, although we have range check logic in the host side(which checks
>>> the legality of a GM address), the correctness of a GM from vGPU side
>>
>> Oh, I thought that part is direct for performance reasons (avoiding
>> translation). It's not then?
>
>>> are supposed to be guaranteed by the drm mm allocator - all those
>>> ballooned out spaces are marked as reserved.
>>
>> I was thinking about a malicious i915 driver instance, or simply a bug
>> in DRM or i915 which breaks the blocked ranges assumption.
>
> Cover letter had a link to a longish paper explaining how all this works
> in detail.
>
> Short summary:
>    * Direct access is denied by simply mapping only the guests piece
>      of memory into the guest address space.
>    * Indirect access (via exec buffer commands) is denied by verifying
>      the exec buffer commands (map read-only + verify + hand over to
>      the hardware when checks pass).
>    * The ballooning basically makes the guest aware what the offset for
>      its piece of memory has, so the references in the execbuffer don't
>      need to be translated, only verified.

Thanks, I did read the blog and powerpoint from the link, but didn't 
find what you wrote below even after the second read. Probably missed 
due to lack of domain knowledge.

Anyway, I just wanted to hear that the DRM allocation range is not the 
only thing preventing access outside the allocated range. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Yu, Zhang]

On 12/16/2014 9:19 PM, Tvrtko Ursulin wrote:
>
> On 12/16/2014 12:51 PM, Yu, Zhang wrote:
>> Thank you very much for your thorough review, Tvrtko. :)
>>
>> On 12/12/2014 1:33 AM, Tvrtko Ursulin wrote:
>>>
>>> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>>>> Introduce a PV INFO structure, to facilitate the Intel GVT-g
>>>> technology, which is a GPU virtualization solution with mediated
>>>> pass-through. This page contains the shared information between
>>>> i915 driver and the host emulator. For now, this structure utilizes
>>>> an area of 4K bypes on HSW GPU's unused MMIO space to support
>>>
>>> bytes
>> Got it. Thanks.
>>>
>>>> existing production. Future hardware will have the reserved window
>>>
>>> It is slightly unclear to me what "existing production" means?
>> Our current gpu virtualization implementation for HSW. It's compared
>> with future implematations for BDW etc.
>
> So maybe instead of "existing production" say "currently supported
> hardware" or similar?
How about we remove "to support existing production" in this sentence? 
The "currently supported hardware" seems a bit redundant. :)
>
>>>> architecturally defined, and layout of the page will be added in
>>>> future BSpec.
>>>>
>>>> The i915 driver load routine detects if it is running in a VM by
>>>> reading the contents of this PV INFO page. Thereafter a flag,
>>>> vgpu.active is set, and intel_vgpu_active() is used by checking
>>>> this flag to conclude if GPU is virtualized with Intel GVT-g. By
>>>> now, intel_vgpu_active() will return true, only when the driver
>>>> is running as a guest in the Intel GVT-g enhanced environment on
>>>> HSW platform.
>>>>
>>>> v2:
>>>> take Chris' comments:
>>>>     - call the i915_check_vgpu() in intel_uncore_init()
>>>>     - sanitize i915_check_vgpu() by adding BUILD_BUG_ON() and debug
>>>> info
>>>> take Daniel's comments:
>>>>     - put the definition of PV INFO into a new header - i915_vgt_if.h
>>>> other changes:
>>>>     - access mmio regs by readq/readw in i915_check_vgpu()
>>>>
>>>> v3:
>>>> take Daniel's comments:
>>>>     - move the i915/vgt interfaces into a new i915_vgpu.c
>>>>     - update makefile
>>>>     - add kerneldoc to functions which are non-static
>>>>     - add a DOC: section describing some of the high-level design
>>>>     - update drm docbook
>>>> other changes:
>>>>     - rename i915_vgt_if.h to i915_vgpu.h
>>>>
>>>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>>>> ---
>>>>   Documentation/DocBook/drm.tmpl      |  5 +++
>>>>   drivers/gpu/drm/i915/Makefile       |  3 ++
>>>>   drivers/gpu/drm/i915/i915_drv.h     | 11 +++++
>>>>   drivers/gpu/drm/i915/i915_vgpu.c    | 85
>>>> +++++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/i915_vgpu.h    | 85
>>>> +++++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/intel_uncore.c |  3 ++
>>>>   6 files changed, 192 insertions(+)
>>>>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.c
>>>>   create mode 100644 drivers/gpu/drm/i915/i915_vgpu.h
>>>>
>>>> diff --git a/Documentation/DocBook/drm.tmpl
>>>> b/Documentation/DocBook/drm.tmpl
>>>> index 64d9c1e..e4db4cf 100644
>>>> --- a/Documentation/DocBook/drm.tmpl
>>>> +++ b/Documentation/DocBook/drm.tmpl
>>>> @@ -3832,6 +3832,11 @@ int num_ioctls;</synopsis>
>>>>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
>>>>   !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
>>>>         </sect2>
>>>> +      <sect2>
>>>> +        <title>Intel GVT-g Guest Support(vGPU)</title>
>>>> +!Pdrivers/gpu/drm/i915/i915_vgpu.c Intel GVT-g guest support
>>>> +!Idrivers/gpu/drm/i915/i915_vgpu.c
>>>> +      </sect2>
>>>>       </sect1>
>>>>       <sect1>
>>>>         <title>Display Hardware Handling</title>
>>>> diff --git a/drivers/gpu/drm/i915/Makefile
>>>> b/drivers/gpu/drm/i915/Makefile
>>>> index 891e584..d9aa5ca 100644
>>>> --- a/drivers/gpu/drm/i915/Makefile
>>>> +++ b/drivers/gpu/drm/i915/Makefile
>>>> @@ -79,6 +79,9 @@ i915-y += dvo_ch7017.o \
>>>>         intel_sdvo.o \
>>>>         intel_tv.o
>>>>
>>>> +# virtual gpu code
>>>> +i915-y += i915_vgpu.o
>>>> +
>>>>   # legacy horrors
>>>>   i915-y += i915_dma.o \
>>>>         i915_ums.o
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 54691bc..e1e221e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1470,6 +1470,10 @@ struct i915_workarounds {
>>>>       u32 count;
>>>>   };
>>>>
>>>> +struct i915_virtual_gpu {
>>>> +    bool active;
>>>> +};
>>>> +
>>>>   struct drm_i915_private {
>>>>       struct drm_device *dev;
>>>>       struct kmem_cache *slab;
>>>> @@ -1482,6 +1486,8 @@ struct drm_i915_private {
>>>>
>>>>       struct intel_uncore uncore;
>>>>
>>>> +    struct i915_virtual_gpu vgpu;
>>>> +
>>>>       struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>>>>
>>>>
>>>> @@ -2311,6 +2317,11 @@ extern void intel_uncore_check_errors(struct
>>>> drm_device *dev);
>>>>   extern void intel_uncore_fini(struct drm_device *dev);
>>>>   extern void intel_uncore_forcewake_reset(struct drm_device *dev,
>>>> bool restore);
>>>>
>>>> +static inline bool intel_vgpu_active(struct drm_device *dev)
>>>> +{
>>>> +    return to_i915(dev)->vgpu.active;
>>>> +}
>>>> +
>>>>   void
>>>>   i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe
>>>> pipe,
>>>>                u32 status_mask);
>>>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
>>>> b/drivers/gpu/drm/i915/i915_vgpu.c
>>>> new file mode 100644
>>>> index 0000000..3f6b797
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
>>>> @@ -0,0 +1,85 @@
>>>> +/*
>>>> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person
>>>> obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without
>>>> limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>>> sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to whom
>>>> the
>>>> + * Software is furnished to do so, subject to the following
>>>> conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including
>>>> the next
>>>> + * paragraph) shall be included in all copies or substantial portions
>>>> of the
>>>> + * Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>>>> SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>>> OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>>> ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>>> DEALINGS IN THE
>>>> + * SOFTWARE.
>>>> + */
>>>> +
>>>> +#include "intel_drv.h"
>>>> +#include "i915_vgpu.h"
>>>> +
>>>> +
>>>> +/**
>>>> + * DOC: Intel GVT-g guest support
>>>> + *
>>>> + * Intel GVT-g is a graphics virtualization technology which shares
>>>> the
>>>> + * GPU among multiple virtual machines on a time-sharing basis. Each
>>>> + * virtual machine is presented a virtual GPU (vGPU), which has
>>>> equivalent
>>>> + * features as the underlying physical GPU (pGPU), so i915 driver can
>>>> run
>>>> + * seamlessly in a virtual machine. This file provides vGPU specific
>>>> + * optimizations when running in a virtual machine, to reduce the
>>>> complexity
>>>> + * of vGPU emulation and to improve the overall performance.
>>>> + *
>>>> + * A primary function introduced here is so-called "address space
>>>> ballooning"
>>>> + * technique. Intel GVT-g partitions global graphics memory among
>>>> multiple VMs,
>>>> + * so each VM can directly access a portion of the memory w/o
>>>> hypervisor's
>>>> + * intervention, e.g. filling textures or queuing commands. However
>>>> w/ the
>>>
>>> Personal preference, but It may be nicer to avoid "w/o" and similar
>>> shorthand in documentation.
>> OK. Will fix it. :)
>>>
>>>> + * partitioning an unmodified i915 driver would assume a smaller
>>>> graphics
>>>> + * memory starting from address ZERO, then requires vGPU emulation
>>>> module to
>>>> + * translate the graphics address between 'guest view' and 'host
>>>> view', for
>>>> + * all registers and command opcodes which contain a graphics memory
>>>> address.
>>>> + * To reduce the complexity, Intel GVT-g introduces "address space
>>>> ballooning",
>>>> + * by telling the exact partitioning knowledge to each guest i915
>>>> driver, which
>>>> + * then reserves and prevents non-allocated portions from allocation.
>>>> Thus vGPU
>>>> + * emulation module module only needs to scan and validate graphics
>>>> addresses
>>>> + * w/o complexity of address translation.
>>>> + *
>>>> + */
>>>> +
>>>> +/**
>>>> + * i915_check_vgpu - detect virtual GPU
>>>> + * @dev: drm device *
>>>> + *
>>>> + * This function is called at the initialization stage, to detect
>>>> whether
>>>> + * running on a vGPU.
>>>> + */
>>>> +void i915_check_vgpu(struct drm_device *dev)
>>>> +{
>>>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +    uint64_t magic;
>>>> +    uint32_t version;
>>>> +
>>>> +    BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
>>>> +
>>>> +    if (!IS_HASWELL(dev))
>>>> +        return;
>>>> +
>>>> +    magic = readq(dev_priv->regs + vgtif_reg(magic));
>>>> +    if (magic != VGT_MAGIC)
>>>> +        return;
>>>> +
>>>> +    version = INTEL_VGT_IF_VERSION_ENCODE(
>>>> +        readw(dev_priv->regs + vgtif_reg(version_major)),
>>>> +        readw(dev_priv->regs + vgtif_reg(version_minor)));
>>>> +    if (version != INTEL_VGT_IF_VERSION)
>>>> +        return;
>>>
>>> Would it be useful to something on version mismatch?
>> Well. Thanks for your notification. :)
>> In fact, this version judgement is just for future usages. By now, we
>> have only one vgt interface version, 0x10000. But you are right, unlike
>> the judgement for 'magic', which specifies the Intel GVT-g feature, and
>> can be returned directly when false, failure for the 'version' may
>> should trigger something like unload the driver. However, if so, this
>> means we may need to:
>> 1> return something in i915_check_vgpu();
>> 2> change the prototype of i915_check_vgpu() and its caller
>> intel_uncore_init();
>> 3> check the return value of intel_uncore_init() in i915_driver_load()
>> Is this approach acceptable?
>
> I would leave this for the future version. I was simply suggesting
> logging something on version mismatch. Just because it is unexpected so
> there is a trace in the logs why feature is not available. It's up to you.
>
>>>
>>>> +    dev_priv->vgpu.active = true;
>>>> +    DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h
>>>> b/drivers/gpu/drm/i915/i915_vgpu.h
>>>> new file mode 100644
>>>> index 0000000..5f41d01c
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
>>>> @@ -0,0 +1,85 @@
>>>> +/*
>>>> + * Copyright(c) 2011-2014 Intel Corporation. All rights reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person
>>>> obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without
>>>> limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>>> sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to whom
>>>> the
>>>> + * Software is furnished to do so, subject to the following
>>>> conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including
>>>> the next
>>>> + * paragraph) shall be included in all copies or substantial portions
>>>> of the
>>>> + * Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>>>> SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>>> OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>>> ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>>> DEALINGS IN THE
>>>> + * SOFTWARE.
>>>> + */
>>>> +
>>>> +#ifndef _I915_VGPU_H_
>>>> +#define _I915_VGPU_H_
>>>> +
>>>> +/* The MMIO offset of the shared info between guest and host
>>>> emulator */
>>>> +#define VGT_PVINFO_PAGE    0x78000
>>>> +#define VGT_PVINFO_SIZE    0x1000
>>>> +
>>>> +/*
>>>> + * The following structure pages are defined in GEN MMIO space
>>>> + * for virtualization. (One page for now)
>>>> + */
>>>> +#define VGT_MAGIC         0x4776544776544776    /* 'vGTvGTvG' */
>>>> +#define VGT_VERSION_MAJOR 1
>>>> +#define VGT_VERSION_MINOR 0
>>>> +
>>>> +#define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 |
>>>> (minor))
>>>> +#define INTEL_VGT_IF_VERSION \
>>>> +    INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
>>>> +
>>>> +struct vgt_if {
>>>> +    uint64_t magic;        /* VGT_MAGIC */
>>>> +    uint16_t version_major;
>>>> +    uint16_t version_minor;
>>>> +    uint32_t vgt_id;    /* ID of vGT instance */
>>>> +    uint32_t rsv1[12];    /* pad to offset 0x40 */
>>>
>>> In general I thought stdint types should be avoided in the kernel in
>>> favour of u32 and company. But I see i915 is full of them already so
>>> maybe my information is outdated.
>>>
>> Let's keep the _t types, like explanations in Daniel's mail. Is this OK?
>
> Yes.
>
>>>> +    /*
>>>> +     *  Data structure to describe the balooning info of resources.
>>>> +     *  Each VM can only have one portion of continuous area for now.
>>>> +     *  (May support scattered resource in future)
>>>> +     *  (starting from offset 0x40)
>>>> +     */
>>>> +    struct {
>>>> +        /* Aperture register balooning */
>>>> +        struct {
>>>> +            uint32_t my_base;
>>>> +            uint32_t my_size;
>>>> +        } low_gmadr;    /* aperture */
>>>> +        /* GMADR register balooning */
>>>> +        struct {
>>>> +            uint32_t my_base;
>>>> +            uint32_t my_size;
>>>> +        } high_gmadr;    /* non aperture */
>>>> +        /* allowed fence registers */
>>>> +        uint32_t fence_num;
>>>> +        uint32_t rsv2[3];
>>>> +    } avail_rs;        /* available/assigned resource */
>>>
>>> I didn't figure out yet why the concept is to pass in two ranges to mark
>>> as reserved, instead of one usable/allowed range and then implicitly all
>>> the rest as reserved?
>>>
>> Like you said in 2/3 comments, this 2 part is for mappable and
>> non-mapplable respectively. :)
>
> Are high and low memory as terms known in the i915 space so far as
> synonyms for mappable and non-mappable?
>
Well, I'm not sure.
Daniel, do you think the "low gmadr/high gmadr" will cause any 
misunderstanding?
>>>> +    uint32_t rsv3[0x200 - 24];    /* pad to half page */
>>>> +    /*
>>>> +     * The bottom half page is for response from Gfx driver to
>>>> hypervisor.
>>>> +     * Set to reserved fields temporarily by now.
>>>> +     */
>>>> +    uint32_t rsv4;
>>>> +    uint32_t display_ready;    /* ready for display owner switch */
>>>> +    uint32_t rsv5[0x200 - 2];    /* pad to one page */
>>>> +} __packed;
>>>
>>> You probably don't need __packed since you have BUILD_BUG_ON on size
>>> mismatch and pad to full size with reserved fields. I don't think it
>>> matters though.
>>>
>> Well, IIRC, this __packed is added to follow Chris's comments. How about
>> we keep it? Or will it hurt any place? :)
>
> The only purpose for packed here I can see is a double guard along the
> BUILD_BUG_ON. As I said I don't think it makes any difference so you can
> leave it in.
OK. Thanks.
>
> Regards,
>
> Tvrtko
>
>
B.R.
Yu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Yu, Zhang]

On 12/16/2014 11:15 PM, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 12/16/2014 02:39 PM, Gerd Hoffmann wrote:
>>>>> Out of curiosity, what will be the mechanism to prevent a vGPU
>>>>> instance
>>>>> from ignoring the ballooning data? Must be something in the hypervisor
>>>>> blocking pass-through access to such domains?
>>>> Well, although we have range check logic in the host side(which checks
>>>> the legality of a GM address), the correctness of a GM from vGPU side
>>>
>>> Oh, I thought that part is direct for performance reasons (avoiding
>>> translation). It's not then?
>>
>>>> are supposed to be guaranteed by the drm mm allocator - all those
>>>> ballooned out spaces are marked as reserved.
>>>
>>> I was thinking about a malicious i915 driver instance, or simply a bug
>>> in DRM or i915 which breaks the blocked ranges assumption.
>>
>> Cover letter had a link to a longish paper explaining how all this works
>> in detail.
>>
>> Short summary:
>>    * Direct access is denied by simply mapping only the guests piece
>>      of memory into the guest address space.
>>    * Indirect access (via exec buffer commands) is denied by verifying
>>      the exec buffer commands (map read-only + verify + hand over to
>>      the hardware when checks pass).
>>    * The ballooning basically makes the guest aware what the offset for
>>      its piece of memory has, so the references in the execbuffer don't
>>      need to be translated, only verified.
>
> Thanks, I did read the blog and powerpoint from the link, but didn't
> find what you wrote below even after the second read. Probably missed
> due to lack of domain knowledge.
>
> Anyway, I just wanted to hear that the DRM allocation range is not the
> only thing preventing access outside the allocated range. :)
Thanks, Tvrtko & Gerd.
DRM allocation range is not the only guarantee. We also have range check 
functions in our host code. MMIO accesses and ring buffer commands will 
be checked/scanned in the host side to see if there's any illegal GM 
address.

>
> Regards,
>
> Tvrtko
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Yu, Zhang]

On 12/12/2014 9:13 PM, Tvrtko Ursulin wrote:
>
>
> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>> Framebuffer compression is disabled when driver detects it's
>> running in a Intel GVT-g enlightened VM, because FBC is not
>> emulated and there is no stolen memory for a vGPU.
>>
>> v2:
>> take Chris' comments:
>>     - move the code into intel_update_fbc()
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 7a69eba..3bc5d93 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -544,6 +544,10 @@ void intel_update_fbc(struct drm_device *dev)
>>           return;
>>       }
>>
>> +    /* disable framebuffer compression in vGPU */
>> +    if (intel_vgpu_active(dev))
>> +        i915.enable_fbc = 0;
>> +
>>       /*
>>        * If FBC is already on, we just have to verify that we can
>>        * keep it that way...
>>
>
> Looks like you'll need to rebase this one, I see no intel_update_fbc in
> my tree. :(
>
OK. I'll rebase this in next version. :)
> Regards,
>
> Tvrtko
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/8] drm/i915: Add the display switch logic for vGPU in i915 driver

[Yu, Zhang]

On 12/15/2014 4:16 PM, Daniel Vetter wrote:
> On Thu, Nov 13, 2014 at 08:02:46PM +0800, Yu Zhang wrote:
>> Display switch logic is added to notify the host side that
>> current vGPU have a valid surface to show. It does so by
>> writing the display_ready field in PV INFO page, and then
>> will be handled in the host side. This is useful to avoid
>> trickiness when the VM's framebuffer is being accessed in
>> the middle of VM modesetting, e.g. compositing the framebuffer
>> in the host side.
>>
>> v2:
>> 	- move the notification code outside the 'else' in load sequence
>> 	- remove the notification code in intel_crtc_set_config()
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_dma.c  | 11 +++++++++++
>>   drivers/gpu/drm/i915/i915_vgpu.h |  7 +++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 9a73533..06daa5c 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -36,6 +36,7 @@
>>   #include "intel_drv.h"
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>> +#include "i915_vgpu.h"
>>   #include "i915_trace.h"
>>   #include <linux/pci.h>
>>   #include <linux/console.h>
>> @@ -1780,6 +1781,16 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>   		dev_priv->ums.mm_suspended = 1;
>>   	}
>>
>> +	if (intel_vgpu_active(dev)) {
>> +		/*
>> +		 * Notify a valid surface after modesetting,
>> +		 * when running inside a VM.
>> +		 */
>> +		struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> +		I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
>> +	}
>> +
>>   	i915_setup_sysfs(dev);
>>
>>   	if (INTEL_INFO(dev)->num_pipes) {
>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
>> index f538b18..9d35fb8 100644
>> --- a/drivers/gpu/drm/i915/i915_vgpu.h
>> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
>> @@ -80,6 +80,13 @@ struct vgt_if {
>>   #define vgtif_reg(x) \
>>   	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>>
>> +/* vGPU display status to be used by the host side */
>> +enum vgt_display_status {
>> +	VGT_DRV_DISPLAY_NOT_READY = 0,
>> +	VGT_DRV_DISPLAY_READY,  /* ready for display switch */
>> +};
>
> Nitpick: Since this is a virtual register I prefer we use plain #defines
> with explicit codes for everything instead of enums. Makes it clearer
> which values are used and all that.
> -Daniel
Got it. #define will be used. And I will rebase this patch.
>
Thanks
Yu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/8] drm/i915: Disable power management for i915 driver in VM

[Yu, Zhang]

On 12/12/2014 9:27 PM, Tvrtko Ursulin wrote:
>
>
> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>> With Intel GVT-g, GPU power management is controlled by
>> host driver, so there is no need to provide virtualized
>> GPU PM support. In the future it might be useful to gather
>> VM input for freq boost, but now let's disable it simply.
>>
>> v2:
>> take Chris' comments:
>>          - do not special case this to gen6+
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 3bc5d93..3722bd4 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5314,6 +5314,10 @@ void intel_enable_gt_powersave(struct
>> drm_device *dev)
>>   {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> +    /* Powersaving is controlled by the host when inside a VM */
>> +    if (intel_vgpu_active(dev))
>> +        return;
>> +
>>       if (IS_IRONLAKE_M(dev)) {
>>           mutex_lock(&dev->struct_mutex);
>>           ironlake_enable_drps(dev);
>>
>
> I was looking for potential call sites of this which come earlier than
> i915_check_vgpu. Didn't find any but found RPS (intel_pm_setup) - what's
> with that - should it be disabled as well?
>
IIUC, the intel_pm_setup() only add the intel_gen6_powersave_work() to 
the delayed_resume_work queue. Real call of this routine will be delayed 
to intel_enable_gt_powersave(). So I guess no necessary to do anything 
in intel_pm_setup(). :)
> Otherwise,
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
B.R.
Yu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tian, Kevin]

> From: Tvrtko Ursulin
> Sent: Tuesday, December 16, 2014 9:44 PM
> 
> 
> On 12/16/2014 01:32 PM, Yu, Zhang wrote:
> > On 12/12/2014 9:07 PM, Tvrtko Ursulin wrote:
> >> On 11/13/2014 12:02 PM, Yu Zhang wrote:
> >>> With Intel GVT-g, the fence registers are partitioned by multiple
> >>> vGPU instances in different VMs. Routine i915_gem_load() is modified
> >>> to reset the num_fence_regs, when the driver detects it's running in
> >>> a VM. And the allocated fence number is provided in PV INFO page
> >>> structure.
> >>>
> >>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> >>> Signed-off-by: Jike Song <jike.song@intel.com>
> >>> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_gem.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> >>> b/drivers/gpu/drm/i915/i915_gem.c
> >>> index 1de94cc..0c8b32e 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>> @@ -29,6 +29,7 @@
> >>>   #include <drm/drm_vma_manager.h>
> >>>   #include <drm/i915_drm.h>
> >>>   #include "i915_drv.h"
> >>> +#include "i915_vgpu.h"
> >>>   #include "i915_trace.h"
> >>>   #include "intel_drv.h"
> >>>   #include <linux/oom.h>
> >>> @@ -5014,6 +5015,10 @@ i915_gem_load(struct drm_device *dev)
> >>>       else
> >>>           dev_priv->num_fence_regs = 8;
> >>>
> >>> +    if (intel_vgpu_active(dev))
> >>> +        dev_priv->num_fence_regs =
> >>> +                I915_READ(vgtif_reg(avail_rs.fence_num));
> >>> +
> >>>       /* Initialize fence registers to zero */
> >>>       INIT_LIST_HEAD(&dev_priv->mm.fence_list);
> >>>       i915_gem_restore_fences(dev);
> >>>
> >>
> >> You don't need a start offset and number of allocated fences per domain?
> > The PV INFO structure is shared between each vgpu and host, so I guess
> > this is per domain?
> > Not sure if I get your exact meaning. :)
> 
> I didn't figure out how each domain knowns which fences to use? They
> know how many, but which ones?
> 

the offset is maintained in the host side, which will be added to guest index
in relevant emulation path.

Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Tian, Kevin]

> From: Yu, Zhang
> Sent: Wednesday, December 17, 2014 10:50 AM
> >>>> +    /*
> >>>> +     *  Data structure to describe the balooning info of resources.
> >>>> +     *  Each VM can only have one portion of continuous area for
> now.
> >>>> +     *  (May support scattered resource in future)
> >>>> +     *  (starting from offset 0x40)
> >>>> +     */
> >>>> +    struct {
> >>>> +        /* Aperture register balooning */
> >>>> +        struct {
> >>>> +            uint32_t my_base;
> >>>> +            uint32_t my_size;
> >>>> +        } low_gmadr;    /* aperture */
> >>>> +        /* GMADR register balooning */
> >>>> +        struct {
> >>>> +            uint32_t my_base;
> >>>> +            uint32_t my_size;
> >>>> +        } high_gmadr;    /* non aperture */
> >>>> +        /* allowed fence registers */
> >>>> +        uint32_t fence_num;
> >>>> +        uint32_t rsv2[3];
> >>>> +    } avail_rs;        /* available/assigned resource */
> >>>
> >>> I didn't figure out yet why the concept is to pass in two ranges to mark
> >>> as reserved, instead of one usable/allowed range and then implicitly all
> >>> the rest as reserved?
> >>>
> >> Like you said in 2/3 comments, this 2 part is for mappable and
> >> non-mapplable respectively. :)
> >
> > Are high and low memory as terms known in the i915 space so far as
> > synonyms for mappable and non-mappable?
> >
> Well, I'm not sure.
> Daniel, do you think the "low gmadr/high gmadr" will cause any
> misunderstanding?

for i915 possibly mappable and non-mappable are more intuitive, but
for end user they finally need to know mappable is actually in low end
and non-mappable is in high end, so low/high would be simpler to 
them. since this is the information used between host and guest, we're
open to whatever meaningful to i915, and our host vgt driver can still
keep low/high notations to user.

Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tian, Kevin]

> From: Yu, Zhang
> Sent: Wednesday, December 17, 2014 11:10 AM
> > On 12/16/2014 02:39 PM, Gerd Hoffmann wrote:
> >>>>> Out of curiosity, what will be the mechanism to prevent a vGPU
> >>>>> instance
> >>>>> from ignoring the ballooning data? Must be something in the hypervisor
> >>>>> blocking pass-through access to such domains?
> >>>> Well, although we have range check logic in the host side(which checks
> >>>> the legality of a GM address), the correctness of a GM from vGPU side
> >>>
> >>> Oh, I thought that part is direct for performance reasons (avoiding
> >>> translation). It's not then?

yes, just for performance reason to avoid translation

> >>
> >>>> are supposed to be guaranteed by the drm mm allocator - all those
> >>>> ballooned out spaces are marked as reserved.
> >>>
> >>> I was thinking about a malicious i915 driver instance, or simply a bug
> >>> in DRM or i915 which breaks the blocked ranges assumption.

host will validate all the addresses submitted by the guest, w/ the assumption
that ballooning is in place so if the address is outside of the allocated machine
graphics memory address, it's caught and prevented. We use ballooning to
let guest handle the correctness itself, but having Xen to ensure isolation.

> >>
> >> Cover letter had a link to a longish paper explaining how all this works
> >> in detail.
> >>
> >> Short summary:
> >>    * Direct access is denied by simply mapping only the guests piece
> >>      of memory into the guest address space.
> >>    * Indirect access (via exec buffer commands) is denied by verifying
> >>      the exec buffer commands (map read-only + verify + hand over to
> >>      the hardware when checks pass).
> >>    * The ballooning basically makes the guest aware what the offset for
> >>      its piece of memory has, so the references in the execbuffer don't
> >>      need to be translated, only verified.

correct. 

> >
> > Thanks, I did read the blog and powerpoint from the link, but didn't
> > find what you wrote below even after the second read. Probably missed
> > due to lack of domain knowledge.
> >
> > Anyway, I just wanted to hear that the DRM allocation range is not the
> > only thing preventing access outside the allocated range. :)
> Thanks, Tvrtko & Gerd.
> DRM allocation range is not the only guarantee. We also have range check
> functions in our host code. MMIO accesses and ring buffer commands will
> be checked/scanned in the host side to see if there's any illegal GM
> address.
> 

and if you read the paper referred in 1st mail, we call our ballooning as
address space ballooning which is different from traditional ballooning
technique used in memory virtualization, w/ the major difference as:

* traditional ballooning only care about the size. As long as the desired
number of pages can be ballooned out, we don't care whether they
actually come from.

* address space ballooning used in our case care about both size and
address. We need the guest to balloon out a specified range of addresses
as explained earlier.

whether to grow/shrink dynamic is not the key difference between
two approaches. yes, we only support static ballooning now at boot time,
but the same interface can be extended to support dynamic ballooning
in the future, w/ more cooperation from guest driver in the future.

So I think using ballooning is a right fit here, though the policy is very
simple now.

Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tian, Kevin]

> From: Tvrtko Ursulin
> Sent: Friday, December 12, 2014 9:00 PM
> > @@ -1683,6 +1684,16 @@ int i915_gem_setup_global_gtt(struct
> drm_device *dev,
> >
> >   	/* Subtract the guard page ... */
> >   	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> > +
> > +	dev_priv->gtt.base.start = start;
> > +	dev_priv->gtt.base.total = end - start;
> > +
> > +	if (intel_vgpu_active(dev)) {
> > +		ret = intel_vgt_balloon(dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> 
> Out of curiosity, what will be the mechanism to prevent a vGPU instance
> from ignoring the ballooning data? Must be something in the hypervisor
> blocking pass-through access to such domains?
> 
> And probably GPU reset should also be disallowed for vGPU instances?
> 

GPU reset is allowed and the host side needs to emulate the vGPU reset
behavior. A simple example is that we need remove this vGPU from the
scheduling queue, upon a vGPU reset request.

Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/8] drm/i915: Create vGPU specific write MMIO to reduce traps

[Yu, Zhang]

On 12/12/2014 9:31 PM, Tvrtko Ursulin wrote:
>
> On 11/13/2014 12:02 PM, Yu Zhang wrote:
>> In the virtualized environment, forcewake operations are not
>> necessory for the driver, because mmio accesses will be trapped
>
> necessary
Thanks.
>
>> and emulated by the host side, and real forcewake operations are
>> also done in the host. New mmio write handlers are added to directly
>> call the __raw_i915_write, therefore will reduce many traps and
>> increase the overall performance for drivers running in the VM
>> with Intel GVT-g enhancement.
>>
>> v2:
>> take Chris' comments:
>>          - register the mmio hooks in intel_uncore_init()
>> v3:
>> take Daniel's comments:
>>          - use macros to assign mmio write functions for vGPU
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index cae27bb..b76c21d 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -727,6 +727,14 @@ hsw_write##x(struct drm_i915_private *dev_priv,
>> off_t reg, u##x val, bool trace)
>>       REG_WRITE_FOOTER; \
>>   }
>>
>> +#define __vgpu_write(x) \
>> +static void \
>> +vgpu_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val,
>> bool trace) { \
>> +    REG_WRITE_HEADER; \
>> +    __raw_i915_write##x(dev_priv, reg, val); \
>> +    REG_WRITE_FOOTER; \
>> +}
>> +
>>   static const u32 gen8_shadowed_regs[] = {
>>       FORCEWAKE_MT,
>>       GEN6_RPNSWREQ,
>> @@ -821,6 +829,10 @@ __gen4_write(8)
>>   __gen4_write(16)
>>   __gen4_write(32)
>>   __gen4_write(64)
>> +__vgpu_write(8)
>> +__vgpu_write(16)
>> +__vgpu_write(32)
>> +__vgpu_write(64)
>>
>>   #undef __chv_write
>>   #undef __gen8_write
>> @@ -828,6 +840,7 @@ __gen4_write(64)
>>   #undef __gen6_write
>>   #undef __gen5_write
>>   #undef __gen4_write
>> +#undef __vgpu_write
>>   #undef REG_WRITE_FOOTER
>>   #undef REG_WRITE_HEADER
>>
>> @@ -939,6 +952,9 @@ void intel_uncore_init(struct drm_device *dev)
>>           break;
>>       }
>>
>> +    if (intel_vgpu_active(dev))
>> +        ASSIGN_WRITE_MMIO_VFUNCS(vgpu);
>> +
>>       i915_check_and_clear_faults(dev);
>>   }
>>   #undef ASSIGN_WRITE_MMIO_VFUNCS
>>
>
> Maybe I am missing something obvious, but why are read variants not needed?
Wah. You got me. Guess I missed this during rebase. Thank you, Tvrtko.
>
> Regards,
>
> Tvrtko
>
>
B.R.
Yu

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Gerd Hoffmann]

On Di, 2014-12-16 at 15:01 +0000, Tvrtko Ursulin wrote:
> Hi,
> 
> On 12/16/2014 02:41 PM, Gerd Hoffmann wrote:
> >> I didn't figure out how each domain knowns which fences to use? They
> >> know how many, but which ones?
> >
> > I think the guest doesn't have to know because mmio access is trapped by
> > the hypervisor anyway.
> 
> Okay.. so the point is to ensure no domain can hog all fences, but if 
> the hypervisor is already tracking everything why is this limiting 
> required then in the guest? Hypervisor could just say all fences are 
> allocated since it knows how much it gave out per guest.

There is no dynamic allocation.  Each guest gets a fixed number of
fences assigned which it can use, pretty much like memory where each
guest gets a fixed piece too.

It's not possible to allow guests direct access to the fence registers
though.  And if every fence register access traps into the hypervisor
anyway the hypervisor can easily map the guest virtual fence to host
physical fence, so there is no need to tell the guest which fences it
owns, the number of fences is enough.

cheers,
  Gerd


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tvrtko Ursulin]

On 12/17/2014 07:33 AM, Gerd Hoffmann wrote:
> On Di, 2014-12-16 at 15:01 +0000, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 12/16/2014 02:41 PM, Gerd Hoffmann wrote:
>>>> I didn't figure out how each domain knowns which fences to use? They
>>>> know how many, but which ones?
>>>
>>> I think the guest doesn't have to know because mmio access is trapped by
>>> the hypervisor anyway.
>>
>> Okay.. so the point is to ensure no domain can hog all fences, but if
>> the hypervisor is already tracking everything why is this limiting
>> required then in the guest? Hypervisor could just say all fences are
>> allocated since it knows how much it gave out per guest.
>
> There is no dynamic allocation.  Each guest gets a fixed number of
> fences assigned which it can use, pretty much like memory where each
> guest gets a fixed piece too.
>
> It's not possible to allow guests direct access to the fence registers
> though.  And if every fence register access traps into the hypervisor
> anyway the hypervisor can easily map the guest virtual fence to host
> physical fence, so there is no need to tell the guest which fences it
> owns, the number of fences is enough.

That exactly is the part I don't understand - if it is not required to 
tell the guest which fences it owns, why it is required to say how many?

With this scheme it could be one guest wants more and can't get them 
even if no one else is using any. If not limited they could be 
dynamically allocated by the hypervisor, no?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tvrtko Ursulin]

On 12/17/2014 05:20 AM, Tian, Kevin wrote:
> and if you read the paper referred in 1st mail, we call our ballooning as
> address space ballooning which is different from traditional ballooning
> technique used in memory virtualization, w/ the major difference as:
>
> * traditional ballooning only care about the size. As long as the desired
> number of pages can be ballooned out, we don't care whether they
> actually come from.
>
> * address space ballooning used in our case care about both size and
> address. We need the guest to balloon out a specified range of addresses
> as explained earlier.
>
> whether to grow/shrink dynamic is not the key difference between
> two approaches. yes, we only support static ballooning now at boot time,
> but the same interface can be extended to support dynamic ballooning
> in the future, w/ more cooperation from guest driver in the future.
>
> So I think using ballooning is a right fit here, though the policy is very
> simple now.

Personally I think the first part of the argument does not hold since 
ballooning is the key word here - address space or memory makes little 
difference to the metaphor. It is more like partitioning of blanking as 
it stands.

You only get away with it since you hint of plans to make it dynamic in 
the future. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Gerd Hoffmann]

  Hi,

> > It's not possible to allow guests direct access to the fence registers
> > though.  And if every fence register access traps into the hypervisor
> > anyway the hypervisor can easily map the guest virtual fence to host
> > physical fence, so there is no need to tell the guest which fences it
> > owns, the number of fences is enough.
> 
> That exactly is the part I don't understand - if it is not required to 
> tell the guest which fences it owns, why it is required to say how many?

There is a fixed assignment of fences to guests, so it's a fixed number.
But as the hypervisor is involved in any fence access anyway there is no
need for the guest to know which of the fences it owns, the hypervisor
can remap that transparently for the guest, without performance penalty.

> With this scheme it could be one guest wants more and can't get them 
> even if no one else is using any. If not limited they could be 
> dynamically allocated by the hypervisor, no?

Should be possible as extension, but I think[1] for now the goal is to
stay as close as possible to physical hardware, where you can't
dynamically allocate fences in the first place.

cheers,
  Gerd

[1] Not being deeply involved into vgpu development, just reviewing
    the bits with some kvm background.


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Yu, Zhang]

On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
>    Hi,
>
>>> It's not possible to allow guests direct access to the fence registers
>>> though.  And if every fence register access traps into the hypervisor
>>> anyway the hypervisor can easily map the guest virtual fence to host
>>> physical fence, so there is no need to tell the guest which fences it
>>> owns, the number of fences is enough.
>>
>> That exactly is the part I don't understand - if it is not required to
>> tell the guest which fences it owns, why it is required to say how many?
>
> There is a fixed assignment of fences to guests, so it's a fixed number.
> But as the hypervisor is involved in any fence access anyway there is no
> need for the guest to know which of the fences it owns, the hypervisor
> can remap that transparently for the guest, without performance penalty.
Thanks Gerd. Exactly.
Although fence registers are parititioned to vGPU, it is not necessary 
for a vGPU to know the physical mmio addresses of the allocated fence 
registers.
For example, vGPU 1 with fence size 4 can access the fence registers 
from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can 
access the fence registers from 0x100000-0x10003f. Although this seems 
conflicting, it does not matter. Because these mmio addresses are all 
supposed to be trapped in the host side, which will keep a record of the 
real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2), 
and then does the remapping. Therefore, the physical operations on the 
fence register will be performed by host code on different ones(say, 
0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).
>
>> With this scheme it could be one guest wants more and can't get them
>> even if no one else is using any. If not limited they could be
>> dynamically allocated by the hypervisor, no?
>
> Should be possible as extension, but I think[1] for now the goal is to
> stay as close as possible to physical hardware, where you can't
> dynamically allocate fences in the first place.
Yes. By now we have feature to configure the fence numbers for a vGPU. 
But we do not support the dynamical allocations.
>
> cheers,
>    Gerd
>
> [1] Not being deeply involved into vgpu development, just reviewing
>      the bits with some kvm background.
>
>
>
>
Thanks
Yu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tvrtko Ursulin]

On 12/17/2014 11:25 AM, Yu, Zhang wrote:
> On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
>>    Hi,
>>
>>>> It's not possible to allow guests direct access to the fence registers
>>>> though.  And if every fence register access traps into the hypervisor
>>>> anyway the hypervisor can easily map the guest virtual fence to host
>>>> physical fence, so there is no need to tell the guest which fences it
>>>> owns, the number of fences is enough.
>>>
>>> That exactly is the part I don't understand - if it is not required to
>>> tell the guest which fences it owns, why it is required to say how many?
>>
>> There is a fixed assignment of fences to guests, so it's a fixed number.
>> But as the hypervisor is involved in any fence access anyway there is no
>> need for the guest to know which of the fences it owns, the hypervisor
>> can remap that transparently for the guest, without performance penalty.
> Thanks Gerd. Exactly.
> Although fence registers are parititioned to vGPU, it is not necessary
> for a vGPU to know the physical mmio addresses of the allocated fence
> registers.
> For example, vGPU 1 with fence size 4 can access the fence registers
> from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can
> access the fence registers from 0x100000-0x10003f. Although this seems
> conflicting, it does not matter. Because these mmio addresses are all
> supposed to be trapped in the host side, which will keep a record of the
> real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2),
> and then does the remapping. Therefore, the physical operations on the
> fence register will be performed by host code on different ones(say,
> 0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).

Okay, I think I get it now. What I had in mind is not really possible 
without a dedicated hypervisor<->guest communication channel. Or in 
other words you would have to extend the way i915 allocates them from 
mmio writes to something bi-directional.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Daniel Vetter]

On Wed, Dec 17, 2014 at 11:50:13AM +0000, Tvrtko Ursulin wrote:
> 
> On 12/17/2014 11:25 AM, Yu, Zhang wrote:
> >On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>>>It's not possible to allow guests direct access to the fence registers
> >>>>though.  And if every fence register access traps into the hypervisor
> >>>>anyway the hypervisor can easily map the guest virtual fence to host
> >>>>physical fence, so there is no need to tell the guest which fences it
> >>>>owns, the number of fences is enough.
> >>>
> >>>That exactly is the part I don't understand - if it is not required to
> >>>tell the guest which fences it owns, why it is required to say how many?
> >>
> >>There is a fixed assignment of fences to guests, so it's a fixed number.
> >>But as the hypervisor is involved in any fence access anyway there is no
> >>need for the guest to know which of the fences it owns, the hypervisor
> >>can remap that transparently for the guest, without performance penalty.
> >Thanks Gerd. Exactly.
> >Although fence registers are parititioned to vGPU, it is not necessary
> >for a vGPU to know the physical mmio addresses of the allocated fence
> >registers.
> >For example, vGPU 1 with fence size 4 can access the fence registers
> >from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can
> >access the fence registers from 0x100000-0x10003f. Although this seems
> >conflicting, it does not matter. Because these mmio addresses are all
> >supposed to be trapped in the host side, which will keep a record of the
> >real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2),
> >and then does the remapping. Therefore, the physical operations on the
> >fence register will be performed by host code on different ones(say,
> >0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).
> 
> Okay, I think I get it now. What I had in mind is not really possible
> without a dedicated hypervisor<->guest communication channel. Or in other
> words you would have to extend the way i915 allocates them from mmio writes
> to something bi-directional.

You could virtualize fences the same way we virtualize fences for
userspace for gtt mmap access: If we need to steal a fences we simply need
to unmap the relevant gtt mmio range from the guest ptes. This should work
well since on current platforms the only thing that really needs fences is
cpu access, the gpu doesn't need them. Well except for some oddball cases
in the display block, but those are virtualized anyway (not fbc for guests
or anything else like that).

This would also fit a bit more closely with how the host manages fences,
so benefiting the new kvm/xengt-on-i915 mode for the host instead of the
current implementation which also virtualizes host i915 access cycles.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Daniel Vetter]

On Wed, Dec 17, 2014 at 06:10:23PM +0100, Daniel Vetter wrote:
> On Wed, Dec 17, 2014 at 11:50:13AM +0000, Tvrtko Ursulin wrote:
> > 
> > On 12/17/2014 11:25 AM, Yu, Zhang wrote:
> > >On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
> > >>   Hi,
> > >>
> > >>>>It's not possible to allow guests direct access to the fence registers
> > >>>>though.  And if every fence register access traps into the hypervisor
> > >>>>anyway the hypervisor can easily map the guest virtual fence to host
> > >>>>physical fence, so there is no need to tell the guest which fences it
> > >>>>owns, the number of fences is enough.
> > >>>
> > >>>That exactly is the part I don't understand - if it is not required to
> > >>>tell the guest which fences it owns, why it is required to say how many?
> > >>
> > >>There is a fixed assignment of fences to guests, so it's a fixed number.
> > >>But as the hypervisor is involved in any fence access anyway there is no
> > >>need for the guest to know which of the fences it owns, the hypervisor
> > >>can remap that transparently for the guest, without performance penalty.
> > >Thanks Gerd. Exactly.
> > >Although fence registers are parititioned to vGPU, it is not necessary
> > >for a vGPU to know the physical mmio addresses of the allocated fence
> > >registers.
> > >For example, vGPU 1 with fence size 4 can access the fence registers
> > >from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can
> > >access the fence registers from 0x100000-0x10003f. Although this seems
> > >conflicting, it does not matter. Because these mmio addresses are all
> > >supposed to be trapped in the host side, which will keep a record of the
> > >real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2),
> > >and then does the remapping. Therefore, the physical operations on the
> > >fence register will be performed by host code on different ones(say,
> > >0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).
> > 
> > Okay, I think I get it now. What I had in mind is not really possible
> > without a dedicated hypervisor<->guest communication channel. Or in other
> > words you would have to extend the way i915 allocates them from mmio writes
> > to something bi-directional.
> 
> You could virtualize fences the same way we virtualize fences for
> userspace for gtt mmap access: If we need to steal a fences we simply need
> to unmap the relevant gtt mmio range from the guest ptes. This should work
> well since on current platforms the only thing that really needs fences is
> cpu access, the gpu doesn't need them. Well except for some oddball cases
> in the display block, but those are virtualized anyway (not fbc for guests
> or anything else like that).
> 
> This would also fit a bit more closely with how the host manages fences,
> so benefiting the new kvm/xengt-on-i915 mode for the host instead of the
> current implementation which also virtualizes host i915 access cycles.

Btw this isn't a blocker for the current implementation since we can
always just tell guests that they can use all fences when we implement
this. So the current, simpler implementation isn't restricting us in any
meaningful way.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tian, Kevin]

> From: Daniel Vetter
> Sent: Thursday, December 18, 2014 1:10 AM
> 
> On Wed, Dec 17, 2014 at 11:50:13AM +0000, Tvrtko Ursulin wrote:
> >
> > On 12/17/2014 11:25 AM, Yu, Zhang wrote:
> > >On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
> > >>   Hi,
> > >>
> > >>>>It's not possible to allow guests direct access to the fence registers
> > >>>>though.  And if every fence register access traps into the hypervisor
> > >>>>anyway the hypervisor can easily map the guest virtual fence to host
> > >>>>physical fence, so there is no need to tell the guest which fences it
> > >>>>owns, the number of fences is enough.
> > >>>
> > >>>That exactly is the part I don't understand - if it is not required to
> > >>>tell the guest which fences it owns, why it is required to say how many?
> > >>
> > >>There is a fixed assignment of fences to guests, so it's a fixed number.
> > >>But as the hypervisor is involved in any fence access anyway there is no
> > >>need for the guest to know which of the fences it owns, the hypervisor
> > >>can remap that transparently for the guest, without performance penalty.
> > >Thanks Gerd. Exactly.
> > >Although fence registers are parititioned to vGPU, it is not necessary
> > >for a vGPU to know the physical mmio addresses of the allocated fence
> > >registers.
> > >For example, vGPU 1 with fence size 4 can access the fence registers
> > >from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can
> > >access the fence registers from 0x100000-0x10003f. Although this seems
> > >conflicting, it does not matter. Because these mmio addresses are all
> > >supposed to be trapped in the host side, which will keep a record of the
> > >real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2),
> > >and then does the remapping. Therefore, the physical operations on the
> > >fence register will be performed by host code on different ones(say,
> > >0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).
> >
> > Okay, I think I get it now. What I had in mind is not really possible
> > without a dedicated hypervisor<->guest communication channel. Or in other
> > words you would have to extend the way i915 allocates them from mmio
> writes
> > to something bi-directional.
> 
> You could virtualize fences the same way we virtualize fences for
> userspace for gtt mmap access: If we need to steal a fences we simply need
> to unmap the relevant gtt mmio range from the guest ptes. This should work
> well since on current platforms the only thing that really needs fences is
> cpu access, the gpu doesn't need them. Well except for some oddball cases
> in the display block, but those are virtualized anyway (not fbc for guests
> or anything else like that).

doing this w/o guest's awareness is challenging. bear in mind that vCPU 
scheduling (in hypervisor) is split from vGPU scheduling (in xengt driver).
so xengt driver doesn't know when cpu access happens, and thus steal
a fence under the hood is problematic.

based on current enlightenment framework, it's possible to have driver
allocate/release fences through PVINFO window. That way we can avoid
the current static partitioning scheme, and the changes shouldn't be
that much.

btw curious I heard that typically 2-4 fences are OK for most workloads.
Is that true? If yes, this fence partition along is not the only limiting
factor on #VM. the 512M aperture size is more limiting (but will be better
starting from bdw)

> 
> This would also fit a bit more closely with how the host manages fences,
> so benefiting the new kvm/xengt-on-i915 mode for the host instead of the
> current implementation which also virtualizes host i915 access cycles.
> -Daniel

for host yes we won't virtualize fence then. We're evaluating how to 
remove most mediations on the host paths, with few exceptions e.g.
for registers which requires manual save/restore at context swtich
time.

Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Daniel Vetter]

On Thu, Dec 18, 2014 at 12:36:29AM +0000, Tian, Kevin wrote:
> > From: Daniel Vetter
> > Sent: Thursday, December 18, 2014 1:10 AM
> > 
> > On Wed, Dec 17, 2014 at 11:50:13AM +0000, Tvrtko Ursulin wrote:
> > >
> > > On 12/17/2014 11:25 AM, Yu, Zhang wrote:
> > > >On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
> > > >>   Hi,
> > > >>
> > > >>>>It's not possible to allow guests direct access to the fence registers
> > > >>>>though.  And if every fence register access traps into the hypervisor
> > > >>>>anyway the hypervisor can easily map the guest virtual fence to host
> > > >>>>physical fence, so there is no need to tell the guest which fences it
> > > >>>>owns, the number of fences is enough.
> > > >>>
> > > >>>That exactly is the part I don't understand - if it is not required to
> > > >>>tell the guest which fences it owns, why it is required to say how many?
> > > >>
> > > >>There is a fixed assignment of fences to guests, so it's a fixed number.
> > > >>But as the hypervisor is involved in any fence access anyway there is no
> > > >>need for the guest to know which of the fences it owns, the hypervisor
> > > >>can remap that transparently for the guest, without performance penalty.
> > > >Thanks Gerd. Exactly.
> > > >Although fence registers are parititioned to vGPU, it is not necessary
> > > >for a vGPU to know the physical mmio addresses of the allocated fence
> > > >registers.
> > > >For example, vGPU 1 with fence size 4 can access the fence registers
> > > >from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can
> > > >access the fence registers from 0x100000-0x10003f. Although this seems
> > > >conflicting, it does not matter. Because these mmio addresses are all
> > > >supposed to be trapped in the host side, which will keep a record of the
> > > >real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2),
> > > >and then does the remapping. Therefore, the physical operations on the
> > > >fence register will be performed by host code on different ones(say,
> > > >0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).
> > >
> > > Okay, I think I get it now. What I had in mind is not really possible
> > > without a dedicated hypervisor<->guest communication channel. Or in other
> > > words you would have to extend the way i915 allocates them from mmio
> > writes
> > > to something bi-directional.
> > 
> > You could virtualize fences the same way we virtualize fences for
> > userspace for gtt mmap access: If we need to steal a fences we simply need
> > to unmap the relevant gtt mmio range from the guest ptes. This should work
> > well since on current platforms the only thing that really needs fences is
> > cpu access, the gpu doesn't need them. Well except for some oddball cases
> > in the display block, but those are virtualized anyway (not fbc for guests
> > or anything else like that).
> 
> doing this w/o guest's awareness is challenging. bear in mind that vCPU 
> scheduling (in hypervisor) is split from vGPU scheduling (in xengt driver).
> so xengt driver doesn't know when cpu access happens, and thus steal
> a fence under the hood is problematic.

Well i915 also doesn't interfere with the linux scheduler at all to manage
fences. All you have to do is kick out the ptes before yanking the fence
away, no need to talk to the scheduler at all. Ofc if some other vm is
using that area it'll immediately fault on the cpu, but the fault handler
can then rectify this and steal some other fence that's not used right
away again hopefully.

> based on current enlightenment framework, it's possible to have driver
> allocate/release fences through PVINFO window. That way we can avoid
> the current static partitioning scheme, and the changes shouldn't be
> that much.
> 
> btw curious I heard that typically 2-4 fences are OK for most workloads.
> Is that true? If yes, this fence partition along is not the only limiting
> factor on #VM. the 512M aperture size is more limiting (but will be better
> starting from bdw)
> 
> > 
> > This would also fit a bit more closely with how the host manages fences,
> > so benefiting the new kvm/xengt-on-i915 mode for the host instead of the
> > current implementation which also virtualizes host i915 access cycles.
> > -Daniel
> 
> for host yes we won't virtualize fence then. We're evaluating how to 
> remove most mediations on the host paths, with few exceptions e.g.
> for registers which requires manual save/restore at context swtich
> time.

My idea would be to use additional MI_LRI/SRM commands before/after the
call to load the context into the ring using the functions in
i915_gem_context.c. This is exactly what the execbuf code does too:
There's lots of additional stuff (per-platform) it does around the actual
context switch. So even there I think you can implement the VM switch on
top of i915 infrastructure. Maybe we need to duplicate/export some
execbuf internals, but that's a lot better than duplicating the entire
driver and having 2 drivers fight over the same hw.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/8] drm/i915: Partition the fence registers for vGPU in i915 driver

[Tian, Kevin]

> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Thursday, December 18, 2014 4:08 PM
> 
> On Thu, Dec 18, 2014 at 12:36:29AM +0000, Tian, Kevin wrote:
> > > From: Daniel Vetter
> > > Sent: Thursday, December 18, 2014 1:10 AM
> > >
> > > On Wed, Dec 17, 2014 at 11:50:13AM +0000, Tvrtko Ursulin wrote:
> > > >
> > > > On 12/17/2014 11:25 AM, Yu, Zhang wrote:
> > > > >On 12/17/2014 7:06 PM, Gerd Hoffmann wrote:
> > > > >>   Hi,
> > > > >>
> > > > >>>>It's not possible to allow guests direct access to the fence registers
> > > > >>>>though.  And if every fence register access traps into the
> hypervisor
> > > > >>>>anyway the hypervisor can easily map the guest virtual fence to host
> > > > >>>>physical fence, so there is no need to tell the guest which fences it
> > > > >>>>owns, the number of fences is enough.
> > > > >>>
> > > > >>>That exactly is the part I don't understand - if it is not required to
> > > > >>>tell the guest which fences it owns, why it is required to say how
> many?
> > > > >>
> > > > >>There is a fixed assignment of fences to guests, so it's a fixed number.
> > > > >>But as the hypervisor is involved in any fence access anyway there is
> no
> > > > >>need for the guest to know which of the fences it owns, the hypervisor
> > > > >>can remap that transparently for the guest, without performance
> penalty.
> > > > >Thanks Gerd. Exactly.
> > > > >Although fence registers are parititioned to vGPU, it is not necessary
> > > > >for a vGPU to know the physical mmio addresses of the allocated fence
> > > > >registers.
> > > > >For example, vGPU 1 with fence size 4 can access the fence registers
> > > > >from 0x100000-10001f; at the same time, vGPU 2 with fence size 8 can
> > > > >access the fence registers from 0x100000-0x10003f. Although this
> seems
> > > > >conflicting, it does not matter. Because these mmio addresses are all
> > > > >supposed to be trapped in the host side, which will keep a record of the
> > > > >real fence offset of different vGPUs(say 0 for vGPU 1 and 4 for vGPU 2),
> > > > >and then does the remapping. Therefore, the physical operations on the
> > > > >fence register will be performed by host code on different ones(say,
> > > > >0x100000-10001fh for vGPU 1 and 0x100020-0x10005f for vGPU 2).
> > > >
> > > > Okay, I think I get it now. What I had in mind is not really possible
> > > > without a dedicated hypervisor<->guest communication channel. Or in
> other
> > > > words you would have to extend the way i915 allocates them from mmio
> > > writes
> > > > to something bi-directional.
> > >
> > > You could virtualize fences the same way we virtualize fences for
> > > userspace for gtt mmap access: If we need to steal a fences we simply
> need
> > > to unmap the relevant gtt mmio range from the guest ptes. This should
> work
> > > well since on current platforms the only thing that really needs fences is
> > > cpu access, the gpu doesn't need them. Well except for some oddball cases
> > > in the display block, but those are virtualized anyway (not fbc for guests
> > > or anything else like that).
> >
> > doing this w/o guest's awareness is challenging. bear in mind that vCPU
> > scheduling (in hypervisor) is split from vGPU scheduling (in xengt driver).
> > so xengt driver doesn't know when cpu access happens, and thus steal
> > a fence under the hood is problematic.
> 
> Well i915 also doesn't interfere with the linux scheduler at all to manage
> fences. All you have to do is kick out the ptes before yanking the fence
> away, no need to talk to the scheduler at all. Ofc if some other vm is
> using that area it'll immediately fault on the cpu, but the fault handler
> can then rectify this and steal some other fence that's not used right
> away again hopefully.

yes, this would work, but I'd think having guest to allocate/release fence
through PVINFO is simpler and less hypervisor specific. 

> 
> > based on current enlightenment framework, it's possible to have driver
> > allocate/release fences through PVINFO window. That way we can avoid
> > the current static partitioning scheme, and the changes shouldn't be
> > that much.
> >
> > btw curious I heard that typically 2-4 fences are OK for most workloads.
> > Is that true? If yes, this fence partition along is not the only limiting
> > factor on #VM. the 512M aperture size is more limiting (but will be better
> > starting from bdw)
> >
> > >
> > > This would also fit a bit more closely with how the host manages fences,
> > > so benefiting the new kvm/xengt-on-i915 mode for the host instead of the
> > > current implementation which also virtualizes host i915 access cycles.
> > > -Daniel
> >
> > for host yes we won't virtualize fence then. We're evaluating how to
> > remove most mediations on the host paths, with few exceptions e.g.
> > for registers which requires manual save/restore at context swtich
> > time.
> 
> My idea would be to use additional MI_LRI/SRM commands before/after the
> call to load the context into the ring using the functions in
> i915_gem_context.c. This is exactly what the execbuf code does too:
> There's lots of additional stuff (per-platform) it does around the actual
> context switch. So even there I think you can implement the VM switch on
> top of i915 infrastructure. Maybe we need to duplicate/export some
> execbuf internals, but that's a lot better than duplicating the entire
> driver and having 2 drivers fight over the same hw.
> -Daniel
> --

yes, that's the direction what we agreed before and definitely we don't
want to duplicate entire driver (btw even with current i915-on-xengt
model our driver code is much smaller since we only focus on resource
management. :-)) We'll try to leverage existing i915 functions as many 
as possible. For major changes, we'll summarize and send out for design 
review to reach agreement. for above example, yes what you described 
should work, by hooking xengt specific register list in 915_gem_context.c

Thanks
Kevin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx