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

[John Harrison <john.c.harrison@intel.com>]

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@intel.com>
>>> Cc: John Harrison <John.C.Harrison@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;
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@Intel.com>


> Daniele
>
>>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@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;
>>
>