Re: [Intel-gfx] [RFC-v1 06/16] drm/i915/pxp: Implement funcs to get/set PXP tag
Hi Joonas,
Yes, I have removed this commit for single session patch series.
-Original Message-
From: Joonas Lahtinen
Sent: Monday, December 7, 2020 3:52 AM
To: Huang, Sean Z ; Intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v1 06/16] drm/i915/pxp: Implement funcs to
get/set PXP tag
Quoting Huang, Sean Z (2020-12-07 02:21:24)
> Implement the functions to get/set the PXP tag, which is 32-bit
> bitwise value containing the hardware session info, such as its
> session id, protection mode or whether it's enabled.
>
> Signed-off-by: Huang, Sean Z
By my understanding, this patch should not be needed at all for singleton
session? So I'm mostly skipping review here.
> -/**
> - * check_if_protected_type0_sessions_are_attacked - To check if type0 active
> sessions are attacked.
> - * @i915: i915 device handle.
> - *
> - * Return: true if HW shows protected sessions are attacked, false otherwise.
> - */
> -static bool check_if_protected_type0_sessions_are_attacked(struct
> drm_i915_private *i915) -{
> - i915_reg_t kcr_status_reg = KCR_STATUS_1;
> - u32 reg_value = 0;
> - u32 mask = 0x8000;
> - int ret;
> -
> - if (!i915)
> - return false;
> -
> - if (i915->pxp.ctx->global_state_attacked)
> - return true;
> -
> - ret = pxp_sm_reg_read(i915, kcr_status_reg.reg, _value);
> - if (ret) {
> - drm_err(>drm, "Failed to pxp_sm_reg_read\n");
> - goto end;
> - }
> -
> - if (reg_value & mask)
> - return true;
> -end:
> - return false;
> -}
Removal of code added previously in the series?
> int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915) {
> int ret;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> index 222a879be96d..b5012948f971 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> @@ -20,6 +20,9 @@
> #define GEN12_KCR_TSIP_LOW _MMIO(0x32264) /* KCR type1 session in play
> 0-31 */
> #define GEN12_KCR_TSIP_HIGH _MMIO(0x32268) /* KCR type1 session in play
> 32-63 */
>
> +#define SESSION_TYPE_MASK BIT(7)
> +#define SESSION_ID_MASK (BIT(7) - 1)
> +
> enum pxp_session_types {
> SESSION_TYPE_TYPE0 = 0,
> SESSION_TYPE_TYPE1 = 1,
> @@ -36,6 +39,21 @@ enum pxp_protection_modes {
> PROTECTION_MODE_ALL
> };
>
> +struct pxp_tag {
> + union {
> + u32 value;
> + struct {
> + u32 session_id : 8;
> + u32 instance_id : 8;
> + u32 enable : 1;
> + u32 hm : 1;
> + u32 reserved_1 : 1;
> + u32 sm : 1;
> + u32 reserved_2 : 12;
> + };
It is not obvious if this is a software-only field. If it's software only, we
should just make these into normal variables. If it's hardware related, it
should be documented as a bitfield, like other hardware writes. We avoid using
this construct in i915.
Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC-v1 06/16] drm/i915/pxp: Implement funcs to get/set PXP tag
Quoting Huang, Sean Z (2020-12-07 02:21:24)
> Implement the functions to get/set the PXP tag, which is 32-bit
> bitwise value containing the hardware session info, such as its
> session id, protection mode or whether it's enabled.
>
> Signed-off-by: Huang, Sean Z
By my understanding, this patch should not be needed at all for
singleton session? So I'm mostly skipping review here.
> -/**
> - * check_if_protected_type0_sessions_are_attacked - To check if type0 active
> sessions are attacked.
> - * @i915: i915 device handle.
> - *
> - * Return: true if HW shows protected sessions are attacked, false otherwise.
> - */
> -static bool check_if_protected_type0_sessions_are_attacked(struct
> drm_i915_private *i915)
> -{
> - i915_reg_t kcr_status_reg = KCR_STATUS_1;
> - u32 reg_value = 0;
> - u32 mask = 0x8000;
> - int ret;
> -
> - if (!i915)
> - return false;
> -
> - if (i915->pxp.ctx->global_state_attacked)
> - return true;
> -
> - ret = pxp_sm_reg_read(i915, kcr_status_reg.reg, _value);
> - if (ret) {
> - drm_err(>drm, "Failed to pxp_sm_reg_read\n");
> - goto end;
> - }
> -
> - if (reg_value & mask)
> - return true;
> -end:
> - return false;
> -}
Removal of code added previously in the series?
> int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915)
> {
> int ret;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> index 222a879be96d..b5012948f971 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> @@ -20,6 +20,9 @@
> #define GEN12_KCR_TSIP_LOW _MMIO(0x32264) /* KCR type1 session in play
> 0-31 */
> #define GEN12_KCR_TSIP_HIGH _MMIO(0x32268) /* KCR type1 session in play
> 32-63 */
>
> +#define SESSION_TYPE_MASK BIT(7)
> +#define SESSION_ID_MASK (BIT(7) - 1)
> +
> enum pxp_session_types {
> SESSION_TYPE_TYPE0 = 0,
> SESSION_TYPE_TYPE1 = 1,
> @@ -36,6 +39,21 @@ enum pxp_protection_modes {
> PROTECTION_MODE_ALL
> };
>
> +struct pxp_tag {
> + union {
> + u32 value;
> + struct {
> + u32 session_id : 8;
> + u32 instance_id : 8;
> + u32 enable : 1;
> + u32 hm : 1;
> + u32 reserved_1 : 1;
> + u32 sm : 1;
> + u32 reserved_2 : 12;
> + };
It is not obvious if this is a software-only field. If it's software
only, we should just make these into normal variables. If it's hardware
related, it should be documented as a bitfield, like other hardware
writes. We avoid using this construct in i915.
Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC-v1 06/16] drm/i915/pxp: Implement funcs to get/set PXP tag
Implement the functions to get/set the PXP tag, which is 32-bit
bitwise value containing the hardware session info, such as its
session id, protection mode or whether it's enabled.
Signed-off-by: Huang, Sean Z
---
drivers/gpu/drm/i915/pxp/intel_pxp_sm.c | 125 ++--
drivers/gpu/drm/i915/pxp/intel_pxp_sm.h | 18
2 files changed, 112 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
index 6413f401d939..38c8b6d08b61 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
@@ -41,6 +41,16 @@ static int pxp_reg_write(struct drm_i915_private *i915, u32
offset, u32 regval)
return 0;
}
+static u8 pxp_get_session_id(int session_index, int session_type)
+{
+ u8 session_id = session_index & SESSION_ID_MASK;
+
+ if (session_type == SESSION_TYPE_TYPE1)
+ session_id |= SESSION_TYPE_MASK;
+
+ return session_id;
+}
+
/**
* is_sw_session_active - Check if the given sw session id is active.
* @i915: i915 device handle.
@@ -78,6 +88,90 @@ static bool is_sw_session_active(struct drm_i915_private
*i915, int session_type
return false;
}
+static int pxp_set_pxp_tag(struct drm_i915_private *i915, int session_type,
+ int session_idx, int protection_mode)
+{
+ struct pxp_tag *pxp_tag;
+
+ if (!i915 || session_type >= SESSION_TYPE_MAX)
+ return -EINVAL;
+
+ if (session_type == SESSION_TYPE_TYPE0 && session_idx <
MAX_TYPE0_SESSIONS) {
+ pxp_tag = (struct pxp_tag
*)>pxp.ctx->type0_session_pxp_tag[session_idx];
+ } else if (session_type == SESSION_TYPE_TYPE1 && session_idx <
MAX_TYPE1_SESSIONS) {
+ pxp_tag = (struct pxp_tag
*)>pxp.ctx->type1_session_pxp_tag[session_idx];
+ } else {
+ drm_err(>drm, "Failed to %s, bad params
session_type=[%d], session_idx=[%d]\n",
+ __func__, session_type, session_idx);
+ return -EINVAL;
+ }
+
+ switch (protection_mode) {
+ case PROTECTION_MODE_NONE:
+ {
+ pxp_tag->enable = false;
+ pxp_tag->hm = false;
+ pxp_tag->sm = false;
+ break;
+ }
+ case PROTECTION_MODE_LM:
+ {
+ pxp_tag->enable = true;
+ pxp_tag->hm = false;
+ pxp_tag->sm = false;
+ pxp_tag->instance_id++;
+ break;
+ }
+ case PROTECTION_MODE_HM:
+ {
+ pxp_tag->enable = true;
+ pxp_tag->hm = true;
+ pxp_tag->sm = false;
+ pxp_tag->instance_id++;
+ break;
+ }
+ case PROTECTION_MODE_SM:
+ {
+ pxp_tag->enable = true;
+ pxp_tag->hm = true;
+ pxp_tag->sm = true;
+ pxp_tag->instance_id++;
+ break;
+ }
+ default:
+ drm_err(>drm, "Failed to %s, bad params
protection_mode=[%d]\n",
+ __func__, protection_mode);
+ return -EINVAL;
+ }
+
+ pxp_tag->session_id = pxp_get_session_id(session_idx, session_type);
+ return 0;
+}
+
+u32 intel_pxp_get_pxp_tag(struct drm_i915_private *i915, int session_idx,
+ int session_type, u32 *session_is_alive)
+{
+ struct pxp_tag *pxp_tag;
+
+ if (!i915 || session_type >= SESSION_TYPE_MAX)
+ return -EINVAL;
+
+ if (session_type == SESSION_TYPE_TYPE0 && session_idx <
MAX_TYPE0_SESSIONS) {
+ pxp_tag = (struct pxp_tag
*)>pxp.ctx->type0_session_pxp_tag[session_idx];
+ } else if (session_type == SESSION_TYPE_TYPE1 && session_idx <
MAX_TYPE1_SESSIONS) {
+ pxp_tag = (struct pxp_tag
*)>pxp.ctx->type1_session_pxp_tag[session_idx];
+ } else {
+ drm_err(>drm, "Failed to %s, bad params
session_type=[%d], session_idx=[%d]\n",
+ __func__, session_type, session_idx);
+ return -EINVAL;
+ }
+
+ if (session_is_alive)
+ *session_is_alive = pxp_tag->enable;
+
+ return pxp_tag->value;
+}
+
static bool is_hw_type0_session_in_play(struct drm_i915_private *i915, int
session_index)
{
u32 regval_sip = 0;
@@ -172,37 +266,6 @@ static int sync_hw_sw_state(struct drm_i915_private *i915,
int session_index, in
return ret;
}
-/**
- * check_if_protected_type0_sessions_are_attacked - To check if type0 active
sessions are attacked.
- * @i915: i915 device handle.
- *
- * Return: true if HW shows protected sessions are attacked, false otherwise.
- */
-static bool check_if_protected_type0_sessions_are_attacked(struct
drm_i915_private *i915)
-{
- i915_reg_t kcr_status_reg = KCR_STATUS_1;
- u32 reg_value = 0;
- u32 mask = 0x8000;
- int ret;
-
- if (!i915)
-
