Re: [Intel-gfx] [PATCH] drm/i915/huc: check HuC and GuC version compatibility on MTL

[Andrzej Hajda]

On 11.07.2023 22:31, Daniele Ceraolo Spurio wrote:
Due to a change in the auth flow on MTL, GuC 70.7.0 and newer will only
be able to authenticate HuC 8.5.1 and newer. The plan is to update the 2
binaries sinchronously in linux-firmware so that the fw repo always has
a matching pair that works; still, it's better to check in the kernel so
we can print an error message and abort HuC loading if the binaries are
out of sync instead of failing the authentication.
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
Cc: John Harrison <john.c.harri...@intel.com>
---
  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 42 ++++++++++++++++++++++++
  1 file changed, 42 insertions(+)

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 08e16017584b..f0cc5bb47fa0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -803,11 +803,53 @@ static int try_firmware_load(struct intel_uc_fw *uc_fw, 
const struct firmware **
        return 0;
  }
  
+static int check_mtl_huc_guc_compatibility(struct intel_gt *gt,
+                                          struct intel_uc_fw_file 
*huc_selected)
+{
+       struct intel_uc_fw_file *guc_selected = &gt->uc.guc.fw.file_selected;
+       struct intel_uc_fw_ver *huc_ver = &huc_selected->ver;
+       struct intel_uc_fw_ver *guc_ver = &guc_selected->ver;
+       bool new_huc;
+       bool new_guc;
+
+       /* we can only do this check after having fetched both GuC and HuC */
+       GEM_BUG_ON(!huc_selected->path || !guc_selected->path);
+
+       /*
+        * Due to changes in the authentication flow for MTL, HuC 8.5.1 or newer
+        * requires GuC 70.7.0 or newer. Older HuC binaries will instead require
+        * GuC < 70.7.0.
+        */
+       new_huc = huc_ver->major > 8 ||
+                 (huc_ver->major == 8 && huc_ver->minor > 5) ||
+                 (huc_ver->major == 8 && huc_ver->minor == 5 && huc_ver->patch 
>= 1);
+
+       new_guc = guc_ver->major > 70 ||
+                 (guc_ver->major == 70 && guc_ver->minor >= 7);

Wouldn't be more readable to define sth like UC_VER_FULL(v)
then use UC_VER_FULL(huc_ver) >= IP_VER_FULL(8, 5, 1).
I am not sure if it is worth for two checks.


+
+       if (new_huc != new_guc) {
+               UNEXPECTED(gt, "HuC %u.%u.%u is incompatible with GuC 
%u.%u.%u\n",
+                          huc_ver->major, huc_ver->minor, huc_ver->patch,
+                          guc_ver->major, guc_ver->minor, guc_ver->patch);
+               gt_info(gt, "MTL GuC 70.7.0+ and HuC 8.5.1+ don't work with older 
releases\n");
+               return -ENOEXEC;
+       }
+
+       return 0;
+}
+
  int intel_uc_check_file_version(struct intel_uc_fw *uc_fw, bool *old_ver)
  {
        struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
        struct intel_uc_fw_file *wanted = &uc_fw->file_wanted;
        struct intel_uc_fw_file *selected = &uc_fw->file_selected;
+       int ret;
+
+       if (IS_METEORLAKE(gt->i915) && uc_fw->type == INTEL_UC_FW_TYPE_HUC) {

Moving this check inside check function would make it more generic, up 
to you.

Reviewed-by: Andrzej Hajda <andrzej.ha...@intel.com>

Regards
Andrzej


+               ret = check_mtl_huc_guc_compatibility(gt, selected);
+               if (ret)
+                       return ret;
+       }
  
  	if (!wanted->ver.major || !selected->ver.major)
                return 0;