On Tue, Jul 28, 2026 at 10:29:21PM +0100, Pavel Begunkov wrote:
> From: Anuj Gupta <[email protected]>
>
> Add SGL support in addition to PRP for dmabuf-backed requests,
> coalescing the mapping's sg_table into NVMe SGL data descriptors.
>
> Signed-off-by: Anuj Gupta <[email protected]>
> [pavel: rebased]
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
> drivers/nvme/host/pci.c | 191 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 187 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f1b67c191892..cbb321fb7c50 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1287,12 +1287,18 @@ static blk_status_t nvme_pci_setup_data_prp(struct
> request *req,
> return BLK_STS_IOERR;
> }
>
> +static void nvme_pci_sgl_set_data_addr(struct nvme_sgl_desc *sge,
> + dma_addr_t addr, u32 len)
> +{
> + sge->addr = cpu_to_le64(addr);
> + sge->length = cpu_to_le32(len);
> + sge->type = NVME_SGL_FMT_DATA_DESC << 4;
> +}
> +
> static void nvme_pci_sgl_set_data(struct nvme_sgl_desc *sge,
> struct blk_dma_iter *iter)
> {
> - sge->addr = cpu_to_le64(iter->addr);
> - sge->length = cpu_to_le32(iter->len);
> - sge->type = NVME_SGL_FMT_DATA_DESC << 4;
> + nvme_pci_sgl_set_data_addr(sge, iter->addr, iter->len);
> }
The naming is a bit confusing (and me passing the iter to
nvme_pci_sgl_set_data is probably at faul for that). So maybe
spin out a prep patch to rename the old nvme_pci_sgl_set_data
to nvme_pci_dma_iter_set_sgl or so, and then add the new one
as nvme_pci_sgl_set_data (as before the dma_iter conversion).
>
> +static unsigned int nvme_pci_dmabuf_sgl_nents(struct request *req,
> + dma_addr_t *first_dma,
> + u32 *first_len)
This is a really good example why the aligning to the opening braces
produces totally unreadble code..
But I also don't understand what the use case for this function
is to start with. struct sg_table tells us how many segments
exist on the DMA side in the nents member, which should be just
fine for the SGL threshold calculation.
> +{
> + struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> + struct bio *bio = req->bio;
> + struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
> + size_t length = blk_rq_payload_bytes(req);
> + struct nvme_sgl_desc *sg_list = NULL;
> + dma_addr_t sgl_dma = 0, last_end = 0;
> + unsigned int mapped = 0;
> + unsigned long tmp;
> + struct scatterlist *sg;
> + size_t offset, remaining;
> + bool have = false;
> +
> + if (!entries)
> + return BLK_STS_IOERR;
> + if (entries > NVME_MAX_SEGS)
> + return BLK_STS_AGAIN;
Given that the block layer enforced data in rw/command and the
max_segments limit, why do we need the extra check here?
> + continue;
> + }
> +
> + addr += offset;
> + sg_len -= offset;
> + offset = 0;
> +
> + while (sg_len && remaining) {
These can't be false on the first iteration, so maybe turn this into
a do {} while loop?
> + u32 chunk = min_t(size_t, remaining, sg_len);
> +
> + if (have && last_end == addr) {
> + u32 old = le32_to_cpu(sg_list[mapped -
> 1].length);
> +
> + sg_list[mapped - 1].length = cpu_to_le32(old +
> chunk);
Overly long line.
> + } else {
> + if (WARN_ON_ONCE(mapped == entries))
> + goto err_free;
> + nvme_pci_sgl_set_data_addr(&sg_list[mapped++],
> + addr, chunk);
> + }
Why do we need this merging? dma_map_sg should have already done
any interesting merging, or am I missing something?
> + if (use_sgl != SGL_UNSUPPORTED) {
> + dma_addr_t first_dma;
> + u32 first_len;
> + unsigned int entries;
> +
> + entries = nvme_pci_dmabuf_sgl_nents(req, &first_dma,
> + &first_len);
> +
> + if (use_sgl == SGL_FORCED) {
> + ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq,
> + entries,
> first_dma, first_len);
> + return ret == BLK_STS_AGAIN ? BLK_STS_IOERR :
> ret;
> + }
> +
> + if (sgl_threshold && entries &&
> + DIV_ROUND_UP(blk_rq_payload_bytes(req), entries) >=
> + sgl_threshold) {
> + ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq,
> + entries,
> first_dma, first_len);
> + if (ret != BLK_STS_AGAIN)
> + return ret;
> + }
> + }
Various overly long lines. Please factor out a helper for the
decisions to use sgl vs not instead of open coding it here.
