On Wed, 2022-12-07 at 10:10 +0000, Tvrtko Ursulin wrote:
> On 06/12/2022 18:26, Teres Alexis, Alan Previn wrote:
> >
> >
> > On Tue, 2022-12-06 at 10:04 +0000, Tvrtko Ursulin wrote:
> > > On 06/12/2022 00:03, Alan Previn wrote:
> > > >
> > Alan: [snip]
> > >
> > > >
> > > > -struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > > > +bool intel_pxp_is_supported(const struct intel_pxp *pxp)
> > > > {
> > > > - return container_of(pxp, struct intel_gt, pxp);
> > > > + if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
> > > > + return false;
> > > > + else if (!pxp)
> > > > + return false;
> > > > + return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp &&
> > > > VDBOX_MASK(pxp->ctrl_gt));
> > >
> > > Intel_pxp_is_supported operating on the pxp reads a bit funny when one
> > > of the checks is for NULL passed in object to start with.
> > >
> > > And all callers pass in i915->pxp so my immediate thought is whether
> > > i915_pxp_is_supported(i915) was considered?
> >
> >
> > Alan: I think you might need to track back through the last couple of
> > months of this patch (probably back to rev4 or
> > 5)... I was told the coding practice is intel_subsystem_function(struct
> > subsystem...) so pxp should have pxp as its
> > input structure. We needed to make exceptions for init/fini because
> > ptr-to-ptr is worse - but we all agreed we dont want
> > viral include header hiearchys so dynamic allocation is the right way to
> > go. ('we' included Jani + Rodrigo). As such i
>
> I said nothing about dynamic allocation. I said, since you are promoting pxp
> to be under i915, and have a this:
>
> intel_pxp_is_supported(pxp)
> {
> ...
> if (!pxp)
> return false;
>
> (There's even a gt->i915->has_pxp in there, and a Kconfig based check, so it
> really does not feel this should operate on the pxp.)
>
> And callers to all these three function pass in, most even directly,
> i915->pxp, that passing in a NULL pxp to function tell you you passed in NULL
> pxp reads confused.
>
Alan: So prior to rev-9 i always allocated i915->pxp and would use the other
params such as the pxp->ce or those pxp-
>is_arb_valid without checking the validity of the pxp ptr for those helpers.
>Daniele said its very unlikely but
possible to have i915->pxp allocation fail and other functions get called and
so said its better to explicitily leave
i915->pxp as null for cases where the backend tee-hw was also not available
(and hence the ptr checking added in those
helpers).
> I asked if this alternative was considered (function prefix follow target
> object):
>
> i915_pxp_is_supported(i915)
>
> Or if you want to follow the alternative preference (function prefix follows
> file prefix):
>
> intel_pxp_is_supported(i915)
>
Alan: I believe somewhere in rev 5 or 6 this was discussed by Rodrigo/Janie ...
IIRC the summary was that coding
practice is to go with the function name + first param in the form of:
intel_[subsystem]_function(struct [subsystem]
*...) and we decided to stick with that for everything except the exceptions
for init/fini.
I can re-spin if you think its better to extend that exception to
intel_pxp_is_supported/active.
Hmmm... OR...
actually, what do you think about extending this for all other top-level entry
points that may called externally? (there
are several more: irq handler, huc-loading code, debugfs, pxp-key-check,
suspend-resume. Change all for consistency
(agnostic to whether they are prepend with a call to is_enabled/active first).
And then ... there is an completely different alternative method (building on
top of Rev8 instead):
Perhaps we can also always allocate i915->pxp to be valid and only use other
params like "pxp->component", "pxp->ce",
etc external facing functions. But i think this is not the right way - if the
far future were to see all subsystems get
dynamically allocated (and reducing header-file-triggered build times), then it
makes sense to expect nly valid
subsystems to be allocated.
> Nothing about dropping the dynamic allocation in this question...
>
> Oh would IS_ENABLED(CONFIG_DRM_I915_PXP) be right in
> intel_pxp_is_enabled/active? If so maybe worth to add for some extra dead
> code elimination.
>
Alan: Adding IS_ENABLED(CONFIG_DRM_I915_PXP) in intel_pxp_is_enabled/active
should be fine. But take note that on
platforms that DO support pxp, we would go thru that new line and then check
'pxp' and 'pxp->ce' validity again so 3
cycles as opposed to 2 to accomplish the same thing - seems unnecessary to me.
NOTE: intel_pxp_is_supported is special
since it's called from within intel_pxp_init so it needs these extra checks for
the build config, has-pxp and engine-
mask.
> > wont change this - but i will wait for your confirmation before i re-rev.
> > Side note: with all due respect it would be
> > nice to have comments preceeded with "nack" or "nit" or "question".
>
> "Discussion in progress please hold".
>
> > Alan: [snip]
> > > >
> > > >
> > > >
> >
> > > > @@ -138,31 +152,63 @@ static void pxp_init_full(struct intel_pxp *pxp)
> > > > destroy_vcs_context(pxp);
> > > > }
> > > >
> > > > -void intel_pxp_init(struct intel_pxp *pxp)
> > > > +static struct intel_gt *pxp_get_ctrl_gt(struct drm_i915_private *i915)
> > > > {
> > > > - struct intel_gt *gt = pxp_to_gt(pxp);
> > > > + struct intel_gt *gt = NULL;
> > > > + int i = 0;
> > >
> > > No need to init.
> > Alan: Sorry - i hate not initing local vars - is this a nack?
>
> Then you hate consistency as well? :) It is a very typical review feedback.
> Look around the mailing list and the code and count how many needlessly
> initialized locals we have. You just align it with the codebase and move on..
>
Alan: Ofc - its already fixed in rev10. :)
> > > >
> > > > - /* we rely on the mei PXP module */
> > > > - if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > > > - return;
> > > > + for_each_gt(gt, i915, i) {
> > > > + /* There can be only one GT with GSC-CS that supports
> > > > PXP */
> > > > + if (HAS_ENGINE(gt, GSC0))
> > > > + return gt;
> > > > + }
> > > > +
> > > > + /* Else we rely on the GT-0 with mei PXP module */
> > > > + if (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && !i915->media_gt)
> > > > + return &i915->gt0;
> > > > +
> > >
> > > None of this makes sense unless CONFIG_DRM_I915_PXP, right?
> > Alan: No - when we dont support PXP as a feature we still need the backend
> > Tee-link infrastructure that PXP provides for
> > GSC HuC authentication for DG2 - this existing code path. I can add some
> > additional comments. (im using Tee losely here
> > since its not actual Tee but an intel specific framework to provide access
> > to security firwmare).
>
> Okay so is the answer i915->pxp is not the same PXP as CONFIG_DRM_I915_PXP.
> Latter is kind of the user facing part, while i915->pxp is set when the PXP
> hardware actually exists. I got this right?
>
Alan: Yes'-ish'. i915->pxp is only set when [1] we want the user-facing-feature
AND the HW is there or [2] the hardware
is there AND we need it for specific platform usages like DG2 HuC
authentication. For example, we could have a ADL with
a DG1. ADL will be the case of [1] while D1 will not see either case. However,
if we had a DG2 instead of a DG1, then
i915->pxp would still be there (but not have the user-facing feature).
> If so how about splitting intel_pxp_is_supported into two (or more?) so it
> does not answer two separate questions?
>
Alan: I'm not sure i understand what you mean. "intel_pxp_is_supported" is only
answering the user-facing question and
not if the backend is require for PXP.
> > >
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +int intel_pxp_init(struct drm_i915_private *i915)
> > > > +{
> > > > + i915->pxp = kzalloc(sizeof(*i915->pxp), GFP_KERNEL);
> > > > + if (!i915->pxp)
> > > > + return -ENOMEM;
> > > > +
> > > > + i915->pxp->ctrl_gt = pxp_get_ctrl_gt(i915);
> > > > + if (!i915->pxp->ctrl_gt) {
> > > > + kfree(i915->pxp);
> > > > + i915->pxp = NULL;
> > > > + return -ENODEV;
> > > > + }
> > >
> > > If you store ctrl_gt in a local then you don't have to allocate until
> > > you'll know you need it, however..
> > Alan: see my reply below.
>
> To be clear I was merely suggesting to avoid the need to free something just
> allocated:
>
> int intel_pxp_init(struct drm_i915_private *i915)
> {
> struct intel_gt *ctrl_gt;
> struct intel_pxp *pxp;
>
> ctrl_gt = pxp_get_ctrl_gt(i915);
> if (!ctrl_gt)
> return -ENODEV;
>
> pxp = kzalloc(...)
> if (!pxp)
> return -ENOMEM;
> ...
>
> Because it's kind of pointless to alloc+free on every old platform under the
> sun.
Alan: oh okay - yes, ofc - sorry, i think was lacking coffee when i read that
the first time.
>
> And maybe pxp_get_ctrl_gt should be renamed to some variant of "is needed",
> but I am not clear yet on that.
>
> > >
> > > >
> > > > /*
> > > > * If HuC is loaded by GSC but PXP is disabled, we can skip the
> > > > init of
> > > > * the full PXP session/object management and just init the tee
> > > > channel.
> > > > */
> > > > - if (HAS_PXP(gt->i915))
> > > > - pxp_init_full(pxp);
> > > > - else if (intel_huc_is_loaded_by_gsc(>->uc.huc) &&
> > > > intel_uc_uses_huc(>->uc))
> > > > - intel_pxp_tee_component_init(pxp);
> > > > + if (intel_pxp_is_supported(i915->pxp))
> > > > + pxp_init_full(i915->pxp);
> > > > + else if
> > > > (intel_huc_is_loaded_by_gsc(&i915->pxp->ctrl_gt->uc.huc) &&
> > > > + intel_uc_uses_huc(&i915->pxp->ctrl_gt->uc))
> > > > + intel_pxp_tee_component_init(i915->pxp);
> > >
> > > ... intel_pxp_is_supported() returnsed false so what is the purpose of
> > > the "else if" branch?
> > >
> > > Which of the conditions in intel_pxp_is_supported can it fail on to get
> > > here?
> > >
> > > And purpose of exiting init with supported = no but i915->pxp set?
> > >
> > Alan: So this was prior existing code flow i did not change - but i can add
> > an "else if (intel_pxp_tee_is_needed())" and
> > that can be a wrapper around those gsc-huc-authentication and tee backend
> > transport dependency needs.
>
> Hm, okay. So here you have:
>
>
> + if (intel_pxp_is_supported(i915->pxp))
> + pxp_init_full(i915->pxp);
> + else if (pxp_backend_tee_is_needed(i915->pxp))
> + intel_pxp_tee_component_init(i915->pxp);
> +
> + return 0;
>
> 1)
>
> If both these branches are false, is there a purpose for the dangling
> i915->pxp object?
>
> Or if they cannot both be false is this the clearest way to express the flow?
> ("else GEM_WARN_ON()" at least?)
Alan: Both branches can be false.
>
> 2)
>
> If there are no vdboxes is the last (else if) branch needed? If it is for
> loading the HuC then possibly sounds like no. So maybe the vdbox check can be
> early in pxp_get_ctrl_gt?
>
Alan: Yes that's possible. So in that case we are folding a lot more checks
into the ctrl-gt. But we need a name that
truly reflects that it does. Coz now its gonna check for "MediaGT with GSC on
MTL" or "GT0 on pre-MTL but with MEI
KConfig AND VDBOX) so something like "get_suitable_ctrl_gt_for_backend_tee()".
However, this still doesnt mean the two
branches will never be both false - the 2nd branch can still be false - we
could have VDBOX but huc fw not avail.
Something like this at the end? Should we free up pxp is not used at all? :
int intel_pxp_init(struct drm_i915_private *i915)
{
intel_gt *gt;
intel_pxp *pxp;
/*
* NOTE: Get a suitable ctrl_gt before checking intel_pxp_is_supported
* since we still also it if backend tee transport is needed for other
* platform requirements. Suitable gt checks include
gsc-engine/vdbox/Kconfig
*/
gt = pxp_find_suitable_ctrl_gt(i915);
if (!gt)
return -ENODEV;
pxp = kzalloc(sizeof(*i915->pxp), GFP_KERNEL);
if (!pxp)
return -ENOMEM;
pxp->ctrl_gt = gt;
/*
* If HuC is loaded by GSC but PXP is disabled, we can skip the init of
* the full PXP session/object management and just init the tee channel.
*/
if (intel_pxp_is_supported(pxp)) {
pxp_init_full(pxp);
} else if (pxp_backend_tee_is_needed(pxp)) {
intel_pxp_tee_component_init(pxp);
} else {
kfree(pxp);
pxp = NULL;
}
i915->pxp = pxp;
return 0;
}
> Regards,
>
> Tvrtko
>
Alan:[snip]
