[RFCv2,02/14] drm/i915/gvt: Introduce the basic architecture of GVT-g

Submitted by Wang, Zhi A on Feb. 18, 2016, 11:42 a.m.

Details

Message ID 1455795741-3487-3-git-send-email-zhi.a.wang@intel.com
State New
Headers show
Series "gvt: Hacking i915 for GVT context requirement" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Wang, Zhi A Feb. 18, 2016, 11:42 a.m.
This patch introduces the very basic framework of GVT-g device model,
includes basic prototypes, definitions, initialization.

v2:
- Introduce i915_gvt.c.
It's necessary to introduce the stubs between i915 driver and GVT-g host,
as GVT-g components is configurable in kernel config. When disabled, the
stubs here do nothing.

Take Joonas's comments:
- Replace boolean return value with int.
- Replace customized info/warn/debug macros with DRM macros.
- Document all non-static functions like i915.
- Remove empty and unused functions.
- Replace magic number with marcos.
- Set GVT-g in kernel config to "n" by default.

Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/Kconfig         |  15 ++
 drivers/gpu/drm/i915/Makefile        |   2 +
 drivers/gpu/drm/i915/gvt/Makefile    |   5 +
 drivers/gpu/drm/i915/gvt/debug.h     |  57 +++++
 drivers/gpu/drm/i915/gvt/gvt.c       | 393 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h       | 100 +++++++++
 drivers/gpu/drm/i915/gvt/hypercall.h |  30 +++
 drivers/gpu/drm/i915/gvt/mpt.h       |  34 +++
 drivers/gpu/drm/i915/gvt/params.c    |  32 +++
 drivers/gpu/drm/i915/gvt/params.h    |  34 +++
 drivers/gpu/drm/i915/gvt/reg.h       |  34 +++
 drivers/gpu/drm/i915/i915_dma.c      |  14 ++
 drivers/gpu/drm/i915/i915_drv.h      |  12 ++
 drivers/gpu/drm/i915/i915_gvt.c      |  93 +++++++++
 drivers/gpu/drm/i915/i915_gvt.h      |  49 +++++
 15 files changed, 904 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gvt/Makefile
 create mode 100644 drivers/gpu/drm/i915/gvt/debug.h
 create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c
 create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h
 create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h
 create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h
 create mode 100644 drivers/gpu/drm/i915/gvt/params.c
 create mode 100644 drivers/gpu/drm/i915/gvt/params.h
 create mode 100644 drivers/gpu/drm/i915/gvt/reg.h
 create mode 100644 drivers/gpu/drm/i915/i915_gvt.c
 create mode 100644 drivers/gpu/drm/i915/i915_gvt.h

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 4c59793..082e77d 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -45,3 +45,18 @@  config DRM_I915_PRELIMINARY_HW_SUPPORT
 	  option changes the default for that module option.
 
 	  If in doubt, say "N".
+
+config DRM_I915_GVT
+        tristate "GVT-g host driver"
+        depends on DRM_I915
+        default n
+        help
+          Enabling GVT-g mediated graphics passthrough technique for Intel i915
+          based integrated graphics card. With GVT-g, it's possible to have one
+          integrated i915 device shared by multiple VMs. Performance critical
+          opterations such as apperture accesses and ring buffer operations
+          are pass-throughed to VM, with a minimal set of conflicting resources
+          (e.g. display settings) mediated by vGT driver. The benefit of vGT
+          is on both the performance, given that each VM could directly operate
+          its aperture space and submit commands like running on native, and
+          the feature completeness, given that a true GEN hardware is exposed.
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0851de07..c65026c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -91,6 +91,8 @@  i915-y += dvo_ch7017.o \
 	  intel_sdvo.o \
 	  intel_tv.o
 
+obj-$(CONFIG_DRM_I915_GVT)  += i915_gvt.o gvt/
+
 # virtual gpu code
 i915-y += i915_vgpu.o
 
diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
new file mode 100644
index 0000000..959305f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -0,0 +1,5 @@ 
+GVT_SOURCE := gvt.o params.o
+
+ccflags-y                      += -I$(src) -I$(src)/.. -Wall -Werror -Wno-unused-function
+i915_gvt-y                     := $(GVT_SOURCE)
+obj-$(CONFIG_DRM_I915_GVT)     += i915_gvt.o
diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
new file mode 100644
index 0000000..0747f28
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/debug.h
@@ -0,0 +1,57 @@ 
+/*
+ * Copyright(c) 2011-2016 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 __GVT_DEBUG_H__
+#define __GVT_DEBUG_H__
+
+#define gvt_info(fmt, args...) \
+	DRM_INFO("gvt: "fmt"\n", ##args)
+
+#define gvt_err(fmt, args...) \
+	DRM_ERROR("gvt: "fmt"\n", ##args)
+
+#define gvt_warn(condition, fmt, args...) \
+	WARN((condition), "gvt: "fmt"\n", ##args)
+
+#define gvt_warn_once(condition, fmt, args...) \
+	WARN_ONCE((condition), "gvt: "fmt"\n", ##args)
+
+#define gvt_dbg(level, fmt, args...) \
+	DRM_DEBUG_DRIVER("gvt: "fmt"\n", ##args)
+
+enum {
+	GVT_DBG_CORE = (1 << 0),
+	GVT_DBG_MM = (1 << 1),
+	GVT_DBG_IRQ = (1 << 2),
+};
+
+#define gvt_dbg_core(fmt, args...) \
+	gvt_dbg(GVT_DBG_CORE, fmt, ##args)
+
+#define gvt_dbg_mm(fmt, args...) \
+	gvt_dbg(GVT_DBG_MM, fmt, ##args)
+
+#define gvt_dbg_irq(fmt, args...) \
+	gvt_dbg(GVT_DBG_IRQ, fmt, ##args)
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
new file mode 100644
index 0000000..52cfa32
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -0,0 +1,393 @@ 
+/*
+ * Copyright(c) 2011-2016 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 <linux/types.h>
+#include <xen/xen.h>
+#include <linux/kthread.h>
+
+#include "gvt.h"
+
+struct gvt_host gvt_host;
+
+static const char * const supported_hypervisors[] = {
+	[GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
+	[GVT_HYPERVISOR_TYPE_KVM] = "KVM",
+};
+
+static int gvt_init_host(void)
+{
+	struct gvt_host *host = &gvt_host;
+
+	if (!gvt.enable) {
+		gvt_dbg_core("GVT-g has been disabled by kernel parameter");
+		return -EINVAL;
+	}
+
+	if (host->initialized) {
+		gvt_err("GVT-g has already been initialized!");
+		return -EINVAL;
+	}
+
+	/* Xen DOM U */
+	if (xen_domain() && !xen_initial_domain())
+		return -ENODEV;
+
+	if (xen_initial_domain()) {
+		/* Xen Dom0 */
+		host->kdm = try_then_request_module(
+				symbol_get(xengt_kdm), "xengt");
+		host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
+	} else {
+		/* not in Xen. Try KVMGT */
+		host->kdm = try_then_request_module(
+				symbol_get(kvmgt_kdm), "kvm");
+		host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
+	}
+
+	if (!host->kdm)
+		return -EINVAL;
+
+	if (!hypervisor_detect_host())
+		return -ENODEV;
+
+	gvt_info("Running with hypervisor %s in host mode",
+			supported_hypervisors[host->hypervisor_type]);
+
+	idr_init(&host->device_idr);
+	mutex_init(&host->device_idr_lock);
+
+	host->initialized = true;
+	return 0;
+}
+
+static int init_device_info(struct pgt_device *pdev)
+{
+	struct gvt_device_info *info = &pdev->device_info;
+
+	if (!IS_BROADWELL(pdev->dev_priv)) {
+		DRM_DEBUG_DRIVER("Unsupported GEN device");
+		return -ENODEV;
+	}
+
+	if (IS_BROADWELL(pdev->dev_priv)) {
+		info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
+		/*
+		 * The layout of BAR0 in BDW:
+		 * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->|
+		 *
+		 * GTT offset in BAR0 starts from 8MB to 16MB, and
+		 * Whatever GTT size is configured in BIOS,
+		 * the size of BAR0 is always 16MB. The actual configured
+		 * GTT size can be found in GMCH_CTRL.
+		 */
+		info->gtt_start_offset = (1UL << 23); /* 8MB */
+		info->max_gtt_size = (1UL << 23); /* 8MB */
+		info->gtt_entry_size = 8;
+		info->gtt_entry_size_shift = 3;
+		info->gmadr_bytes_in_cmd = 8;
+		info->mmio_size = 2 * 1024 * 1024; /* 2MB */
+	}
+
+	return 0;
+}
+
+static void init_initial_cfg_space_state(struct pgt_device *pdev)
+{
+	struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
+	int i;
+
+	gvt_dbg_core("init initial cfg space, id %d", pdev->id);
+
+	for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
+		pci_read_config_dword(pci_dev, i,
+				(u32 *)&pdev->initial_cfg_space[i]);
+
+	for (i = 0; i < GVT_BAR_NUM; i++) {
+		pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
+		gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
+	}
+}
+
+static void clean_initial_mmio_state(struct pgt_device *pdev)
+{
+	if (pdev->gttmmio_va) {
+		iounmap(pdev->gttmmio_va);
+		pdev->gttmmio_va = NULL;
+	}
+
+	if (pdev->gmadr_va) {
+		iounmap(pdev->gmadr_va);
+		pdev->gmadr_va = NULL;
+	}
+}
+
+static int init_initial_mmio_state(struct pgt_device *pdev)
+{
+	struct gvt_device_info *info = &pdev->device_info;
+
+	u64 bar0, bar1;
+	int rc;
+
+	gvt_dbg_core("init initial mmio state, id %d", pdev->id);
+
+	bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
+	bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
+
+	pdev->gttmmio_base = bar0 & ~0xf;
+	pdev->reg_num = info->mmio_size / 4;
+	pdev->gmadr_base = bar1 & ~0xf;
+
+	pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
+	if (!pdev->gttmmio_va) {
+		gvt_err("fail to map GTTMMIO BAR.");
+		return -EFAULT;
+	}
+
+	pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
+	if (!pdev->gmadr_va) {
+		gvt_err("fail to map GMADR BAR.");
+		rc = -EFAULT;
+		goto err;
+	}
+
+	gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
+	gvt_dbg_core("mmio size: %x", pdev->mmio_size);
+	gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
+			pdev->gmadr_base);
+	gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
+	gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
+
+	return 0;
+err:
+	clean_initial_mmio_state(pdev);
+	return rc;
+}
+
+static int gvt_service_thread(void *data)
+{
+	struct pgt_device *pdev = (struct pgt_device *)data;
+	int r;
+
+	gvt_dbg_core("service thread start, pgt %d", pdev->id);
+
+	while (!kthread_should_stop()) {
+		r = wait_event_interruptible(pdev->service_thread_wq,
+				kthread_should_stop() || pdev->service_request);
+
+		if (kthread_should_stop())
+			break;
+
+		if (gvt_warn_once(r,
+			"service thread is waken up by unexpected signal."))
+			continue;
+	}
+
+	return 0;
+}
+
+static void clean_service_thread(struct pgt_device *pdev)
+{
+	if (pdev->service_thread) {
+		kthread_stop(pdev->service_thread);
+		pdev->service_thread = NULL;
+	}
+}
+
+static int init_service_thread(struct pgt_device *pdev)
+{
+	init_waitqueue_head(&pdev->service_thread_wq);
+
+	pdev->service_thread = kthread_run(gvt_service_thread,
+			pdev, "gvt_service_thread%d", pdev->id);
+
+	if (!pdev->service_thread) {
+		gvt_err("fail to start service thread.");
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+static void clean_pgt_device(struct pgt_device *pdev)
+{
+	clean_service_thread(pdev);
+	clean_initial_mmio_state(pdev);
+}
+
+static int init_pgt_device(struct pgt_device *pdev,
+	struct drm_i915_private *dev_priv)
+{
+	int rc;
+
+	rc = init_device_info(pdev);
+	if (rc)
+		return rc;
+
+	init_initial_cfg_space_state(pdev);
+
+	rc = init_initial_mmio_state(pdev);
+	if (rc)
+		goto err;
+
+	rc = init_service_thread(pdev);
+	if (rc)
+		goto err;
+
+	return 0;
+err:
+	clean_pgt_device(pdev);
+	return rc;
+}
+
+static int post_init_pgt_device(struct pgt_device *pdev)
+{
+	return 0;
+}
+
+static void free_pgt_device(struct pgt_device *pdev)
+{
+	struct gvt_host *host = &gvt_host;
+
+	mutex_lock(&host->device_idr_lock);
+	idr_remove(&host->device_idr, pdev->id);
+	mutex_unlock(&host->device_idr_lock);
+
+	vfree(pdev);
+}
+
+static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
+{
+	struct gvt_host *host = &gvt_host;
+	struct pgt_device *pdev = NULL;
+
+	pdev = vzalloc(sizeof(*pdev));
+	if (!pdev)
+		return NULL;
+
+	mutex_lock(&host->device_idr_lock);
+	pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
+	mutex_unlock(&host->device_idr_lock);
+
+	if (pdev->id < 0) {
+		gvt_err("fail to allocate pgt device id.");
+		goto err;
+	}
+
+	mutex_init(&pdev->lock);
+	pdev->dev_priv = dev_priv;
+	idr_init(&pdev->instance_idr);
+
+	return pdev;
+err:
+	free_pgt_device(pdev);
+	return NULL;
+}
+
+void gvt_destroy_pgt_device(void *private_data)
+{
+	struct pgt_device *pdev = (struct pgt_device *)private_data;
+
+	clean_pgt_device(pdev);
+	free_pgt_device(pdev);
+}
+
+/**
+ * gvt_create_pgt_device - create a GVT physical device
+ * @dev: drm device
+ *
+ * This function is called at the initialization stage, to create a physical
+ * GVT device and initialize necessary GVT components for it.
+ *
+ * Returns:
+ * pointer to the pgt_device structure, NULL if failed.
+ */
+void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
+{
+	struct pgt_device *pdev = NULL;
+	struct gvt_host *host = &gvt_host;
+	int rc;
+
+	if (!host->initialized && !gvt_init_host()) {
+		gvt_err("gvt_init_host fail");
+		return NULL;
+	}
+
+	gvt_dbg_core("create new pgt device, i915 dev_priv: %p", dev_priv);
+
+	pdev = alloc_pgt_device(dev_priv);
+	if (!pdev) {
+		gvt_err("fail to allocate memory for pgt device.");
+		goto err;
+	}
+
+	gvt_dbg_core("init pgt device, id %d", pdev->id);
+
+	rc = init_pgt_device(pdev, dev_priv);
+	if (rc) {
+		gvt_err("fail to init physical device state.");
+		goto err;
+	}
+
+	gvt_dbg_core("pgt device creation done, id %d", pdev->id);
+
+	return pdev;
+err:
+	if (pdev) {
+		gvt_destroy_pgt_device(pdev);
+		pdev = NULL;
+	}
+	return NULL;
+}
+
+/**
+ * gvt_post_init_pgt_device - post init a GVT physical device
+ * @dev: drm device
+ *
+ * This function is called at the end of the initialization stage, to
+ * post-initialize a physical GVT device and initialize necessary
+ * GVT components rely on i915 components.
+ *
+ * Returns:
+ * zero on success, non-zero if failed.
+ */
+int gvt_post_init_pgt_device(void *private_data)
+{
+	struct pgt_device *pdev = (struct pgt_device *)private_data;
+	struct gvt_host *host = &gvt_host;
+	int rc;
+
+	if (!host->initialized) {
+		gvt_err("gvt_host haven't been initialized.");
+		return -ENODEV;
+	}
+
+	gvt_dbg_core("post init pgt device %d", pdev->id);
+
+	rc = post_init_pgt_device(pdev);
+	if (rc) {
+		gvt_err("fail to post init physical device state.");
+		return rc;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
new file mode 100644
index 0000000..d450198
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -0,0 +1,100 @@ 
+/*
+ * Copyright(c) 2011-2016 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 _GVT_H_
+#define _GVT_H_
+
+#include "i915_drv.h"
+#include "i915_vgpu.h"
+
+#include "debug.h"
+#include "params.h"
+#include "reg.h"
+#include "hypercall.h"
+#include "mpt.h"
+
+#define GVT_MAX_VGPU 8
+
+enum {
+	GVT_HYPERVISOR_TYPE_XEN = 0,
+	GVT_HYPERVISOR_TYPE_KVM,
+};
+
+struct gvt_host {
+	bool initialized;
+	int hypervisor_type;
+	struct mutex device_idr_lock;
+	struct idr device_idr;
+	struct gvt_kernel_dm *kdm;
+};
+
+extern struct gvt_host gvt_host;
+extern struct gvt_kernel_dm xengt_kdm;
+extern struct gvt_kernel_dm kvmgt_kdm;
+
+/* Describe the limitation of HW.*/
+struct gvt_device_info {
+	u64 max_gtt_gm_sz;
+	u32 gtt_start_offset;
+	u32 gtt_end_offset;
+	u32 max_gtt_size;
+	u32 gtt_entry_size;
+	u32 gtt_entry_size_shift;
+	u32 gmadr_bytes_in_cmd;
+	u32 mmio_size;
+};
+
+struct vgt_device {
+	int id;
+	int vm_id;
+	struct pgt_device *pdev;
+	bool warn_untrack;
+};
+
+struct pgt_device {
+	struct mutex lock;
+	int id;
+
+	struct drm_i915_private *dev_priv;
+	struct idr instance_idr;
+
+	struct gvt_device_info device_info;
+
+	u8 initial_cfg_space[GVT_CFG_SPACE_SZ];
+	u64 bar_size[GVT_BAR_NUM];
+
+	u64 gttmmio_base;
+	void *gttmmio_va;
+
+	u64 gmadr_base;
+	void *gmadr_va;
+
+	u32 mmio_size;
+	u32 reg_num;
+
+	wait_queue_head_t service_thread_wq;
+	struct task_struct *service_thread;
+	unsigned long service_request;
+};
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
new file mode 100644
index 0000000..0a41874
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -0,0 +1,30 @@ 
+/*
+ * Copyright(c) 2011-2016 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 _GVT_HYPERCALL_H_
+#define _GVT_HYPERCALL_H_
+
+struct gvt_kernel_dm {
+};
+
+#endif /* _GVT_HYPERCALL_H_ */
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
new file mode 100644
index 0000000..e594bb8
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -0,0 +1,34 @@ 
+/*
+ * Copyright(c) 2011-2016 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 _GVT_MPT_H_
+#define _GVT_MPT_H_
+
+struct vgt_device;
+
+static inline bool hypervisor_detect_host(void)
+{
+	return false;
+}
+
+#endif /* _GVT_MPT_H_ */
diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
new file mode 100644
index 0000000..d381d17
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/params.c
@@ -0,0 +1,32 @@ 
+/*
+ * Copyright(c) 2011-2016 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 "gvt.h"
+
+struct gvt_kernel_params gvt = {
+	.enable = false,
+	.debug = 0,
+};
+
+module_param_named(gvt_enable, gvt.enable, bool, 0600);
+MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
new file mode 100644
index 0000000..d2955b9
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/params.h
@@ -0,0 +1,34 @@ 
+/*
+ * Copyright(c) 2011-2016 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 _GVT_PARAMS_H_
+#define _GVT_PARAMS_H_
+
+struct gvt_kernel_params {
+	bool enable;
+	int debug;
+};
+
+extern struct gvt_kernel_params gvt;
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/reg.h b/drivers/gpu/drm/i915/gvt/reg.h
new file mode 100644
index 0000000..d363b74
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/reg.h
@@ -0,0 +1,34 @@ 
+/*
+ * Copyright(c) 2011-2016 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 _GVT_REG_H
+#define _GVT_REG_H
+
+#define GVT_CFG_SPACE_SZ	256
+#define GVT_BAR_NUM		4
+
+#define GVT_REG_CFG_SPACE_BAR0	0x10
+#define GVT_REG_CFG_SPACE_BAR1	0x18
+#define GVT_REG_CFG_SPACE_BAR2	0x20
+
+#endif
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 1c6d227..f3bed37 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -35,6 +35,7 @@ 
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_gvt.h"
 #include "i915_vgpu.h"
 #include "i915_trace.h"
 #include <linux/pci.h>
@@ -1045,6 +1046,10 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_uncore_init(dev);
 
+	ret = intel_gvt_init(dev);
+	if (ret)
+		goto out_uncore_fini;
+
 	ret = i915_gem_gtt_init(dev);
 	if (ret)
 		goto out_uncore_fini;
@@ -1135,6 +1140,12 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto out_power_well;
 	}
 
+	ret = intel_gvt_post_init(dev);
+	if (ret) {
+		DRM_ERROR("failed to post init pgt device\n");
+		goto out_power_well;
+	}
+
 	/*
 	 * Notify a valid surface after modesetting,
 	 * when running inside a VM.
@@ -1177,6 +1188,7 @@  out_gem_unload:
 out_gtt:
 	i915_global_gtt_cleanup(dev);
 out_uncore_fini:
+	intel_gvt_cleanup(dev);
 	intel_uncore_fini(dev);
 	i915_mmio_cleanup(dev);
 put_bridge:
@@ -1223,6 +1235,8 @@  int i915_driver_unload(struct drm_device *dev)
 
 	intel_modeset_cleanup(dev);
 
+	intel_gvt_cleanup(dev);
+
 	/*
 	 * free the memory space allocated for the child device
 	 * config parsed from VBT
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20d9dbd..2f897c3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1705,6 +1705,10 @@  struct i915_workarounds {
 	u32 hw_whitelist_count[I915_NUM_RINGS];
 };
 
+struct i915_gvt {
+	void *pgt_device;
+};
+
 struct i915_virtual_gpu {
 	bool active;
 };
@@ -1744,6 +1748,8 @@  struct drm_i915_private {
 
 	struct i915_virtual_gpu vgpu;
 
+	struct i915_gvt gvt;
+
 	struct intel_guc guc;
 
 	struct intel_csr csr;
@@ -2780,6 +2786,12 @@  void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv,
 void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
 					enum forcewake_domains domains);
 void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
+
+static inline bool intel_gvt_active(struct drm_device *dev)
+{
+	return to_i915(dev)->gvt.pgt_device ? true : false;
+}
+
 static inline bool intel_vgpu_active(struct drm_device *dev)
 {
 	return to_i915(dev)->vgpu.active;
diff --git a/drivers/gpu/drm/i915/i915_gvt.c b/drivers/gpu/drm/i915/i915_gvt.c
new file mode 100644
index 0000000..3ca7232
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_gvt.c
@@ -0,0 +1,93 @@ 
+/*
+ * Copyright(c) 2011-2016 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 "i915_drv.h"
+#include "i915_gvt.h"
+
+/**
+ * DOC: Intel GVT-g host 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 the englightments
+ * of GVT and the necessary components used by GVT in i915 driver.
+ */
+
+/**
+ * intel_gvt_init - initialize GVT components at the beginning of i915
+ * driver loading.
+ * @dev: drm device *
+ *
+ * This function is called at the beginning of the initialization stage,
+ * to initialize the GVT components that have to be initialized
+ * before HW gets touched by other i915 components.
+ */
+int intel_gvt_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	dev_priv->gvt.pgt_device = gvt_create_pgt_device(dev_priv);
+	if (intel_gvt_active(dev))
+		DRM_DEBUG_DRIVER("GVT-g is running in host mode\n");
+
+	return 0;
+}
+
+/**
+ * intel_gvt_post_init - initialize GVT components at the end of i915
+ * driver loading.
+ * @dev: drm device *
+ *
+ * This function is called at the end of the initialization stage,
+ * to initialize the GVT components that have to be initialized after
+ * other i915 components are ready.
+ */
+int intel_gvt_post_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (!intel_gvt_active(dev))
+		return 0;
+
+	return gvt_post_init_pgt_device(dev_priv->gvt.pgt_device);
+}
+
+/**
+ * intel_gvt_cleanup - cleanup GVT components when i915 driver is unloading
+ * @dev: drm device *
+ *
+ * This function is called at the i915 driver unloading stage, to shutdown
+ * GVT components and release the related resources.
+ */
+void intel_gvt_cleanup(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (!intel_gvt_active(dev))
+		return;
+
+	gvt_destroy_pgt_device(dev_priv->gvt.pgt_device);
+	dev_priv->gvt.pgt_device = NULL;
+}
diff --git a/drivers/gpu/drm/i915/i915_gvt.h b/drivers/gpu/drm/i915/i915_gvt.h
new file mode 100644
index 0000000..30cc207
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_gvt.h
@@ -0,0 +1,49 @@ 
+/*
+ * Copyright(c) 2011-2016 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_GVT_H_
+#define _I915_GVT_H_
+
+#ifdef CONFIG_DRM_I915_GVT
+extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
+extern int gvt_post_init_pgt_device(void *data);
+extern void gvt_destroy_pgt_device(void *data);
+
+extern int intel_gvt_init(struct drm_device *dev);
+extern int intel_gvt_post_init(struct drm_device *dev);
+extern void intel_gvt_cleanup(struct drm_device *dev);
+#else
+extern int intel_gvt_init(struct drm_device *dev)
+{
+	return 0;
+}
+extern int intel_gvt_post_init(struct drm_device *dev)
+{
+	return 0;
+}
+extern void intel_gvt_cleanup(struct drm_device *dev)
+{
+}
+#endif
+
+#endif /* _I915_GVT_H_ */

Comments

Hi,

Code is looking a lot better.

A general question still; why you seem to be preferring multi-stage
alloc and destroy?

Are there going to be scenarios when things will be allocated but not
initialized? I don't see a such use scenario.

I wouldn't split the init functions down as much as you currently do
because that'll require a lot of boilerplate code to propagate the
errors up, which is currently not done. The boilerplate for propagation
becomes necessary when the teardown function is complex, but currently
the teardown itself is less lines of code than the function
boilerplate.

So just squash those into gvt_device_create() and gvt_device_destroy()
where _create() will propagate any lower level errors up and tear down
a partially initialized struct. _destroy() can then expect to just tear
the whole struct down with no ifs.

Regards, Joonas

On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
> This patch introduces the very basic framework of GVT-g device model,
> includes basic prototypes, definitions, initialization.
> 
> v2:
> - Introduce i915_gvt.c.
> It's necessary to introduce the stubs between i915 driver and GVT-g host,
> as GVT-g components is configurable in kernel config. When disabled, the
> stubs here do nothing.
> 
> Take Joonas's comments:
> - Replace boolean return value with int.
> - Replace customized info/warn/debug macros with DRM macros.
> - Document all non-static functions like i915.
> - Remove empty and unused functions.
> - Replace magic number with marcos.
> - Set GVT-g in kernel config to "n" by default.
> 
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig         |  15 ++
>  drivers/gpu/drm/i915/Makefile        |   2 +
>  drivers/gpu/drm/i915/gvt/Makefile    |   5 +
>  drivers/gpu/drm/i915/gvt/debug.h     |  57 +++++
>  drivers/gpu/drm/i915/gvt/gvt.c       | 393 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.h       | 100 +++++++++
>  drivers/gpu/drm/i915/gvt/hypercall.h |  30 +++
>  drivers/gpu/drm/i915/gvt/mpt.h       |  34 +++
>  drivers/gpu/drm/i915/gvt/params.c    |  32 +++
>  drivers/gpu/drm/i915/gvt/params.h    |  34 +++
>  drivers/gpu/drm/i915/gvt/reg.h       |  34 +++
>  drivers/gpu/drm/i915/i915_dma.c      |  14 ++
>  drivers/gpu/drm/i915/i915_drv.h      |  12 ++
>  drivers/gpu/drm/i915/i915_gvt.c      |  93 +++++++++
>  drivers/gpu/drm/i915/i915_gvt.h      |  49 +++++
>  15 files changed, 904 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/gvt/Makefile
>  create mode 100644 drivers/gpu/drm/i915/gvt/debug.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/params.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/params.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/reg.h
>  create mode 100644 drivers/gpu/drm/i915/i915_gvt.c
>  create mode 100644 drivers/gpu/drm/i915/i915_gvt.h
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 4c59793..082e77d 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -45,3 +45,18 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
>  	  option changes the default for that module option.
>  
>  	  If in doubt, say "N".
> +
> +config DRM_I915_GVT
> +        tristate "GVT-g host driver"
> +        depends on DRM_I915
> +        default n
> +        help
> +          Enabling GVT-g mediated graphics passthrough technique for Intel i915
> +          based integrated graphics card. With GVT-g, it's possible to have one
> +          integrated i915 device shared by multiple VMs. Performance critical
> +          opterations such as apperture accesses and ring buffer operations
> +          are pass-throughed to VM, with a minimal set of conflicting resources
> +          (e.g. display settings) mediated by vGT driver. The benefit of vGT
> +          is on both the performance, given that each VM could directly operate
> +          its aperture space and submit commands like running on native, and
> +          the feature completeness, given that a true GEN hardware is exposed.
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0851de07..c65026c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -91,6 +91,8 @@ i915-y += dvo_ch7017.o \
>  	  intel_sdvo.o \
>  	  intel_tv.o
>  
> +obj-$(CONFIG_DRM_I915_GVT)  += i915_gvt.o gvt/
> +
>  # virtual gpu code
>  i915-y += i915_vgpu.o
>  
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
> new file mode 100644
> index 0000000..959305f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -0,0 +1,5 @@
> +GVT_SOURCE := gvt.o params.o
> +
> +ccflags-y                      += -I$(src) -I$(src)/.. -Wall -Werror -Wno-unused-function
> +i915_gvt-y                     := $(GVT_SOURCE)
> +obj-$(CONFIG_DRM_I915_GVT)     += i915_gvt.o
> diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
> new file mode 100644
> index 0000000..0747f28
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/debug.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright(c) 2011-2016 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 __GVT_DEBUG_H__
> +#define __GVT_DEBUG_H__
> +
> +#define gvt_info(fmt, args...) \
> +	DRM_INFO("gvt: "fmt"\n", ##args)
> +

Just;

	DRM_INFO("gvt: " fmt, ##args)

Do not automatically add newlines, it will confuse developers. Applies
to all printing.

> +#define gvt_err(fmt, args...) \
> +	DRM_ERROR("gvt: "fmt"\n", ##args)
> +

Same here.

> +#define gvt_warn(condition, fmt, args...) \
> +	WARN((condition), "gvt: "fmt"\n", ##args)
> +
> +#define gvt_warn_once(condition, fmt, args...) \
> +	WARN_ONCE((condition), "gvt: "fmt"\n", ##args)

WARN and WARN_ONCE will include backtrace so prefixing is unnecessary.
I would not prefix them at all. Just use what i915 kernel module
already uses. If needed, split them to their own file first,
i915_debug.h.
> +
> +#define gvt_dbg(level, fmt, args...) \
> +	DRM_DEBUG_DRIVER("gvt: "fmt"\n", ##args)
> +
> +enum {
> +	GVT_DBG_CORE = (1 << 0),
> +	GVT_DBG_MM = (1 << 1),
> +	GVT_DBG_IRQ = (1 << 2),
> +};

This enum is not of use.

> +
> +#define gvt_dbg_core(fmt, args...) \
> +	gvt_dbg(GVT_DBG_CORE, fmt, ##args)
> +

Rahter like this (not to lose the topic)?
	gvt_dbg("core: " fmt, ##args)

> +#define gvt_dbg_mm(fmt, args...) \
> +	gvt_dbg(GVT_DBG_MM, fmt, ##args)
> +
> +#define gvt_dbg_irq(fmt, args...) \
> +	gvt_dbg(GVT_DBG_IRQ, fmt, ##args)
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> new file mode 100644
> index 0000000..52cfa32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -0,0 +1,393 @@
> +/*
> + * Copyright(c) 2011-2016 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 
> +#include 
> +#include 
> +
> +#include "gvt.h"
> +
> +struct gvt_host gvt_host;
> +
> +static const char * const supported_hypervisors[] = {
> +	[GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
> +	[GVT_HYPERVISOR_TYPE_KVM] = "KVM",
> +};
> +
> +static int gvt_init_host(void)
> +{
> +	struct gvt_host *host = &gvt_host;
> +

Is it really that much more to write gvt_host.initialized? Counting the
"->" vs "." it's three letters...

> +	if (!gvt.enable) {
> +		gvt_dbg_core("GVT-g has been disabled by kernel parameter");
> +		return -EINVAL;
> +	}
> +
> +	if (host->initialized) {
> +		gvt_err("GVT-g has already been initialized!");
> +		return -EINVAL;
> +	}
> +
> +	/* Xen DOM U */
> +	if (xen_domain() && !xen_initial_domain())
> +		return -ENODEV;
> +
> +	if (xen_initial_domain()) {
> +		/* Xen Dom0 */
> +		host->kdm = try_then_request_module(
> +				symbol_get(xengt_kdm), "xengt");
> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
> +	} else {
> +		/* not in Xen. Try KVMGT */
> +		host->kdm = try_then_request_module(
> +				symbol_get(kvmgt_kdm), "kvm");
> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
> +	}
> +
> +	if (!host->kdm)
> +		return -EINVAL;

I think this error should be reported, to aid detecting problems in
module loading.

> +
> +	if (!hypervisor_detect_host())
> +		return -ENODEV;
> +

This func should be prefixed gvt_, as it is not local to this file.

> +	gvt_info("Running with hypervisor %s in host mode",
> +			supported_hypervisors[host->hypervisor_type]);
> +
> +	idr_init(&host->device_idr);
> +	mutex_init(&host->device_idr_lock);
> +
> +	host->initialized = true;
> +	return 0;
> +}
> +
> +static int init_device_info(struct pgt_device *pdev)
> +{
> +	struct gvt_device_info *info = &pdev->device_info;
> +
> +	if (!IS_BROADWELL(pdev->dev_priv)) {
> +		DRM_DEBUG_DRIVER("Unsupported GEN device");
> +		return -ENODEV;
> +	}

This could be "else" clause on the next if and will allow easier adding
of future platforms.

> +
> +	if (IS_BROADWELL(pdev->dev_priv)) {
> +		info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
> +		/*
> +		 * The layout of BAR0 in BDW:
> +		 * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->|
> +		 *
> +		 * GTT offset in BAR0 starts from 8MB to 16MB, and
> +		 * Whatever GTT size is configured in BIOS,
> +		 * the size of BAR0 is always 16MB. The actual configured
> +		 * GTT size can be found in GMCH_CTRL.
> +		 */
> +		info->gtt_start_offset = (1UL << 23); /* 8MB */
> +		info->max_gtt_size = (1UL << 23); /* 8MB */
> +		info->gtt_entry_size = 8;
> +		info->gtt_entry_size_shift = 3;
> +		info->gmadr_bytes_in_cmd = 8;
> +		info->mmio_size = 2 * 1024 * 1024; /* 2MB */
> +	}
> +
> +	return 0;
> +}
> +
> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
> +{
> +	struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
> +	int i;
> +
> +	gvt_dbg_core("init initial cfg space, id %d", pdev->id);
> +
> +	for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
> +		pci_read_config_dword(pci_dev, i,
> +				(u32 *)&pdev->initial_cfg_space[i]);
> +
> +	for (i = 0; i < GVT_BAR_NUM; i++) {
> +		pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
> +		gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
> +	}
> +}
> +
> +static void clean_initial_mmio_state(struct pgt_device *pdev)
> +{
> +	if (pdev->gttmmio_va) {
> +		iounmap(pdev->gttmmio_va);
> +		pdev->gttmmio_va = NULL;
> +	}
> +
> +	if (pdev->gmadr_va) {
> +		iounmap(pdev->gmadr_va);
> +		pdev->gmadr_va = NULL;
> +	}
> +}
> +
> +static int init_initial_mmio_state(struct pgt_device *pdev)
> +{
> +	struct gvt_device_info *info = &pdev->device_info;
> +
> +	u64 bar0, bar1;
> +	int rc;
> +
> +	gvt_dbg_core("init initial mmio state, id %d", pdev->id);
> +
> +	bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
> +	bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
> +
> +	pdev->gttmmio_base = bar0 & ~0xf;
> +	pdev->reg_num = info->mmio_size / 4;
> +	pdev->gmadr_base = bar1 & ~0xf;

Magic numbers still.

> +
> +	pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
> +	if (!pdev->gttmmio_va) {
> +		gvt_err("fail to map GTTMMIO BAR.");
> +		return -EFAULT;
> +	}
> +
> +	pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
> +	if (!pdev->gmadr_va) {
> +		gvt_err("fail to map GMADR BAR.");
> +		rc = -EFAULT;
> +		goto err;
> +	}
> +
> +	gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
> +	gvt_dbg_core("mmio size: %x", pdev->mmio_size);
> +	gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
> +			pdev->gmadr_base);
> +	gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
> +	gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
> +
> +	return 0;
> +err:
> +	clean_initial_mmio_state(pdev);
> +	return rc;
> +}
> +
> +static int gvt_service_thread(void *data)
> +{
> +	struct pgt_device *pdev = (struct pgt_device *)data;
> +	int r;
> +
> +	gvt_dbg_core("service thread start, pgt %d", pdev->id);
> +
> +	while (!kthread_should_stop()) {
> +		r = wait_event_interruptible(pdev->service_thread_wq,
> +				kthread_should_stop() || pdev->service_request);
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (gvt_warn_once(r,
> +			"service thread is waken up by unexpected signal."))
> +			continue;
> +	}
> +
> +	return 0;
> +}
> +
> +static void clean_service_thread(struct pgt_device *pdev)
> +{
> +	if (pdev->service_thread) {
> +		kthread_stop(pdev->service_thread);
> +		pdev->service_thread = NULL;
> +	}
> +}
> +
> +static int init_service_thread(struct pgt_device *pdev)
> +{
> +	init_waitqueue_head(&pdev->service_thread_wq);
> +
> +	pdev->service_thread = kthread_run(gvt_service_thread,
> +			pdev, "gvt_service_thread%d", pdev->id);
> +
> +	if (!pdev->service_thread) {
> +		gvt_err("fail to start service thread.");
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}
> +
> +static void clean_pgt_device(struct pgt_device *pdev)
> +{
> +	clean_service_thread(pdev);
> +	clean_initial_mmio_state(pdev);
> +}
> +
> +static int init_pgt_device(struct pgt_device *pdev,
> +	struct drm_i915_private *dev_priv)
> +{
> +	int rc;

	int ret;

> +
> +	rc = init_device_info(pdev);
> +	if (rc)
> +		return rc;
> +
> +	init_initial_cfg_space_state(pdev);
> +
> +	rc = init_initial_mmio_state(pdev);
> +	if (rc)
> +		goto err;
> +
> +	rc = init_service_thread(pdev);
> +	if (rc)
> +		goto err;
> +
> +	return 0;
> +err:
> +	clean_pgt_device(pdev);

Add teardown path, see below.

> +	return rc;
> +}
> +
> +static int post_init_pgt_device(struct pgt_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static void free_pgt_device(struct pgt_device *pdev)
> +{
> +	struct gvt_host *host = &gvt_host;
> +
> +	mutex_lock(&host->device_idr_lock);
> +	idr_remove(&host->device_idr, pdev->id);
> +	mutex_unlock(&host->device_idr_lock);
> +
> +	vfree(pdev);
> +}
> +
> +static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
> +{
> +	struct gvt_host *host = &gvt_host;
> +	struct pgt_device *pdev = NULL;
> +
> +	pdev = vzalloc(sizeof(*pdev));
> +	if (!pdev)
> +		return NULL;

This is a memory error, would like to see this propagated up as

	return ERR_PTR(-ENOMEM);

> +
> +	mutex_lock(&host->device_idr_lock);
> +	pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
> +	mutex_unlock(&host->device_idr_lock);
> +
> +	if (pdev->id < 0) {
> +		gvt_err("fail to allocate pgt device id.");
> +		goto err;
> +	}
> +

Same here, propagate the error.

> +	mutex_init(&pdev->lock);
> +	pdev->dev_priv = dev_priv;
> +	idr_init(&pdev->instance_idr);
> +
> +	return pdev;
> +err:
> +	free_pgt_device(pdev);

I'd like to see a teardown path here. Then free_pgt_device() (or rather
destroy_pgt_device() can expect everything to be initialized and when
it it is called, it doesn't need ifs. This makes the driver code more
robust.

Or are we expecting only partially initialized structs for some reason?

> +	return NULL;
> +}
> +
> +void gvt_destroy_pgt_device(void *private_data)
> +{
> +	struct pgt_device *pdev = (struct pgt_device *)private_data;
> +
> +	clean_pgt_device(pdev);
> +	free_pgt_device(pdev);

Why multiple calls to destroy a device, there's only one alloc still?

> +}
> +
> +/**
> + * gvt_create_pgt_device - create a GVT physical device
> + * @dev: drm device
> + *
> + * This function is called at the initialization stage, to create a physical
> + * GVT device and initialize necessary GVT components for it.
> + *
> + * Returns:
> + * pointer to the pgt_device structure, NULL if failed.
> + */
> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
> +{
> +	struct pgt_device *pdev = NULL;
> +	struct gvt_host *host = &gvt_host;
> +	int rc;
> +
> +	if (!host->initialized && !gvt_init_host()) {
> +		gvt_err("gvt_init_host fail");
> +		return NULL;
> +	}
> +
> +	gvt_dbg_core("create new pgt device, i915 dev_priv: %p", dev_priv);
> +
> +	pdev = alloc_pgt_device(dev_priv);
> +	if (!pdev) {
> +		gvt_err("fail to allocate memory for pgt device.");
> +		goto err;
> +	}
> +
> +	gvt_dbg_core("init pgt device, id %d", pdev->id);
> +
> +	rc = init_pgt_device(pdev, dev_priv);
> +	if (rc) {

Just call the return value variable ret like everywhere.

> +		gvt_err("fail to init physical device state.");
> +		goto err;
> +	}
> +
> +	gvt_dbg_core("pgt device creation done, id %d", pdev->id);
> +
> +	return pdev;
> +err:
> +	if (pdev) {
> +		gvt_destroy_pgt_device(pdev);
> +		pdev = NULL;
> +	}

Proper goto label based teardown path should be used.

err_destroy_pgt:
	gvt_destroy_pgt_device(pdev);
err:
> +	return NULL;
> +}
> +
> +/**
> + * gvt_post_init_pgt_device - post init a GVT physical device
> + * @dev: drm device

Double check the kerneldocs to be correct per arguments of function.

> + *
> + * This function is called at the end of the initialization stage, to
> + * post-initialize a physical GVT device and initialize necessary
> + * GVT components rely on i915 components.
> + *
> + * Returns:
> + * zero on success, non-zero if failed.
> + */
> +int gvt_post_init_pgt_device(void *private_data)
> +{
> +	struct pgt_device *pdev = (struct pgt_device *)private_data;
> +	struct gvt_host *host = &gvt_host;
> +	int rc;
> +
> +	if (!host->initialized) {
> +		gvt_err("gvt_host haven't been initialized.");
> +		return -ENODEV;
> +	}
> +
> +	gvt_dbg_core("post init pgt device %d", pdev->id);
> +
> +	rc = post_init_pgt_device(pdev);
> +	if (rc) {
> +		gvt_err("fail to post init physical device state.");
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> new file mode 100644
> index 0000000..d450198
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_H_
> +#define _GVT_H_
> +
> +#include "i915_drv.h"
> +#include "i915_vgpu.h"
> +
> +#include "debug.h"
> +#include "params.h"
> +#include "reg.h"
> +#include "hypercall.h"
> +#include "mpt.h"
> +
> +#define GVT_MAX_VGPU 8
> +
> +enum {
> +	GVT_HYPERVISOR_TYPE_XEN = 0,
> +	GVT_HYPERVISOR_TYPE_KVM,
> +};
> +
> +struct gvt_host {
> +	bool initialized;
> +	int hypervisor_type;
> +	struct mutex device_idr_lock;
> +	struct idr device_idr;
> +	struct gvt_kernel_dm *kdm;
> +};
> +
> +extern struct gvt_host gvt_host;

-->

> +extern struct gvt_kernel_dm xengt_kdm;
> +extern struct gvt_kernel_dm kvmgt_kdm;

<-- These should likely be declared somewhere in include/ rather than
here.

> +
> +/* Describe the limitation of HW.*/
> +struct gvt_device_info {
> +	u64 max_gtt_gm_sz;
> +	u32 gtt_start_offset;
> +	u32 gtt_end_offset;
> +	u32 max_gtt_size;
> +	u32 gtt_entry_size;
> +	u32 gtt_entry_size_shift;
> +	u32 gmadr_bytes_in_cmd;
> +	u32 mmio_size;
> +};
> +
> +struct vgt_device {
> +	int id;
> +	int vm_id;
> +	struct pgt_device *pdev;
> +	bool warn_untrack;
> +};
> +
> +struct pgt_device {

Comments to this and other structs about what the memebers are.

> +	struct mutex lock;
> +	int id;
> +
> +	struct drm_i915_private *dev_priv;
> +	struct idr instance_idr;
> +
> +	struct gvt_device_info device_info;
> +
> +	u8 initial_cfg_space[GVT_CFG_SPACE_SZ];
> +	u64 bar_size[GVT_BAR_NUM];
> +
> +	u64 gttmmio_base;
> +	void *gttmmio_va;
> +
> +	u64 gmadr_base;
> +	void *gmadr_va;
> +
> +	u32 mmio_size;
> +	u32 reg_num;
> +
> +	wait_queue_head_t service_thread_wq;
> +	struct task_struct *service_thread;
> +	unsigned long service_request;
> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> new file mode 100644
> index 0000000..0a41874
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_HYPERCALL_H_
> +#define _GVT_HYPERCALL_H_
> +
> +struct gvt_kernel_dm {
> +};

I would prefer more code introduced here in the initial patch, or this
can be introduced later in the series as whole.

It unnecessarily complicates the review if some files and calls are
introduced with no documentation and implementation and only later
their implementation is added.

I can't really review if using a structure is a good idea if I can't
see the context or implementation of their use.

> +
> +#endif /* _GVT_HYPERCALL_H_ */
> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> new file mode 100644
> index 0000000..e594bb8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_MPT_H_
> +#define _GVT_MPT_H_
> +
> +struct vgt_device;
> +
> +static inline bool hypervisor_detect_host(void)
> +{
> +	return false;
> +}

This is also not very reviewable and there's an unnecessary forward
declaration.

> +
> +#endif /* _GVT_MPT_H_ */
> diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
> new file mode 100644
> index 0000000..d381d17
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/params.c
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright(c) 2011-2016 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 "gvt.h"
> +
> +struct gvt_kernel_params gvt = {
> +	.enable = false,
> +	.debug = 0,
> +};
> +
> +module_param_named(gvt_enable, gvt.enable, bool, 0600);

This should just be

	module_param_named(enable, gvt.enable, bool, ...)

otherwise parameter has to be passed at boot time like this:
	
	gvt.gvt_enable=1

Where we want

	gvt.enable=1

Right?

> +MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
> new file mode 100644
> index 0000000..d2955b9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/params.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_PARAMS_H_
> +#define _GVT_PARAMS_H_
> +
> +struct gvt_kernel_params {
> +	bool enable;
> +	int debug;

This member is unused currently, can be dropped.

> +};
> +
> +extern struct gvt_kernel_params gvt;
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/reg.h b/drivers/gpu/drm/i915/gvt/reg.h
> new file mode 100644
> index 0000000..d363b74
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/reg.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_REG_H
> +#define _GVT_REG_H
> +
> +#define GVT_CFG_SPACE_SZ	256
> +#define GVT_BAR_NUM		4
> +
> +#define GVT_REG_CFG_SPACE_BAR0	0x10
> +#define GVT_REG_CFG_SPACE_BAR1	0x18
> +#define GVT_REG_CFG_SPACE_BAR2	0x20

Some documentation here would be great.

> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 1c6d227..f3bed37 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -35,6 +35,7 @@
>  #include "intel_drv.h"
>  #include 
>  #include "i915_drv.h"
> +#include "i915_gvt.h"
>  #include "i915_vgpu.h"
>  #include "i915_trace.h"
>  #include 
> @@ -1045,6 +1046,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_uncore_init(dev);
>  
> +	ret = intel_gvt_init(dev);
> +	if (ret)
> +		goto out_uncore_fini;
> +
>  	ret = i915_gem_gtt_init(dev);
>  	if (ret)
>  		goto out_uncore_fini;

This needs to become "goto out_gvt_cleanup;"

> @@ -1135,6 +1140,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto out_power_well;
>  	}
>  
> +	ret = intel_gvt_post_init(dev);
> +	if (ret) {
> +		DRM_ERROR("failed to post init pgt device\n");
> +		goto out_power_well;
> +	}
> +
>  	/*
>  	 * Notify a valid surface after modesetting,
>  	 * when running inside a VM.
> @@ -1177,6 +1188,7 @@ out_gem_unload:
>  out_gtt:
>  	i915_global_gtt_cleanup(dev);

out_gvt_cleanup:

>  out_uncore_fini:
> 
> +	intel_gvt_cleanup(dev);

This needs to be lifted up to its own label ensure proper teardown if
i915_gem_gtt_init() fails.

>  	intel_uncore_fini(dev);
>  	i915_mmio_cleanup(dev);
>  put_bridge:
> @@ -1223,6 +1235,8 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	intel_modeset_cleanup(dev);
>  
> +	intel_gvt_cleanup(dev);
> +
>  	/*
>  	 * free the memory space allocated for the child device
>  	 * config parsed from VBT
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20d9dbd..2f897c3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1705,6 +1705,10 @@ struct i915_workarounds {
>  	u32 hw_whitelist_count[I915_NUM_RINGS];
>  };
>  
> +struct i915_gvt {
> +	void *pgt_device;
> +};
> +
>  struct i915_virtual_gpu {
>  	bool active;
>  };
> @@ -1744,6 +1748,8 @@ struct drm_i915_private {
>  
>  	struct i915_virtual_gpu vgpu;
>  
> +	struct i915_gvt gvt;
> +
>  	struct intel_guc guc;
>  
>  	struct intel_csr csr;
> @@ -2780,6 +2786,12 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv,
>  void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
>  					enum forcewake_domains domains);
>  void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
> +
> +static inline bool intel_gvt_active(struct drm_device *dev)
> +{
> +	return to_i915(dev)->gvt.pgt_device ? true : false;
> +}
> +
>  static inline bool intel_vgpu_active(struct drm_device *dev)
>  {
>  	return to_i915(dev)->vgpu.active;
> diff --git a/drivers/gpu/drm/i915/i915_gvt.c b/drivers/gpu/drm/i915/i915_gvt.c
> new file mode 100644
> index 0000000..3ca7232
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gvt.c
> @@ -0,0 +1,93 @@
> +/*
> + * Copyright(c) 2011-2016 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 "i915_drv.h"
> +#include "i915_gvt.h"
> +
> +/**
> + * DOC: Intel GVT-g host 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 the englightments
> + * of GVT and the necessary components used by GVT in i915 driver.
> + */
> +
> +/**
> + * intel_gvt_init - initialize GVT components at the beginning of i915
> + * driver loading.
> + * @dev: drm device *
> + *
> + * This function is called at the beginning of the initialization stage,
> + * to initialize the GVT components that have to be initialized
> + * before HW gets touched by other i915 components.
> + */
> +int intel_gvt_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	dev_priv->gvt.pgt_device = gvt_create_pgt_device(dev_priv);
> +	if (intel_gvt_active(dev))
> +		DRM_DEBUG_DRIVER("GVT-g is running in host mode\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_gvt_post_init - initialize GVT components at the end of i915
> + * driver loading.
> + * @dev: drm device *
> + *
> + * This function is called at the end of the initialization stage,
> + * to initialize the GVT components that have to be initialized after
> + * other i915 components are ready.
> + */
> +int intel_gvt_post_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!intel_gvt_active(dev))
> +		return 0;
> +
> +	return gvt_post_init_pgt_device(dev_priv->gvt.pgt_device);
> +}
> +
> +/**
> + * intel_gvt_cleanup - cleanup GVT components when i915 driver is unloading
> + * @dev: drm device *
> + *
> + * This function is called at the i915 driver unloading stage, to shutdown
> + * GVT components and release the related resources.
> + */
> +void intel_gvt_cleanup(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!intel_gvt_active(dev))
> +		return;
> +
> +	gvt_destroy_pgt_device(dev_priv->gvt.pgt_device);
> +	dev_priv->gvt.pgt_device = NULL;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gvt.h b/drivers/gpu/drm/i915/i915_gvt.h
> new file mode 100644
> index 0000000..30cc207
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gvt.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright(c) 2011-2016 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_GVT_H_
> +#define _I915_GVT_H_

I think this file could be intel_gvt.h (all remaining functions are
intel_gvt), and have respective intel_gvt.c file.

> +
> +#ifdef CONFIG_DRM_I915_GVT

Starting here --->

> +extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
> +extern int gvt_post_init_pgt_device(void *data);
> +extern void gvt_destroy_pgt_device(void *data);
> +

<-- to here, these should go to their own include file at
include/drm/i915_gvt.h For an example, see include/drm/i915_drm.h

The respective export symbols then need to be exported, For an example
see;

	EXPORT_SYMBOL_GPL(i915_gpu_raise);

> +extern int intel_gvt_init(struct drm_device *dev);
> +extern int intel_gvt_post_init(struct drm_device *dev);
> +extern void intel_gvt_cleanup(struct drm_device *dev);
> +#else
> +extern int intel_gvt_init(struct drm_device *dev)

These should, by convention, rather be static inline;

static inline int intel_gvt_init(...)

> +{
> +	return 0;
> +}
> +extern int intel_gvt_post_init(struct drm_device *dev)
> +{
> +	return 0;
> +}
> +extern void intel_gvt_cleanup(struct drm_device *dev)
> +{
> +}
> +#endif
> +
> +#endif /* _I915_GVT_H_ */
A one more note below.

On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
> This patch introduces the very basic framework of GVT-g device model,
> includes basic prototypes, definitions, initialization.
> 
> v2:
> - Introduce i915_gvt.c.
> It's necessary to introduce the stubs between i915 driver and GVT-g host,
> as GVT-g components is configurable in kernel config. When disabled, the
> stubs here do nothing.
> 
> Take Joonas's comments:
> - Replace boolean return value with int.
> - Replace customized info/warn/debug macros with DRM macros.
> - Document all non-static functions like i915.
> - Remove empty and unused functions.
> - Replace magic number with marcos.
> - Set GVT-g in kernel config to "n" by default.
> 
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig         |  15 ++
>  drivers/gpu/drm/i915/Makefile        |   2 +
>  drivers/gpu/drm/i915/gvt/Makefile    |   5 +
>  drivers/gpu/drm/i915/gvt/debug.h     |  57 +++++
>  drivers/gpu/drm/i915/gvt/gvt.c       | 393 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.h       | 100 +++++++++
>  drivers/gpu/drm/i915/gvt/hypercall.h |  30 +++
>  drivers/gpu/drm/i915/gvt/mpt.h       |  34 +++
>  drivers/gpu/drm/i915/gvt/params.c    |  32 +++
>  drivers/gpu/drm/i915/gvt/params.h    |  34 +++
>  drivers/gpu/drm/i915/gvt/reg.h       |  34 +++
>  drivers/gpu/drm/i915/i915_dma.c      |  14 ++
>  drivers/gpu/drm/i915/i915_drv.h      |  12 ++
>  drivers/gpu/drm/i915/i915_gvt.c      |  93 +++++++++
>  drivers/gpu/drm/i915/i915_gvt.h      |  49 +++++
>  15 files changed, 904 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/gvt/Makefile
>  create mode 100644 drivers/gpu/drm/i915/gvt/debug.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/params.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/params.h
>  create mode 100644 drivers/gpu/drm/i915/gvt/reg.h
>  create mode 100644 drivers/gpu/drm/i915/i915_gvt.c
>  create mode 100644 drivers/gpu/drm/i915/i915_gvt.h
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 4c59793..082e77d 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -45,3 +45,18 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
>  	  option changes the default for that module option.
>  
>  	  If in doubt, say "N".
> +
> +config DRM_I915_GVT
> +        tristate "GVT-g host driver"
> +        depends on DRM_I915
> +        default n
> +        help
> +          Enabling GVT-g mediated graphics passthrough technique for Intel i915
> +          based integrated graphics card. With GVT-g, it's possible to have one
> +          integrated i915 device shared by multiple VMs. Performance critical
> +          opterations such as apperture accesses and ring buffer operations
> +          are pass-throughed to VM, with a minimal set of conflicting resources
> +          (e.g. display settings) mediated by vGT driver. The benefit of vGT
> +          is on both the performance, given that each VM could directly operate
> +          its aperture space and submit commands like running on native, and
> +          the feature completeness, given that a true GEN hardware is exposed.
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0851de07..c65026c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -91,6 +91,8 @@ i915-y += dvo_ch7017.o \
>  	  intel_sdvo.o \
>  	  intel_tv.o
>  
> +obj-$(CONFIG_DRM_I915_GVT)  += i915_gvt.o gvt/
> +
>  # virtual gpu code
>  i915-y += i915_vgpu.o
>  
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
> new file mode 100644
> index 0000000..959305f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -0,0 +1,5 @@
> +GVT_SOURCE := gvt.o params.o
> +
> +ccflags-y                      += -I$(src) -I$(src)/.. -Wall -Werror -Wno-unused-function
> +i915_gvt-y                     := $(GVT_SOURCE)

(This name conflicts with upper level i915_gvt, which I suggested
renaming to intel_gvt.c. A one more reason more so this can be kept as
is).

As the module will be called i915_gvt, I bet the debug prefix should be
changed to reflect that.

So it should look like;

${WHATEVER_DRM_PRINTS} i915-gvt: core: Core debug
${WHATEVER_DRM_PRINTS} i915-gvt: mm: Memory debug

Regards, Joonas

> +obj-$(CONFIG_DRM_I915_GVT)     += i915_gvt.o
> diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
> new file mode 100644
> index 0000000..0747f28
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/debug.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright(c) 2011-2016 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 __GVT_DEBUG_H__
> +#define __GVT_DEBUG_H__
> +
> +#define gvt_info(fmt, args...) \
> +	DRM_INFO("gvt: "fmt"\n", ##args)
> +
> +#define gvt_err(fmt, args...) \
> +	DRM_ERROR("gvt: "fmt"\n", ##args)
> +
> +#define gvt_warn(condition, fmt, args...) \
> +	WARN((condition), "gvt: "fmt"\n", ##args)
> +
> +#define gvt_warn_once(condition, fmt, args...) \
> +	WARN_ONCE((condition), "gvt: "fmt"\n", ##args)
> +
> +#define gvt_dbg(level, fmt, args...) \
> +	DRM_DEBUG_DRIVER("gvt: "fmt"\n", ##args)
> +
> +enum {
> +	GVT_DBG_CORE = (1 << 0),
> +	GVT_DBG_MM = (1 << 1),
> +	GVT_DBG_IRQ = (1 << 2),
> +};
> +
> +#define gvt_dbg_core(fmt, args...) \
> +	gvt_dbg(GVT_DBG_CORE, fmt, ##args)
> +
> +#define gvt_dbg_mm(fmt, args...) \
> +	gvt_dbg(GVT_DBG_MM, fmt, ##args)
> +
> +#define gvt_dbg_irq(fmt, args...) \
> +	gvt_dbg(GVT_DBG_IRQ, fmt, ##args)
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> new file mode 100644
> index 0000000..52cfa32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -0,0 +1,393 @@
> +/*
> + * Copyright(c) 2011-2016 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 
> +#include 
> +#include 
> +
> +#include "gvt.h"
> +
> +struct gvt_host gvt_host;
> +
> +static const char * const supported_hypervisors[] = {
> +	[GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
> +	[GVT_HYPERVISOR_TYPE_KVM] = "KVM",
> +};
> +
> +static int gvt_init_host(void)
> +{
> +	struct gvt_host *host = &gvt_host;
> +
> +	if (!gvt.enable) {
> +		gvt_dbg_core("GVT-g has been disabled by kernel parameter");
> +		return -EINVAL;
> +	}
> +
> +	if (host->initialized) {
> +		gvt_err("GVT-g has already been initialized!");
> +		return -EINVAL;
> +	}
> +
> +	/* Xen DOM U */
> +	if (xen_domain() && !xen_initial_domain())
> +		return -ENODEV;
> +
> +	if (xen_initial_domain()) {
> +		/* Xen Dom0 */
> +		host->kdm = try_then_request_module(
> +				symbol_get(xengt_kdm), "xengt");
> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
> +	} else {
> +		/* not in Xen. Try KVMGT */
> +		host->kdm = try_then_request_module(
> +				symbol_get(kvmgt_kdm), "kvm");
> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
> +	}
> +
> +	if (!host->kdm)
> +		return -EINVAL;
> +
> +	if (!hypervisor_detect_host())
> +		return -ENODEV;
> +
> +	gvt_info("Running with hypervisor %s in host mode",
> +			supported_hypervisors[host->hypervisor_type]);
> +
> +	idr_init(&host->device_idr);
> +	mutex_init(&host->device_idr_lock);
> +
> +	host->initialized = true;
> +	return 0;
> +}
> +
> +static int init_device_info(struct pgt_device *pdev)
> +{
> +	struct gvt_device_info *info = &pdev->device_info;
> +
> +	if (!IS_BROADWELL(pdev->dev_priv)) {
> +		DRM_DEBUG_DRIVER("Unsupported GEN device");
> +		return -ENODEV;
> +	}
> +
> +	if (IS_BROADWELL(pdev->dev_priv)) {
> +		info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
> +		/*
> +		 * The layout of BAR0 in BDW:
> +		 * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->|
> +		 *
> +		 * GTT offset in BAR0 starts from 8MB to 16MB, and
> +		 * Whatever GTT size is configured in BIOS,
> +		 * the size of BAR0 is always 16MB. The actual configured
> +		 * GTT size can be found in GMCH_CTRL.
> +		 */
> +		info->gtt_start_offset = (1UL << 23); /* 8MB */
> +		info->max_gtt_size = (1UL << 23); /* 8MB */
> +		info->gtt_entry_size = 8;
> +		info->gtt_entry_size_shift = 3;
> +		info->gmadr_bytes_in_cmd = 8;
> +		info->mmio_size = 2 * 1024 * 1024; /* 2MB */
> +	}
> +
> +	return 0;
> +}
> +
> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
> +{
> +	struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
> +	int i;
> +
> +	gvt_dbg_core("init initial cfg space, id %d", pdev->id);
> +
> +	for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
> +		pci_read_config_dword(pci_dev, i,
> +				(u32 *)&pdev->initial_cfg_space[i]);
> +
> +	for (i = 0; i < GVT_BAR_NUM; i++) {
> +		pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
> +		gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
> +	}
> +}
> +
> +static void clean_initial_mmio_state(struct pgt_device *pdev)
> +{
> +	if (pdev->gttmmio_va) {
> +		iounmap(pdev->gttmmio_va);
> +		pdev->gttmmio_va = NULL;
> +	}
> +
> +	if (pdev->gmadr_va) {
> +		iounmap(pdev->gmadr_va);
> +		pdev->gmadr_va = NULL;
> +	}
> +}
> +
> +static int init_initial_mmio_state(struct pgt_device *pdev)
> +{
> +	struct gvt_device_info *info = &pdev->device_info;
> +
> +	u64 bar0, bar1;
> +	int rc;
> +
> +	gvt_dbg_core("init initial mmio state, id %d", pdev->id);
> +
> +	bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
> +	bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
> +
> +	pdev->gttmmio_base = bar0 & ~0xf;
> +	pdev->reg_num = info->mmio_size / 4;
> +	pdev->gmadr_base = bar1 & ~0xf;
> +
> +	pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
> +	if (!pdev->gttmmio_va) {
> +		gvt_err("fail to map GTTMMIO BAR.");
> +		return -EFAULT;
> +	}
> +
> +	pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
> +	if (!pdev->gmadr_va) {
> +		gvt_err("fail to map GMADR BAR.");
> +		rc = -EFAULT;
> +		goto err;
> +	}
> +
> +	gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
> +	gvt_dbg_core("mmio size: %x", pdev->mmio_size);
> +	gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
> +			pdev->gmadr_base);
> +	gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
> +	gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
> +
> +	return 0;
> +err:
> +	clean_initial_mmio_state(pdev);
> +	return rc;
> +}
> +
> +static int gvt_service_thread(void *data)
> +{
> +	struct pgt_device *pdev = (struct pgt_device *)data;
> +	int r;
> +
> +	gvt_dbg_core("service thread start, pgt %d", pdev->id);
> +
> +	while (!kthread_should_stop()) {
> +		r = wait_event_interruptible(pdev->service_thread_wq,
> +				kthread_should_stop() || pdev->service_request);
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (gvt_warn_once(r,
> +			"service thread is waken up by unexpected signal."))
> +			continue;
> +	}
> +
> +	return 0;
> +}
> +
> +static void clean_service_thread(struct pgt_device *pdev)
> +{
> +	if (pdev->service_thread) {
> +		kthread_stop(pdev->service_thread);
> +		pdev->service_thread = NULL;
> +	}
> +}
> +
> +static int init_service_thread(struct pgt_device *pdev)
> +{
> +	init_waitqueue_head(&pdev->service_thread_wq);
> +
> +	pdev->service_thread = kthread_run(gvt_service_thread,
> +			pdev, "gvt_service_thread%d", pdev->id);
> +
> +	if (!pdev->service_thread) {
> +		gvt_err("fail to start service thread.");
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}
> +
> +static void clean_pgt_device(struct pgt_device *pdev)
> +{
> +	clean_service_thread(pdev);
> +	clean_initial_mmio_state(pdev);
> +}
> +
> +static int init_pgt_device(struct pgt_device *pdev,
> +	struct drm_i915_private *dev_priv)
> +{
> +	int rc;
> +
> +	rc = init_device_info(pdev);
> +	if (rc)
> +		return rc;
> +
> +	init_initial_cfg_space_state(pdev);
> +
> +	rc = init_initial_mmio_state(pdev);
> +	if (rc)
> +		goto err;
> +
> +	rc = init_service_thread(pdev);
> +	if (rc)
> +		goto err;
> +
> +	return 0;
> +err:
> +	clean_pgt_device(pdev);
> +	return rc;
> +}
> +
> +static int post_init_pgt_device(struct pgt_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static void free_pgt_device(struct pgt_device *pdev)
> +{
> +	struct gvt_host *host = &gvt_host;
> +
> +	mutex_lock(&host->device_idr_lock);
> +	idr_remove(&host->device_idr, pdev->id);
> +	mutex_unlock(&host->device_idr_lock);
> +
> +	vfree(pdev);
> +}
> +
> +static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
> +{
> +	struct gvt_host *host = &gvt_host;
> +	struct pgt_device *pdev = NULL;
> +
> +	pdev = vzalloc(sizeof(*pdev));
> +	if (!pdev)
> +		return NULL;
> +
> +	mutex_lock(&host->device_idr_lock);
> +	pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
> +	mutex_unlock(&host->device_idr_lock);
> +
> +	if (pdev->id < 0) {
> +		gvt_err("fail to allocate pgt device id.");
> +		goto err;
> +	}
> +
> +	mutex_init(&pdev->lock);
> +	pdev->dev_priv = dev_priv;
> +	idr_init(&pdev->instance_idr);
> +
> +	return pdev;
> +err:
> +	free_pgt_device(pdev);
> +	return NULL;
> +}
> +
> +void gvt_destroy_pgt_device(void *private_data)
> +{
> +	struct pgt_device *pdev = (struct pgt_device *)private_data;
> +
> +	clean_pgt_device(pdev);
> +	free_pgt_device(pdev);
> +}
> +
> +/**
> + * gvt_create_pgt_device - create a GVT physical device
> + * @dev: drm device
> + *
> + * This function is called at the initialization stage, to create a physical
> + * GVT device and initialize necessary GVT components for it.
> + *
> + * Returns:
> + * pointer to the pgt_device structure, NULL if failed.
> + */
> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
> +{
> +	struct pgt_device *pdev = NULL;
> +	struct gvt_host *host = &gvt_host;
> +	int rc;
> +
> +	if (!host->initialized && !gvt_init_host()) {
> +		gvt_err("gvt_init_host fail");
> +		return NULL;
> +	}
> +
> +	gvt_dbg_core("create new pgt device, i915 dev_priv: %p", dev_priv);
> +
> +	pdev = alloc_pgt_device(dev_priv);
> +	if (!pdev) {
> +		gvt_err("fail to allocate memory for pgt device.");
> +		goto err;
> +	}
> +
> +	gvt_dbg_core("init pgt device, id %d", pdev->id);
> +
> +	rc = init_pgt_device(pdev, dev_priv);
> +	if (rc) {
> +		gvt_err("fail to init physical device state.");
> +		goto err;
> +	}
> +
> +	gvt_dbg_core("pgt device creation done, id %d", pdev->id);
> +
> +	return pdev;
> +err:
> +	if (pdev) {
> +		gvt_destroy_pgt_device(pdev);
> +		pdev = NULL;
> +	}
> +	return NULL;
> +}
> +
> +/**
> + * gvt_post_init_pgt_device - post init a GVT physical device
> + * @dev: drm device
> + *
> + * This function is called at the end of the initialization stage, to
> + * post-initialize a physical GVT device and initialize necessary
> + * GVT components rely on i915 components.
> + *
> + * Returns:
> + * zero on success, non-zero if failed.
> + */
> +int gvt_post_init_pgt_device(void *private_data)
> +{
> +	struct pgt_device *pdev = (struct pgt_device *)private_data;
> +	struct gvt_host *host = &gvt_host;
> +	int rc;
> +
> +	if (!host->initialized) {
> +		gvt_err("gvt_host haven't been initialized.");
> +		return -ENODEV;
> +	}
> +
> +	gvt_dbg_core("post init pgt device %d", pdev->id);
> +
> +	rc = post_init_pgt_device(pdev);
> +	if (rc) {
> +		gvt_err("fail to post init physical device state.");
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> new file mode 100644
> index 0000000..d450198
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_H_
> +#define _GVT_H_
> +
> +#include "i915_drv.h"
> +#include "i915_vgpu.h"
> +
> +#include "debug.h"
> +#include "params.h"
> +#include "reg.h"
> +#include "hypercall.h"
> +#include "mpt.h"
> +
> +#define GVT_MAX_VGPU 8
> +
> +enum {
> +	GVT_HYPERVISOR_TYPE_XEN = 0,
> +	GVT_HYPERVISOR_TYPE_KVM,
> +};
> +
> +struct gvt_host {
> +	bool initialized;
> +	int hypervisor_type;
> +	struct mutex device_idr_lock;
> +	struct idr device_idr;
> +	struct gvt_kernel_dm *kdm;
> +};
> +
> +extern struct gvt_host gvt_host;
> +extern struct gvt_kernel_dm xengt_kdm;
> +extern struct gvt_kernel_dm kvmgt_kdm;
> +
> +/* Describe the limitation of HW.*/
> +struct gvt_device_info {
> +	u64 max_gtt_gm_sz;
> +	u32 gtt_start_offset;
> +	u32 gtt_end_offset;
> +	u32 max_gtt_size;
> +	u32 gtt_entry_size;
> +	u32 gtt_entry_size_shift;
> +	u32 gmadr_bytes_in_cmd;
> +	u32 mmio_size;
> +};
> +
> +struct vgt_device {
> +	int id;
> +	int vm_id;
> +	struct pgt_device *pdev;
> +	bool warn_untrack;
> +};
> +
> +struct pgt_device {
> +	struct mutex lock;
> +	int id;
> +
> +	struct drm_i915_private *dev_priv;
> +	struct idr instance_idr;
> +
> +	struct gvt_device_info device_info;
> +
> +	u8 initial_cfg_space[GVT_CFG_SPACE_SZ];
> +	u64 bar_size[GVT_BAR_NUM];
> +
> +	u64 gttmmio_base;
> +	void *gttmmio_va;
> +
> +	u64 gmadr_base;
> +	void *gmadr_va;
> +
> +	u32 mmio_size;
> +	u32 reg_num;
> +
> +	wait_queue_head_t service_thread_wq;
> +	struct task_struct *service_thread;
> +	unsigned long service_request;
> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> new file mode 100644
> index 0000000..0a41874
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_HYPERCALL_H_
> +#define _GVT_HYPERCALL_H_
> +
> +struct gvt_kernel_dm {
> +};
> +
> +#endif /* _GVT_HYPERCALL_H_ */
> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> new file mode 100644
> index 0000000..e594bb8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_MPT_H_
> +#define _GVT_MPT_H_
> +
> +struct vgt_device;
> +
> +static inline bool hypervisor_detect_host(void)
> +{
> +	return false;
> +}
> +
> +#endif /* _GVT_MPT_H_ */
> diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
> new file mode 100644
> index 0000000..d381d17
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/params.c
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright(c) 2011-2016 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 "gvt.h"
> +
> +struct gvt_kernel_params gvt = {
> +	.enable = false,
> +	.debug = 0,
> +};
> +
> +module_param_named(gvt_enable, gvt.enable, bool, 0600);
> +MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
> new file mode 100644
> index 0000000..d2955b9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/params.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_PARAMS_H_
> +#define _GVT_PARAMS_H_
> +
> +struct gvt_kernel_params {
> +	bool enable;
> +	int debug;
> +};
> +
> +extern struct gvt_kernel_params gvt;
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/reg.h b/drivers/gpu/drm/i915/gvt/reg.h
> new file mode 100644
> index 0000000..d363b74
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/reg.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 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 _GVT_REG_H
> +#define _GVT_REG_H
> +
> +#define GVT_CFG_SPACE_SZ	256
> +#define GVT_BAR_NUM		4
> +
> +#define GVT_REG_CFG_SPACE_BAR0	0x10
> +#define GVT_REG_CFG_SPACE_BAR1	0x18
> +#define GVT_REG_CFG_SPACE_BAR2	0x20
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 1c6d227..f3bed37 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -35,6 +35,7 @@
>  #include "intel_drv.h"
>  #include 
>  #include "i915_drv.h"
> +#include "i915_gvt.h"
>  #include "i915_vgpu.h"
>  #include "i915_trace.h"
>  #include 
> @@ -1045,6 +1046,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_uncore_init(dev);
>  
> +	ret = intel_gvt_init(dev);
> +	if (ret)
> +		goto out_uncore_fini;
> +
>  	ret = i915_gem_gtt_init(dev);
>  	if (ret)
>  		goto out_uncore_fini;
> @@ -1135,6 +1140,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto out_power_well;
>  	}
>  
> +	ret = intel_gvt_post_init(dev);
> +	if (ret) {
> +		DRM_ERROR("failed to post init pgt device\n");
> +		goto out_power_well;
> +	}
> +
>  	/*
>  	 * Notify a valid surface after modesetting,
>  	 * when running inside a VM.
> @@ -1177,6 +1188,7 @@ out_gem_unload:
>  out_gtt:
>  	i915_global_gtt_cleanup(dev);
>  out_uncore_fini:
> +	intel_gvt_cleanup(dev);
>  	intel_uncore_fini(dev);
>  	i915_mmio_cleanup(dev);
>  put_bridge:
> @@ -1223,6 +1235,8 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	intel_modeset_cleanup(dev);
>  
> +	intel_gvt_cleanup(dev);
> +
>  	/*
>  	 * free the memory space allocated for the child device
>  	 * config parsed from VBT
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20d9dbd..2f897c3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1705,6 +1705,10 @@ struct i915_workarounds {
>  	u32 hw_whitelist_count[I915_NUM_RINGS];
>  };
>  
> +struct i915_gvt {
> +	void *pgt_device;
> +};
> +
>  struct i915_virtual_gpu {
>  	bool active;
>  };
> @@ -1744,6 +1748,8 @@ struct drm_i915_private {
>  
>  	struct i915_virtual_gpu vgpu;
>  
> +	struct i915_gvt gvt;
> +
>  	struct intel_guc guc;
>  
>  	struct intel_csr csr;
> @@ -2780,6 +2786,12 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv,
>  void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
>  					enum forcewake_domains domains);
>  void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
> +
> +static inline bool intel_gvt_active(struct drm_device *dev)
> +{
> +	return to_i915(dev)->gvt.pgt_device ? true : false;
> +}
> +
>  static inline bool intel_vgpu_active(struct drm_device *dev)
>  {
>  	return to_i915(dev)->vgpu.active;
> diff --git a/drivers/gpu/drm/i915/i915_gvt.c b/drivers/gpu/drm/i915/i915_gvt.c
> new file mode 100644
> index 0000000..3ca7232
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gvt.c
> @@ -0,0 +1,93 @@
> +/*
> + * Copyright(c) 2011-2016 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 "i915_drv.h"
> +#include "i915_gvt.h"
> +
> +/**
> + * DOC: Intel GVT-g host 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 the englightments
> + * of GVT and the necessary components used by GVT in i915 driver.
> + */
> +
> +/**
> + * intel_gvt_init - initialize GVT components at the beginning of i915
> + * driver loading.
> + * @dev: drm device *
> + *
> + * This function is called at the beginning of the initialization stage,
> + * to initialize the GVT components that have to be initialized
> + * before HW gets touched by other i915 components.
> + */
> +int intel_gvt_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	dev_priv->gvt.pgt_device = gvt_create_pgt_device(dev_priv);
> +	if (intel_gvt_active(dev))
> +		DRM_DEBUG_DRIVER("GVT-g is running in host mode\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_gvt_post_init - initialize GVT components at the end of i915
> + * driver loading.
> + * @dev: drm device *
> + *
> + * This function is called at the end of the initialization stage,
> + * to initialize the GVT components that have to be initialized after
> + * other i915 components are ready.
> + */
> +int intel_gvt_post_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!intel_gvt_active(dev))
> +		return 0;
> +
> +	return gvt_post_init_pgt_device(dev_priv->gvt.pgt_device);
> +}
> +
> +/**
> + * intel_gvt_cleanup - cleanup GVT components when i915 driver is unloading
> + * @dev: drm device *
> + *
> + * This function is called at the i915 driver unloading stage, to shutdown
> + * GVT components and release the related resources.
> + */
> +void intel_gvt_cleanup(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!intel_gvt_active(dev))
> +		return;
> +
> +	gvt_destroy_pgt_device(dev_priv->gvt.pgt_device);
> +	dev_priv->gvt.pgt_device = NULL;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gvt.h b/drivers/gpu/drm/i915/i915_gvt.h
> new file mode 100644
> index 0000000..30cc207
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gvt.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright(c) 2011-2016 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_GVT_H_
> +#define _I915_GVT_H_
> +
> +#ifdef CONFIG_DRM_I915_GVT
> +extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
> +extern int gvt_post_init_pgt_device(void *data);
> +extern void gvt_destroy_pgt_device(void *data);
> +
> +extern int intel_gvt_init(struct drm_device *dev);
> +extern int intel_gvt_post_init(struct drm_device *dev);
> +extern void intel_gvt_cleanup(struct drm_device *dev);
> +#else
> +extern int intel_gvt_init(struct drm_device *dev)
> +{
> +	return 0;
> +}
> +extern int intel_gvt_post_init(struct drm_device *dev)
> +{
> +	return 0;
> +}
> +extern void intel_gvt_cleanup(struct drm_device *dev)
> +{
> +}
> +#endif
> +
> +#endif /* _I915_GVT_H_ */
On ke, 2016-02-24 at 07:45 +0000, Tian, Kevin wrote:
> > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]
> > Sent: Tuesday, February 23, 2016 8:42 PM
> > 
> > Hi,
> > 
> > Code is looking a lot better.
> > 
> > A general question still; why you seem to be preferring multi-stage
> > alloc and destroy?
> 
> One reason for multi-stage, IMO, is that GVT needs to do a snapshot
> of initial MMIO state before i915 driver does actual initialization. That
> snapshot will be presented to each VM which can then observe same
> state as it would observe on a bare metal. Then the majority of
> initialization needs to wait after i915 driver completes initialization. 
> Zhi can correct me here.
> 

Sorry for being unclear, I was referring to pgt_device initialization.

Regards, Joonas

> Thanks
> Kevin
On 02/24/16 16:08, Tian, Kevin wrote:
>> From: Wang, Zhi A
>> Sent: Thursday, February 18, 2016 7:42 PM
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
>> new file mode 100644
>> index 0000000..52cfa32
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> [...]
>> +
>> +#include <linux/types.h>
>> +#include <xen/xen.h>
>
> would this inclusion lead to a dependency on Xen?
>
>> +#include <linux/kthread.h>
>> +
>> +#include "gvt.h"
>> +
>> +struct gvt_host gvt_host;
>> +
>> +static const char * const supported_hypervisors[] = {
>> +	[GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
>> +	[GVT_HYPERVISOR_TYPE_KVM] = "KVM",
>> +};
>> +
>> +static int gvt_init_host(void)
>> +{
>> +	struct gvt_host *host = &gvt_host;
>> +
>> +	if (!gvt.enable) {
>> +		gvt_dbg_core("GVT-g has been disabled by kernel parameter");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (host->initialized) {
>> +		gvt_err("GVT-g has already been initialized!");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Xen DOM U */
>> +	if (xen_domain() && !xen_initial_domain())
>> +		return -ENODEV;
>> +
>> +	if (xen_initial_domain()) {
>> +		/* Xen Dom0 */
>> +		host->kdm = try_then_request_module(
>> +				symbol_get(xengt_kdm), "xengt");
>> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
>> +	} else {
>> +		/* not in Xen. Try KVMGT */
>> +		host->kdm = try_then_request_module(
>> +				symbol_get(kvmgt_kdm), "kvm");
>> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
>> +	}
>
> It'd be clearer to add a comment here that above is only a short-term
> solution. It's supposed to have a general registration framework in
> the future so any hypervisor (Xen or KVM) can register their callbacks
> at run-time, then we'll not need such direct Xen/KVM references or
> hard assumption in driver code. That framework is now still under
> discussion with Xen/KVM community. It doesn't prevent reviewing of
> other bits, but better to document it clear here.
>
I'm keeping thinking if we should split this patch into much smaller 
patches and just push some basic definitions and functions for GVT 
context patch review here before MPT framework is finally figured out 
with RH guys?

>> +static int init_device_info(struct pgt_device *pdev)
>> +{
>> +	struct gvt_device_info *info = &pdev->device_info;
>> +
>> +	if (!IS_BROADWELL(pdev->dev_priv)) {
>> +		DRM_DEBUG_DRIVER("Unsupported GEN device");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (IS_BROADWELL(pdev->dev_priv)) {
>> +		info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
>> +		/*
>> +		 * The layout of BAR0 in BDW:
>> +		 * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->|
>> +		 *
>> +		 * GTT offset in BAR0 starts from 8MB to 16MB, and
>> +		 * Whatever GTT size is configured in BIOS,
>> +		 * the size of BAR0 is always 16MB. The actual configured
>> +		 * GTT size can be found in GMCH_CTRL.
>> +		 */
>> +		info->gtt_start_offset = (1UL << 23); /* 8MB */
>> +		info->max_gtt_size = (1UL << 23); /* 8MB */
>> +		info->gtt_entry_size = 8;
>> +		info->gtt_entry_size_shift = 3;
>> +		info->gmadr_bytes_in_cmd = 8;
>> +		info->mmio_size = 2 * 1024 * 1024; /* 2MB */
>
> Above are pure device info. Joonas, do you think whether it makes
> sense to make them to drm_i915_private, though gvt is the only
> user today?
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
>
> 'init' -> 'setup'
>
>> +{
>> +	struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
>> +	int i;
>> +
>> +	gvt_dbg_core("init initial cfg space, id %d", pdev->id);
>> +
>> +	for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
>> +		pci_read_config_dword(pci_dev, i,
>> +				(u32 *)&pdev->initial_cfg_space[i]);
>> +
>> +	for (i = 0; i < GVT_BAR_NUM; i++) {
>> +		pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
>> +		gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
>> +	}
>> +}
>> +
>> +static void clean_initial_mmio_state(struct pgt_device *pdev)
>> +{
>> +	if (pdev->gttmmio_va) {
>> +		iounmap(pdev->gttmmio_va);
>> +		pdev->gttmmio_va = NULL;
>> +	}
>> +
>> +	if (pdev->gmadr_va) {
>> +		iounmap(pdev->gmadr_va);
>> +		pdev->gmadr_va = NULL;
>> +	}
>
> Can we reuse existing mapping in i915?
>
Yes, but we have to flush the tlb stuffs like i915, as i915 maps GTT 
MMIOs as WC...

>> +}
>> +
>> +static int init_initial_mmio_state(struct pgt_device *pdev)
>> +{
>
> 'init' -> 'setup'
>
>> +	struct gvt_device_info *info = &pdev->device_info;
>> +
>> +	u64 bar0, bar1;
>> +	int rc;
>> +
>> +	gvt_dbg_core("init initial mmio state, id %d", pdev->id);
>> +
>> +	bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
>> +	bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
>> +
>> +	pdev->gttmmio_base = bar0 & ~0xf;
>> +	pdev->reg_num = info->mmio_size / 4;
>> +	pdev->gmadr_base = bar1 & ~0xf;
>> +
>> +	pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
>> +	if (!pdev->gttmmio_va) {
>> +		gvt_err("fail to map GTTMMIO BAR.");
>> +		return -EFAULT;
>> +	}
>> +
>> +	pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
>> +	if (!pdev->gmadr_va) {
>> +		gvt_err("fail to map GMADR BAR.");
>> +		rc = -EFAULT;
>> +		goto err;
>> +	}
>> +
>> +	gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
>> +	gvt_dbg_core("mmio size: %x", pdev->mmio_size);
>> +	gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
>> +			pdev->gmadr_base);
>> +	gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
>> +	gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
>> +
>
> since you called 'initial_mmio_state', suppose we should do a MMIO snapshot
> here.
>
Or we move these code into basic mmio emulation patch? :)
>   [...]
>> +
>> +static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
>> +{
>> +	struct gvt_host *host = &gvt_host;
>> +	struct pgt_device *pdev = NULL;
>> +
>> +	pdev = vzalloc(sizeof(*pdev));
>> +	if (!pdev)
>> +		return NULL;
>> +
>> +	mutex_lock(&host->device_idr_lock);
>> +	pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
>
> curious what such ID help here? We already have either dev_priv or
> pgt_device pointer passed around. Isn't it enough to mark a device?
>
This code piece comes from our pgt device list. currently seems there is 
no for_each_pgt_device() requirement, will remove it in the next version
> [...]
>> +
>> +/**
>> + * gvt_create_pgt_device - create a GVT physical device
>> + * @dev: drm device
>> + *
>> + * This function is called at the initialization stage, to create a physical
>> + * GVT device and initialize necessary GVT components for it.
>> + *
>> + * Returns:
>> + * pointer to the pgt_device structure, NULL if failed.
>> + */
>> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
>
> should we remove 'pgt' completely? We can always use 'gvt_device'
> as reference to the object, and then above can be gvt_create_device
> or gvt_create_physical_device, or if 'create' is a bit misleading maybe
> gvt_initialize_device is cleaner?
>
OK. :)
> Thanks
> Kevin
>
Hi Joonas:

Thanks for you time and comments. :) See my replies below.

On 02/23/16 20:42, Joonas Lahtinen wrote:
> Hi,
>
> Code is looking a lot better.
>
> A general question still; why you seem to be preferring multi-stage
> alloc and destroy?
>
> Are there going to be scenarios when things will be allocated but not
> initialized? I don't see a such use scenario.
>
> I wouldn't split the init functions down as much as you currently do
> because that'll require a lot of boilerplate code to propagate the
> errors up, which is currently not done. The boilerplate for propagation
> becomes necessary when the teardown function is complex, but currently
> the teardown itself is less lines of code than the function
> boilerplate.
>
> So just squash those into gvt_device_create() and gvt_device_destroy()
> where _create() will propagate any lower level errors up and tear down
> a partially initialized struct. _destroy() can then expect to just tear
> the whole struct down with no ifs.
>
OK. Sure no problem.
> Regards, Joonas
>
> On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
>> This patch introduces the very basic framework of GVT-g device model,
>> includes basic prototypes, definitions, initialization.
>>
>> v2:
>> - Introduce i915_gvt.c.
>> It's necessary to introduce the stubs between i915 driver and GVT-g host,
>> as GVT-g components is configurable in kernel config. When disabled, the
>> stubs here do nothing.
>>
>> Take Joonas's comments:
>> - Replace boolean return value with int.
>> - Replace customized info/warn/debug macros with DRM macros.
>> - Document all non-static functions like i915.
>> - Remove empty and unused functions.
>> - Replace magic number with marcos.
>> - Set GVT-g in kernel config to "n" by default.
>>
>> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Kconfig         |  15 ++
>>   drivers/gpu/drm/i915/Makefile        |   2 +
>>   drivers/gpu/drm/i915/gvt/Makefile    |   5 +
>>   drivers/gpu/drm/i915/gvt/debug.h     |  57 +++++
>>   drivers/gpu/drm/i915/gvt/gvt.c       | 393 +++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gvt/gvt.h       | 100 +++++++++
>>   drivers/gpu/drm/i915/gvt/hypercall.h |  30 +++
>>   drivers/gpu/drm/i915/gvt/mpt.h       |  34 +++
>>   drivers/gpu/drm/i915/gvt/params.c    |  32 +++
>>   drivers/gpu/drm/i915/gvt/params.h    |  34 +++
>>   drivers/gpu/drm/i915/gvt/reg.h       |  34 +++
>>   drivers/gpu/drm/i915/i915_dma.c      |  14 ++
>>   drivers/gpu/drm/i915/i915_drv.h      |  12 ++
>>   drivers/gpu/drm/i915/i915_gvt.c      |  93 +++++++++
>>   drivers/gpu/drm/i915/i915_gvt.h      |  49 +++++
>>   15 files changed, 904 insertions(+)
>>   create mode 100644 drivers/gpu/drm/i915/gvt/Makefile
>>   create mode 100644 drivers/gpu/drm/i915/gvt/debug.h
>>   create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c
>>   create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h
>>   create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h
>>   create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h
>>   create mode 100644 drivers/gpu/drm/i915/gvt/params.c
>>   create mode 100644 drivers/gpu/drm/i915/gvt/params.h
>>   create mode 100644 drivers/gpu/drm/i915/gvt/reg.h
>>   create mode 100644 drivers/gpu/drm/i915/i915_gvt.c
>>   create mode 100644 drivers/gpu/drm/i915/i915_gvt.h
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index 4c59793..082e77d 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -45,3 +45,18 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
>>   	  option changes the default for that module option.
>>
>>   	  If in doubt, say "N".
>> +
>> +config DRM_I915_GVT
>> +        tristate "GVT-g host driver"
>> +        depends on DRM_I915
>> +        default n
>> +        help
>> +          Enabling GVT-g mediated graphics passthrough technique for Intel i915
>> +          based integrated graphics card. With GVT-g, it's possible to have one
>> +          integrated i915 device shared by multiple VMs. Performance critical
>> +          opterations such as apperture accesses and ring buffer operations
>> +          are pass-throughed to VM, with a minimal set of conflicting resources
>> +          (e.g. display settings) mediated by vGT driver. The benefit of vGT
>> +          is on both the performance, given that each VM could directly operate
>> +          its aperture space and submit commands like running on native, and
>> +          the feature completeness, given that a true GEN hardware is exposed.
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 0851de07..c65026c 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -91,6 +91,8 @@ i915-y += dvo_ch7017.o \
>>   	  intel_sdvo.o \
>>   	  intel_tv.o
>>
>> +obj-$(CONFIG_DRM_I915_GVT)  += i915_gvt.o gvt/
>> +
>>   # virtual gpu code
>>   i915-y += i915_vgpu.o
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
>> new file mode 100644
>> index 0000000..959305f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/Makefile
>> @@ -0,0 +1,5 @@
>> +GVT_SOURCE := gvt.o params.o
>> +
>> +ccflags-y                      += -I$(src) -I$(src)/.. -Wall -Werror -Wno-unused-function
>> +i915_gvt-y                     := $(GVT_SOURCE)
>> +obj-$(CONFIG_DRM_I915_GVT)     += i915_gvt.o
>> diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
>> new file mode 100644
>> index 0000000..0747f28
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/debug.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 __GVT_DEBUG_H__
>> +#define __GVT_DEBUG_H__
>> +
>> +#define gvt_info(fmt, args...) \
>> +	DRM_INFO("gvt: "fmt"\n", ##args)
>> +
>
> Just;
>
> 	DRM_INFO("gvt: " fmt, ##args)
>
> Do not automatically add newlines, it will confuse developers. Applies
> to all printing.
>
OK. Will improve that in the next version
>> +#define gvt_err(fmt, args...) \
>> +	DRM_ERROR("gvt: "fmt"\n", ##args)
>> +
>
> Same here.
>
>> +#define gvt_warn(condition, fmt, args...) \
>> +	WARN((condition), "gvt: "fmt"\n", ##args)
>> +
>> +#define gvt_warn_once(condition, fmt, args...) \
>> +	WARN_ONCE((condition), "gvt: "fmt"\n", ##args)
>
> WARN and WARN_ONCE will include backtrace so prefixing is unnecessary.
> I would not prefix them at all. Just use what i915 kernel module
> already uses. If needed, split them to their own file first,
> i915_debug.h.
OK. Thanks.:)
>> +
>> +#define gvt_dbg(level, fmt, args...) \
>> +	DRM_DEBUG_DRIVER("gvt: "fmt"\n", ##args)
>> +
>> +enum {
>> +	GVT_DBG_CORE = (1 << 0),
>> +	GVT_DBG_MM = (1 << 1),
>> +	GVT_DBG_IRQ = (1 << 2),
>> +};
>
> This enum is not of use.
>
>> +
>> +#define gvt_dbg_core(fmt, args...) \
>> +	gvt_dbg(GVT_DBG_CORE, fmt, ##args)
>> +
>
> Rahter like this (not to lose the topic)?
> 	gvt_dbg("core: " fmt, ##args)
>
Thanks for the idea. It's adorable. :)
>> +#define gvt_dbg_mm(fmt, args...) \
>> +	gvt_dbg(GVT_DBG_MM, fmt, ##args)
>> +
>> +#define gvt_dbg_irq(fmt, args...) \
>> +	gvt_dbg(GVT_DBG_IRQ, fmt, ##args)
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
>> new file mode 100644
>> index 0000000..52cfa32
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
>> @@ -0,0 +1,393 @@
>> +/*
>> + * Copyright(c) 2011-2016 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
>> +#include
>> +#include
>> +
>> +#include "gvt.h"
>> +
>> +struct gvt_host gvt_host;
>> +
>> +static const char * const supported_hypervisors[] = {
>> +	[GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
>> +	[GVT_HYPERVISOR_TYPE_KVM] = "KVM",
>> +};
>> +
>> +static int gvt_init_host(void)
>> +{
>> +	struct gvt_host *host = &gvt_host;
>> +
>
> Is it really that much more to write gvt_host.initialized? Counting the
> "->" vs "." it's three letters...
>
>> +	if (!gvt.enable) {
>> +		gvt_dbg_core("GVT-g has been disabled by kernel parameter");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (host->initialized) {
>> +		gvt_err("GVT-g has already been initialized!");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Xen DOM U */
>> +	if (xen_domain() && !xen_initial_domain())
>> +		return -ENODEV;
>> +
>> +	if (xen_initial_domain()) {
>> +		/* Xen Dom0 */
>> +		host->kdm = try_then_request_module(
>> +				symbol_get(xengt_kdm), "xengt");
>> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
>> +	} else {
>> +		/* not in Xen. Try KVMGT */
>> +		host->kdm = try_then_request_module(
>> +				symbol_get(kvmgt_kdm), "kvm");
>> +		host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
>> +	}
>> +
>> +	if (!host->kdm)
>> +		return -EINVAL;
>
> I think this error should be reported, to aid detecting problems in
> module loading.
>
Sure.
>> +
>> +	if (!hypervisor_detect_host())
>> +		return -ENODEV;
>> +
>
> This func should be prefixed gvt_, as it is not local to this file.
>
Will improve that.
>> +	gvt_info("Running with hypervisor %s in host mode",
>> +			supported_hypervisors[host->hypervisor_type]);
>> +
>> +	idr_init(&host->device_idr);
>> +	mutex_init(&host->device_idr_lock);
>> +
>> +	host->initialized = true;
>> +	return 0;
>> +}
>> +
>> +static int init_device_info(struct pgt_device *pdev)
>> +{
>> +	struct gvt_device_info *info = &pdev->device_info;
>> +
>> +	if (!IS_BROADWELL(pdev->dev_priv)) {
>> +		DRM_DEBUG_DRIVER("Unsupported GEN device");
>> +		return -ENODEV;
>> +	}
>
> This could be "else" clause on the next if and will allow easier adding
> of future platforms.
>
Will refine it into dedicated functions. :)
>> +
>> +	if (IS_BROADWELL(pdev->dev_priv)) {
>> +		info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
>> +		/*
>> +		 * The layout of BAR0 in BDW:
>> +		 * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->|
>> +		 *
>> +		 * GTT offset in BAR0 starts from 8MB to 16MB, and
>> +		 * Whatever GTT size is configured in BIOS,
>> +		 * the size of BAR0 is always 16MB. The actual configured
>> +		 * GTT size can be found in GMCH_CTRL.
>> +		 */
>> +		info->gtt_start_offset = (1UL << 23); /* 8MB */
>> +		info->max_gtt_size = (1UL << 23); /* 8MB */
>> +		info->gtt_entry_size = 8;
>> +		info->gtt_entry_size_shift = 3;
>> +		info->gmadr_bytes_in_cmd = 8;
>> +		info->mmio_size = 2 * 1024 * 1024; /* 2MB */
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
>> +{
>> +	struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
>> +	int i;
>> +
>> +	gvt_dbg_core("init initial cfg space, id %d", pdev->id);
>> +
>> +	for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
>> +		pci_read_config_dword(pci_dev, i,
>> +				(u32 *)&pdev->initial_cfg_space[i]);
>> +
>> +	for (i = 0; i < GVT_BAR_NUM; i++) {
>> +		pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
>> +		gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
>> +	}
>> +}
>> +
>> +static void clean_initial_mmio_state(struct pgt_device *pdev)
>> +{
>> +	if (pdev->gttmmio_va) {
>> +		iounmap(pdev->gttmmio_va);
>> +		pdev->gttmmio_va = NULL;
>> +	}
>> +
>> +	if (pdev->gmadr_va) {
>> +		iounmap(pdev->gmadr_va);
>> +		pdev->gmadr_va = NULL;
>> +	}
>> +}
>> +
>> +static int init_initial_mmio_state(struct pgt_device *pdev)
>> +{
>> +	struct gvt_device_info *info = &pdev->device_info;
>> +
>> +	u64 bar0, bar1;
>> +	int rc;
>> +
>> +	gvt_dbg_core("init initial mmio state, id %d", pdev->id);
>> +
>> +	bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
>> +	bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
>> +
>> +	pdev->gttmmio_base = bar0 & ~0xf;
>> +	pdev->reg_num = info->mmio_size / 4;
>> +	pdev->gmadr_base = bar1 & ~0xf;
>
> Magic numbers still.
>
My bad. :)
>> +
>> +	pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
>> +	if (!pdev->gttmmio_va) {
>> +		gvt_err("fail to map GTTMMIO BAR.");
>> +		return -EFAULT;
>> +	}
>> +
>> +	pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
>> +	if (!pdev->gmadr_va) {
>> +		gvt_err("fail to map GMADR BAR.");
>> +		rc = -EFAULT;
>> +		goto err;
>> +	}
>> +
>> +	gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
>> +	gvt_dbg_core("mmio size: %x", pdev->mmio_size);
>> +	gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
>> +			pdev->gmadr_base);
>> +	gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
>> +	gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
>> +
>> +	return 0;
>> +err:
>> +	clean_initial_mmio_state(pdev);
>> +	return rc;
>> +}
>> +
>> +static int gvt_service_thread(void *data)
>> +{
>> +	struct pgt_device *pdev = (struct pgt_device *)data;
>> +	int r;
>> +
>> +	gvt_dbg_core("service thread start, pgt %d", pdev->id);
>> +
>> +	while (!kthread_should_stop()) {
>> +		r = wait_event_interruptible(pdev->service_thread_wq,
>> +				kthread_should_stop() || pdev->service_request);
>> +
>> +		if (kthread_should_stop())
>> +			break;
>> +
>> +		if (gvt_warn_once(r,
>> +			"service thread is waken up by unexpected signal."))
>> +			continue;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void clean_service_thread(struct pgt_device *pdev)
>> +{
>> +	if (pdev->service_thread) {
>> +		kthread_stop(pdev->service_thread);
>> +		pdev->service_thread = NULL;
>> +	}
>> +}
>> +
>> +static int init_service_thread(struct pgt_device *pdev)
>> +{
>> +	init_waitqueue_head(&pdev->service_thread_wq);
>> +
>> +	pdev->service_thread = kthread_run(gvt_service_thread,
>> +			pdev, "gvt_service_thread%d", pdev->id);
>> +
>> +	if (!pdev->service_thread) {
>> +		gvt_err("fail to start service thread.");
>> +		return -ENOSPC;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void clean_pgt_device(struct pgt_device *pdev)
>> +{
>> +	clean_service_thread(pdev);
>> +	clean_initial_mmio_state(pdev);
>> +}
>> +
>> +static int init_pgt_device(struct pgt_device *pdev,
>> +	struct drm_i915_private *dev_priv)
>> +{
>> +	int rc;
>
> 	int ret;
>
>> +
>> +	rc = init_device_info(pdev);
>> +	if (rc)
>> +		return rc;
>> +
>> +	init_initial_cfg_space_state(pdev);
>> +
>> +	rc = init_initial_mmio_state(pdev);
>> +	if (rc)
>> +		goto err;
>> +
>> +	rc = init_service_thread(pdev);
>> +	if (rc)
>> +		goto err;
>> +
>> +	return 0;
>> +err:
>> +	clean_pgt_device(pdev);
>
> Add teardown path, see below.
>
>> +	return rc;
>> +}
>> +
>> +static int post_init_pgt_device(struct pgt_device *pdev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void free_pgt_device(struct pgt_device *pdev)
>> +{
>> +	struct gvt_host *host = &gvt_host;
>> +
>> +	mutex_lock(&host->device_idr_lock);
>> +	idr_remove(&host->device_idr, pdev->id);
>> +	mutex_unlock(&host->device_idr_lock);
>> +
>> +	vfree(pdev);
>> +}
>> +
>> +static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
>> +{
>> +	struct gvt_host *host = &gvt_host;
>> +	struct pgt_device *pdev = NULL;
>> +
>> +	pdev = vzalloc(sizeof(*pdev));
>> +	if (!pdev)
>> +		return NULL;
>
> This is a memory error, would like to see this propagated up as
>
> 	return ERR_PTR(-ENOMEM);
>
>> +
>> +	mutex_lock(&host->device_idr_lock);
>> +	pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
>> +	mutex_unlock(&host->device_idr_lock);
>> +
>> +	if (pdev->id < 0) {
>> +		gvt_err("fail to allocate pgt device id.");
>> +		goto err;
>> +	}
>> +
>
> Same here, propagate the error.
>
>> +	mutex_init(&pdev->lock);
>> +	pdev->dev_priv = dev_priv;
>> +	idr_init(&pdev->instance_idr);
>> +
>> +	return pdev;
>> +err:
>> +	free_pgt_device(pdev);
>
> I'd like to see a teardown path here. Then free_pgt_device() (or rather
> destroy_pgt_device() can expect everything to be initialized and when
> it it is called, it doesn't need ifs. This makes the driver code more
> robust.
>
> Or are we expecting only partially initialized structs for some reason?
>
The idea here is to prevent teardown path maintain burden in future, 
usually during the development, the teardown path is easy to be broken.

Sure, I will follow your idea. :)
>> +	return NULL;
>> +}
>> +
>> +void gvt_destroy_pgt_device(void *private_data)
>> +{
>> +	struct pgt_device *pdev = (struct pgt_device *)private_data;
>> +
>> +	clean_pgt_device(pdev);
>> +	free_pgt_device(pdev);
>
> Why multiple calls to destroy a device, there's only one alloc still?
>
clean_pgt_device() will call the clena_xxx() function of each 
sub-components. Each sub-components will stop and release the resource 
it's using. and free_pgt_device() will free the "pgt device" remove it 
from the IDR pool. :)
>> +}
>> +
>> +/**
>> + * gvt_create_pgt_device - create a GVT physical device
>> + * @dev: drm device
>> + *
>> + * This function is called at the initialization stage, to create a physical
>> + * GVT device and initialize necessary GVT components for it.
>> + *
>> + * Returns:
>> + * pointer to the pgt_device structure, NULL if failed.
>> + */
>> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
>> +{
>> +	struct pgt_device *pdev = NULL;
>> +	struct gvt_host *host = &gvt_host;
>> +	int rc;
>> +
>> +	if (!host->initialized && !gvt_init_host()) {
>> +		gvt_err("gvt_init_host fail");
>> +		return NULL;
>> +	}
>> +
>> +	gvt_dbg_core("create new pgt device, i915 dev_priv: %p", dev_priv);
>> +
>> +	pdev = alloc_pgt_device(dev_priv);
>> +	if (!pdev) {
>> +		gvt_err("fail to allocate memory for pgt device.");
>> +		goto err;
>> +	}
>> +
>> +	gvt_dbg_core("init pgt device, id %d", pdev->id);
>> +
>> +	rc = init_pgt_device(pdev, dev_priv);
>> +	if (rc) {
>
> Just call the return value variable ret like everywhere.
>
Sure. Good idea.
>> +		gvt_err("fail to init physical device state.");
>> +		goto err;
>> +	}
>> +
>> +	gvt_dbg_core("pgt device creation done, id %d", pdev->id);
>> +
>> +	return pdev;
>> +err:
>> +	if (pdev) {
>> +		gvt_destroy_pgt_device(pdev);
>> +		pdev = NULL;
>> +	}
>
> Proper goto label based teardown path should be used.
>
Thanks. will change all error label like that.
> err_destroy_pgt:
> 	gvt_destroy_pgt_device(pdev);
> err:
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * gvt_post_init_pgt_device - post init a GVT physical device
>> + * @dev: drm device
>
> Double check the kerneldocs to be correct per arguments of function.
>
Will correct it. :)
>> + *
>> + * This function is called at the end of the initialization stage, to
>> + * post-initialize a physical GVT device and initialize necessary
>> + * GVT components rely on i915 components.
>> + *
>> + * Returns:
>> + * zero on success, non-zero if failed.
>> + */
>> +int gvt_post_init_pgt_device(void *private_data)
>> +{
>> +	struct pgt_device *pdev = (struct pgt_device *)private_data;
>> +	struct gvt_host *host = &gvt_host;
>> +	int rc;
>> +
>> +	if (!host->initialized) {
>> +		gvt_err("gvt_host haven't been initialized.");
>> +		return -ENODEV;
>> +	}
>> +
>> +	gvt_dbg_core("post init pgt device %d", pdev->id);
>> +
>> +	rc = post_init_pgt_device(pdev);
>> +	if (rc) {
>> +		gvt_err("fail to post init physical device state.");
>> +		return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> new file mode 100644
>> index 0000000..d450198
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 _GVT_H_
>> +#define _GVT_H_
>> +
>> +#include "i915_drv.h"
>> +#include "i915_vgpu.h"
>> +
>> +#include "debug.h"
>> +#include "params.h"
>> +#include "reg.h"
>> +#include "hypercall.h"
>> +#include "mpt.h"
>> +
>> +#define GVT_MAX_VGPU 8
>> +
>> +enum {
>> +	GVT_HYPERVISOR_TYPE_XEN = 0,
>> +	GVT_HYPERVISOR_TYPE_KVM,
>> +};
>> +
>> +struct gvt_host {
>> +	bool initialized;
>> +	int hypervisor_type;
>> +	struct mutex device_idr_lock;
>> +	struct idr device_idr;
>> +	struct gvt_kernel_dm *kdm;
>> +};
>> +
>> +extern struct gvt_host gvt_host;
>
> -->
>
>> +extern struct gvt_kernel_dm xengt_kdm;
>> +extern struct gvt_kernel_dm kvmgt_kdm;
>
> <-- These should likely be declared somewhere in include/ rather than
> here.
>
Will check how i915 does. :)
>> +
>> +/* Describe the limitation of HW.*/
>> +struct gvt_device_info {
>> +	u64 max_gtt_gm_sz;
>> +	u32 gtt_start_offset;
>> +	u32 gtt_end_offset;
>> +	u32 max_gtt_size;
>> +	u32 gtt_entry_size;
>> +	u32 gtt_entry_size_shift;
>> +	u32 gmadr_bytes_in_cmd;
>> +	u32 mmio_size;
>> +};
>> +
>> +struct vgt_device {
>> +	int id;
>> +	int vm_id;
>> +	struct pgt_device *pdev;
>> +	bool warn_untrack;
>> +};
>> +
>> +struct pgt_device {
>
> Comments to this and other structs about what the memebers are.
>
>> +	struct mutex lock;
>> +	int id;
>> +
>> +	struct drm_i915_private *dev_priv;
>> +	struct idr instance_idr;
>> +
>> +	struct gvt_device_info device_info;
>> +
>> +	u8 initial_cfg_space[GVT_CFG_SPACE_SZ];
>> +	u64 bar_size[GVT_BAR_NUM];
>> +
>> +	u64 gttmmio_base;
>> +	void *gttmmio_va;
>> +
>> +	u64 gmadr_base;
>> +	void *gmadr_va;
>> +
>> +	u32 mmio_size;
>> +	u32 reg_num;
>> +
>> +	wait_queue_head_t service_thread_wq;
>> +	struct task_struct *service_thread;
>> +	unsigned long service_request;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
>> new file mode 100644
>> index 0000000..0a41874
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 _GVT_HYPERCALL_H_
>> +#define _GVT_HYPERCALL_H_
>> +
>> +struct gvt_kernel_dm {
>> +};
>
My bad. :)
> I would prefer more code introduced here in the initial patch, or this
> can be introduced later in the series as whole.
>
> It unnecessarily complicates the review if some files and calls are
> introduced with no documentation and implementation and only later
> their implementation is added.
>
> I can't really review if using a structure is a good idea if I can't
> see the context or implementation of their use.
>
>> +
>> +#endif /* _GVT_HYPERCALL_H_ */
>> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
>> new file mode 100644
>> index 0000000..e594bb8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 _GVT_MPT_H_
>> +#define _GVT_MPT_H_
>> +
>> +struct vgt_device;
>> +
>> +static inline bool hypervisor_detect_host(void)
>> +{
>> +	return false;
>> +}
>
> This is also not very reviewable and there's an unnecessary forward
> declaration.
>
>> +
>> +#endif /* _GVT_MPT_H_ */
>> diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
>> new file mode 100644
>> index 0000000..d381d17
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/params.c
>> @@ -0,0 +1,32 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 "gvt.h"
>> +
>> +struct gvt_kernel_params gvt = {
>> +	.enable = false,
>> +	.debug = 0,
>> +};
>> +
>> +module_param_named(gvt_enable, gvt.enable, bool, 0600);
>
> This should just be
>
> 	module_param_named(enable, gvt.enable, bool, ...)
>
> otherwise parameter has to be passed at boot time like this:
> 	
> 	gvt.gvt_enable=1
>
> Where we want
>
> 	gvt.enable=1
>
> Right?
>
Yes. Will move these params into intel_gvt.c

>> +MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
>> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
>> new file mode 100644
>> index 0000000..d2955b9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/params.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 _GVT_PARAMS_H_
>> +#define _GVT_PARAMS_H_
>> +
>> +struct gvt_kernel_params {
>> +	bool enable;
>> +	int debug;
>
> This member is unused currently, can be dropped.
>
Will remove that.
>> +};
>> +
>> +extern struct gvt_kernel_params gvt;
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/gvt/reg.h b/drivers/gpu/drm/i915/gvt/reg.h
>> new file mode 100644
>> index 0000000..d363b74
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/reg.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 _GVT_REG_H
>> +#define _GVT_REG_H
>> +
>> +#define GVT_CFG_SPACE_SZ	256
>> +#define GVT_BAR_NUM		4
>> +
>> +#define GVT_REG_CFG_SPACE_BAR0	0x10
>> +#define GVT_REG_CFG_SPACE_BAR1	0x18
>> +#define GVT_REG_CFG_SPACE_BAR2	0x20
>
> Some documentation here would be great.
>
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 1c6d227..f3bed37 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -35,6 +35,7 @@
>>   #include "intel_drv.h"
>>   #include
>>   #include "i915_drv.h"
>> +#include "i915_gvt.h"
>>   #include "i915_vgpu.h"
>>   #include "i915_trace.h"
>>   #include
>> @@ -1045,6 +1046,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>
>>   	intel_uncore_init(dev);
>>
>> +	ret = intel_gvt_init(dev);
>> +	if (ret)
>> +		goto out_uncore_fini;
>> +
>>   	ret = i915_gem_gtt_init(dev);
>>   	if (ret)
>>   		goto out_uncore_fini;
>
> This needs to become "goto out_gvt_cleanup;"
>
Good idea. :)
>> @@ -1135,6 +1140,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>   		goto out_power_well;
>>   	}
>>
>> +	ret = intel_gvt_post_init(dev);
>> +	if (ret) {
>> +		DRM_ERROR("failed to post init pgt device\n");
>> +		goto out_power_well;
>> +	}
>> +
>>   	/*
>>   	 * Notify a valid surface after modesetting,
>>   	 * when running inside a VM.
>> @@ -1177,6 +1188,7 @@ out_gem_unload:
>>   out_gtt:
>>   	i915_global_gtt_cleanup(dev);
>
> out_gvt_cleanup:
>
>>   out_uncore_fini:
>>
>> +	intel_gvt_cleanup(dev);
>
> This needs to be lifted up to its own label ensure proper teardown if
> i915_gem_gtt_init() fails.
>
>>   	intel_uncore_fini(dev);
>>   	i915_mmio_cleanup(dev);
>>   put_bridge:
>> @@ -1223,6 +1235,8 @@ int i915_driver_unload(struct drm_device *dev)
>>
>>   	intel_modeset_cleanup(dev);
>>
>> +	intel_gvt_cleanup(dev);
>> +
>>   	/*
>>   	 * free the memory space allocated for the child device
>>   	 * config parsed from VBT
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 20d9dbd..2f897c3 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1705,6 +1705,10 @@ struct i915_workarounds {
>>   	u32 hw_whitelist_count[I915_NUM_RINGS];
>>   };
>>
>> +struct i915_gvt {
>> +	void *pgt_device;
>> +};
>> +
>>   struct i915_virtual_gpu {
>>   	bool active;
>>   };
>> @@ -1744,6 +1748,8 @@ struct drm_i915_private {
>>
>>   	struct i915_virtual_gpu vgpu;
>>
>> +	struct i915_gvt gvt;
>> +
>>   	struct intel_guc guc;
>>
>>   	struct intel_csr csr;
>> @@ -2780,6 +2786,12 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv,
>>   void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
>>   					enum forcewake_domains domains);
>>   void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
>> +
>> +static inline bool intel_gvt_active(struct drm_device *dev)
>> +{
>> +	return to_i915(dev)->gvt.pgt_device ? true : false;
>> +}
>> +
>>   static inline bool intel_vgpu_active(struct drm_device *dev)
>>   {
>>   	return to_i915(dev)->vgpu.active;
>> diff --git a/drivers/gpu/drm/i915/i915_gvt.c b/drivers/gpu/drm/i915/i915_gvt.c
>> new file mode 100644
>> index 0000000..3ca7232
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_gvt.c
>> @@ -0,0 +1,93 @@
>> +/*
>> + * Copyright(c) 2011-2016 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 "i915_drv.h"
>> +#include "i915_gvt.h"
>> +
>> +/**
>> + * DOC: Intel GVT-g host 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 the englightments
>> + * of GVT and the necessary components used by GVT in i915 driver.
>> + */
>> +
>> +/**
>> + * intel_gvt_init - initialize GVT components at the beginning of i915
>> + * driver loading.
>> + * @dev: drm device *
>> + *
>> + * This function is called at the beginning of the initialization stage,
>> + * to initialize the GVT components that have to be initialized
>> + * before HW gets touched by other i915 components.
>> + */
>> +int intel_gvt_init(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> +	dev_priv->gvt.pgt_device = gvt_create_pgt_device(dev_priv);
>> +	if (intel_gvt_active(dev))
>> +		DRM_DEBUG_DRIVER("GVT-g is running in host mode\n");
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * intel_gvt_post_init - initialize GVT components at the end of i915
>> + * driver loading.
>> + * @dev: drm device *
>> + *
>> + * This function is called at the end of the initialization stage,
>> + * to initialize the GVT components that have to be initialized after
>> + * other i915 components are ready.
>> + */
>> +int intel_gvt_post_init(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> +	if (!intel_gvt_active(dev))
>> +		return 0;
>> +
>> +	return gvt_post_init_pgt_device(dev_priv->gvt.pgt_device);
>> +}
>> +
>> +/**
>> + * intel_gvt_cleanup - cleanup GVT components when i915 driver is unloading
>> + * @dev: drm device *
>> + *
>> + * This function is called at the i915 driver unloading stage, to shutdown
>> + * GVT components and release the related resources.
>> + */
>> +void intel_gvt_cleanup(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> +	if (!intel_gvt_active(dev))
>> +		return;
>> +
>> +	gvt_destroy_pgt_device(dev_priv->gvt.pgt_device);
>> +	dev_priv->gvt.pgt_device = NULL;
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_gvt.h b/drivers/gpu/drm/i915/i915_gvt.h
>> new file mode 100644
>> index 0000000..30cc207
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_gvt.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * Copyright(c) 2011-2016 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_GVT_H_
>> +#define _I915_GVT_H_
>
> I think this file could be intel_gvt.h (all remaining functions are
> intel_gvt), and have respective intel_gvt.c file.
>
>> +
>> +#ifdef CONFIG_DRM_I915_GVT
>
> Starting here --->
>
>> +extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
>> +extern int gvt_post_init_pgt_device(void *data);
>> +extern void gvt_destroy_pgt_device(void *data);
>> +
>
> <-- to here, these should go to their own include file at
> include/drm/i915_gvt.h For an example, see include/drm/i915_drm.h
>
> The respective export symbols then need to be exported, For an example
> see;
>
> 	EXPORT_SYMBOL_GPL(i915_gpu_raise);
>
>> +extern int intel_gvt_init(struct drm_device *dev);
>> +extern int intel_gvt_post_init(struct drm_device *dev);
>> +extern void intel_gvt_cleanup(struct drm_device *dev);
>> +#else
>> +extern int intel_gvt_init(struct drm_device *dev)
>
> These should, by convention, rather be static inline;
>
> static inline int intel_gvt_init(...)
>
Sorry I miss to check them here. Surely should be static inline.....
>> +{
>> +	return 0;
>> +}
>> +extern int intel_gvt_post_init(struct drm_device *dev)
>> +{
>> +	return 0;
>> +}
>> +extern void intel_gvt_cleanup(struct drm_device *dev)
>> +{
>> +}
>> +#endif
>> +
>> +#endif /* _I915_GVT_H_ */