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

Tvrtko Ursulin
2014-12-11T17:34:22Z

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

bytes

> existing production. Future hardware will have the reserved window

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

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

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

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

Would it be useful to something on version mismatch?

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

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

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

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

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

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

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

Regards,

Tvrtko