Re: [Intel-gfx] [PATCH v6 5/6] drm/i915/pxp: Trigger the global teardown for before suspending

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jan 23, 2023 at 09:31:49PM -0800, Alan Previn wrote:
> A driver bug was recently discovered where the security firmware was
> receiving internal HW signals indicating that session key expirations
> had occurred. Architecturally, the firmware was expecting a response
> from the GuC to acknowledge the event with the firmware side.
> However the OS was in a suspended state and GuC had been reset.
> 
> Internal specifications actually required the driver to ensure
> that all active sessions be properly cleaned up in such cases where
> the system is suspended and the GuC potentially unable to respond.
> 
> This patch adds the global teardown code in i915's suspend_prepare
> code path.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Reviewed-by: Juston Li <justonli@chromium.org>

it should probably contain some revision history above so the
reviewer can know what changed from the time he reviewed and now.

it looks we have another function that I don't remember...
Juston, could you please confirm your review stands in this version?
if so feel free to add my ack with that.

> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 65 +++++++++++++++++---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  1 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  2 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  6 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
>  5 files changed, 66 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index cfc9af8b3d21..9d4c7724e98e 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -270,6 +270,60 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
>  	return bound;
>  }
>  
> +static int __pxp_global_teardown_final(struct intel_pxp *pxp)
> +{
> +	if (!pxp->arb_is_valid)
> +		return 0;
> +	/*
> +	 * To ensure synchronous and coherent session teardown completion
> +	 * in response to suspend or shutdown triggers, don't use a worker.
> +	 */
> +	intel_pxp_mark_termination_in_progress(pxp);
> +	intel_pxp_terminate(pxp, false);
> +
> +	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
> +{
> +	if (pxp->arb_is_valid)
> +		return 0;
> +	/*
> +	 * The arb-session is currently inactive and we are doing a reset and restart
> +	 * due to a runtime event. Use the worker that was designed for this.
> +	 */
> +	pxp_queue_termination(pxp);
> +
> +	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +void intel_pxp_end(struct intel_pxp *pxp)
> +{
> +	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> +	intel_wakeref_t wakeref;
> +
> +	if (!intel_pxp_is_enabled(pxp))
> +		return;
> +
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	mutex_lock(&pxp->arb_mutex);
> +
> +	if (__pxp_global_teardown_final(pxp))
> +		drm_dbg(&i915->drm, "PXP end timed out\n");
> +
> +	mutex_unlock(&pxp->arb_mutex);
> +
> +	intel_pxp_fini_hw(pxp);
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +}
> +
>  /*
>   * the arb session is restarted from the irq work when we receive the
>   * termination completion interrupt
> @@ -286,16 +340,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
>  
>  	mutex_lock(&pxp->arb_mutex);
>  
> -	if (pxp->arb_is_valid)
> -		goto unlock;
> -
> -	pxp_queue_termination(pxp);
> -
> -	if (!wait_for_completion_timeout(&pxp->termination,
> -					msecs_to_jiffies(250))) {
> -		ret = -ETIMEDOUT;
> +	ret = __pxp_global_teardown_restart(pxp);
> +	if (ret)
>  		goto unlock;
> -	}
>  
>  	/* make sure the compiler doesn't optimize the double access */
>  	barrier();
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 9658d3005222..3ded0890cd27 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
>  void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
>  
>  int intel_pxp_start(struct intel_pxp *pxp);
> +void intel_pxp_end(struct intel_pxp *pxp);
>  
>  int intel_pxp_key_check(struct intel_pxp *pxp,
>  			struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index 892d39cc61c1..e427464aa131 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
>  	if (!intel_pxp_is_enabled(pxp))
>  		return;
>  
> -	pxp->arb_is_valid = false;
> +	intel_pxp_end(pxp);
>  
>  	intel_pxp_invalidate(pxp);
>  }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index 74ed7e16e481..448cacb0465d 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -115,11 +115,11 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>  	return ret;
>  }
>  
> -static void pxp_terminate(struct intel_pxp *pxp)
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart)
>  {
>  	int ret;
>  
> -	pxp->hw_state_invalidated = true;
> +	pxp->hw_state_invalidated = post_invalidation_needs_restart;
>  
>  	/*
>  	 * if we fail to submit the termination there is no point in waiting for
> @@ -167,7 +167,7 @@ static void pxp_session_work(struct work_struct *work)
>  
>  	if (events & PXP_TERMINATION_REQUEST) {
>  		events &= ~PXP_TERMINATION_COMPLETE;
> -		pxp_terminate(pxp);
> +		intel_pxp_terminate(pxp, true);
>  	}
>  
>  	if (events & PXP_TERMINATION_COMPLETE)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> index 903ac52cffa1..ba5788127109 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> @@ -12,9 +12,14 @@ struct intel_pxp;
>  
>  #ifdef CONFIG_DRM_I915_PXP
>  void intel_pxp_session_management_init(struct intel_pxp *pxp);
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart);
>  #else
>  static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
>  {
>  }
> +
> +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool post_invalidation_needs_restart)
> +{
> +}
>  #endif
>  #endif /* __INTEL_PXP_SESSION_H__ */
> -- 
> 2.39.0
>