Skip to contentSkip to conversation

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

PATCH In review Jani Nikula 2 participants 3 messages Dec 13, 2018

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Jani Nikula

author
#0 lore raw

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

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ä <[email protected]>
Cc Imre Deak <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_opregion.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_opregion.c +10 -2
index b8f106d9ecf8..700fddaa8d9e 100644
@@ -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;
 		}
 	}
Signature · 6 lines
2.11.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#1 lore raw
Quoted 4 lines · Jani Nikula

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.

Quoted 8 lines

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.

Quoted 45 lines
Fixes 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
Cc Ville Syrjälä <[email protected]>
Cc Imre Deak <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_opregion.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_opregion.c +10 -2
index b8f106d9ecf8..700fddaa8d9e 100644
@@ -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;
 		}
 	}
Signature · 5 lines
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#2 lore raw

✗ Fi.CI.BAT: failure for drm/i915/opregion: rvda is relative from opregion base, not absolute

In reply to Jani Nikula
== Series Details ==

== Series Details ==

Series: drm/i915/opregion: rvda is relative from opregion base, not absolute
URL : https://patchwork.freedesktop.org/series/53996/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5311 -> Patchwork_11088
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with Patchwork_11088 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_11088, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/53996/revisions/1/mbox/

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in Patchwork_11088:

### IGT changes ###

#### Possible regressions ####

* igt@gem_exec_suspend@basic-s3:
- fi-apl-guc: PASS -> DMESG-WARN

* igt@pm_rpm@basic-rte:
- fi-byt-j1900: PASS -> FAIL

* {igt@runner@aborted}:
- fi-apl-guc: NOTRUN -> FAIL

#### Warnings ####

* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
- fi-kbl-7567u: PASS -> SKIP +33

* igt@pm_rpm@basic-pci-d3-state:
- fi-byt-j1900: PASS -> SKIP

Known issues
------------

Here are the changes found in Patchwork_11088 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@i915_module_load@reload-with-fault-injection:
- fi-kbl-7567u: PASS -> DMESG-WARN [fdo#105602] / [fdo#108529] +1

* igt@kms_flip@basic-flip-vs-dpms:
- fi-icl-u3: NOTRUN -> DMESG-WARN [fdo#108924] / [fdo#109044]

* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]

* igt@pm_rpm@module-reload:
- fi-kbl-7567u: PASS -> DMESG-WARN [fdo#108529]

* {igt@runner@aborted}:
- fi-icl-u3: NOTRUN -> FAIL [fdo#108924]
- fi-icl-y: NOTRUN -> FAIL [fdo#108070]

#### Possible fixes ####

* igt@gem_ctx_create@basic-files:
- fi-kbl-7560u: INCOMPLETE [fdo#103665] -> PASS

#### Warnings ####

* igt@kms_chamelium@common-hpd-after-suspend:
- fi-kbl-7567u: DMESG-WARN [fdo#108473] -> DMESG-FAIL [fdo#105079]

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
[fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#108070]: https://bugs.freedesktop.org/show_bug.cgi?id=108070
[fdo#108473]: https://bugs.freedesktop.org/show_bug.cgi?id=108473
[fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
[fdo#108924]: https://bugs.freedesktop.org/show_bug.cgi?id=108924
[fdo#109044]: https://bugs.freedesktop.org/show_bug.cgi?id=109044

Participating hosts (47 -> 44)
------------------------------

Additional (2): fi-icl-y fi-icl-u3
Missing (5): fi-kbl-soraka fi-ilk-m540 fi-bxt-dsi fi-byt-squawks fi-ctg-p8600

Build changes
-------------

* Linux: CI_DRM_5311 -> Patchwork_11088

CI_DRM_5311: a42fd8bf199784ee4ff1cdb5ee03eedd9a535d4a @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4746: 2c793666d8c8328733f5769b16ae5858fee97f3f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11088: 715b0f2a84bf94eedb0f5b73ca71f56b5d75b92a @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

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

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11088/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx