[v3,2/3] drm/i915/opregion: rvda is relative from opregion base in opregion 2.1+
diff mbox series

Message ID 20190208184254.24123-2-jani.nikula@intel.com
State New
Headers show
Series
  • [v3,1/3] drm/i915/opregion: fix version check
Related show

Commit Message

Jani Nikula Feb. 8, 2019, 6:42 p.m. UTC
Starting from opregion version 2.1 (roughly corresponding to ICL+) the
RVDA field is relative from the beginning of opregion, not absolute
address.

Fix the error path while at it.

v2: Make relative vs. absolute conditional on the opregion version,
    bumped for the purpose. Turned out there are machines relying on
    absolute RVDA in the wild.

v3: Fix the version checks

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 | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

Comments

Ville Syrjala Feb. 8, 2019, 6:57 p.m. UTC | #1
On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote:
> Starting from opregion version 2.1 (roughly corresponding to ICL+) the
> RVDA field is relative from the beginning of opregion, not absolute
> address.
> 
> Fix the error path while at it.
> 
> v2: Make relative vs. absolute conditional on the opregion version,
>     bumped for the purpose. Turned out there are machines relying on
>     absolute RVDA in the wild.
> 
> v3: Fix the version checks
> 
> 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 | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index f1b841580521..5e00ee9270b5 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -123,7 +123,8 @@ struct opregion_asle {
>  	u64 fdss;
>  	u32 fdsp;
>  	u32 stat;
> -	u64 rvda;	/* Physical address of raw vbt data */
> +	u64 rvda;	/* Physical (2.0) or relative from opregion (2.1+)
> +			 * address of raw VBT data. */
>  	u32 rvds;	/* Size of raw vbt data */
>  	u8 rsvd[58];
>  } __packed;
> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  
>  	if (opregion->header->over.major >= 2 && opregion->asle &&
>  	    opregion->asle->rvda && opregion->asle->rvds) {
> -		opregion->rvda = memremap(opregion->asle->rvda,
> -					  opregion->asle->rvds,
> +		resource_size_t rvda = opregion->asle->rvda;
> +
> +		/*
> +		 * opregion 2.0: rvda is the physical VBT address.
> +		 *
> +		 * opregion 2.1+: rvda is unsigned, relative offset from
> +		 * opregion base, and should never point within opregion.
> +		 */
> +		if (opregion->header->over.major > 2 ||
> +		    opregion->header->over.minor >= 1) {

What happens with version 1.1?

> +			WARN_ON(rvda < OPREGION_SIZE);
> +
> +			rvda += asls;
> +		}
> +
> +		opregion->rvda = memremap(rvda, opregion->asle->rvds,
>  					  MEMREMAP_WB);
> +
>  		vbt = opregion->rvda;
>  		vbt_size = opregion->asle->rvds;
>  		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
> @@ -976,6 +992,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;
>  		}
>  	}
>  
> -- 
> 2.20.1
Jani Nikula Feb. 8, 2019, 7 p.m. UTC | #2
On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote:
>> Starting from opregion version 2.1 (roughly corresponding to ICL+) the
>> RVDA field is relative from the beginning of opregion, not absolute
>> address.
>> 
>> Fix the error path while at it.
>> 
>> v2: Make relative vs. absolute conditional on the opregion version,
>>     bumped for the purpose. Turned out there are machines relying on
>>     absolute RVDA in the wild.
>> 
>> v3: Fix the version checks
>> 
>> 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 | 24 +++++++++++++++++++++---
>>  1 file changed, 21 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index f1b841580521..5e00ee9270b5 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -123,7 +123,8 @@ struct opregion_asle {
>>  	u64 fdss;
>>  	u32 fdsp;
>>  	u32 stat;
>> -	u64 rvda;	/* Physical address of raw vbt data */
>> +	u64 rvda;	/* Physical (2.0) or relative from opregion (2.1+)
>> +			 * address of raw VBT data. */
>>  	u32 rvds;	/* Size of raw vbt data */
>>  	u8 rsvd[58];
>>  } __packed;
>> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>>  
>>  	if (opregion->header->over.major >= 2 && opregion->asle &&

Push this line to short term memory.

>>  	    opregion->asle->rvda && opregion->asle->rvds) {
>> -		opregion->rvda = memremap(opregion->asle->rvda,
>> -					  opregion->asle->rvds,
>> +		resource_size_t rvda = opregion->asle->rvda;
>> +
>> +		/*
>> +		 * opregion 2.0: rvda is the physical VBT address.
>> +		 *
>> +		 * opregion 2.1+: rvda is unsigned, relative offset from
>> +		 * opregion base, and should never point within opregion.
>> +		 */
>> +		if (opregion->header->over.major > 2 ||
>> +		    opregion->header->over.minor >= 1) {
>
> What happens with version 1.1?

Pop. ;)

BR,
Jani.

>
>> +			WARN_ON(rvda < OPREGION_SIZE);
>> +
>> +			rvda += asls;
>> +		}
>> +
>> +		opregion->rvda = memremap(rvda, opregion->asle->rvds,
>>  					  MEMREMAP_WB);
>> +
>>  		vbt = opregion->rvda;
>>  		vbt_size = opregion->asle->rvds;
>>  		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
>> @@ -976,6 +992,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;
>>  		}
>>  	}
>>  
>> -- 
>> 2.20.1
Ville Syrjala Feb. 8, 2019, 7:10 p.m. UTC | #3
On Fri, Feb 08, 2019 at 09:00:59PM +0200, Jani Nikula wrote:
> On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote:
> >> Starting from opregion version 2.1 (roughly corresponding to ICL+) the
> >> RVDA field is relative from the beginning of opregion, not absolute
> >> address.
> >> 
> >> Fix the error path while at it.
> >> 
> >> v2: Make relative vs. absolute conditional on the opregion version,
> >>     bumped for the purpose. Turned out there are machines relying on
> >>     absolute RVDA in the wild.
> >> 
> >> v3: Fix the version checks
> >> 
> >> 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 | 24 +++++++++++++++++++++---
> >>  1 file changed, 21 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >> index f1b841580521..5e00ee9270b5 100644
> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> @@ -123,7 +123,8 @@ struct opregion_asle {
> >>  	u64 fdss;
> >>  	u32 fdsp;
> >>  	u32 stat;
> >> -	u64 rvda;	/* Physical address of raw vbt data */
> >> +	u64 rvda;	/* Physical (2.0) or relative from opregion (2.1+)
> >> +			 * address of raw VBT data. */
> >>  	u32 rvds;	/* Size of raw vbt data */
> >>  	u8 rsvd[58];
> >>  } __packed;
> >> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >>  
> >>  	if (opregion->header->over.major >= 2 && opregion->asle &&
> 
> Push this line to short term memory.

Stack overflow.

Series is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> >>  	    opregion->asle->rvda && opregion->asle->rvds) {
> >> -		opregion->rvda = memremap(opregion->asle->rvda,
> >> -					  opregion->asle->rvds,
> >> +		resource_size_t rvda = opregion->asle->rvda;
> >> +
> >> +		/*
> >> +		 * opregion 2.0: rvda is the physical VBT address.
> >> +		 *
> >> +		 * opregion 2.1+: rvda is unsigned, relative offset from
> >> +		 * opregion base, and should never point within opregion.
> >> +		 */
> >> +		if (opregion->header->over.major > 2 ||
> >> +		    opregion->header->over.minor >= 1) {
> >
> > What happens with version 1.1?
> 
> Pop. ;)
> 
> BR,
> Jani.
> 
> >
> >> +			WARN_ON(rvda < OPREGION_SIZE);
> >> +
> >> +			rvda += asls;
> >> +		}
> >> +
> >> +		opregion->rvda = memremap(rvda, opregion->asle->rvds,
> >>  					  MEMREMAP_WB);
> >> +
> >>  		vbt = opregion->rvda;
> >>  		vbt_size = opregion->asle->rvds;
> >>  		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
> >> @@ -976,6 +992,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;
> >>  		}
> >>  	}
> >>  
> >> -- 
> >> 2.20.1
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Feb. 11, 2019, 3:36 p.m. UTC | #4
On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 08, 2019 at 09:00:59PM +0200, Jani Nikula wrote:
>> On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote:
>> >> Starting from opregion version 2.1 (roughly corresponding to ICL+) the
>> >> RVDA field is relative from the beginning of opregion, not absolute
>> >> address.
>> >> 
>> >> Fix the error path while at it.
>> >> 
>> >> v2: Make relative vs. absolute conditional on the opregion version,
>> >>     bumped for the purpose. Turned out there are machines relying on
>> >>     absolute RVDA in the wild.
>> >> 
>> >> v3: Fix the version checks
>> >> 
>> >> 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 | 24 +++++++++++++++++++++---
>> >>  1 file changed, 21 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> >> index f1b841580521..5e00ee9270b5 100644
>> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> >> @@ -123,7 +123,8 @@ struct opregion_asle {
>> >>  	u64 fdss;
>> >>  	u32 fdsp;
>> >>  	u32 stat;
>> >> -	u64 rvda;	/* Physical address of raw vbt data */
>> >> +	u64 rvda;	/* Physical (2.0) or relative from opregion (2.1+)
>> >> +			 * address of raw VBT data. */
>> >>  	u32 rvds;	/* Size of raw vbt data */
>> >>  	u8 rsvd[58];
>> >>  } __packed;
>> >> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>> >>  
>> >>  	if (opregion->header->over.major >= 2 && opregion->asle &&
>> 
>> Push this line to short term memory.
>
> Stack overflow.
>
> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, pushed patches 1&2 to dinq.

BR,
Jani.

>
>> 
>> >>  	    opregion->asle->rvda && opregion->asle->rvds) {
>> >> -		opregion->rvda = memremap(opregion->asle->rvda,
>> >> -					  opregion->asle->rvds,
>> >> +		resource_size_t rvda = opregion->asle->rvda;
>> >> +
>> >> +		/*
>> >> +		 * opregion 2.0: rvda is the physical VBT address.
>> >> +		 *
>> >> +		 * opregion 2.1+: rvda is unsigned, relative offset from
>> >> +		 * opregion base, and should never point within opregion.
>> >> +		 */
>> >> +		if (opregion->header->over.major > 2 ||
>> >> +		    opregion->header->over.minor >= 1) {
>> >
>> > What happens with version 1.1?
>> 
>> Pop. ;)
>> 
>> BR,
>> Jani.
>> 
>> >
>> >> +			WARN_ON(rvda < OPREGION_SIZE);
>> >> +
>> >> +			rvda += asls;
>> >> +		}
>> >> +
>> >> +		opregion->rvda = memremap(rvda, opregion->asle->rvds,
>> >>  					  MEMREMAP_WB);
>> >> +
>> >>  		vbt = opregion->rvda;
>> >>  		vbt_size = opregion->asle->rvds;
>> >>  		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
>> >> @@ -976,6 +992,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;
>> >>  		}
>> >>  	}
>> >>  
>> >> -- 
>> >> 2.20.1
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index f1b841580521..5e00ee9270b5 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -123,7 +123,8 @@  struct opregion_asle {
 	u64 fdss;
 	u32 fdsp;
 	u32 stat;
-	u64 rvda;	/* Physical address of raw vbt data */
+	u64 rvda;	/* Physical (2.0) or relative from opregion (2.1+)
+			 * address of raw VBT data. */
 	u32 rvds;	/* Size of raw vbt data */
 	u8 rsvd[58];
 } __packed;
@@ -964,9 +965,24 @@  int intel_opregion_setup(struct drm_i915_private *dev_priv)
 
 	if (opregion->header->over.major >= 2 && opregion->asle &&
 	    opregion->asle->rvda && opregion->asle->rvds) {
-		opregion->rvda = memremap(opregion->asle->rvda,
-					  opregion->asle->rvds,
+		resource_size_t rvda = opregion->asle->rvda;
+
+		/*
+		 * opregion 2.0: rvda is the physical VBT address.
+		 *
+		 * opregion 2.1+: rvda is unsigned, relative offset from
+		 * opregion base, and should never point within opregion.
+		 */
+		if (opregion->header->over.major > 2 ||
+		    opregion->header->over.minor >= 1) {
+			WARN_ON(rvda < OPREGION_SIZE);
+
+			rvda += asls;
+		}
+
+		opregion->rvda = memremap(rvda, opregion->asle->rvds,
 					  MEMREMAP_WB);
+
 		vbt = opregion->rvda;
 		vbt_size = opregion->asle->rvds;
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
@@ -976,6 +992,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;
 		}
 	}