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]