[Intel-gfx] [PATCH] drm/i915/huc: check HuC and GuC version compatibility on MTL
John Harrison
john.c.harrison at intel.com
Mon Jul 17 18:18:12 UTC 2023
On 7/12/2023 10:03, Ceraolo Spurio, Daniele wrote:
> On 7/12/2023 3:03 AM, Andrzej Hajda wrote:
>> 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
synchronously
>>> 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.ceraolospurio at intel.com>
>>> Cc: John Harrison <John.C.Harrison at 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 =
>>> >->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;
Could put both of these bools on a single line.
>>> +
>>> + /* 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.
>
> We've been trying to avoid those kind of macros because the version
> would need to be a u64 under the hood (each version number is a u16)
> and therefore type casting would be required to make all the shifting
> work, which makes the macro nasty to look at and as you said IMO not
> worth it for just 2 checks. Note that the GuC is the exception because
> it guarantees its version fits in a u32, so there is some macro use in
> the GuC-specific code.
Pretty sure I did originally try to go the u64 version route but it
caused a lot more problems than it solved. I forget the details but in
addition to all the extra casting mentioned above, I vaguely recall
there issues with 32bit compilers/architectures or some such. Hence we
only have the 8bit-per-version-component/32bit-merged macros that are
for use with the GuC version and only the GuC version.
Given that this is (hopefully) a one off hack to cope with a one off
bug, I would stick with the unrolled code rather than adding extra
complications.
>
>>
>>
>>> +
>>> + 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.
>
> This will hopefully never apply to any other platform. This is a light
> breach of the HuC compatibility contract, so I really don't want to
> have a generic function to handle it. I want it to be clear from a
> higher level that this is an exception for a specific platform. Maybe
> worth adding a comment? Would something like the following make things
> clearer?
>
> /*
> * MTL has some compatibility issues with early GuC/HuC binaries
> * not working with newer ones. This is specific to MTL and we
> * don't expect it to extend to other platforms.
> */
>
I agree with Daniele about keeping this the exception not the norm. The
comment works for me.
Typo in commit message and a declaration nit-pick but otherwise:
Reviewed-by: John Harrison <John.C.Harrison at Intel.com>
> Daniele
>
>>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda at 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;
>>
>
More information about the dri-devel
mailing list