From 14be93236158db86b68a173b3460fe71c1906e58 Mon Sep 17 00:00:00 2001
From: John Harrison <John.C.Harrison@Intel.com>
Date: Tue, 2 May 2023 16:40:05 -0700
Subject: drm/i915/uc: Enhancements to firmware table validation
Git-commit: edba77891b58a1f0626daf8598717b8efb307bc8
Patch-mainline: v6.5-rc1
References: jsc#PED-3527 jsc#PED-5475 jsc#PED-6068 jsc#PED-6070 jsc#PED-6116 jsc#PED-6120 jsc#PED-5065 jsc#PED-5477 jsc#PED-5511 jsc#PED-6041 jsc#PED-6069 jsc#PED-6071
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.
This requires adding a firmware global flag to indicate an issue with
any of the per firmware tables. This is done rather than adding a new
state enum as a new enum value would be a much more invasive change -
lots of places would need updating to support the new error state.
Note also that this change means that a table error will cause the
driver to wedge even on platforms that don't require firmware files.
This is intentional as per the above backport concern - someone doing
backports is not guaranteed to test on every platform that they may
potential affect. So forcing a failure on all platforms ensures that
the problem will be noticed and corrected immediately.
v2: Change to unconditionally fail module load on a validation error
(review feedback/discussion with Daniele).
v3: Add a new flag to track table validation errors (review
feedback/discussion with Daniele).
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230502234007.1762014-5-John.C.Harrison@Intel.com
Acked-by: Patrik Jakobsson <pjakobsson@suse.de>
---
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 3 +
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 +
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 161 +++++++++++++----------
3 files changed, 99 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 996168312340..1381943b8973 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -432,6 +432,9 @@ static bool uc_is_wopcm_locked(struct intel_uc *uc)
static int __uc_check_hw(struct intel_uc *uc)
{
+ if (uc->fw_table_invalid)
+ return -EIO;
+
if (!intel_uc_supports_guc(uc))
return 0;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 5d0f1bcc381e..d585524d94de 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -36,6 +36,7 @@ struct intel_uc {
struct drm_i915_gem_object *load_err_log;
bool reset_in_progress;
+ bool fw_table_invalid;
};
void intel_uc_init_early(struct intel_uc *uc);
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 10e48cbcf494..b9ab370ce984 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)
@@ -430,7 +452,8 @@ static void __uc_fw_user_override(struct drm_i915_private *i915, struct intel_uc
void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
enum intel_uc_fw_type type)
{
- struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, type)->i915;
+ struct intel_gt *gt = ____uc_fw_to_gt(uc_fw, type);
+ struct drm_i915_private *i915 = gt->i915;
/*
* we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
@@ -443,6 +466,12 @@ 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)) {
+ gt->uc.fw_table_invalid = true;
+ intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_NOT_SUPPORTED);
+ return;
+ }
+
__uc_fw_auto_select(i915, uc_fw);
__uc_fw_user_override(i915, uc_fw);
}
--
2.42.0