Re: [PATCH v2 5/8] drm/i915/huc: Copy huc rsa only once

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Wed, 24 Jul 2019 04:21:50 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> The binary is perma-pinned and the rsa is not going to change, so copy
> it only once and not on every load.

as this new location is accessible from the GuC, what if GuC (or whoever
else) corrupts it ? with stale RSA we will fail to authenticate HuC on
subsequent resets.

>
> v2: onion unwind (Chris)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Fernando Pacheco <fernando.pacheco@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> #v1
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 27 +++++++++++++++++++----
>  drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  1 -
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 17 --------------
>  3 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 7804ea5f699c..41f62bdf6022 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -50,6 +50,7 @@ static int intel_huc_rsa_data_create(struct intel_huc  
> *huc)
>  	struct intel_gt *gt = huc_to_gt(huc);
>  	struct intel_guc *guc = &gt->uc.guc;
>  	struct i915_vma *vma;
> +	size_t copied;
>  	void *vaddr;
> 	/*
> @@ -62,6 +63,7 @@ static int intel_huc_rsa_data_create(struct intel_huc  
> *huc)
>  	 * the authentication since its GGTT offset will be GuC
>  	 * accessible.
>  	 */
> +	GEM_BUG_ON(huc->fw.rsa_size > PAGE_SIZE);
>  	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
>  	if (IS_ERR(vma))
>  		return PTR_ERR(vma);
> @@ -72,26 +74,43 @@ static int intel_huc_rsa_data_create(struct  
> intel_huc *huc)
>  		return PTR_ERR(vaddr);
>  	}
> +	copied = intel_uc_fw_copy_rsa(&huc->fw, vaddr, vma->size);
> +	GEM_BUG_ON(copied < huc->fw.rsa_size);
> +
> +	i915_gem_object_unpin_map(vma->obj);
> +
>  	huc->rsa_data = vma;
> -	huc->rsa_data_vaddr = vaddr;
> 	return 0;
>  }
> static void intel_huc_rsa_data_destroy(struct intel_huc *huc)
>  {
> -	i915_vma_unpin_and_release(&huc->rsa_data, I915_VMA_RELEASE_MAP);
> +	i915_vma_unpin_and_release(&huc->rsa_data, 0);
>  }
> int intel_huc_init(struct intel_huc *huc)
>  {
>  	int err;
> -	err = intel_huc_rsa_data_create(huc);
> +	err = intel_uc_fw_init(&huc->fw);
>  	if (err)
>  		return err;
> -	return intel_uc_fw_init(&huc->fw);
> +	/*
> +	 * HuC firmware image is outside GuC accessible range.
> +	 * Copy the RSA signature out of the image into
> +	 * a perma-pinned region set aside for it
> +	 */
> +	err = intel_huc_rsa_data_create(huc);
> +	if (err)
> +		goto out_fini;
> +
> +	return 0;
> +
> +out_fini:
> +	intel_uc_fw_fini(&huc->fw);
> +	return err;
>  }
> void intel_huc_fini(struct intel_huc *huc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> index ea340f85bc46..4465209ce233 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> @@ -35,7 +35,6 @@ struct intel_huc {
> 	/* HuC-specific additions */
>  	struct i915_vma *rsa_data;
> -	void *rsa_data_vaddr;
> 	struct {
>  		i915_reg_t reg;
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> index 8f119ff291fa..f7049f0c7444 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -36,21 +36,6 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
>  	intel_uc_fw_init_early(huc_to_gt(huc)->i915, huc_fw,  
> INTEL_UC_FW_TYPE_HUC);
>  }
> -static void huc_xfer_rsa(struct intel_huc *huc)
> -{
> -	size_t copied;
> -
> -	/*
> -	 * HuC firmware image is outside GuC accessible range.
> -	 * Copy the RSA signature out of the image into
> -	 * the perma-pinned region set aside for it
> -	 */
> -	GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size);
> -	copied = intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr,
> -				      huc->rsa_data->size);
> -	GEM_BUG_ON(copied < huc->fw.rsa_size);
> -}
> -
>  static int huc_xfer_ucode(struct intel_huc *huc)
>  {
>  	struct intel_uc_fw *huc_fw = &huc->fw;
> @@ -110,8 +95,6 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw)
>  {
>  	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
> -	huc_xfer_rsa(huc);
> -
>  	return huc_xfer_ucode(huc);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx