drm/i915/opregion: rvda is relative from opregion base, not absolute

Submitted by Jani Nikula on Dec. 13, 2018, 1:47 p.m.

Details

Message ID 20181213134704.30945-1-jani.nikula@intel.com
State New
Headers show
Series "drm/i915/opregion: rvda is relative from opregion base, not absolute" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula Dec. 13, 2018, 1:47 p.m.
We've supported the opregion RVDA/RVDS fields for VBT size >= 6 KB since
commit 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6
KB"). That's three years, almost to the date.

The implementation was based on spec only, in anticipation of systems
with big VBT. Now, the spec has been changed. The RVDA is supposed to be
relative from the beginning of opregion, not absolute address.

This is obviously a backward/forward incompatible change. I've been told
there are no systems out there using the field. Fingers crossed. This
will still be problematic for older kernels, and we can only try to
backport the fix.

Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index b8f106d9ecf8..700fddaa8d9e 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -119,7 +119,7 @@  struct opregion_asle {
 	u64 fdss;
 	u32 fdsp;
 	u32 stat;
-	u64 rvda;	/* Physical address of raw vbt data */
+	u64 rvda;	/* Address of raw vbt data, relative from opregion */
 	u32 rvds;	/* Size of raw vbt data */
 	u8 rsvd[58];
 } __packed;
@@ -955,7 +955,13 @@  int intel_opregion_setup(struct drm_i915_private *dev_priv)
 
 	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
 	    opregion->asle->rvda && opregion->asle->rvds) {
-		opregion->rvda = memremap(opregion->asle->rvda,
+		/*
+		 * rvda is unsigned, relative from opregion base, and should
+		 * never point within opregion.
+		 */
+		WARN_ON(opregion->asle->rvda < OPREGION_SIZE);
+
+		opregion->rvda = memremap(asls + opregion->asle->rvda,
 					  opregion->asle->rvds,
 					  MEMREMAP_WB);
 		vbt = opregion->rvda;
@@ -967,6 +973,8 @@  int intel_opregion_setup(struct drm_i915_private *dev_priv)
 			goto out;
 		} else {
 			DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n");
+			memunmap(opregion->rvda);
+			opregion->rvda = NULL;
 		}
 	}
 

Comments

On Thu, 13 Dec 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> We've supported the opregion RVDA/RVDS fields for VBT size >= 6 KB since
> commit 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6
> KB"). That's three years, almost to the date.
>
> The implementation was based on spec only, in anticipation of systems
> with big VBT. Now, the spec has been changed. The RVDA is supposed to be
> relative from the beginning of opregion, not absolute address.
>
> This is obviously a backward/forward incompatible change. I've been told
> there are no systems out there using the field. Fingers crossed. This
> will still be problematic for older kernels, and we can only try to
> backport the fix.

Should add this to the commit message:

While at it, add the missing memunmap() on the failure path for
completeness.

>
> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index b8f106d9ecf8..700fddaa8d9e 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -119,7 +119,7 @@ struct opregion_asle {
>  	u64 fdss;
>  	u32 fdsp;
>  	u32 stat;
> -	u64 rvda;	/* Physical address of raw vbt data */
> +	u64 rvda;	/* Address of raw vbt data, relative from opregion */
>  	u32 rvds;	/* Size of raw vbt data */
>  	u8 rsvd[58];
>  } __packed;
> @@ -955,7 +955,13 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  
>  	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
>  	    opregion->asle->rvda && opregion->asle->rvds) {
> -		opregion->rvda = memremap(opregion->asle->rvda,
> +		/*
> +		 * rvda is unsigned, relative from opregion base, and should
> +		 * never point within opregion.
> +		 */
> +		WARN_ON(opregion->asle->rvda < OPREGION_SIZE);
> +
> +		opregion->rvda = memremap(asls + opregion->asle->rvda,
>  					  opregion->asle->rvds,
>  					  MEMREMAP_WB);
>  		vbt = opregion->rvda;
> @@ -967,6 +973,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  			goto out;
>  		} else {
>  			DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n");
> +			memunmap(opregion->rvda);
> +			opregion->rvda = NULL;
>  		}
>  	}