[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 18:49:53 PST 2014
On 12/16/2014 9:19 PM, Tvrtko Ursulin wrote:
>
> 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?
How about we remove "to support existing production" in this sentence?
The "currently supported hardware" seems a bit redundant. :)
>
>>>> 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?
>
Well, I'm not sure.
Daniel, do you think the "low gmadr/high gmadr" will cause any
misunderstanding?
>>>> + 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.
OK. Thanks.
>
> Regards,
>
> Tvrtko
>
>
B.R.
Yu
More information about the Intel-gfx
mailing list