[Intel-gfx] [PATCH v3 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Dec 16 05:19:48 PST 2014
On 12/16/2014 12:51 PM, Yu, Zhang wrote:
> 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.
So maybe instead of "existing production" say "currently supported
hardware" or similar?
>>> 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?
I would leave this for the future version. I was simply suggesting
logging something on version mismatch. Just because it is unexpected so
there is a trace in the logs why feature is not available. It's up to you.
>>
>>> + 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?
Yes.
>>> + /*
>>> + * 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. :)
Are high and low memory as terms known in the i915 space so far as
synonyms for mappable and non-mappable?
>>> + 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? :)
The only purpose for packed here I can see is a double guard along the
BUILD_BUG_ON. As I said I don't think it makes any difference so you can
leave it in.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list