Properly take MMIO path
Subsystems (2)
cover Properly take MMIO path
This is NVMe and block specific patches from my DMA series [1] which improved DMA physical API to properly support MMIO memory.
Thanks
[1] https://lore.kernel.org/all/[email protected]/
Leon Romanovsky (4):
blk-mq-dma: migrate to dma_map_phys instead of map_page
blk-mq-dma: unify DMA unmap routine
block-dma: properly take MMIO path
nvme-pci: unmap MMIO pages with appropriate interface
block/blk-mq-dma.c | 52 +++++++++++++++++++++++++++++++++-- drivers/nvme/host/pci.c | 18 ++++++++---- include/linux/bio-integrity.h | 1 + include/linux/blk-integrity.h | 3 +- include/linux/blk-mq-dma.h | 35 ++--------------------- include/linux/blk_types.h | 2 ++ 6 files changed, 68 insertions(+), 43 deletions(-)
Signature · 1 line
2.51.0
PATCH 1/4 blk-mq-dma: migrate to dma_map_phys instead of map_page
From: Leon Romanovsky <[email protected]>
After introduction of dma_map_phys(), there is no need to convert from physical address to struct page in order to map page. So let's use it directly.
--- block/blk-mq-dma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
block/blk-mq-dma.c +2 -2
@@ -93,8 +93,8 @@ static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec)static bool blk_dma_map_direct (struct request *req,struct device *dma_dev,struct blk_dma_iter *iter,struct phys_vec *vec) { - iter->addr =dma_map_page (dma_dev,phys_to_page (vec->paddr), -offset_in_page (vec->paddr), vec->len,rq_dma_dir (req)); + iter->addr =dma_map_phys (dma_dev, vec->paddr, vec->len, +rq_dma_dir (req),0 );if (dma_mapping_error (dma_dev, iter->addr)) { iter->status =BLK_STS_RESOURCE ;return false;
Signature · 1 line
2.51.0
PATCH 2/4 blk-mq-dma: unify DMA unmap routine
In reply to Leon RomanovskyFrom: Leon Romanovsky <[email protected]>
Combine regular and meta-intgerity DMA unmapping flow to one blk_rq_dma_unmap(). This allows us to handle addition of new DMA_ATTR_MMIO flow without adding extra function parameters.
--- block/blk-mq-dma.c | 29 +++++++++++++++++++++++++++++ include/linux/blk-integrity.h | 3 +-- include/linux/blk-mq-dma.h | 35 ++--------------------------------- 3 files changed, 32 insertions(+), 35 deletions(-)
block/blk-mq-dma.c +29 -0
@@ -260,6 +260,35 @@ bool blk_rq_dma_map_iter_next(struct request *req, struct device *dma_dev, }EXPORT_SYMBOL_GPL (blk_rq_dma_map_iter_next); +/** +* blk_rq_dma_unmap - try to DMA unmap a request +* @req: request to unmap +* @dma_dev: device to unmap from +* @state: DMA IOVA state +* @mapped_len: number of bytes to unmap +* +* Returns %false if the callers need to manually unmap every DMA segment +* mapped using @iter or %true if no work is left to be done. +*/ +bool blk_rq_dma_unmap (struct request *req,struct device *dma_dev, +struct dma_iova_state *state,size_t mapped_len) +{ +struct bio_integrity_payload *bip =bio_integrity (req->bio); + +if ((!bip && req->cmd_flags &REQ_P2PDMA ) || +bio_integrity_flagged (req->bio,BIP_P2P_DMA )) +return true; + +if (dma_use_iova (state)) { +dma_iova_destroy (dma_dev, state, mapped_len,rq_dma_dir (req), +0 ); +return true; + } + +return !dma_need_unmap (dma_dev); +} +EXPORT_SYMBOL (blk_rq_dma_unmap); +static inline struct scatterlist *blk_next_sg (struct scatterlist **sg,struct scatterlist *sglist) {
include/linux/blk-integrity.h +1 -2
@@ -32,8 +32,7 @@ static inline bool blk_rq_integrity_dma_unmap(struct request *req, struct device *dma_dev, struct dma_iova_state *state, size_t mapped_len) { - return blk_dma_unmap(req, dma_dev, state, mapped_len, - bio_integrity(req->bio)->bip_flags & BIP_P2P_DMA); + return blk_rq_dma_unmap(req, dma_dev, state, mapped_len); }int blk_rq_count_integrity_sg (struct request_queue *,struct bio *);
include/linux/blk-mq-dma.h +2 -33
@@ -29,6 +29,8 @@ bool blk_rq_dma_map_iter_start(struct request *req, struct device *dma_dev, struct dma_iova_state *state, struct blk_dma_iter *iter);bool blk_rq_dma_map_iter_next (struct request *req,struct device *dma_dev,struct dma_iova_state *state,struct blk_dma_iter *iter); +bool blk_rq_dma_unmap (struct request *req,struct device *dma_dev, +struct dma_iova_state *state,size_t mapped_len);/** * blk_rq_dma_map_coalesce - were all segments coalesced?
@@ -42,37 +44,4 @@ static inline bool blk_rq_dma_map_coalesce(struct dma_iova_state *state) return dma_use_iova(state); } -/** -* blk_dma_unmap - try to DMA unmap a request -* @req: request to unmap -* @dma_dev: device to unmap from -* @state: DMA IOVA state -* @mapped_len: number of bytes to unmap -* @is_p2p: true if mapped with PCI_P2PDMA_MAP_BUS_ADDR -* -* Returns %false if the callers need to manually unmap every DMA segment -* mapped using @iter or %true if no work is left to be done. -*/ -static inline bool blk_dma_unmap (struct request *req,struct device *dma_dev, -struct dma_iova_state *state,size_t mapped_len,bool is_p2p) -{ -if (is_p2p) -return true; - -if (dma_use_iova (state)) { -dma_iova_destroy (dma_dev, state, mapped_len,rq_dma_dir (req), -0 ); -return true; - } - -return !dma_need_unmap (dma_dev); -} - -static inline bool blk_rq_dma_unmap (struct request *req,struct device *dma_dev, -struct dma_iova_state *state,size_t mapped_len) -{ -return blk_dma_unmap (req, dma_dev, state, mapped_len, - req->cmd_flags &REQ_P2PDMA ); -} -#endif /* BLK_MQ_DMA_H */
Signature · 1 line
2.51.0
PATCH 3/4 block-dma: properly take MMIO path
In reply to Leon RomanovskyFrom: Leon Romanovsky <[email protected]>
Make sure that CPU is not synced and IOMMU is configured to take MMIO path by providing newly introduced DMA_ATTR_MMIO attribute.
--- block/blk-mq-dma.c | 23 ++++++++++++++++++++--- include/linux/bio-integrity.h | 1 + include/linux/blk_types.h | 2 ++ 3 files changed, 23 insertions(+), 3 deletions(-)
block/blk-mq-dma.c +20 -3
@@ -93,8 +93,13 @@ static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec)static bool blk_dma_map_direct (struct request *req,struct device *dma_dev,struct blk_dma_iter *iter,struct phys_vec *vec) { +unsigned int attrs =0 ; + +if (req->cmd_flags &REQ_MMIO ) + attrs |=DMA_ATTR_MMIO ; + iter->addr =dma_map_phys (dma_dev, vec->paddr, vec->len, -rq_dma_dir (req),0 ); +rq_dma_dir (req), attrs);if (dma_mapping_error (dma_dev, iter->addr)) { iter->status =BLK_STS_RESOURCE ;return false;
@@ -109,14 +114,17 @@ static bool blk_rq_dma_map_iova(struct request *req, struct device *dma_dev, {enum dma_data_direction dir =rq_dma_dir (req);unsigned int mapped =0 ; +unsigned int attrs =0 ;int error; iter->addr = state->addr; iter->len =dma_iova_size (state); +if (req->cmd_flags &REQ_MMIO ) + attrs |=DMA_ATTR_MMIO ; do { error = dma_iova_link(dma_dev, state, vec->paddr, mapped, - vec->len, dir, 0); + vec->len, dir, attrs); if (error) break; mapped += vec->len;
@@ -184,6 +192,11 @@ static bool blk_dma_map_iter_start(struct request *req, struct device *dma_dev,* P2P transfers through the host bridge are treated the * same as non-P2P transfers below and during unmap. */ +if (iter->iter.is_integrity) +bio_integrity (req->bio)->bip_flags |=BIP_MMIO ; +else + req->cmd_flags |=REQ_MMIO ; + fallthrough;case PCI_P2PDMA_MAP_NONE :break ;default :
@@ -274,14 +287,18 @@ bool blk_rq_dma_unmap(struct request *req, struct device *dma_dev, struct dma_iova_state *state, size_t mapped_len) {struct bio_integrity_payload *bip =bio_integrity (req->bio); +unsigned int attrs =0 ;if ((!bip && req->cmd_flags &REQ_P2PDMA ) ||bio_integrity_flagged (req->bio,BIP_P2P_DMA ))return true;if (dma_use_iova (state)) { +if ((!bip && req->cmd_flags &REQ_MMIO ) || +bio_integrity_flagged (req->bio,BIP_MMIO )) + attrs |=DMA_ATTR_MMIO ;dma_iova_destroy (dma_dev, state, mapped_len,rq_dma_dir (req), -0 ); + attrs);return true; }
include/linux/bio-integrity.h +1 -0
@@ -14,6 +14,7 @@ enum bip_flags {BIP_CHECK_REFTAG =1 <<6 ,/* reftag check */ BIP_CHECK_APPTAG =1 <<7 ,/* apptag check */ BIP_P2P_DMA =1 <<8 ,/* using P2P address */ + BIP_MMIO = 1 << 9, /* contains MMIO memory */ }; struct bio_integrity_payload {
include/linux/blk_types.h +2 -0
@@ -382,6 +382,7 @@ enum req_flag_bits { __REQ_FS_PRIVATE,/* for file system (submitter) use */ __REQ_ATOMIC,/* for atomic write operations */ __REQ_P2PDMA,/* contains P2P DMA pages */ + __REQ_MMIO, /* contains MMIO memory *//* * Command specific flags, keep last: */
@@ -415,6 +416,7 @@ enum req_flag_bits {#define REQ_FS_PRIVATE (__force blk_opf_t)(1ULL << __REQ_FS_PRIVATE)#define REQ_ATOMIC (__force blk_opf_t)(1ULL << __REQ_ATOMIC)#define REQ_P2PDMA (__force blk_opf_t)(1ULL << __REQ_P2PDMA) +#define REQ_MMIO (__force blk_opf_t)(1ULL << __REQ_MMIO)#define REQ_NOUNMAP (__force blk_opf_t)(1ULL << __REQ_NOUNMAP)
Signature · 1 line
2.51.0
PATCH 4/4 nvme-pci: unmap MMIO pages with appropriate interface
In reply to Leon RomanovskyFrom: Leon Romanovsky <[email protected]>
Block layer maps MMIO memory through dma_map_phys() interface with help of DMA_ATTR_MMIO attribute. There is a need to unmap that memory with the appropriate unmap function, something which wasn't possible before adding new REQ attribute to block layer in previous patch.
--- drivers/nvme/host/pci.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/nvme/host/pci.c +13 -5
@@ -689,11 +689,15 @@ static void nvme_free_prps(struct request *req) {struct nvme_iod *iod =blk_mq_rq_to_pdu (req);struct nvme_queue *nvmeq = req->mq_hctx->driver_data; +unsigned int attrs =0 ;unsigned int i; +if (req->cmd_flags &REQ_MMIO ) + attrs |=DMA_ATTR_MMIO ; +for (i =0 ; i < iod->nr_dma_vecs; i++) -dma_unmap_page (nvmeq->dev->dev, iod->dma_vecs[i].addr, - iod->dma_vecs[i].len,rq_dma_dir (req)); +dma_unmap_phys (nvmeq->dev->dev, iod->dma_vecs[i].addr, + iod->dma_vecs[i].len,rq_dma_dir (req), attrs);mempool_free (iod->dma_vecs, nvmeq->dev->dmavec_mempool); }
@@ -704,16 +708,20 @@ static void nvme_free_sgls(struct request *req, struct nvme_sgl_desc *sge,enum dma_data_direction dir =rq_dma_dir (req);unsigned int len =le32_to_cpu (sge->length);struct device *dma_dev = nvmeq->dev->dev; +unsigned int attrs =0 ;unsigned int i; +if (req->cmd_flags &REQ_MMIO ) + attrs |=DMA_ATTR_MMIO ; +if (sge->type == (NVME_SGL_FMT_DATA_DESC <<4 )) { -dma_unmap_page (dma_dev,le64_to_cpu (sge->addr), len, dir); +dma_unmap_phys (dma_dev,le64_to_cpu (sge->addr), len, dir, attrs);return ; }for (i =0 ; i < len /sizeof (*sg_list); i++) -dma_unmap_page (dma_dev,le64_to_cpu (sg_list[i].addr), -le32_to_cpu (sg_list[i].length), dir); +dma_unmap_phys (dma_dev,le64_to_cpu (sg_list[i].addr), +le32_to_cpu (sg_list[i].length), dir, attrs); }static void nvme_unmap_metadata (struct request *req)
Signature · 1 line
2.51.0
Keith Busch
maintainerRe: [PATCH 2/4] blk-mq-dma: unify DMA unmap routine
In reply to Leon Romanovskyblock/blk-mq-dma.c bool blk_rq_dma_map_iter_next(struct request *req, struct device *dma_dev, · line 280
Quoted 8 lines · Leon Romanovsky
block/blk-mq-dma.c +8
-0+bool blk_rq_dma_unmap (struct request *req,struct device *dma_dev, +struct dma_iova_state *state,size_t mapped_len) +{ +struct bio_integrity_payload *bip =bio_integrity (req->bio); + +if ((!bip && req->cmd_flags &REQ_P2PDMA ) || +bio_integrity_flagged (req->bio,BIP_P2P_DMA )) +return true;
I don't think you can unify it at this part here because the data payload might not be P2P but the integrity payload could be. The data payload needs to proceed to the next unmapping step in that case, but this change would have it return true early.
Keith Busch
maintainerRe: [PATCH 3/4] block-dma: properly take MMIO path
In reply to Leon Romanovskyblock/blk-mq-dma.c static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec) · line 99
Quoted 7 lines · Leon Romanovsky
static bool blk_dma_map_direct(struct request *req, struct device
*dma_dev,block/blk-mq-dma.c +4
-0struct blk_dma_iter *iter, struct phys_vec *vec) { + unsigned int attrs = 0; + +if (req->cmd_flags &REQ_MMIO ) + attrs |=DMA_ATTR_MMIO ;
Since data and integrity paylods use these same functions and may point to different kinds of memory, I think you'd have to pass the 'attrs' from the caller since it knows which flags to check for MMIO dma.
Re: [PATCH 3/4] block-dma: properly take MMIO path
Quoted 12 lines · Keith Busch
Review
Quoted 7 lines · Leon Romanovsky
static bool blk_dma_map_direct(struct request *req, struct device
*dma_dev,(quoted diff) +4
-0struct blk_dma_iter *iter, struct phys_vec *vec) { + unsigned int attrs = 0; + + if (req->cmd_flags & REQ_MMIO) + attrs |= DMA_ATTR_MMIO;Since data and integrity paylods use these same functions and may point to different kinds of memory, I think you'd have to pass the 'attrs' from the caller since it knows which flags to check for MMIO dma.
I think that hunk will fix it.
block/blk-mq-dma.c +3 -1
@@ -88,9 +88,11 @@ static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec)static bool blk_dma_map_direct (struct request *req,struct device *dma_dev,struct blk_dma_iter *iter,struct phys_vec *vec) { +struct bio_integrity_payload *bip =bio_integrity (req->bio);unsigned int attrs =0 ; -if (req->cmd_flags &REQ_MMIO ) +if ((!bip && req->cmd_flags &REQ_MMIO ) || +bio_integrity_flagged (req->bio,BIP_MMIO )) attrs |=DMA_ATTR_MMIO ; iter->addr = dma_map_phys(dma_dev, vec->paddr, vec->len,
~
Keith Busch
maintainerRe: [PATCH 3/4] block-dma: properly take MMIO path
Quoted 21 lines · Leon Romanovsky
Quoted 12 lines · Keith Busch
Review
Quoted 7 lines · Leon Romanovsky
static bool blk_dma_map_direct(struct request *req, struct device
*dma_dev,(quoted diff) +4
-0struct blk_dma_iter *iter, struct phys_vec *vec) { + unsigned int attrs = 0; + + if (req->cmd_flags & REQ_MMIO) + attrs |= DMA_ATTR_MMIO;Since data and integrity paylods use these same functions and may point to different kinds of memory, I think you'd have to pass the 'attrs' from the caller since it knows which flags to check for MMIO dma.
I think that hunk will fix it.
block/blk-mq-dma.c +0
-0
block/blk-mq-dma.c static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec) · line 96
Quoted 10 lines
block/blk-mq-dma.c +3
-1@@ -88,9 +88,11 @@ static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec)static bool blk_dma_map_direct (struct request *req,struct device *dma_dev,struct blk_dma_iter *iter,struct phys_vec *vec) { +struct bio_integrity_payload *bip =bio_integrity (req->bio);unsigned int attrs =0 ; -if (req->cmd_flags &REQ_MMIO ) +if ((!bip && req->cmd_flags &REQ_MMIO ) || +bio_integrity_flagged (req->bio,BIP_MMIO )) attrs |=DMA_ATTR_MMIO ;
If cmd_flags has REQ_MMIO set, but the integrity flag doesn't have BIP_MMIO set, you will skip setting DMA_ATTR_MMIO, but I think we need that set when this is called for the data payload.
Re: [PATCH 2/4] blk-mq-dma: unify DMA unmap routine
In reply to Keith BuschQuoted 14 lines · Keith Busch
block/blk-mq-dma.c bool blk_rq_dma_map_iter_next(struct request *req, struct device *dma_dev, · line 280
Quoted 8 lines · Leon Romanovsky
block/blk-mq-dma.c +8
-0+bool blk_rq_dma_unmap (struct request *req,struct device *dma_dev, +struct dma_iova_state *state,size_t mapped_len) +{ +struct bio_integrity_payload *bip =bio_integrity (req->bio); + +if ((!bip && req->cmd_flags &REQ_P2PDMA ) || +bio_integrity_flagged (req->bio,BIP_P2P_DMA )) +return true;I don't think you can unify it at this part here because the data payload might not be P2P but the integrity payload could be. The data payload needs to proceed to the next unmapping step in that case, but this change would have it return true early.
I was under wrong impression that request has or data payload or integrity, but never both :(.
Thanks
Dear patch submitter,
Blktests CI has tested the following submission:
Status: FAILURE Name: [0/4] Properly take MMIO path Patchwork: https://patchwork.kernel.org/project/linux-block/list/?series=1010816&state=* Run record: https://github.com/linux-blktests/linux-block/actions/runs/18488572174
Failed test cases: nvme/005
Re: [PATCH 1/4] blk-mq-dma: migrate to dma_map_phys instead of map_page
In reply to Leon RomanovskyLooks good:
Christoph Hellwig
maintainerRe: [PATCH 3/4] block-dma: properly take MMIO path
In reply to Keith BuschQuoted 28 lines · Keith Busch
Quoted 27 lines · Leon Romanovsky
Quoted 12 lines · Keith Busch
Review
Quoted 7 lines · Leon Romanovsky
static bool blk_dma_map_direct(struct request *req, struct device
*dma_dev,(quoted diff) +4
-0struct blk_dma_iter *iter, struct phys_vec *vec) { + unsigned int attrs = 0; + + if (req->cmd_flags & REQ_MMIO) + attrs |= DMA_ATTR_MMIO;Since data and integrity paylods use these same functions and may point to different kinds of memory, I think you'd have to pass the 'attrs' from the caller since it knows which flags to check for MMIO dma.
I think that hunk will fix it.
block/blk-mq-dma.c +1
-0@@ -88,9 +88,11 @@ static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec)static bool blk_dma_map_direct (struct request *req,struct device *dma_dev,struct blk_dma_iter *iter,struct phys_vec *vec) { +struct bio_integrity_payload *bip =bio_integrity (req->bio);unsigned int attrs =0 ;
Quoted 8 lines
block/blk-mq-dma.c static bool blk_dma_map_bus(struct blk_dma_iter *iter, struct phys_vec *vec) · line 96
Quoted 4 lines
block/blk-mq-dma.c +2
-1-if (req->cmd_flags &REQ_MMIO ) +if ((!bip && req->cmd_flags &REQ_MMIO ) || +bio_integrity_flagged (req->bio,BIP_MMIO )) attrs |=DMA_ATTR_MMIO ;If
cmd_flagshasREQ_MMIOset, but the integrity flag doesn't haveBIP_MMIOset, you will skip settingDMA_ATTR_MMIO, but I think we need that set when this is called for the data payload.
Yes, we just need to check the relevant flag in the caller and pass it in. Or maybe just stash it in struct blk_dma_iter.
Christoph Hellwig
maintainerRe: [PATCH 4/4] nvme-pci: unmap MMIO pages with appropriate interface
In reply to Leon RomanovskyQuoted 7 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Block layer maps MMIO memory through
dma_map_phys()interface with help ofDMA_ATTR_MMIOattribute. There is a need to unmap that memory with the appropriate unmap function, something which wasn't possible before adding new REQ attribute to block layer in previous patch.
This should go into the same patch that switches to dma_map_phys. Unless I'm missing something it also misses passing the flag for the metadata mapping.
Btw, where is all this going? Are you trying to remove the auto detection of P2P in the low-level dma mapping routines? If so that should probably go into at very least the cover lttter, but probably also the commit logs.
Re: [PATCH 4/4] nvme-pci: unmap MMIO pages with appropriate interface
Quoted 10 lines · Christoph Hellwig
Quoted 7 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Block layer maps MMIO memory through
dma_map_phys()interface with help ofDMA_ATTR_MMIOattribute. There is a need to unmap that memory with the appropriate unmap function, something which wasn't possible before adding new REQ attribute to block layer in previous patch.This should go into the same patch that switches to
dma_map_phys.
I don't think so, dma_map_phys() patch [1] doesn't change any behavior and dma_map_page() is equal to dma_map_phys(... , attr = 0),
Quoted 2 lines
Unless I'm missing something it also misses passing the flag for the metadata mapping.
Yes, I didn't realize that same request can have both metadata and data payloads.
Quoted 5 lines
Btw, where is all this going? Are you trying to remove the auto detection of P2P in the low-level dma mapping routines? If so that should probably go into at very least the cover lttter, but probably also the commit logs.
It is an outcome of multiple things:
1. We missed setting of IOMMU_MMIO flag in dma-iommu.c flow for p2p pages and for that we need some external indication as memory type is already known to the callers.
2. Robin expressed concerns about overloading DMA_ATTR_SKIP_CPU_SYNC.
[1] https://lore.kernel.org/all/a40705f38a9f3c757f30228b9b848ce0a87cbcdd.1760369219.git.leon@kernel.org/
[2] https://lore.kernel.org/all/[email protected]/