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