On Thu, Jan 12, 2023 at 05:18: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.ale...@intel.com>
> Reviewed-by: Juston Li <justo...@chromium.org>
> ---
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 60 +++++++++++++++++---
> 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 | 9 ++-
> drivers/gpu/drm/i915/pxp/intel_pxp_session.h | 5 ++
> 5 files changed, 64 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..f3d9e7989eb7 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -270,6 +270,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 use 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);
> + }
I really don't see why you need 1 function for totally 2 different cases.
Why not 2 functions then?
> +
> + 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_locked(pxp, true))
> + 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 +335,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 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..d8278c4002e3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -115,11 +115,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;
o.O
pxp->hw_state_invalidate = restart_arb;
?
or even a better name for the restart_arb to already indicate that is
the hw_state_invalidate ?
>
> /*
> * if we fail to submit the termination there is no point in waiting for
> @@ -167,7 +170,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..4f944b63b5b6 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 restart_arb);
> #else
> static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
> {
> }
> +
> +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool
> restart_arb)
> +{
> +}
> #endif
> #endif /* __INTEL_PXP_SESSION_H__ */
> --
> 2.39.0
>
