Re: [Intel-gfx] [RFC-v1 06/16] drm/i915/pxp: Implement funcs to get/set PXP tag

["Huang, Sean Z" <sean.z.huang@intel.com>]

Hi Joonas,

Yes, I have removed this commit for single session patch series.

-----Original Message-----
From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> 
Sent: Monday, December 7, 2020 3:52 AM
To: Huang, Sean Z <sean.z.huang@intel.com>; 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 <sean.z.huang@intel.com>

By my understanding, this patch should not be needed at all for singleton session? So I'm mostly skipping review here.

<SNIP>

> -/**
> - * 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 = 0x80000000;
> -       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, &reg_value);
> -       if (ret) {
> -               drm_err(&i915->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