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

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

[Daniele Ceraolo Spurio]

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.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;
+
+	/* 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);
+
+	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) {
+		ret = check_mtl_huc_guc_compatibility(gt, selected);
+		if (ret)
+			return ret;
+	}
 
 	if (!wanted->ver.major || !selected->ver.major)
 		return 0;
-- 
2.41.0

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.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;
> +
> +	/* 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.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;

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

[Ceraolo Spurio, Daniele]

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
>> 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;
>> +
>> +    /* 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.

>
>
>> +
>> +    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.
  */

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;
>

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

[John Harrison]

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;
>>
>