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

[Zhi Wang]

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
>