Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/pxp: Trigger the global teardown for before suspending
On Mon, 2022-11-28 at 16:48 -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
> ---
> 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 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(>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(>runtime_pm);
> +
> + mutex_lock(>arb_mutex);
> +
> + if (__pxp_global_teardown_locked(pxp, true))
> + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end timed
> out\n");
> +
> + mutex_unlock(>arb_mutex);
> +
> + intel_pxp_fini_hw(pxp);
> + intel_runtime_pm_put(>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(>arb_mutex);
>
> - if (pxp->arb_is_valid)
> - goto unlock;
> -
> - pxp_queue_termination(pxp);
> -
> - if (!wait_for_completion_timeout(>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 bbeb8ed8e211..a06b65850246 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -26,6 +26,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 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-gfx] [PATCH v2 2/3] drm/i915/pxp: Trigger the global teardown for before suspending
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
---
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 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(>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(>runtime_pm);
+
+ mutex_lock(>arb_mutex);
+
+ if (__pxp_global_teardown_locked(pxp, true))
+ drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end timed out\n");
+
+ mutex_unlock(>arb_mutex);
+
+ intel_pxp_fini_hw(pxp);
+ intel_runtime_pm_put(>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(>arb_mutex);
- if (pxp->arb_is_valid)
- goto unlock;
-
- pxp_queue_termination(pxp);
-
- if (!wait_for_completion_timeout(>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 bbeb8ed8e211..a06b65850246 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -26,6 +26,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 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 8eb886d3f2a0..794ccdd00bb9 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
