Re: [PATCH v7 05/17] drm/i915/pxp: Implement funcs to create the TEE channel

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

On 8/31/2021 2:08 PM, Rodrigo Vivi wrote:
> On Fri, Aug 27, 2021 at 06:27:26PM -0700, Daniele Ceraolo Spurio wrote:
> > From: "Huang, Sean Z" <sean.z.huang@xxxxxxxxx>
> >
> > Implement the funcs to create the TEE channel, so kernel can
> > send the TEE commands directly to TEE for creating the arbitrary
> > (default) session.
> >
> > v2: fix locking, don't pollute dev_priv (Chris)
> >
> > v3: wait for mei PXP component to be bound.
> >
> > v4: drop the wait, as the component might be bound after i915 load
> > completes. We'll instead check when sending a tee message.
> >
> > v5: fix an issue with mei_pxp module removal
> >
> > Signed-off-by: Huang, Sean Z <sean.z.huang@xxxxxxxxx>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> #v4
> > ---
> >   drivers/gpu/drm/i915/Makefile              |  3 +-
> >   drivers/gpu/drm/i915/pxp/intel_pxp.c       | 13 ++++
> >   drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   | 76 ++++++++++++++++++++++
> >   drivers/gpu/drm/i915/pxp/intel_pxp_tee.h   | 14 ++++
> >   drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  6 ++
> >   5 files changed, 111 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 157644ef5886..cc9fe99ca5c5 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -282,7 +282,8 @@ i915-y += i915_perf.o
> >   
> >   # Protected execution platform (PXP) support
> >   i915-$(CONFIG_DRM_I915_PXP) += \
> > -	pxp/intel_pxp.o
> > +	pxp/intel_pxp.o \
> > +	pxp/intel_pxp_tee.o
> >   
> >   # Post-mortem debug and GPU hang state capture
> >   i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > index 7b2053902146..400deaea2d8a 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > @@ -3,6 +3,7 @@
> >    * Copyright(c) 2020 Intel Corporation.
> >    */
> >   #include "intel_pxp.h"
> > +#include "intel_pxp_tee.h"
> >   #include "gt/intel_context.h"
> >   #include "i915_drv.h"
> >   
> > @@ -50,7 +51,16 @@ void intel_pxp_init(struct intel_pxp *pxp)
> >   	if (ret)
> >   		return;
> >   
> > +	ret = intel_pxp_tee_component_init(pxp);
> > +	if (ret)
> > +		goto out_context;
> > +
> >   	drm_info(&gt->i915->drm, "Protected Xe Path (PXP) protected content support initialized\n");
> > +
> > +	return;
> > +
> > +out_context:
> > +	destroy_vcs_context(pxp);
> >   }
> >   
> >   void intel_pxp_fini(struct intel_pxp *pxp)
> > @@ -58,5 +68,8 @@ void intel_pxp_fini(struct intel_pxp *pxp)
> >   	if (!intel_pxp_is_enabled(pxp))
> >   		return;
> >   
> > +	intel_pxp_tee_component_fini(pxp);
> > +
> >   	destroy_vcs_context(pxp);
> > +
> >   }
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > new file mode 100644
> > index 000000000000..2f28f34c721d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > @@ -0,0 +1,76 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright(c) 2020 Intel Corporation.
> > + */
> > +
> > +#include <linux/component.h>
> > +#include "drm/i915_pxp_tee_interface.h"
> > +#include "drm/i915_component.h"
> > +#include "i915_drv.h"
> > +#include "intel_pxp.h"
> > +#include "intel_pxp_tee.h"
> > +
> > +static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
> > +{
> > +	return &kdev_to_i915(i915_kdev)->gt.pxp;
> > +}
> > +
> > +/**
> > + * i915_pxp_tee_component_bind - bind function to pass the function pointers to pxp_tee
> > + * @i915_kdev: pointer to i915 kernel device
> > + * @tee_kdev: pointer to tee kernel device
> > + * @data: pointer to pxp_tee_master containing the function pointers
> > + *
> > + * This bind function is called during the system boot or resume from system sleep.
> > + *
> > + * Return: return 0 if successful.
> > + */
> > +static int i915_pxp_tee_component_bind(struct device *i915_kdev,
> > +				       struct device *tee_kdev, void *data)
> > +{
> > +	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> > +
> > +	pxp->pxp_component = data;
> > +	pxp->pxp_component->tee_dev = tee_kdev;
> > +
> > +	return 0;
> > +}
> > +
> > +static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
> > +					  struct device *tee_kdev, void *data)
> > +{
> > +	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> > +
> > +	pxp->pxp_component = NULL;
> > +}
> > +
> > +static const struct component_ops i915_pxp_tee_component_ops = {
> > +	.bind   = i915_pxp_tee_component_bind,
> > +	.unbind = i915_pxp_tee_component_unbind,
> > +};
> > +
> > +int intel_pxp_tee_component_init(struct intel_pxp *pxp)
> > +{
> > +	int ret;
> > +	struct intel_gt *gt = pxp_to_gt(pxp);
> > +	struct drm_i915_private *i915 = gt->i915;
> > +
> > +	ret = component_add_typed(i915->drm.dev, &i915_pxp_tee_component_ops,
> > +				  I915_COMPONENT_PXP);
> > +	if (ret < 0) {
> > +		drm_err(&i915->drm, "Failed to add PXP component (%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	pxp->pxp_component_added = true;
> > +
> > +	return 0;
> > +}
> > +
> > +void intel_pxp_tee_component_fini(struct intel_pxp *pxp)
> > +{
> > +	struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> > +
> > +	if (fetch_and_zero(&pxp->pxp_component_added))
> +Daniel.
>
> If I'm understanding his locking guidelines well, I believe we should
> start avoiding even these functions.
>
> If there's a risk of init and fini functions to conflict we should
> use regular mutexes.
>
> In case they are called from obvious probe unprobe places than we
> could simply set to false and del the component. But if you have
> to add this here on this latest revision to fix a bug
> I'm assuming we will need a proper mutex.

There is no risk of conflict here, but since we don't abort driver init 
if pxp_init fails, we can end up calling pxp_fini on driver unload even 
if pxp_init was unsuccessful, so we need a variable to track the init 
state of the component. The bug I fixed was actually that reusing 
pxp->pxp_component for this tracking doesn't work, because the PCI 
subsystem unbinds the component on suspend, which clears 
pxp->pxp_component; the component itself is still there even if not 
currently bound and needs removal, hence the use of a different variable.

Daniele

> > +		component_del(i915->drm.dev, &i915_pxp_tee_component_ops);
> > +}
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h
> > new file mode 100644
> > index 000000000000..23d050a5d3e7
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> > + */
> > +
> > +#ifndef __INTEL_PXP_TEE_H__
> > +#define __INTEL_PXP_TEE_H__
> > +
> > +#include "intel_pxp.h"
> > +
> > +int intel_pxp_tee_component_init(struct intel_pxp *pxp);
> > +void intel_pxp_tee_component_fini(struct intel_pxp *pxp);
> > +
> > +#endif /* __INTEL_PXP_TEE_H__ */
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> > index bd12c520e60a..3a8e17e591bd 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> > @@ -6,9 +6,15 @@
> >   #ifndef __INTEL_PXP_TYPES_H__
> >   #define __INTEL_PXP_TYPES_H__
> >   
> > +#include <linux/types.h>
> > +
> >   struct intel_context;
> > +struct i915_pxp_component;
> >   
> >   struct intel_pxp {
> > +	struct i915_pxp_component *pxp_component;
> > +	bool pxp_component_added;
> > +
> >   	struct intel_context *ce;
> >   };
> >   
> > --
> > 2.25.1