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

[Dmitry Baryshkov]

On 08/03/2023 16:06, Maximilian Luz wrote:
> 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.

Initially I thought that a custom bus might be required, to provide 
custom probe function. After giving it a thought, I think you can get 
away with using aux bus. So, embed the struct auxiliary_device into 
qseecom_app_device.

> > > +    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

--
With best wishes
Dmitry