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

[Tian, Kevin]

> 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.

> +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?

> +}
> +
> +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.

 [...]
> +
> +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?

[...]
> +
> +/**
> + * 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?

Thanks
Kevin