| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8c59e08a-99cb-473b-999c-e7d08bc2124b@oss.qualcomm.com>
Date: Mon, 17 Nov 2025 12:32:59 +0530
From: Kumari Pallavi <kumari.pallavi@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: kpallavi@....qualcomm.com, srini@...nel.org, amahesh@....qualcomm.com,
arnd@...db.de, gregkh@...uxfoundation.org, quic_bkumar@...cinc.com,
ekansh.gupta@....qualcomm.com, linux-kernel@...r.kernel.org,
quic_chennak@...cinc.com, dri-devel@...ts.freedesktop.org,
linux-arm-msm@...r.kernel.org, jingyi.wang@....qualcomm.com,
aiqun.yu@....qualcomm.com, ktadakam@....qualcomm.com
Subject: Re: [PATCH v3 3/4] misc: fastrpc: Add support for new DSP IOVA
formatting
On 11/14/2025 9:21 PM, Bjorn Andersson wrote:
> On Fri, Nov 14, 2025 at 02:11:41PM +0530, Kumari Pallavi wrote:
>> Implement the new IOVA formatting required by the DSP architecture change
>> on Kaanapali SoC. Place the SID for DSP DMA transactions at bit 56 in the
>> physical address. This placement is necessary for the DSPs to correctly
>> identify streams and operate as intended.
>> To address this, set SID position to bit 56 via OF matching on the fastrpc
>> node; otherwise, default to legacy 32-bit placement.
>> This change ensures consistent SID placement across DSPs.
>>
>
> In patch 2 I said I think it would be a good idea to separate the two
> perspectives (Linux/SMMU vs remote addresses).
>
> Looking ta this patch I'm completely convinced that it's the right thing
> to do!
>
>> Signed-off-by: Kumari Pallavi <kumari.pallavi@....qualcomm.com>
>> ---
>> drivers/misc/fastrpc.c | 46 +++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index d6a7960fe716..bcf3c7f8d3e9 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -33,7 +33,6 @@
>> #define FASTRPC_ALIGN 128
>> #define FASTRPC_MAX_FDLIST 16
>> #define FASTRPC_MAX_CRCLIST 64
>> -#define FASTRPC_PHYS(p) ((p) & 0xffffffff)
>> #define FASTRPC_CTX_MAX (256)
>> #define FASTRPC_INIT_HANDLE 1
>> #define FASTRPC_DSP_UTILITIES_HANDLE 2
>> @@ -105,6 +104,15 @@
>>
>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>
>> +/* Extract smmu pa from consolidated iova */
>> +#define IPA_TO_DMA_ADDR(iova, sid_pos) (iova & ((1ULL << sid_pos) - 1ULL))
>> +/*
>> + * Prepare the consolidated iova to send to dsp by prepending the sid
>> + * to smmu pa at the appropriate position
>> + */
>> +#define IOVA_FROM_SID_PA(sid, phys, sid_pos) \
>> + (phys += sid << sid_pos)
>
> This is a horrible macro. It looks just like a function taking values,
> it's named to sound like it takes a sid and pa and return an iova, but
> it has side effects.
>
> And what's up with the ordering? Take argument 1 and 3, and put the
> result in argument 2?!
>
Thank you for the feedback regarding the macro implementation. I
understand your concern about readability and hidden side effects.
To address this, I’ve replaced the macro with an inline function
static inline u64 fastrpc_compute_sid_offset(u64 sid, u32 sid_pos)
{
return sid << sid_pos;
}
buf->dma_addr += fastrpc_compute_sid_offset(sid, sid_pos);
Could you confirm if this is in line with what you suggested?
Thanks,
Pallavi
>> +
>> struct fastrpc_phy_page {
>> u64 addr; /* physical or dma address */
>> u64 size; /* size of contiguous region */
>> @@ -257,6 +265,10 @@ struct fastrpc_session_ctx {
>> bool valid;
>> };
>>
>> +struct fastrpc_soc_data {
>> + u32 sid_pos;
>> +};
>> +
>> struct fastrpc_channel_ctx {
>> int domain_id;
>> int sesscount;
>> @@ -278,6 +290,7 @@ struct fastrpc_channel_ctx {
>> bool secure;
>> bool unsigned_support;
>> u64 dma_mask;
>> + const struct fastrpc_soc_data *soc_data;
>> };
>>
>> struct fastrpc_device {
>> @@ -390,7 +403,7 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>> static void fastrpc_buf_free(struct fastrpc_buf *buf)
>> {
>> dma_free_coherent(buf->dev, buf->size, buf->virt,
>> - FASTRPC_PHYS(buf->dma_addr));
>> + IPA_TO_DMA_ADDR(buf->dma_addr, buf->fl->cctx->soc_data->sid_pos));
>> kfree(buf);
>> }
>>
>> @@ -440,7 +453,8 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
>> buf = *obuf;
>>
>> if (fl->sctx && fl->sctx->sid)
>> - buf->dma_addr += ((u64)fl->sctx->sid << 32);
>> + IOVA_FROM_SID_PA((u64)fl->sctx->sid, buf->dma_addr,
>> + fl->cctx->soc_data->sid_pos);
>
> There's no way _you_ can look at this statement and feel that it's going
> to add the first argument shifted by the third argument, to the second
> argument.
>
> Please write that is easy to read, follow and possible to maintain!
>
> Regards,
> Bjorn
>
>>
>> return 0;
>> }
>> @@ -685,7 +699,8 @@ static int fastrpc_dma_buf_attach(struct dma_buf *dmabuf,
>> return -ENOMEM;
>>
>> ret = dma_get_sgtable(buffer->dev, &a->sgt, buffer->virt,
>> - FASTRPC_PHYS(buffer->dma_addr), buffer->size);
>> + IPA_TO_DMA_ADDR(buffer->dma_addr,
>> + buffer->fl->cctx->soc_data->sid_pos), buffer->size);
>> if (ret < 0) {
>> dev_err(buffer->dev, "failed to get scatterlist from DMA API\n");
>> kfree(a);
>> @@ -734,7 +749,8 @@ static int fastrpc_mmap(struct dma_buf *dmabuf,
>> dma_resv_assert_held(dmabuf->resv);
>>
>> return dma_mmap_coherent(buf->dev, vma, buf->virt,
>> - FASTRPC_PHYS(buf->dma_addr), size);
>> + IPA_TO_DMA_ADDR(buf->dma_addr,
>> + buf->fl->cctx->soc_data->sid_pos), size);
>> }
>>
>> static const struct dma_buf_ops fastrpc_dma_buf_ops = {
>> @@ -789,7 +805,8 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>> map->dma_addr = sg_phys(map->table->sgl);
>> } else {
>> map->dma_addr = sg_dma_address(map->table->sgl);
>> - map->dma_addr += ((u64)fl->sctx->sid << 32);
>> + IOVA_FROM_SID_PA((u64)fl->sctx->sid,
>> + map->dma_addr, fl->cctx->soc_data->sid_pos);
>> }
>> for_each_sg(map->table->sgl, sgl, map->table->nents,
>> sgl_index)
>> @@ -2289,6 +2306,14 @@ static int fastrpc_get_domain_id(const char *domain)
>> return -EINVAL;
>> }
>>
>> +static const struct fastrpc_soc_data kaanapali_soc_data = {
>> + .sid_pos = 56,
>> +};
>> +
>> +static const struct fastrpc_soc_data default_soc_data = {
>> + .sid_pos = 32,
>> +};
>> +
>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> {
>> struct device *rdev = &rpdev->dev;
>> @@ -2297,6 +2322,11 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> const char *domain;
>> bool secure_dsp;
>> unsigned int vmids[FASTRPC_MAX_VMIDS];
>> + const struct fastrpc_soc_data *soc_data = NULL;
>> +
>> + soc_data = device_get_match_data(rdev);
>> + if (!soc_data)
>> + soc_data = &default_soc_data;
>>
>> err = of_property_read_string(rdev->of_node, "label", &domain);
>> if (err) {
>> @@ -2349,6 +2379,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>
>> secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
>> data->secure = secure_dsp;
>> + data->soc_data = soc_data;
>>
>> switch (domain_id) {
>> case ADSP_DOMAIN_ID:
>> @@ -2486,7 +2517,8 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>> }
>>
>> static const struct of_device_id fastrpc_rpmsg_of_match[] = {
>> - { .compatible = "qcom,fastrpc" },
>> + { .compatible = "qcom,kaanapali-fastrpc", .data = &kaanapali_soc_data },
>> + { .compatible = "qcom,fastrpc", .data = &default_soc_data },
>> { },
>> };
>> MODULE_DEVICE_TABLE(of, fastrpc_rpmsg_of_match);
>> --
>> 2.34.1
>>
>>
Powered by blists - more mailing lists