| Message ID | 1495441177-12451-1-git-send-email-tina.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ma, 2017-05-22 at 16:19 +0800, Tina Zhang wrote: > Enable the guest i915 full ppgtt functionality when host can provide this > capability. vgt_caps is introduced to guest i915 driver to get the vgpu > capabilities from the device model. VGT_CPAS_FULL_PPGTT is one of the > capabilities type to let guest i915 dirver know that the guest i915 full > ppgtt is supported by device model. > > Changes since v1: > - Use u32 instead of uint32_t (Joonas) > - Move VGT_CAPS_FULL_PPGTT introduction to this patch and use #define > instead of enum (Joonas) > - Rewrite the vgpu full ppgtt capability checking logic. (Joonas) > - Some coding style refine. (Joonas) > > Changes since v2: > - Divide the whole patch set into two separate patch series, with one > patch in i915 side to check guest i915 full ppgtt capability and enable > it when this capability is supported by the device model, and the other > one in gvt side which fixs the blocking issue and enables the device > model to provide the capability to guest. And this patch focuses on guest > i915 side. (Joonas) > - Change the title from "introduce vgt_caps to pvinfo" to > "Enable guest i915 full ppgtt functionality". (Tina) > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Tina Zhang <tina.zhang@intel.com> I just noticed there is INTEL_VGT_IF_VERSION when I was looking to make sure that vgt_if is zeroed. Neither the version is incremented nor do I see VGT_PVINFO_PAGE getting zeroed. What measures are in place to make sure running a new i915 under older DOM0 won't result in corruption? The dependencies between i915 and gvt are rather tricky, so we'd need INTEL_VGT_IF_VERSION minor increment and also a one line change (zeroing of the new caps register) from gvt code to the same patch, otherwise bisecting will break. Regards, Joonas
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Tuesday, May 30, 2017 4:38 PM > To: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org > Cc: zhenyuw@linux.intel.com; intel-gvt-dev@lists.freedesktop.org > Subject: Re: [PATCH v3] drm/i915: Enable guest i915 full ppgtt functionality > > On ma, 2017-05-22 at 16:19 +0800, Tina Zhang wrote: > > Enable the guest i915 full ppgtt functionality when host can provide > > this capability. vgt_caps is introduced to guest i915 driver to get > > the vgpu capabilities from the device model. VGT_CPAS_FULL_PPGTT is > > one of the capabilities type to let guest i915 dirver know that the > > guest i915 full ppgtt is supported by device model. > > > > Changes since v1: > > - Use u32 instead of uint32_t (Joonas) > > - Move VGT_CAPS_FULL_PPGTT introduction to this patch and use #define > > instead of enum (Joonas) > > - Rewrite the vgpu full ppgtt capability checking logic. (Joonas) > > - Some coding style refine. (Joonas) > > > > Changes since v2: > > - Divide the whole patch set into two separate patch series, with one > > patch in i915 side to check guest i915 full ppgtt capability and > > enable > > it when this capability is supported by the device model, and the > > other > > one in gvt side which fixs the blocking issue and enables the device > > model to provide the capability to guest. And this patch focuses on > > guest > > i915 side. (Joonas) > > - Change the title from "introduce vgt_caps to pvinfo" to > > "Enable guest i915 full ppgtt functionality". (Tina) > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Tina Zhang <tina.zhang@intel.com> > > I just noticed there is INTEL_VGT_IF_VERSION when I was looking to make sure > that vgt_if is zeroed. Neither the version is incremented nor do I > see VGT_PVINFO_PAGE getting zeroed. Vgt_if gets its initial values populated by device model through reading hardware registers, named "SW Virtualization Reserved MMIO rang". These registers are always zeros when reading by device model. So, we use this way to initialize the vgt_if with zeros. About the INTEL_VGT_IF_VERSION, agree that we need to doc a Spec to describe it in detail, I guess. But for this vgt_cap register definition, I'm afraid we don't need to bump the version. Because, actually, this register is already defined in the version 1.0, and we just didn't use it before. > > What measures are in place to make sure running a new i915 under older > DOM0 won't result in corruption? This won't be a problem, I guess, as we have already considered this situation. (Thanks for Zhenyu's comments about this problem, before) I915 with this new patch, will check whether the cap is enabled by device model. As the device model is old, this cap should not be enabled and the i915 in the guest will get value zero, which means device model cannot support the full ppgtt feature. So, in this way, the guest i915 can only use the aliasing ppgtt which is also the only choice for guest i915 before this patch. > > The dependencies between i915 and gvt are rather tricky, so we'd need > INTEL_VGT_IF_VERSION minor increment and also a one line change (zeroing of > the new caps register) from gvt code to the same patch, otherwise bisecting will > break. > > Regards, Joonas > -- > Joonas Lahtinen > Open Source Technology Center > Intel Corporation
On 2017.05.30 11:37:50 +0300, Joonas Lahtinen wrote: > On ma, 2017-05-22 at 16:19 +0800, Tina Zhang wrote: > > Enable the guest i915 full ppgtt functionality when host can provide this > > capability. vgt_caps is introduced to guest i915 driver to get the vgpu > > capabilities from the device model. VGT_CPAS_FULL_PPGTT is one of the > > capabilities type to let guest i915 dirver know that the guest i915 full > > ppgtt is supported by device model. > > > > Changes since v1: > > - Use u32 instead of uint32_t (Joonas) > > - Move VGT_CAPS_FULL_PPGTT introduction to this patch and use #define > > instead of enum (Joonas) > > - Rewrite the vgpu full ppgtt capability checking logic. (Joonas) > > - Some coding style refine. (Joonas) > > > > Changes since v2: > > - Divide the whole patch set into two separate patch series, with one > > patch in i915 side to check guest i915 full ppgtt capability and enable > > it when this capability is supported by the device model, and the other > > one in gvt side which fixs the blocking issue and enables the device > > model to provide the capability to guest. And this patch focuses on guest > > i915 side. (Joonas) > > - Change the title from "introduce vgt_caps to pvinfo" to > > "Enable guest i915 full ppgtt functionality". (Tina) > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Tina Zhang <tina.zhang@intel.com> > > I just noticed there is INTEL_VGT_IF_VERSION when I was looking to make > sure that vgt_if is zeroed. Neither the version is incremented nor do I > see VGT_PVINFO_PAGE getting zeroed. > > What measures are in place to make sure running a new i915 under older > DOM0 won't result in corruption? > > The dependencies between i915 and gvt are rather tricky, so we'd need > INTEL_VGT_IF_VERSION minor increment and also a one line change > (zeroing of the new caps register) from gvt code to the same patch, > otherwise bisecting will break. > That's unfortunate...We can't increment minor now as it would break older guest, need to fix guest pvinfo version compat check, and new caps field is kept as zero if not used, so now depend on that for compatibility.
On ke, 2017-05-31 at 10:49 +0800, Zhenyu Wang wrote: > On 2017.05.30 11:37:50 +0300, Joonas Lahtinen wrote: > > > > I just noticed there is INTEL_VGT_IF_VERSION when I was looking to make > > sure that vgt_if is zeroed. Neither the version is incremented nor do I > > see VGT_PVINFO_PAGE getting zeroed. > > > > What measures are in place to make sure running a new i915 under older > > DOM0 won't result in corruption? > > > > The dependencies between i915 and gvt are rather tricky, so we'd need > > INTEL_VGT_IF_VERSION minor increment and also a one line change > > (zeroing of the new caps register) from gvt code to the same patch, > > otherwise bisecting will break. > > > > That's unfortunate...We can't increment minor now as it would break > older guest, need to fix guest pvinfo version compat check, and new caps > field is kept as zero if not used, so now depend on that for compatibility. Point of minor versions is to indicate no breaking, but to detect new features. But the damage has already been done, so it'd be worthwhile to document this via a comment. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 17883a8..a630e5e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1860,6 +1860,7 @@ struct i915_workarounds { struct i915_virtual_gpu { bool active; + u32 caps; }; /* used in computing the new watermarks state */ diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index a9d78eb..10629d9 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -141,13 +141,12 @@ int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv, has_aliasing_ppgtt = dev_priv->info.has_aliasing_ppgtt; has_full_ppgtt = dev_priv->info.has_full_ppgtt; - has_full_48bit_ppgtt = dev_priv->info.has_full_48bit_ppgtt; - if (intel_vgpu_active(dev_priv)) { - /* emulation is too hard */ - has_full_ppgtt = false; - has_full_48bit_ppgtt = false; - } + if (intel_vgpu_active(dev_priv)) + has_full_ppgtt = intel_vgpu_has_full_ppgtt(dev_priv); + + has_full_48bit_ppgtt = has_full_ppgtt && + dev_priv->info.has_full_48bit_ppgtt; if (!has_aliasing_ppgtt) return 0; diff --git a/drivers/gpu/drm/i915/i915_pvinfo.h b/drivers/gpu/drm/i915/i915_pvinfo.h index c0cb297..8dc0664 100644 --- a/drivers/gpu/drm/i915/i915_pvinfo.h +++ b/drivers/gpu/drm/i915/i915_pvinfo.h @@ -53,12 +53,18 @@ enum vgt_g2v_type { VGT_G2V_MAX, }; +/* + * VGT capabilities type + */ +#define VGT_CAPS_FULL_PPGTT BIT(2) + struct vgt_if { u64 magic; /* VGT_MAGIC */ uint16_t version_major; uint16_t version_minor; u32 vgt_id; /* ID of vGT instance */ - u32 rsv1[12]; /* pad to offset 0x40 */ + u32 vgt_caps; /* VGT capabilities */ + u32 rsv1[11]; /* pad to offset 0x40 */ /* * Data structure to describe the balooning info of resources. * Each VM can only have one portion of continuous area for now. diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c index 4ab8a97..adc4eda 100644 --- a/drivers/gpu/drm/i915/i915_vgpu.c +++ b/drivers/gpu/drm/i915/i915_vgpu.c @@ -77,10 +77,17 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv) return; } + dev_priv->vgpu.caps = __raw_i915_read32(dev_priv, vgtif_reg(vgt_caps)); + dev_priv->vgpu.active = true; DRM_INFO("Virtual GPU for Intel GVT-g detected.\n"); } +bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv) +{ + return dev_priv->vgpu.caps & VGT_CAPS_FULL_PPGTT; +} + struct _balloon_info_ { /* * There are up to 2 regions per mappable/unmappable graphic diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h index 3c3b2d2..b4e04eb 100644 --- a/drivers/gpu/drm/i915/i915_vgpu.h +++ b/drivers/gpu/drm/i915/i915_vgpu.h @@ -27,6 +27,9 @@ #include "i915_pvinfo.h" void i915_check_vgpu(struct drm_i915_private *dev_priv); + +bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv); + int intel_vgt_balloon(struct drm_i915_private *dev_priv); void intel_vgt_deballoon(struct drm_i915_private *dev_priv);