Message-ID: <98f20683-bb1d-47c2-18ca-f3de61c9a91f@linaro.org>
Date:   Thu, 9 Mar 2023 10:45:49 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Maximilian Luz <luzmaximilian@...il.com>,
        Bjorn Andersson <andersson@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Johan Hovold <johan@...nel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Sumit Garg <sumit.garg@...aro.org>,
        Steev Klimaszewski <steev@...i.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] firmware: Add support for Qualcomm Secure
 Execution Environment SCM interface

On 08/03/2023 15:59, Maximilian Luz wrote:
> On 3/7/23 16:32, Dmitry Baryshkov wrote:
>> On 05/03/2023 04:21, Maximilian Luz wrote:
> 
> [...]
> 
>>> +/* -- DMA helpers. 
>>> ---------------------------------------------------------- */
>>> +
>>> +/* DMA requirements for QSEECOM SCM calls. */
>>> +#define QSEECOM_DMA_ALIGNMENT        8
>>> +#define QSEECOM_DMA_ALIGN(ptr)        ALIGN(ptr, QSEECOM_DMA_ALIGNMENT)
>>> +
>>> +/**
>>> + * struct qseecom_dma - DMA memory region.
>>> + * @size: Size of the memory region, in bytes.
>>> + * @virt: Pointer / virtual address to the memory, accessible by the 
>>> kernel.
>>> + * @phys: Physical address of the memory region.
>>> + */
>>> +struct qseecom_dma {
>>> +    unsigned long size;
>>
>> size_t ?
> 
> Will fix.
> 
>>> +    void *virt;
>>> +    dma_addr_t phys;
>>> +};
>>
>> Do we really need this wrapper and the wrappers bellow? They look like 
>> a pure syntax sugar for me, hiding the dma functions from the user.
> 
> The idea was that they take care of proper allocation. The Windows 
> driver that
> I've reverse-engineered this from allocates memory in multiples of 
> PAGE_SIZE,
> so I believe that this might be a requirement (at least on some 
> systems). These
> functions are there to ensure that and with that prevent potential bugs by
> taking that responsibility from the caller.

I see. As I wrote in another comment, it might be better to review the 
whole memory allocation system: pass required sizes, get the data filled.

