Re: [Intel-gfx] [RFC-v19 00/13] Introduce Intel PXP component - Mesa single session

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

Quoting Vivi, Rodrigo (2021-01-07 17:42:12)
> On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote:
> > PXP (Protected Xe Path) is an i915 component, available on
> > GEN12+ that helps to establish the hardware protected session
> > and manage the status of the alive software session, as well
> > as its life cycle.
> > 
> > This patch series is to allow the kernel space to create and
> > manage a single hardware session (a.k.a. default session or
> > arbitrary session). So user can allocate the protected buffer,
> > which is encrypted with the leverage of the arbitrary hardware
> > session.
> > 
> > v2:
> >     - modification based on code review feedbacks received
> >     - passing pxp instead of i915 as function argument
> >     - remove dead code only for multi-session
> >     - move the pxp init call from i915_drv.c to intel_gt.c
> >     - remove the tautology naming
> > 
> > v3:
> >     - rebase to latest drm-tip
> > 
> > v4:
> >     - Append the split non-mesa patch sereis (commit #14 - #21) into
> >       this patch series
> > 
> > v5:
> >     - include "intel_pxp.h" in intel_pxp_sm.h at commit #14 to fix
> >       the build problem.
> > 
> > v6:
> >     - Fix the null pointer arb_session access bug in intel_pxp_arb.c
> > in
> >       "04 [RFC-v5] drm/i915/pxp: Create the arbitrary session after
> >       boot"
> > 
> > v7:
> >     - Use list_for_each_entry_safe instead of list_for_each_entry
> > 
> > v8:
> >     - Add MEI vtag support for PXP multi-session usage
> > 
> > v9:
> >     - Fix error handling bug in commit #5 "Func to send hardware
> > session
> >       termination". In intel_pxp_cmd.c, we should properly assign
> >       "err = PTR_ERR(x)" if hitting the error case "IS_ERR(x)", this
> > is
> >       the only change in v9.
> > 
> > v10
> >     - Remove the multi session commits #14-#21, for now we would like
> > to
> >       keep the multi session patches as downstream.
> >     - Adopt the code review suggestion from Wilson in commit #1
> > 
> > v11
> >     - In commit #05 "drm/i915/pxp: Func to send hardware session
> >       termination", we should not assume VCS0 is always on.
> >       Instead we use available VCS#, could be VCS0, VCS2, etc.
> > 
> > v12
> >     - Add "#include <linux/mutex.h> in #1 intel_pxp_types.h
> > 
> > v13
> >     - Add "#include <linux/mutex.h> in #1 intel_pxp_types.h (#v12
> > didn't
> >       actually update the _types.h file...)
> > 
> > v14
> >     - Add "if (INTEL_GEN(gt->i915) < 12) return;" in #1
> >       intel_pxp_fini(), just skip for non gen12+ sku
> > 
> > v15
> >     In #04:
> >     - Make intel_pxp_arb_reserve_session() as static function to fix
> > the
> >       sparse warning
> >     - Update value of PXP_TEE_ARB_CMD_BIN 
> > 
> > v16
> >     In #04:
> >     - Remove the binary from source code via defining the TEE command
> >       header
> > 
> > v17
> >     - In #04, directly return intel_pxp_tee_component_fini() if
> >       pxp_tee_comp_added is off
> > 
> > v18
> >     - In #09, Add intel_pxp_gem_context_set_protected() to check the
> > arb
> >       session before setting protected flag for gem context
> > 
> >     - In #12, Replace i915_gem_context_set_protected() with
> >       intel_pxp_gem_context_set_protected() to check whether the
> > arbitrary
> >       session is alive
> > 
> > v19
> >     - In #01, put intel_pxp_fini() in intel_gt_driver_remove()
> > instead of
> >       intel_gt_driver_release() since intel_gt_driver_release() is
> > the last
> >       stage of i915 unbind and we shouldn't call the component_del
> > here.
> > 
> >     - In #04, check if arbitrary session is alive before
> >       intel_pxp_arb_create_session()
> 
> Please reduce the amount of series revision and start using --in-reply-
> to so we can easily see what was addressed from the previous comments
> versus what is open.

CI doesn't always work nicely with --in-reply-to :/

The changes made to each patch should be noted in the commit message of
the patch, not just in the cover letter. And level of detail of the
changes doesn't need to be verbose.

That way when reviewing v10 after v9, reviewer can also can through
patches which have v10 change entry in them.

Use trybot instead of the mailing list to catch things like missing
includes, so those should not result in new revisions. Only send to the
mailing list a new revision that you have checked to compile and fully
work.

But most importantly, I notice plenty of previously given feedback that
is unaddressed (type0/type1, unnecessary variables like session type
etc. that can only have single value in this series). So all of the
previous feedback needs addressing before continuing the review.

There is lot to be fixed about the coding style to match surroundings,
but that only makes sense as the last step after the series is otherwise
ready.

Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx