| 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: <83112aa3-5998-4030-98cb-d3d5ee7c0570@oss.qualcomm.com>
Date: Mon, 17 Nov 2025 12:37:33 +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 2/4] misc: fastrpc: Rename phys to dma_addr for clarity
On 11/14/2025 9:14 PM, Bjorn Andersson wrote:
> On Fri, Nov 14, 2025 at 02:11:40PM +0530, Kumari Pallavi wrote:
>> Update all references of buf->phys and map->phys to buf->dma_addr and
>> map->dma_addr to accurately represent that these fields store DMA
>> addresses, not physical addresses. This change improves code clarity
>> and aligns with kernel conventions for dma_addr_t usage.
>>
>> Signed-off-by: Kumari Pallavi <kumari.pallavi@....qualcomm.com>
>> ---
>> drivers/misc/fastrpc.c | 76 ++++++++++++++++++++++--------------------
>> 1 file changed, 40 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index ee652ef01534..d6a7960fe716 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -106,7 +106,7 @@
>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>
>> struct fastrpc_phy_page {
>> - u64 addr; /* physical address */
>> + u64 addr; /* physical or dma address */
>> u64 size; /* size of contiguous region */
>> };
>>
>> @@ -171,7 +171,7 @@ struct fastrpc_msg {
>> u64 ctx; /* invoke caller context */
>> u32 handle; /* handle to invoke */
>> u32 sc; /* scalars structure describing the data */
>> - u64 addr; /* physical address */
>> + u64 addr; /* physical or dma address */
>
> Can you go all the way and make the type dma_addr_t? That way you don't
> need to typecast the dma_alloc_coherent() and dma_free_coherent().
>
> I believe it might complicate the places where you do math on it, but
> that is a good thing, as it highlights those places where you do
> something unexpected.
>
While this not strictly limited to holding a dma_addr_t.
Based on historical behavior, when the FASTRPC_ATTR_SECUREMAP flag is
set, S2 mapping expects a physical address to be passed to the
qcom_scm_assign_mem() API.
reference-
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/misc/fastrpc.c?id=e90d911906196bf987492c94e38f10ca611dfd7b
If you suggest, I can define it as dma_addr_t and perform typecasting to
u64 wherever required.
Thanks,
Pallavi
>> u64 size; /* size of contiguous region */
>> };
>>
>> @@ -194,7 +194,7 @@ struct fastrpc_buf {
>> struct dma_buf *dmabuf;
>> struct device *dev;
>> void *virt;
>> - u64 phys;
>> + u64 dma_addr;
>> u64 size;
>> /* Lock for dma buf attachments */
>> struct mutex lock;
>> @@ -217,7 +217,7 @@ struct fastrpc_map {
>> struct dma_buf *buf;
>> struct sg_table *table;
>> struct dma_buf_attachment *attach;
>> - u64 phys;
>> + u64 dma_addr;
>> u64 size;
>> void *va;
>> u64 len;
>> @@ -320,11 +320,12 @@ static void fastrpc_free_map(struct kref *ref)
>>
>> perm.vmid = QCOM_SCM_VMID_HLOS;
>> perm.perm = QCOM_SCM_PERM_RWX;
>> - err = qcom_scm_assign_mem(map->phys, map->len,
>> + err = qcom_scm_assign_mem(map->dma_addr, map->len,
>> &src_perms, &perm, 1);
>> if (err) {
>> - dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
>> - map->phys, map->len, err);
>> + dev_err(map->fl->sctx->dev,
>> + "Failed to assign memory dma_addr 0x%llx size 0x%llx err %d\n",
>> + map->dma_addr, map->len, err);
>> return;
>> }
>> }
>> @@ -389,7 +390,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->phys));
>> + FASTRPC_PHYS(buf->dma_addr));
>> kfree(buf);
>> }
>>
>> @@ -408,12 +409,12 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
>>
>> buf->fl = fl;
>> buf->virt = NULL;
>> - buf->phys = 0;
>> + buf->dma_addr = 0;
>> buf->size = size;
>> buf->dev = dev;
>> buf->raddr = 0;
>>
>> - buf->virt = dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->phys,
>> + buf->virt = dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->dma_addr,
>> GFP_KERNEL);
>> if (!buf->virt) {
>> mutex_destroy(&buf->lock);
>> @@ -439,7 +440,7 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
>> buf = *obuf;
>>
>> if (fl->sctx && fl->sctx->sid)
>> - buf->phys += ((u64)fl->sctx->sid << 32);
>> + buf->dma_addr += ((u64)fl->sctx->sid << 32);
>>
>> return 0;
>> }
>> @@ -684,7 +685,7 @@ 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->phys), buffer->size);
>> + FASTRPC_PHYS(buffer->dma_addr), buffer->size);
>> if (ret < 0) {
>> dev_err(buffer->dev, "failed to get scatterlist from DMA API\n");
>> kfree(a);
>> @@ -733,7 +734,7 @@ 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->phys), size);
>> + FASTRPC_PHYS(buf->dma_addr), size);
>
> In fact, we invoke dma_alloc_coherent() above to get a dma_addr_t, and
> then we call map, unmap, and free on the lower 32 bits of that
> address...
>
> In other words, each time we reference dma_addr we have that implicit
> thing that it's a composit of a dma_addr_t as seen from Linux's point of
> view (which is matching the addresses in the SMMU page tables) and the
> adjusted address that we use in communication with the firmware to
> direct the accesses to the right SID + iova.
>
> I think it would be quite nice to make this more explicit throughout the
> code, rather than juggling the two perspectives in the same variable.
>
> Regards,
> Bjorn
>
>> }
>>
>> static const struct dma_buf_ops fastrpc_dma_buf_ops = {
>> @@ -785,10 +786,10 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>> map->table = table;
>>
>> if (attr & FASTRPC_ATTR_SECUREMAP) {
>> - map->phys = sg_phys(map->table->sgl);
>> + map->dma_addr = sg_phys(map->table->sgl);
>> } else {
>> - map->phys = sg_dma_address(map->table->sgl);
>> - map->phys += ((u64)fl->sctx->sid << 32);
>> + map->dma_addr = sg_dma_address(map->table->sgl);
>> + map->dma_addr += ((u64)fl->sctx->sid << 32);
>> }
>> for_each_sg(map->table->sgl, sgl, map->table->nents,
>> sgl_index)
>> @@ -815,10 +816,11 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>> dst_perms[1].vmid = fl->cctx->vmperms[0].vmid;
>> dst_perms[1].perm = QCOM_SCM_PERM_RWX;
>> map->attr = attr;
>> - err = qcom_scm_assign_mem(map->phys, (u64)map->len, &src_perms, dst_perms, 2);
>> + err = qcom_scm_assign_mem(map->dma_addr, (u64)map->len, &src_perms, dst_perms, 2);
>> if (err) {
>> - dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
>> - map->phys, map->len, err);
>> + dev_err(sess->dev,
>> + "Failed to assign memory with dma_addr 0x%llx size 0x%llx err %d\n",
>> + map->dma_addr, map->len, err);
>> goto map_err;
>> }
>> }
>> @@ -1009,7 +1011,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>> struct vm_area_struct *vma = NULL;
>>
>> rpra[i].buf.pv = (u64) ctx->args[i].ptr;
>> - pages[i].addr = ctx->maps[i]->phys;
>> + pages[i].addr = ctx->maps[i]->dma_addr;
>>
>> mmap_read_lock(current->mm);
>> vma = find_vma(current->mm, ctx->args[i].ptr);
>> @@ -1036,7 +1038,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>> goto bail;
>>
>> rpra[i].buf.pv = args - ctx->olaps[oix].offset;
>> - pages[i].addr = ctx->buf->phys -
>> + pages[i].addr = ctx->buf->dma_addr -
>> ctx->olaps[oix].offset +
>> (pkt_size - rlen);
>> pages[i].addr = pages[i].addr & PAGE_MASK;
>> @@ -1068,7 +1070,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>> list[i].num = ctx->args[i].length ? 1 : 0;
>> list[i].pgidx = i;
>> if (ctx->maps[i]) {
>> - pages[i].addr = ctx->maps[i]->phys;
>> + pages[i].addr = ctx->maps[i]->dma_addr;
>> pages[i].size = ctx->maps[i]->size;
>> }
>> rpra[i].dma.fd = ctx->args[i].fd;
>> @@ -1150,7 +1152,7 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
>> msg->ctx = ctx->ctxid | fl->pd;
>> msg->handle = handle;
>> msg->sc = ctx->sc;
>> - msg->addr = ctx->buf ? ctx->buf->phys : 0;
>> + msg->addr = ctx->buf ? ctx->buf->dma_addr : 0;
>> msg->size = roundup(ctx->msg_sz, PAGE_SIZE);
>> fastrpc_context_get(ctx);
>>
>> @@ -1306,13 +1308,14 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> if (fl->cctx->vmcount) {
>> u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>
>> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>> + err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
>> (u64)fl->cctx->remote_heap->size,
>> &src_perms,
>> fl->cctx->vmperms, fl->cctx->vmcount);
>> if (err) {
>> - dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
>> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
>> + dev_err(fl->sctx->dev,
>> + "Failed to assign memory with dma_addr 0x%llx size 0x%llx err %d\n",
>> + fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
>> goto err_map;
>> }
>> scm_done = true;
>> @@ -1332,7 +1335,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> args[1].length = inbuf.namelen;
>> args[1].fd = -1;
>>
>> - pages[0].addr = fl->cctx->remote_heap->phys;
>> + pages[0].addr = fl->cctx->remote_heap->dma_addr;
>> pages[0].size = fl->cctx->remote_heap->size;
>>
>> args[2].ptr = (u64)(uintptr_t) pages;
>> @@ -1361,12 +1364,12 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>
>> dst_perms.vmid = QCOM_SCM_VMID_HLOS;
>> dst_perms.perm = QCOM_SCM_PERM_RWX;
>> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>> + err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
>> (u64)fl->cctx->remote_heap->size,
>> &src_perms, &dst_perms, 1);
>> if (err)
>> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
>> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
>> + dev_err(fl->sctx->dev, "Failed to assign memory dma_addr 0x%llx size 0x%llx err %d\n",
>> + fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
>> }
>> err_map:
>> fastrpc_buf_free(fl->cctx->remote_heap);
>> @@ -1455,7 +1458,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>> args[2].length = inbuf.filelen;
>> args[2].fd = init.filefd;
>>
>> - pages[0].addr = imem->phys;
>> + pages[0].addr = imem->dma_addr;
>> pages[0].size = imem->size;
>>
>> args[3].ptr = (u64)(uintptr_t) pages;
>> @@ -1913,7 +1916,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> args[0].ptr = (u64) (uintptr_t) &req_msg;
>> args[0].length = sizeof(req_msg);
>>
>> - pages.addr = buf->phys;
>> + pages.addr = buf->dma_addr;
>> pages.size = buf->size;
>>
>> args[1].ptr = (u64) (uintptr_t) &pages;
>> @@ -1941,11 +1944,12 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
>> u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>
>> - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> + err = qcom_scm_assign_mem(buf->dma_addr, (u64)buf->size,
>> &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>> if (err) {
>> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>> - buf->phys, buf->size, err);
>> + dev_err(fl->sctx->dev,
>> + "Failed to assign memory dma_addr 0x%llx size 0x%llx err %d",
>> + buf->dma_addr, buf->size, err);
>> goto err_assign;
>> }
>> }
>> @@ -2059,7 +2063,7 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
>> args[0].ptr = (u64) (uintptr_t) &req_msg;
>> args[0].length = sizeof(req_msg);
>>
>> - pages.addr = map->phys;
>> + pages.addr = map->dma_addr;
>> pages.size = map->len;
>>
>> args[1].ptr = (u64) (uintptr_t) &pages;
>> --
>> 2.34.1
>>
>>
Powered by blists - more mailing lists