Re: [Intel-gfx] [RFC-v1 08/16] drm/i915/pxp: Create the arbitrary session after boot
I have removed the dead code that specific for multi-session, and will upload
the new version soon.
-Original Message-
From: Joonas Lahtinen
Sent: Monday, December 7, 2020 4:01 AM
To: Huang, Sean Z ; Intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v1 08/16] drm/i915/pxp: Create the arbitrary
session after boot
Quoting Huang, Sean Z (2020-12-07 02:21:26)
> Create the arbitrary session, with the fixed session id 0xf, after
> system boot, for the case that application allocates the protected
> buffer without establishing any protection session. Because the
> hardware requires at least one alive session for protected buffer
> creation. This arbitrary session needs to be re-created after
> teardown or power event because hardware encryption key won't be valid
> after such cases.
>
> Signed-off-by: Huang, Sean Z
Creating the arbitary (default) session only utilizes a minimal amount of the
session management related code introduced by this and previous patches.
All of that dead code needs to be eliminated first, then we need to look at
what level of complexity can be eliminated from the patches.
If you can address the review comments from the earlier patch and re-order the
series according to the given guidance, that'll make the review much more
efficient going forward when the code is only added when it used
Regards, Joonas
> ---
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 47 ++-
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 7 +
> drivers/gpu/drm/i915/pxp/intel_pxp_sm.c | 165 +++
> drivers/gpu/drm/i915/pxp/intel_pxp_sm.h | 8 ++
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 34 +
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.h | 11 ++
> 6 files changed, 271 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 332d9baff29f..10f4b1de07c4 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -9,6 +9,43 @@
> #include "intel_pxp_sm.h"
> #include "intel_pxp_tee.h"
>
> +int intel_pxp_create_arb_session(struct drm_i915_private *i915) {
> + struct pxp_tag pxptag;
> + int ret;
> +
> + lockdep_assert_held(>pxp.ctx->ctx_mutex);
> +
> + if (i915->pxp.ctx->flag_display_hm_surface_keys) {
> + drm_err(>drm, "%s: arb session is alive so skipping the
> creation\n",
> + __func__);
> + return 0;
> + }
> +
> + ret = intel_pxp_sm_reserve_arb_session(i915, );
> + if (ret) {
> + drm_err(>drm, "Failed to reserve session\n");
> + goto end;
> + }
> +
> + ret = intel_pxp_tee_cmd_create_arb_session(i915);
> + if (ret) {
> + drm_err(>drm, "Failed to send tee cmd for arb session
> creation\n");
> + goto end;
> + }
> +
> + ret = pxp_sm_mark_protected_session_in_play(i915, ARB_SESSION_TYPE,
> pxptag.session_id);
> + if (ret) {
> + drm_err(>drm, "Failed to mark session status in
> play\n");
> + goto end;
> + }
> +
> + i915->pxp.ctx->flag_display_hm_surface_keys = true;
> +
> +end:
> + return ret;
> +}
> +
> static void intel_pxp_write_irq_mask_reg(struct drm_i915_private
> *i915, u32 mask) {
> /* crypto mask is in bit31-16 (Engine1 Interrupt Mask) */ @@
> -47,9 +84,17 @@ static int
> intel_pxp_global_terminate_complete_callback(struct drm_i915_private
>
> mutex_lock(>pxp.ctx->ctx_mutex);
>
> - if (i915->pxp.ctx->global_state_attacked)
> + if (i915->pxp.ctx->global_state_attacked) {
> i915->pxp.ctx->global_state_attacked = false;
>
> + /* Re-create the arb session after teardown handle complete */
> + ret = intel_pxp_create_arb_session(i915);
> + if (ret) {
> + drm_err(>drm, "Failed to create arb session\n");
> + goto end;
> + }
> + }
> +end:
> mutex_unlock(>pxp.ctx->ctx_mutex);
>
> return ret;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 308d8d312a6d..e5f6e2b1bdfd 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -41,6 +41,8 @@ struct intel_gt;
> struct drm_i915_private;
>
> #ifdef CONFIG_DRM_I915_PXP
> +int intel_pxp_create_arb_session(struct drm_i915_private *i915);
> +
> voi
Re: [Intel-gfx] [RFC-v1 08/16] drm/i915/pxp: Create the arbitrary session after boot
Quoting Huang, Sean Z (2020-12-07 02:21:26)
> Create the arbitrary session, with the fixed session id 0xf, after
> system boot, for the case that application allocates the protected
> buffer without establishing any protection session. Because the
> hardware requires at least one alive session for protected buffer
> creation. This arbitrary session needs to be re-created after
> teardown or power event because hardware encryption key won't be
> valid after such cases.
>
> Signed-off-by: Huang, Sean Z
Creating the arbitary (default) session only utilizes a minimal amount
of the session management related code introduced by this and previous
patches.
All of that dead code needs to be eliminated first, then we need to look
at what level of complexity can be eliminated from the patches.
If you can address the review comments from the earlier patch and
re-order the series according to the given guidance, that'll make the
review much more efficient going forward when the code is only added
when it used
Regards, Joonas
> ---
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 47 ++-
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 7 +
> drivers/gpu/drm/i915/pxp/intel_pxp_sm.c | 165 +++
> drivers/gpu/drm/i915/pxp/intel_pxp_sm.h | 8 ++
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 34 +
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.h | 11 ++
> 6 files changed, 271 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 332d9baff29f..10f4b1de07c4 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -9,6 +9,43 @@
> #include "intel_pxp_sm.h"
> #include "intel_pxp_tee.h"
>
> +int intel_pxp_create_arb_session(struct drm_i915_private *i915)
> +{
> + struct pxp_tag pxptag;
> + int ret;
> +
> + lockdep_assert_held(>pxp.ctx->ctx_mutex);
> +
> + if (i915->pxp.ctx->flag_display_hm_surface_keys) {
> + drm_err(>drm, "%s: arb session is alive so skipping the
> creation\n",
> + __func__);
> + return 0;
> + }
> +
> + ret = intel_pxp_sm_reserve_arb_session(i915, );
> + if (ret) {
> + drm_err(>drm, "Failed to reserve session\n");
> + goto end;
> + }
> +
> + ret = intel_pxp_tee_cmd_create_arb_session(i915);
> + if (ret) {
> + drm_err(>drm, "Failed to send tee cmd for arb session
> creation\n");
> + goto end;
> + }
> +
> + ret = pxp_sm_mark_protected_session_in_play(i915, ARB_SESSION_TYPE,
> pxptag.session_id);
> + if (ret) {
> + drm_err(>drm, "Failed to mark session status in
> play\n");
> + goto end;
> + }
> +
> + i915->pxp.ctx->flag_display_hm_surface_keys = true;
> +
> +end:
> + return ret;
> +}
> +
> static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
> mask)
> {
> /* crypto mask is in bit31-16 (Engine1 Interrupt Mask) */
> @@ -47,9 +84,17 @@ static int
> intel_pxp_global_terminate_complete_callback(struct drm_i915_private
>
> mutex_lock(>pxp.ctx->ctx_mutex);
>
> - if (i915->pxp.ctx->global_state_attacked)
> + if (i915->pxp.ctx->global_state_attacked) {
> i915->pxp.ctx->global_state_attacked = false;
>
> + /* Re-create the arb session after teardown handle complete */
> + ret = intel_pxp_create_arb_session(i915);
> + if (ret) {
> + drm_err(>drm, "Failed to create arb session\n");
> + goto end;
> + }
> + }
> +end:
> mutex_unlock(>pxp.ctx->ctx_mutex);
>
> return ret;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 308d8d312a6d..e5f6e2b1bdfd 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -41,6 +41,8 @@ struct intel_gt;
> struct drm_i915_private;
>
> #ifdef CONFIG_DRM_I915_PXP
> +int intel_pxp_create_arb_session(struct drm_i915_private *i915);
> +
> void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir);
> int i915_pxp_teardown_required_callback(struct drm_i915_private *i915);
> int i915_pxp_global_terminate_complete_callback(struct drm_i915_private
> *i915);
> @@ -48,6 +50,11 @@ int i915_pxp_global_terminate_complete_callback(struct
> drm_i915_private *i915);
> int intel_pxp_init(struct drm_i915_private *i915);
> void intel_pxp_uninit(struct drm_i915_private *i915);
> #else
> +static inline int intel_pxp_create_arb_session(struct drm_i915_private *i915)
> +{
> + return 0;
> +};
> +
> static inline void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir)
> {
> }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
> index 38c8b6d08b61..056f65fbaf4e 100644
> ---
[Intel-gfx] [RFC-v1 08/16] drm/i915/pxp: Create the arbitrary session after boot
Create the arbitrary session, with the fixed session id 0xf, after
system boot, for the case that application allocates the protected
buffer without establishing any protection session. Because the
hardware requires at least one alive session for protected buffer
creation. This arbitrary session needs to be re-created after
teardown or power event because hardware encryption key won't be
valid after such cases.
Signed-off-by: Huang, Sean Z
---
drivers/gpu/drm/i915/pxp/intel_pxp.c | 47 ++-
drivers/gpu/drm/i915/pxp/intel_pxp.h | 7 +
drivers/gpu/drm/i915/pxp/intel_pxp_sm.c | 165 +++
drivers/gpu/drm/i915/pxp/intel_pxp_sm.h | 8 ++
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 34 +
drivers/gpu/drm/i915/pxp/intel_pxp_tee.h | 11 ++
6 files changed, 271 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 332d9baff29f..10f4b1de07c4 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -9,6 +9,43 @@
#include "intel_pxp_sm.h"
#include "intel_pxp_tee.h"
+int intel_pxp_create_arb_session(struct drm_i915_private *i915)
+{
+ struct pxp_tag pxptag;
+ int ret;
+
+ lockdep_assert_held(>pxp.ctx->ctx_mutex);
+
+ if (i915->pxp.ctx->flag_display_hm_surface_keys) {
+ drm_err(>drm, "%s: arb session is alive so skipping the
creation\n",
+ __func__);
+ return 0;
+ }
+
+ ret = intel_pxp_sm_reserve_arb_session(i915, );
+ if (ret) {
+ drm_err(>drm, "Failed to reserve session\n");
+ goto end;
+ }
+
+ ret = intel_pxp_tee_cmd_create_arb_session(i915);
+ if (ret) {
+ drm_err(>drm, "Failed to send tee cmd for arb session
creation\n");
+ goto end;
+ }
+
+ ret = pxp_sm_mark_protected_session_in_play(i915, ARB_SESSION_TYPE,
pxptag.session_id);
+ if (ret) {
+ drm_err(>drm, "Failed to mark session status in play\n");
+ goto end;
+ }
+
+ i915->pxp.ctx->flag_display_hm_surface_keys = true;
+
+end:
+ return ret;
+}
+
static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
mask)
{
/* crypto mask is in bit31-16 (Engine1 Interrupt Mask) */
@@ -47,9 +84,17 @@ static int
intel_pxp_global_terminate_complete_callback(struct drm_i915_private
mutex_lock(>pxp.ctx->ctx_mutex);
- if (i915->pxp.ctx->global_state_attacked)
+ if (i915->pxp.ctx->global_state_attacked) {
i915->pxp.ctx->global_state_attacked = false;
+ /* Re-create the arb session after teardown handle complete */
+ ret = intel_pxp_create_arb_session(i915);
+ if (ret) {
+ drm_err(>drm, "Failed to create arb session\n");
+ goto end;
+ }
+ }
+end:
mutex_unlock(>pxp.ctx->ctx_mutex);
return ret;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 308d8d312a6d..e5f6e2b1bdfd 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -41,6 +41,8 @@ struct intel_gt;
struct drm_i915_private;
#ifdef CONFIG_DRM_I915_PXP
+int intel_pxp_create_arb_session(struct drm_i915_private *i915);
+
void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir);
int i915_pxp_teardown_required_callback(struct drm_i915_private *i915);
int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915);
@@ -48,6 +50,11 @@ int i915_pxp_global_terminate_complete_callback(struct
drm_i915_private *i915);
int intel_pxp_init(struct drm_i915_private *i915);
void intel_pxp_uninit(struct drm_i915_private *i915);
#else
+static inline int intel_pxp_create_arb_session(struct drm_i915_private *i915)
+{
+ return 0;
+};
+
static inline void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir)
{
}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
index 38c8b6d08b61..056f65fbaf4e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
@@ -41,6 +41,18 @@ static int pxp_reg_write(struct drm_i915_private *i915, u32
offset, u32 regval)
return 0;
}
+static int pxp_get_session_index(struct drm_i915_private *i915, u32 pxp_tag,
+int *session_index_out, int *session_type_out)
+{
+ if (!session_index_out || !session_type_out)
+ return -EINVAL;
+
+ *session_type_out = (pxp_tag & SESSION_TYPE_MASK) ? SESSION_TYPE_TYPE1
: SESSION_TYPE_TYPE0;
+ *session_index_out = pxp_tag & SESSION_ID_MASK;
+
+ return 0;
+}
+
static u8 pxp_get_session_id(int session_index, int session_type)
{
u8 session_id = session_index & SESSION_ID_MASK;
@@ -266,6 +278,159 @@ static int sync_hw_sw_state(struct
