Re: [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

John Harrison <john.c.harrison@xxxxxxxxx> · Fri, 28 Apr 2023 17:16:50 -0700

On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
On 4/20/2023 6:15 PM, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>

The validation of the firmware table was being done inside the code
for scanning the table for the next available firmware blob. Which is
unnecessary. So pull it out into a separate function that is only
called once per blob type at init time.

Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
It was mentioned that potential issues with backports would not be
caught by regular pre-merge CI as that only occurs on tip not stable
branches. Making the validation unconditional and failing driver load
on detecting of a problem ensures that such backports will also be
validated correctly.

v2: Change to unconditionally fail module load on a validation error
(review feedback/discussion with Daniele).

Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
  1 file changed, 92 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c 
b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index c9cd9bb47577f..eb52e8db9ae0b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -233,20 +233,22 @@ struct fw_blobs_by_type {
      u32 count;
  };
  +static const struct uc_fw_platform_requirement blobs_guc[] = {
+    INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
+};
+
+static const struct uc_fw_platform_requirement blobs_huc[] = {
+    INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
+};
+
+static const struct fw_blobs_by_type 
blobs_all[INTEL_UC_FW_NUM_TYPES] = {
+    [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
+    [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
+};
+
  static void
  __uc_fw_auto_select(struct drm_i915_private *i915, struct 
intel_uc_fw *uc_fw)
  {
-    static const struct uc_fw_platform_requirement blobs_guc[] = {
-        INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, 
GUC_FW_BLOB_MMP)
-    };
-    static const struct uc_fw_platform_requirement blobs_huc[] = {
-        INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
-    };
-    static const struct fw_blobs_by_type 
blobs_all[INTEL_UC_FW_NUM_TYPES] = {
-        [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
-        [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
-    };
-    static bool verified[INTEL_UC_FW_NUM_TYPES];
      const struct uc_fw_platform_requirement *fw_blobs;
      enum intel_platform p = INTEL_INFO(i915)->platform;
      u32 fw_count;
@@ -286,6 +288,11 @@ __uc_fw_auto_select(struct drm_i915_private 
*i915, struct intel_uc_fw *uc_fw)
              continue;
            if (uc_fw->file_selected.path) {
+            /*
+             * Continuing an earlier search after a found blob 
failed to load.
+             * Once the previously chosen path has been found, clear 
it out
+             * and let the search continue from there.
+             */
              if (uc_fw->file_selected.path == blob->path)
                  uc_fw->file_selected.path = NULL;
  @@ -306,76 +313,91 @@ __uc_fw_auto_select(struct drm_i915_private 
*i915, struct intel_uc_fw *uc_fw)
          /* Failed to find a match for the last attempt?! */
          uc_fw->file_selected.path = NULL;
      }
+}
  -    /* make sure the list is ordered as expected */
-    if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && 
!verified[uc_fw->type]) {
-        verified[uc_fw->type] = true;
+static bool validate_fw_table_type(struct drm_i915_private *i915, 
enum intel_uc_fw_type type)
+{
+    const struct uc_fw_platform_requirement *fw_blobs;
+    u32 fw_count;
+    int i;
  -        for (i = 1; i < fw_count; i++) {
-            /* Next platform is good: */
-            if (fw_blobs[i].p < fw_blobs[i - 1].p)
-                continue;
+    if (type >= ARRAY_SIZE(blobs_all)) {
+        drm_err(&i915->drm, "No blob array for %s\n", 
intel_uc_fw_type_repr(type));
+        return false;
+    }
  -            /* Next platform revision is good: */
-            if (fw_blobs[i].p == fw_blobs[i - 1].p &&
-                fw_blobs[i].rev < fw_blobs[i - 1].rev)
-                continue;
+    fw_blobs = blobs_all[type].blobs;
+    fw_count = blobs_all[type].count;
  -            /* Platform/revision must be in order: */
-            if (fw_blobs[i].p != fw_blobs[i - 1].p ||
-                fw_blobs[i].rev != fw_blobs[i - 1].rev)
-                goto bad;
+    if (!fw_count)
+        return true;
  -            /* Next major version is good: */
-            if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
-                continue;
+    /* make sure the list is ordered as expected */
+    for (i = 1; i < fw_count; i++) {
+        /* Next platform is good: */
+        if (fw_blobs[i].p < fw_blobs[i - 1].p)
+            continue;
  -            /* New must be before legacy: */
-            if (!fw_blobs[i].blob.legacy && fw_blobs[i - 
1].blob.legacy)
-                goto bad;
+        /* Next platform revision is good: */
+        if (fw_blobs[i].p == fw_blobs[i - 1].p &&
+            fw_blobs[i].rev < fw_blobs[i - 1].rev)
+            continue;
  -            /* New to legacy also means 0.0 to X.Y (HuC), or X.0 
to X.Y (GuC) */
-            if (fw_blobs[i].blob.legacy && !fw_blobs[i - 
1].blob.legacy) {
-                if (!fw_blobs[i - 1].blob.major)
-                    continue;
+        /* Platform/revision must be in order: */
+        if (fw_blobs[i].p != fw_blobs[i - 1].p ||
+            fw_blobs[i].rev != fw_blobs[i - 1].rev)
+            goto bad;
  -                if (fw_blobs[i].blob.major == fw_blobs[i - 
1].blob.major)
-                    continue;
-            }
+        /* Next major version is good: */
+        if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
+            continue;
  -            /* Major versions must be in order: */
-            if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
-                goto bad;
+        /* New must be before legacy: */
+        if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
+            goto bad;
  -            /* Next minor version is good: */
-            if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
+        /* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y 
(GuC) */
+        if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
+            if (!fw_blobs[i - 1].blob.major)
                  continue;
  -            /* Minor versions must be in order: */
-            if (fw_blobs[i].blob.minor != fw_blobs[i - 1].blob.minor)
-                goto bad;
-
-            /* Patch versions must be in order: */
-            if (fw_blobs[i].blob.patch <= fw_blobs[i - 1].blob.patch)
+            if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
                  continue;
+        }
+
+        /* Major versions must be in order: */
+        if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
+            goto bad;
+
+        /* Next minor version is good: */
+        if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
+            continue;
+
+        /* Minor versions must be in order: */
+        if (fw_blobs[i].blob.minor != fw_blobs[i - 1].blob.minor)
+            goto bad;
+
+        /* Patch versions must be in order: */
+        if (fw_blobs[i].blob.patch <= fw_blobs[i - 1].blob.patch)
+            continue;
    bad:
-            drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
-                intel_uc_fw_type_repr(uc_fw->type),
-                intel_platform_name(fw_blobs[i - 1].p), fw_blobs[i - 
1].rev,
-                fw_blobs[i - 1].blob.legacy ? "L" : "v",
-                fw_blobs[i - 1].blob.major,
-                fw_blobs[i - 1].blob.minor,
-                fw_blobs[i - 1].blob.patch,
-                intel_platform_name(fw_blobs[i].p), fw_blobs[i].rev,
-                fw_blobs[i].blob.legacy ? "L" : "v",
-                fw_blobs[i].blob.major,
-                fw_blobs[i].blob.minor,
-                fw_blobs[i].blob.patch);
-
-            uc_fw->file_selected.path = NULL;
-        }
+        drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
+            intel_uc_fw_type_repr(type),
+            intel_platform_name(fw_blobs[i - 1].p), fw_blobs[i - 
1].rev,
+            fw_blobs[i - 1].blob.legacy ? "L" : "v",
+            fw_blobs[i - 1].blob.major,
+            fw_blobs[i - 1].blob.minor,
+            fw_blobs[i - 1].blob.patch,
+            intel_platform_name(fw_blobs[i].p), fw_blobs[i].rev,
+            fw_blobs[i].blob.legacy ? "L" : "v",
+            fw_blobs[i].blob.major,
+            fw_blobs[i].blob.minor,
+            fw_blobs[i].blob.patch);
+        return false;
      }
+
+    return true;
  }
    static const char *__override_guc_firmware_path(struct 
drm_i915_private *i915)
@@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
*uc_fw,
      uc_fw->type = type;
        if (HAS_GT_UC(i915)) {
+        if (!validate_fw_table_type(i915, type)) {
+            intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);

In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
includes the fact that the fw has been selected, which in turns 
implies that file_selected.path is valid. this means that even with 
enable_guc=0 the wants/uses_guc macro might end up returning true, 
which is not something we want.

Daniele
Suggestions for a better plan? Add an another status enum? Nothing 
earlier in the sequence seems appropriate. And the init_early stack does 
not support returning error codes.

John.



+            return;
+        }
+
          __uc_fw_auto_select(i915, uc_fw);
          __uc_fw_user_override(i915, uc_fw);
      }








