drm/i915: Fix GVT-g PVINFO version compatibility check
diff mbox

Message ID 20170605030041.9619-1-zhenyuw@linux.intel.com
State New
Headers show

Commit Message

Zhenyu Wang June 5, 2017, 3 a.m. UTC
Current it's strictly checked if PVINFO version matches 1.0
for GVT-g i915 guest which doesn't help for compatibility at
all and forces GVT-g host can't extend PVINFO easily with version
bump for real compatibility check.

This fixes that to check minimal required PVINFO version instead
and currently minimal 1.0 is required.

Cc: Chuanxiao Dong <chuanxiao.dong@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: stable@vger.kernel.org # 4.10+
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_pvinfo.h | 3 ++-
 drivers/gpu/drm/i915/i915_vgpu.c   | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Joonas Lahtinen June 5, 2017, 1:40 p.m. UTC | #1
On ma, 2017-06-05 at 11:00 +0800, Zhenyu Wang wrote:
> Current it's strictly checked if PVINFO version matches 1.0
> for GVT-g i915 guest which doesn't help for compatibility at
> all and forces GVT-g host can't extend PVINFO easily with version
> bump for real compatibility check.
> 
> This fixes that to check minimal required PVINFO version instead
> and currently minimal 1.0 is required.
> 
> Cc: Chuanxiao Dong <chuanxiao.dong@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: stable@vger.kernel.org # 4.10+
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_pvinfo.h
> @@ -37,7 +37,8 @@
>  #define VGT_VERSION_MINOR 0
>  
>  #define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 | (minor))
> -#define INTEL_VGT_IF_VERSION \
> +/* minimal required PVINFO version */
> +#define INTEL_VGT_IF_VERSION_REQUIRED \
>  	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)

I think this can be dropped completely.

> @@ -72,7 +72,7 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
>  	version = INTEL_VGT_IF_VERSION_ENCODE(
>  		__raw_i915_read16(dev_priv, vgtif_reg(version_major)),
>  		__raw_i915_read16(dev_priv, vgtif_reg(version_minor)));

Here one would check that

	if (version_major != VGT_VERSION_MAJOR)
 		DRM_INFO("VGT interface version mismatch!\n");
		return;
	}

That makes sure that major version must always match (incrementing
major version is a direct indication of interface incompatibility). I'm
not sure if we want to check the minor version at all, at load time.

Instead we should check the minor version for each new feature we know
has been added. That way an older i915 module could keep running in the
DOM0, and the newer i915 modules in VMs just wouldn't attempt to use
the new features until the DOM0 gets an update, too.

Regards, Joonas
Zhenyu Wang June 7, 2017, 9:16 a.m. UTC | #2
On 2017.06.05 16:40:09 +0300, Joonas Lahtinen wrote:
> 
> On ma, 2017-06-05 at 11:00 +0800, Zhenyu Wang wrote:
> > Current it's strictly checked if PVINFO version matches 1.0
> > for GVT-g i915 guest which doesn't help for compatibility at
> > all and forces GVT-g host can't extend PVINFO easily with version
> > bump for real compatibility check.
> > 
> > This fixes that to check minimal required PVINFO version instead
> > and currently minimal 1.0 is required.
> > 
> > Cc: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: stable@vger.kernel.org # 4.10+
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_pvinfo.h
> > @@ -37,7 +37,8 @@
> >  #define VGT_VERSION_MINOR 0
> >  
> >  #define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 | (minor))
> > -#define INTEL_VGT_IF_VERSION \
> > +/* minimal required PVINFO version */
> > +#define INTEL_VGT_IF_VERSION_REQUIRED \
> >  	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
> 
> I think this can be dropped completely.
> 
> > @@ -72,7 +72,7 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
> >  	version = INTEL_VGT_IF_VERSION_ENCODE(
> >  		__raw_i915_read16(dev_priv, vgtif_reg(version_major)),
> >  		__raw_i915_read16(dev_priv, vgtif_reg(version_minor)));
> 
> Here one would check that
> 
> 	if (version_major != VGT_VERSION_MAJOR)
>  		DRM_INFO("VGT interface version mismatch!\n");
> 		return;
> 	}
> 
> That makes sure that major version must always match (incrementing
> major version is a direct indication of interface incompatibility). I'm
> not sure if we want to check the minor version at all, at load time.

We only plan to extend PVInfo definition (use previous reserved/unused
area) but not break compatibility as no requirement to do
incompatibility change for pvinfo, so like to keep compat with older
kernel. So this version check is just for sanity on minimal require,
agree to only check major is enough.

> 
> Instead we should check the minor version for each new feature we know
> has been added. That way an older i915 module could keep running in the
> DOM0, and the newer i915 modules in VMs just wouldn't attempt to use
> the new features until the DOM0 gets an update, too.
> 

yeah, minor version would be bumped for each new feature support, e.g
guc/huc for guest etc, structural extension will get major version
bumped.
Joonas Lahtinen June 12, 2017, 12:46 p.m. UTC | #3
On pe, 2017-06-09 at 08:10 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix GVT-g PVINFO version compatibility check (rev3)
> URL   : https://patchwork.freedesktop.org/series/25275/
> State : success

Applied. Thanks for the patch.

Regards, Joonas

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_pvinfo.h b/drivers/gpu/drm/i915/i915_pvinfo.h
index c0cb2974caac..96434707857e 100644
--- a/drivers/gpu/drm/i915/i915_pvinfo.h
+++ b/drivers/gpu/drm/i915/i915_pvinfo.h
@@ -37,7 +37,8 @@ 
 #define VGT_VERSION_MINOR 0
 
 #define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 | (minor))
-#define INTEL_VGT_IF_VERSION \
+/* minimal required PVINFO version */
+#define INTEL_VGT_IF_VERSION_REQUIRED \
 	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
 
 /*
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 4ab8a973b61f..5798572e6db3 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -72,7 +72,7 @@  void i915_check_vgpu(struct drm_i915_private *dev_priv)
 	version = INTEL_VGT_IF_VERSION_ENCODE(
 		__raw_i915_read16(dev_priv, vgtif_reg(version_major)),
 		__raw_i915_read16(dev_priv, vgtif_reg(version_minor)));
-	if (version != INTEL_VGT_IF_VERSION) {
+	if (version < INTEL_VGT_IF_VERSION_REQUIRED) {
 		DRM_INFO("VGT interface version mismatch!\n");
 		return;
 	}