Re: [PATCH v3 2/4] firmware: Add support for Qualcomm Secure Execution Environment SCM interface

[Maximilian Luz]

On 3/7/23 16:36, Dmitry Baryshkov wrote:
> On 05/03/2023 04:21, Maximilian Luz wrote:
> > Add support for SCM calls to Secure OS and the Secure Execution
> > Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
> > interface. This allows communication with Secure/TZ applications, for
> > example 'uefisecapp' managing access to UEFI variables.
> >
> > The interface is managed by a platform device to ensure correct lifetime
> > and establish a device link to the Qualcomm SCM device.
> >
> > While this patch introduces only a very basic interface without the more
> > advanced features (such as re-entrant and blocking SCM calls and
> > listeners/callbacks), this is enough to talk to the aforementioned
> > 'uefisecapp'.
> >
> > Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
> > ---
> >
> > Changes in v3:
> >   - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
> >   - Move qcom_qseecom.h in accordance with qcom_scm.
> >
> > Changes in v2:
> >   - Bind the interface to a device.
> >   - Establish a device link to the SCM device to ensure proper ordering.
> >   - Register client apps as child devices instead of requiring them to be
> >     specified in the device tree.
> >   - Rename (qctree -> qseecom) to allow differentiation between old
> >     (qseecom) and new (smcinvoke) interfaces to the trusted execution
> >     environment.
> >
> > ---
> >   MAINTAINERS                                |   7 +
> >   drivers/firmware/Kconfig                   |  15 +
> >   drivers/firmware/Makefile                  |   1 +
> >   drivers/firmware/qcom_qseecom.c            | 314 +++++++++++++++++++++
> >   include/linux/firmware/qcom/qcom_qseecom.h | 190 +++++++++++++
> >   5 files changed, 527 insertions(+)
> >   create mode 100644 drivers/firmware/qcom_qseecom.c
> >   create mode 100644 include/linux/firmware/qcom/qcom_qseecom.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9201967d198d..1545914a592c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17380,6 +17380,13 @@ F:    Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
> >   F:    drivers/net/ethernet/qualcomm/rmnet/
> >   F:    include/linux/if_rmnet.h
> > +QUALCOMM SECURE EXECUTION ENVIRONMENT COMMUNICATION DRIVER
> > +M:    Maximilian Luz <luzmaximilian@xxxxxxxxx>
> > +L:    linux-arm-msm@xxxxxxxxxxxxxxx
> > +S:    Maintained
> > +F:    drivers/firmware/qcom_qseecom.c
> > +F:    include/linux/firmware/qcom/qcom_qseecom.h
> > +
> >   QUALCOMM TSENS THERMAL DRIVER
> >   M:    Amit Kucheria <amitk@xxxxxxxxxx>
> >   M:    Thara Gopinath <thara.gopinath@xxxxxxxxx>
>
>
>
> > +
> > +
> > +/* -- Platform specific data. ----------------------------------------------- */
> > +
> > +struct qseecom_data {
> > +    const struct mfd_cell *cells;
>
> The child qseecom devices are not platform devices, so MFD should not be used here. Please use aux devices instead.

Okay, makes sense. Would this still work with your suggestion in patch 4
regarding a custom (?) bus or can the aux bus be used to implement that? From a
quick look, I believe we could use aux bus for this but I haven't worked with
that before, so I don't know if I'm missing something.

> > +    int num_cells;
> > +};
> > +
> > +static const struct of_device_id qseecom_dt_match[] = {
> > +    { .compatible = "qcom,qseecom-sc8280xp", },
>
> Forgot to mention, while doign review. There is no need for this compat until you provide the actual data. Please move it to the patch 4.

Sure, will do that.

> > +    { .compatible = "qcom,qseecom", },
> > +    { }
> > +};
> > +MODULE_DEVICE_TABLE(of, qseecom_dt_match);

Regards,
Max