Re: [PATCH rdma-next 1/2] arm64/io: add memcpy_toio_64

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Jan 24, 2024 at 12:40:29PM +0000, Catalin Marinas wrote:

> > Just to be clear, that means we should drop this patch ("arm64/io: add
> > memcpy_toio_64") for now, right?
> 
> In its current form yes, but that doesn't mean that memcpy_toio_64()
> cannot be reworked differently.

I gave up on touching memcpy_toio_64(), it doesn't work very well
because of the weak alignment

Instead I followed your suggestion to fix __iowrite64_copy()

There are only a couple of places that use this API:

drivers/infiniband/hw/bnxt_re/qplib_rcfw.c:     __iowrite32_copy(mbox->reg.bar_reg, &init, sizeof(init) / 4);
drivers/mtd/nand/raw/mxc_nand.c:        __iowrite32_copy(trg, src, size / 4);
drivers/net/ethernet/amazon/ena/ena_eth_com.c:  __iowrite64_copy(io_sq->desc_addr.pbuf_dev_addr + dst_offset,
drivers/net/ethernet/broadcom/bnxt/bnxt.c:                      __iowrite64_copy(db, tx_push_buf, 16);
drivers/net/ethernet/broadcom/bnxt/bnxt.c:                      __iowrite32_copy(db + 4, tx_push_buf + 1,
drivers/net/ethernet/broadcom/bnxt/bnxt.c:                      __iowrite64_copy(db, tx_push_buf, push_len);
drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c: __iowrite32_copy(bp->bar0 + bar_offset, data, msg_len / 4);
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:        __iowrite64_copy(ring->tqp->mem_base, desc,
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:        __iowrite64_copy(ring->tqp->mem_base + HNS3_MEM_DOORBELL_OFFSET,
drivers/net/ethernet/mellanox/mlx4/en_tx.c:     __iowrite64_copy(dst, src, bytecnt / 8);
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:#define myri10ge_pio_copy(to,from,size) __iowrite64_copy(to,from,size/8)
drivers/net/ethernet/sfc/tx.c:  __iowrite64_copy(*piobuf, data, block_len >> 3);
drivers/net/ethernet/sfc/tx.c:          __iowrite64_copy(*piobuf, copy_buf->buf,
drivers/net/ethernet/sfc/tx.c:          __iowrite64_copy(piobuf, copy_buf->buf,
drivers/net/ethernet/sfc/tx.c:          __iowrite64_copy(tx_queue->piobuf, skb->data,
drivers/net/wireless/mediatek/mt76/mmio.c:      __iowrite32_copy(dev->mmio.regs + offset, data, DIV_ROUND_UP(len, 4));
drivers/net/wireless/ralink/rt2x00/rt2x00mmio.h:        __iowrite32_copy(rt2x00dev->csr.base + offset, value, length >> 2);
drivers/remoteproc/mtk_scp_ipi.c:       __iowrite32_copy(dst + i, src + i, (len - i) / 4);
drivers/rpmsg/qcom_glink_rpm.c:         __iowrite32_copy(pipe->fifo + head, data,
drivers/rpmsg/qcom_glink_rpm.c:         __iowrite32_copy(pipe->fifo, data + len,
drivers/rpmsg/qcom_smd.c:               __iowrite32_copy(dst, src, count / sizeof(u32));
drivers/scsi/lpfc/lpfc_compat.h:        __iowrite32_copy(dest, src, bytes / sizeof(uint32_t));
drivers/slimbus/qcom-ctrl.c:    __iowrite32_copy(ctrl->base + tx_reg, buf, count);
drivers/soc/qcom/qcom_aoss.c:   __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
drivers/soc/qcom/spm.c: __iowrite32_copy(addr, drv->reg_data->seq,
drivers/spi/spi-hisi-sfc-v3xx.c:                __iowrite32_copy(to, from, words);
sound/soc/intel/atom/sst/sst_loader.c:  __iowrite32_copy(dst, src, count / 4);
sound/soc/sof/iomem-utils.c:    __iowrite32_copy(dest, src, m);

At least the networking ones I recognize as performance paths, we
don't want to degrade them.

__iowrite64_copy() has a sufficient API that the compiler can inline
the STP block as this patch did.

I experimented with having memcpy_toio_64() invoke __iowrite64_copy(),
but it did not look very nice. Maybe there is a possible performance
win there, I don't know.

> > > If eight STRs without other operations interleaved give us the
> > > write-combining on most CPUs (with Normal NC), we should go with this
> > > instead of STP.
> > 
> > Agreed; I've sent out a patch to allow the offset addressing at:
> > 
> >   https://lore.kernel.org/linux-arm-kernel/20240124111259.874975-1-mark.rutland@arm.com/
> > 
> > ... and it should be possible to build atop that to use eight STRs.
> 
> That's great, thanks.

It is a nice patch but it does not really help this problem. The
compiler cannot be trusted to use the new writeq() properly, eg clang
doesn't optimize the new constraint at all.

Regardless this has to be a fixed inline assembly block of either STR
or STP.

Jason