Re: [v2,3/4] drm/i915/huc: Prepare for GSC-loaded HuC

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

Because i reviewed this already and the only new change is the relocation
of the function "huc_is_authenticated()" from Patch 1 to this patch while
maintaining the same logic as rev-1, thus:

Acked-by: Alan Previn <alan.previn.teres.alexis@intel.com>


On Wed, 2022-05-04 at 13:48 -0700, Daniele Ceraolo Spurio wrote:
> HuC loading via GSC is performed via a PXP command sent through the mei
> modules, so we need both MEI_GSC and MEI_PXP to be available. Given that
> the GSC will do both the transfer and the authentication, the legacy HuC
> loading paths can be safely skipped.
> Also note that the GSC-loaded HuC survives GT reset.
> 
> v2: move the huc_is_authenticated() function to this patch.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> #v1
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h |  1 +
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c     | 95 ++++++++++++++++++----
>  drivers/gpu/drm/i915/gt/uc/intel_huc.h     |  6 ++
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c  |  5 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c      | 11 ++-
>  5 files changed, 100 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
> index 66027a42cda9e..2516705b9f365 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
> @@ -96,6 +96,7 @@
>  
>  #define GUC_SHIM_CONTROL2		_MMIO(0xc068)
>  #define   GUC_IS_PRIVILEGED		(1<<29)
> +#define   GSC_LOADS_HUC			(1<<30)
>  
>  #define GUC_SEND_INTERRUPT		_MMIO(0xc4c8)
>  #define   GUC_SEND_TRIGGER		  (1<<0)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 7b759b99cf3c8..c36e2bf9b0f29 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -6,6 +6,7 @@
>  #include <linux/types.h>
>  
>  #include "gt/intel_gt.h"
> +#include "intel_guc_reg.h"
>  #include "intel_huc.h"
>  #include "i915_drv.h"
>  
> @@ -17,11 +18,15 @@
>   * capabilities by adding HuC specific commands to batch buffers.
>   *
>   * The kernel driver is only responsible for loading the HuC firmware and
> - * triggering its security authentication, which is performed by the GuC. For
> - * The GuC to correctly perform the authentication, the HuC binary must be
> - * loaded before the GuC one. Loading the HuC is optional; however, not using
> - * the HuC might negatively impact power usage and/or performance of media
> - * workloads, depending on the use-cases.
> + * triggering its security authentication, which is performed by the GuC on
> + * older platforms and by the GSC on newer ones. For the GuC to correctly
> + * perform the authentication, the HuC binary must be loaded before the GuC one.
> + * Loading the HuC is optional; however, not using the HuC might negatively
> + * impact power usage and/or performance of media workloads, depending on the
> + * use-cases.
> + * HuC must be reloaded on events that cause the WOPCM to lose its contents
> + * (S3/S4, FLR); GuC-authenticated HuC must also be reloaded on GuC/GT reset,
> + * while GSC-managed HuC will survive that.
>   *
>   * See https://github.com/intel/media-driver for the latest details on HuC
>   * functionality.
> @@ -54,11 +59,51 @@ void intel_huc_init_early(struct intel_huc *huc)
>  	}
>  }
>  
> +#define HUC_LOAD_MODE_STRING(x) (x ? "GSC" : "legacy")
> +static int check_huc_loading_mode(struct intel_huc *huc)
> +{
> +	struct intel_gt *gt = huc_to_gt(huc);
> +	bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc);
> +	bool hw_uses_gsc = false;
> +
> +	/*
> +	 * The fuse for HuC load via GSC is only valid on platforms that have
> +	 * GuC deprivilege.
> +	 */
> +	if (HAS_GUC_DEPRIVILEGE(gt->i915))
> +		hw_uses_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) &
> +			      GSC_LOADS_HUC;
> +
> +	if (fw_needs_gsc != hw_uses_gsc) {
> +		drm_err(&gt->i915->drm,
> +			"mismatch between HuC FW (%s) and HW (%s) load modes\n",
> +			HUC_LOAD_MODE_STRING(fw_needs_gsc),
> +			HUC_LOAD_MODE_STRING(hw_uses_gsc));
> +		return -ENOEXEC;
> +	}
> +
> +	/* make sure we can access the GSC via the mei driver if we need it */
> +	if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_INTEL_MEI_GSC)) &&
> +	    fw_needs_gsc) {
> +		drm_info(&gt->i915->drm,
> +			 "Can't load HuC due to missing MEI modules\n");
> +		return -EIO;
> +	}
> +
> +	drm_dbg(&gt->i915->drm, "GSC loads huc=%s\n", str_yes_no(fw_needs_gsc));
> +
> +	return 0;
> +}
> +
>  int intel_huc_init(struct intel_huc *huc)
>  {
>  	struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
>  	int err;
>  
> +	err = check_huc_loading_mode(huc);
> +	if (err)
> +		goto out;
> +
>  	err = intel_uc_fw_init(&huc->fw);
>  	if (err)
>  		goto out;
> @@ -96,17 +141,20 @@ int intel_huc_auth(struct intel_huc *huc)
>  	struct intel_guc *guc = &gt->uc.guc;
>  	int ret;
>  
> -	GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw));
> -
>  	if (!intel_uc_fw_is_loaded(&huc->fw))
>  		return -ENOEXEC;
>  
> +	/* GSC will do the auth */
> +	if (intel_huc_is_loaded_by_gsc(huc))
> +		return -ENODEV;
> +
>  	ret = i915_inject_probe_error(gt->i915, -ENXIO);
>  	if (ret)
>  		goto fail;
>  
> -	ret = intel_guc_auth_huc(guc,
> -				 intel_guc_ggtt_offset(guc, huc->fw.rsa_data));
> +	GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw));
> +
> +	ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(guc, huc->fw.rsa_data));
>  	if (ret) {
>  		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
>  		goto fail;
> @@ -133,6 +181,18 @@ int intel_huc_auth(struct intel_huc *huc)
>  	return ret;
>  }
>  
> +static bool huc_is_authenticated(struct intel_huc *huc)
> +{
> +	struct intel_gt *gt = huc_to_gt(huc);
> +	intel_wakeref_t wakeref;
> +	u32 status = 0;
> +
> +	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> +		status = intel_uncore_read(gt->uncore, huc->status.reg);
> +
> +	return (status & huc->status.mask) == huc->status.value;
> +}
> +
>  /**
>   * intel_huc_check_status() - check HuC status
>   * @huc: intel_huc structure
> @@ -150,10 +210,6 @@ int intel_huc_auth(struct intel_huc *huc)
>   */
>  int intel_huc_check_status(struct intel_huc *huc)
>  {
> -	struct intel_gt *gt = huc_to_gt(huc);
> -	intel_wakeref_t wakeref;
> -	u32 status = 0;
> -
>  	switch (__intel_uc_fw_status(&huc->fw)) {
>  	case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
>  		return -ENODEV;
> @@ -167,10 +223,17 @@ int intel_huc_check_status(struct intel_huc *huc)
>  		break;
>  	}
>  
> -	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> -		status = intel_uncore_read(gt->uncore, huc->status.reg);
> +	return huc_is_authenticated(huc);
> +}
>  
> -	return (status & huc->status.mask) == huc->status.value;
> +void intel_huc_update_auth_status(struct intel_huc *huc)
> +{
> +	if (!intel_uc_fw_is_loadable(&huc->fw))
> +		return;
> +
> +	if (huc_is_authenticated(huc))
> +		intel_uc_fw_change_status(&huc->fw,
> +					  INTEL_UC_FIRMWARE_RUNNING);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> index 77d813840d76c..d7e25b6e879eb 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> @@ -27,6 +27,7 @@ int intel_huc_init(struct intel_huc *huc);
>  void intel_huc_fini(struct intel_huc *huc);
>  int intel_huc_auth(struct intel_huc *huc);
>  int intel_huc_check_status(struct intel_huc *huc);
> +void intel_huc_update_auth_status(struct intel_huc *huc);
>  
>  static inline int intel_huc_sanitize(struct intel_huc *huc)
>  {
> @@ -50,6 +51,11 @@ static inline bool intel_huc_is_used(struct intel_huc *huc)
>  	return intel_uc_fw_is_available(&huc->fw);
>  }
>  
> +static inline bool intel_huc_is_loaded_by_gsc(const struct intel_huc *huc)
> +{
> +	return huc->fw.loaded_via_gsc;
> +}
> +
>  void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p);
>  
>  #endif
> 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 e5ef509c70e89..9d6ab1e016395 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -8,7 +8,7 @@
>  #include "i915_drv.h"
>  
>  /**
> - * intel_huc_fw_upload() - load HuC uCode to device
> + * intel_huc_fw_upload() - load HuC uCode to device via DMA transfer
>   * @huc: intel_huc structure
>   *
>   * Called from intel_uc_init_hw() during driver load, resume from sleep and
> @@ -21,6 +21,9 @@
>   */
>  int intel_huc_fw_upload(struct intel_huc *huc)
>  {
> +	if (intel_huc_is_loaded_by_gsc(huc))
> +		return -ENODEV;
> +
>  	/* HW doesn't look at destination address for HuC, so set it to 0 */
>  	return intel_uc_fw_upload(&huc->fw, 0, HUC_UKERNEL);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 8c9ef690ac9d8..0dce94f896a8c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -509,7 +509,16 @@ static int __uc_init_hw(struct intel_uc *uc)
>  	if (ret)
>  		goto err_log_capture;
>  
> -	intel_huc_auth(huc);
> +	/*
> +	 * GSC-loaded HuC is authenticated by the GSC, so we don't need to
> +	 * trigger the auth here. However, given that the HuC loaded this way
> +	 * survive GT reset, we still need to update our SW bookkeeping to make
> +	 * sure it reflects the correct HW status.
> +	 */
> +	if (intel_huc_is_loaded_by_gsc(huc))
> +		intel_huc_update_auth_status(huc);
> +	else
> +		intel_huc_auth(huc);
>  
>  	if (intel_uc_uses_guc_submission(uc))
>  		intel_guc_submission_enable(guc);