| 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: <yenp6ss5u5s4h75s7ggpyecmkfiiwd6vidqh7zu4yd3cahyoh2@iuja6rwwg4pi>
Date: Tue, 18 Nov 2025 10:04:24 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Kumari Pallavi <kumari.pallavi@....qualcomm.com>
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 Mon, Nov 17, 2025 at 12:37:33PM +0530, Kumari Pallavi wrote:
>
>
> 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 I understand correctly, when FASTRPC_ATTR_SECUREMAP is set that
implies that the mapping happens in the firmware SID, i.e. outside of
Linux's view. So it's still a dma_addr_t, but it's mapped 1:1 with the
physical address.
I think this is another good reason to make the changes I suggested
below (which you didn't comment on?). Sometimes this "addr" is the
actual address and sometimes it contains the annotated address.
>
> If you suggest, I can define it as dma_addr_t and perform typecasting to u64
> wherever required.
Yes, maintain the mapping in a dma_addr_t, then you have the two cases:
1) Contexts with SMMU, you cast the dma_addr_t and annotate it with the
sid information before communicate the addresses to the firmware.
2) Contexts without SMMU, you make it clear that your dma_addr_t is 1:1
with physical addresses and you then pass it to qcom_scm_assign_mem() et
al.
That way the "addr" is always the actual iova (with or without mapping)
and the places where it's treated as a physical address or an annotated
"address" becomes very explicit.
Regards,
Bjorn
>
> 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