[Intel-gfx] [PATCH 7/7] drm/i915/uc: Hardening firmware fetch
Chris Wilson
chris at chris-wilson.co.uk
Wed Aug 7 17:45:23 UTC 2019
Quoting Michal Wajdeczko (2019-08-07 18:00:34)
> Insert few more failure points into firmware fetch procedure to check
> use of the wrong blob name or use of the mismatched firmware versions.
>
> Also update some messages (remove ptr, duplicated infos) and stop
> treating all fetch errors as missing firmware case.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 137 ++++++++++++++++-------
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 3 +
> 2 files changed, 97 insertions(+), 43 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 3a3803bfa5a8..4faff1010398 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -153,20 +153,23 @@ static const char *__override_huc_firmware_path(void)
> return "";
> }
>
> -static bool
> -__uc_fw_override(struct intel_uc_fw *uc_fw)
> +static void __uc_fw_user_override(struct intel_uc_fw *uc_fw)
> {
> + const char *path = NULL;
> +
> switch (uc_fw->type) {
> case INTEL_UC_FW_TYPE_GUC:
> - uc_fw->path = __override_guc_firmware_path();
> + path = __override_guc_firmware_path();
> break;
> case INTEL_UC_FW_TYPE_HUC:
> - uc_fw->path = __override_huc_firmware_path();
> + path = __override_huc_firmware_path();
> break;
> }
>
> - uc_fw->user_overridden = uc_fw->path;
> - return uc_fw->user_overridden;
> + if (unlikely(path)) {
> + uc_fw->path = path;
> + uc_fw->user_overridden = true;
> + }
The code got longer for no real improvement in clarity?
Oh, because now uc_fw->path may be non-NULL on entry due to call
reordering.
> }
>
> /**
> @@ -194,8 +197,10 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>
> uc_fw->type = type;
>
> - if (supported && likely(!__uc_fw_override(uc_fw)))
> + if (supported) {
> __uc_fw_auto_select(uc_fw, platform, rev);
> + __uc_fw_user_override(uc_fw);
> + }
>
> if (uc_fw->path && *uc_fw->path)
> uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
> @@ -203,6 +208,41 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
> uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> }
>
> +static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw,
> + struct drm_i915_private *i915, bool user)
> +{
> + int e = user ? -EINVAL : -ESTALE;
> +
> + if (i915_inject_load_error(i915, e)) {
> + /* non-existing blob */
> + uc_fw->path = "<invalid>";
> + uc_fw->user_overridden = user;
> + } else if (i915_inject_load_error(i915, e)) {
> + /* require next major version */
> + uc_fw->major_ver_wanted += 1;
> + uc_fw->minor_ver_wanted = 0;
> + uc_fw->user_overridden = user;
> + } else if (i915_inject_load_error(i915, e)) {
> + /* require next minor version */
> + uc_fw->minor_ver_wanted += 1;
> + uc_fw->user_overridden = user;
> + } else if (uc_fw->major_ver_wanted && i915_inject_load_error(i915, e)) {
> + /* require prev major version */
> + uc_fw->major_ver_wanted -= 1;
> + uc_fw->minor_ver_wanted = 0;
> + uc_fw->user_overridden = user;
> + } else if (uc_fw->minor_ver_wanted && i915_inject_load_error(i915, e)) {
> + /* require prev minor version - hey, this should work! */
> + uc_fw->minor_ver_wanted -= 1;
> + uc_fw->user_overridden = user;
> + } else if (user && i915_inject_load_error(i915, e)) {
> + /* officially unsupported platform */
> + uc_fw->major_ver_wanted = 0;
> + uc_fw->minor_ver_wanted = 0;
> + uc_fw->user_overridden = true;
> + }
Taking self-abuse to a whole new level :)
> +}
> +
> /**
> * intel_uc_fw_fetch - fetch uC firmware
> * @uc_fw: uC firmware
> @@ -222,17 +262,22 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
>
> GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
>
> + err = i915_inject_load_error(i915, -ENXIO);
> + if (err)
> + return err;
> +
> + __force_fw_fetch_failures(uc_fw, i915, true);
> + __force_fw_fetch_failures(uc_fw, i915, false);
Ok. The set of major/minor/user_override make some sort of sense to me,
but I couldn't say if that's a complete set or not.
> @@ -292,29 +343,22 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
> break;
> }
>
> - DRM_DEBUG_DRIVER("%s fw version %u.%u (wanted %u.%u)\n",
> - intel_uc_fw_type_repr(uc_fw->type),
> - uc_fw->major_ver_found, uc_fw->minor_ver_found,
> - uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> -
> - if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) {
> - DRM_NOTE("%s: Skipping firmware version check\n",
> - intel_uc_fw_type_repr(uc_fw->type));
> - } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> - uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> - DRM_NOTE("%s: Wrong firmware version (%u.%u, required %u.%u)\n",
> - intel_uc_fw_type_repr(uc_fw->type),
> + if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> + uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> + dev_warn(i915->drm.dev,
Still a dev_notice(), especially as this is still user controllable.
> + "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> + intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
> uc_fw->major_ver_found, uc_fw->minor_ver_found,
> uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> - err = -ENOEXEC;
> - goto fail;
> + if (!intel_uc_fw_is_overridden(uc_fw)) {
> + err = -ENOEXEC;
> + goto fail;
> + }
> }
> - DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
> + dev_info(i915->drm.dev, "%s firmware %s: fetch failed with error %d\n",
> intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
WARN -> info ? Not at least a notice?
> - DRM_INFO("%s: Firmware can be downloaded from %s\n",
> + dev_info(i915->drm.dev, "%s firmware(s) can be downloaded from %s\n",
> intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
>
> release_firmware(fw); /* OK even if fw is NULL */
Just a question of log levels.
-Chris
More information about the Intel-gfx
mailing list