> 
>>> +
>>> +/**
>>> + * qseecom_dma_alloc() - Allocate a DMA-able memory region suitable 
>>> for QSEECOM
>>> + * SCM calls.
>>> + * @dev:  The device used for DMA memory allocation.
>>> + * @dma:  Where to write the allocated memory addresses and size to.
>>> + * @size: Minimum size of the memory to be allocated.
>>> + * @gfp:  Flags used for allocation.
>>> + *
>>> + * Allocate a DMA-able memory region suitable for interaction with SEE
>>> + * services/applications and the TzOS. The provided size is treated 
>>> as the
>>> + * minimum required size and rounded up, if necessary. The actually 
>>> allocated
>>> + * memory region will be stored in @dma. Allocated memory must be 
>>> freed via
>>> + * qseecom_dma_free().
>>> + *
>>> + * Return: Returns zero on success, -ENOMEM on allocation failure.
>>> + */
>>> +static inline int qseecom_dma_alloc(struct device *dev, struct 
>>> qseecom_dma *dma,
>>> +                    unsigned long size, gfp_t gfp)
>>
>> size_t size
>>
>> gfp is not used
> 
> Right, that should have been passed to dma_alloc_coherent(). Will fix that.
> 
>>> +{
>>> +    size = PAGE_ALIGN(size);
>>> +
>>> +    dma->virt = dma_alloc_coherent(dev, size, &dma->phys, GFP_KERNEL);
>>> +    if (!dma->virt)
>>> +        return -ENOMEM;
>>> +
>>> +    dma->size = size;
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * qseecom_dma_free() - Free a DMA memory region.
>>> + * @dev: The device used for allocation.
>>> + * @dma: The DMA region to be freed.
>>> + *
>>> + * Free a DMA region previously allocated via qseecom_dma_alloc(). 
>>> Note that
>>> + * freeing sub-regions is not supported.
>>> + */
>>> +static inline void qseecom_dma_free(struct device *dev, struct 
>>> qseecom_dma *dma)
>>> +{
>>> +    dma_free_coherent(dev, dma->size, dma->virt, dma->phys);
>>> +}
>>> +
>>> +/**
>>> + * qseecom_dma_realloc() - Re-allocate DMA memory region with the 
>>> requested size.
>>> + * @dev:  The device used for allocation.
>>> + * @dma:  The region descriptor to be updated.
>>> + * @size: The new requested size.
>>> + * @gfp:  Flags used for allocation.
>>> + *
>>> + * Re-allocates a DMA memory region suitable for QSEECOM SCM calls 
>>> to fit the
>>> + * requested amount of bytes, if necessary. Does nothing if the 
>>> provided region
>>> + * already has enough space to store the requested data.
>>> + *
>>> + * See qseecom_dma_alloc() for details.
>>> + *
>>> + * Return: Returns zero on success, -ENOMEM on allocation failure.
>>> + */
>>> +static inline int qseecom_dma_realloc(struct device *dev, struct 
>>> qseecom_dma *dma,
>>> +                      unsigned long size, gfp_t gfp)
>>> +{
>>> +    if (PAGE_ALIGN(size) <= dma->size)
>>> +        return 0;
>>> +
>>> +    qseecom_dma_free(dev, dma);
>>> +    return qseecom_dma_alloc(dev, dma, size, gfp);
>>> +}
>>
>> I'll comment on this function when commenting patch 4.
>>
>>> +
>>> +/**
>>> + * qseecom_dma_aligned() - Create a aligned DMA memory sub-region 
>>> suitable for
>>> + * QSEECOM SCM calls.
>>> + * @base:   Base DMA memory region, in which the new region will 
>>> reside.
>>> + * @out:    Descriptor to store the aligned sub-region in.
>>> + * @offset: The offset inside base region at which to place the new 
>>> sub-region.
>>> + *
>>> + * Creates an aligned DMA memory region suitable for QSEECOM SCM 
>>> calls at or
>>> + * after the given offset. The size of the sub-region will be set to 
>>> the
>>> + * remaining size in the base region after alignment, i.e., the end 
>>> of the
>>> + * sub-region will be equal the end of the base region.
>>> + *
>>> + * Return: Returns zero on success or -EINVAL if the new aligned 
>>> memory address
>>> + * would point outside the base region.
>>> + */
>>> +static inline int qseecom_dma_aligned(const struct qseecom_dma 
>>> *base, struct qseecom_dma *out,
>>> +                      unsigned long offset)
>>> +{
>>> +    void *aligned = (void *)QSEECOM_DMA_ALIGN((uintptr_t)base->virt 
>>> + offset);
>>> +
>>> +    if (aligned - base->virt > base->size)
>>> +        return -EINVAL;
>>> +
>>> +    out->virt = aligned;
>>> +    out->phys = base->phys + (out->virt - base->virt);
>>> +    out->size = base->size - (out->virt - base->virt);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +/* -- Common interface. 
>>> ----------------------------------------------------- */
>>> +
>>> +struct qseecom_device {
>>> +    struct device *dev;
>>> +    struct mutex scm_call_lock;    /* Guards QSEECOM SCM calls. */
>>
>> There can be only one instance of the qseecom call infrastructure. 
>> Make this mutex static in the qcom_scm.c
> 
> Right, will do that.
> 
>>> +};
>>> +
>>> +
>>> +/* -- Secure-OS SCM call interface. 
>>> ----------------------------------------- */
>>> +
>>> +#define QSEECOM_TZ_OWNER_TZ_APPS        48
>>> +#define QSEECOM_TZ_OWNER_QSEE_OS        50
>>> +
>>> +#define QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER    0
>>> +#define QSEECOM_TZ_SVC_APP_MGR            1
>>> +
>>> +enum qseecom_scm_result {
>>> +    QSEECOM_RESULT_SUCCESS            = 0,
>>> +    QSEECOM_RESULT_INCOMPLETE        = 1,
>>> +    QSEECOM_RESULT_BLOCKED_ON_LISTENER    = 2,
>>> +    QSEECOM_RESULT_FAILURE            = 0xFFFFFFFF,
>>> +};
>>> +
>>> +enum qseecom_scm_resp_type {
>>> +    QSEECOM_SCM_RES_APP_ID            = 0xEE01,
>>> +    QSEECOM_SCM_RES_QSEOS_LISTENER_ID    = 0xEE02,
>>> +};
>>> +
>>> +/**
>>> + * struct qseecom_scm_resp - QSEECOM SCM call response.
>>> + * @status:    Status of the SCM call. See &enum qseecom_scm_result.
>>> + * @resp_type: Type of the response. See &enum qseecom_scm_resp_type.
>>> + * @data:      Response data. The type of this data is given in 
>>> @resp_type.
>>> + */
>>> +struct qseecom_scm_resp {
>>> +    u64 status;
>>> +    u64 resp_type;
>>> +    u64 data;
>>> +};
>>> +
>>> +int qseecom_scm_call(struct qseecom_device *qsee, const struct 
>>> qcom_scm_desc *desc,
>>> +             struct qseecom_scm_resp *res);
>>> +
>>> +
>>> +/* -- Secure App interface. 
>>> ------------------------------------------------- */
>>> +
>>> +#define QSEECOM_MAX_APP_NAME_SIZE            64
>>> +
>>> +int qseecom_app_get_id(struct qseecom_device *qsee, const char 
>>> *app_name, u32 *app_id);
>>> +int qseecom_app_send(struct qseecom_device *qsee, u32 app_id, struct 
>>> qseecom_dma *req,
>>> +             struct qseecom_dma *rsp);
>>
>> I think that only these calls should be made public / available to 
>> other modules. qseecom_scm_call also is an internal helper.
> 
> So move all calls to qcom_scm and only make these two public? Or move only
> qseecom_scm_call() to qcom_scm (which would require to make it public 
> there,
> however). Or how would you want this to look?

I think we can make it with just these calls being public. Or even with 
just the second one being public and available to other drivers. If the 
app_id is a part of qseecom_app_device, we can pass that device to the 
qseecom_app_send(). And qseecom_app_get_id() becomes a part of app 
registration.

> 
>>> +
>>> +#endif /* _LINUX_QCOM_QSEECOM_H */
>>
> 
> Regards,
> Max

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists