[Intel-gfx] [PATCH 2/2] drm/i915/pxp: Trigger the global teardown for before suspending

[Juston Li]

On Thu, 2022-11-17 at 16:36 -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 at intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++-
> --
>  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  2 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  2 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  9 ++-
>  4 files changed, 60 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 5efe61f67546..659410ae1b89 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -198,6 +198,55 @@ static bool pxp_component_bound(struct intel_pxp
> *pxp)
>         return bound;
>  }
>  
> +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool
> terminate_for_cleanup)
> +{
> +       if (terminate_for_cleanup) {
> +               if (!pxp->arb_is_valid)
> +                       return 0;
> +               /*
> +                * To ensure synchronous and coherent session
> teardown completion
> +                * in response to suspend or shutdown triggers, don't
> user a worker.
> +                */
> +               intel_pxp_mark_termination_in_progress(pxp);
> +               intel_pxp_terminate(pxp, false);
> +       } else {
> +               if (pxp->arb_is_valid)
> +                       return 0;
> +               /*
> +                * If we are not in final termination, and the arb-
> session is currently
> +                * inactive, we are doing a reset and restart due to
> some 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_to_gt(pxp)->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_locked(pxp, true))
> +               drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end timed
> out\n");
> +
> +       mutex_unlock(&pxp->arb_mutex);
> +
> +       intel_pxp_fini_hw(pxp);

Is intel_pxp_suspend() still needed then if we already fini_hw() here
and mark invalidation in intel_pxp_terminate()?

> +       intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +}
> +
>  /*
>   * the arb session is restarted from the irq work when we receive
> the
>   * termination completion interrupt
> @@ -214,16 +263,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_locked(pxp, false);
> +       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 6ba8fa5bfea0..d001828b3372 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -26,6 +26,8 @@ void intel_pxp_mark_termination_in_progress(struct
> intel_pxp *pxp);
>  void intel_pxp_tee_end_all_fw_sessions(struct intel_pxp *pxp, u32
> sessions_mask);
>  
>  int intel_pxp_start(struct intel_pxp *pxp);
> +void intel_pxp_end(struct intel_pxp *pxp);
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb);
>  
>  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 6a7d4e2ee138..36af52c28e63 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -14,7 +14,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 85e404b5ad0e..fdf30554d80f 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -118,11 +118,14 @@ 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 restart_arb)
>  {
>         int ret;
>  
> -       pxp->hw_state_invalidated = true;
> +       if (restart_arb)
> +               pxp->hw_state_invalidated = true;
> +       else
> +               pxp->hw_state_invalidated = false;
>  
>         /*
>          * if we fail to submit the termination there is no point in
> waiting for
> @@ -170,7 +173,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)