* xen-swiotlb vs phys_to_dma @ 2020-10-02 12:34 ` Christoph Hellwig 0 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2020-10-02 12:34 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel, iommu, Konrad Rzeszutek Wilk Hi Stefano, I've looked over xen-swiotlb in linux-next, that is with your recent changes to take dma offsets into account. One thing that puzzles me is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact that the argument is a dma_addr_t and both other callers translate from a physical to the dma address. Was this an oversight? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* xen-swiotlb vs phys_to_dma @ 2020-10-02 12:34 ` Christoph Hellwig 0 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2020-10-02 12:34 UTC (permalink / raw) To: Stefano Stabellini; +Cc: Konrad Rzeszutek Wilk, xen-devel, iommu Hi Stefano, I've looked over xen-swiotlb in linux-next, that is with your recent changes to take dma offsets into account. One thing that puzzles me is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact that the argument is a dma_addr_t and both other callers translate from a physical to the dma address. Was this an oversight? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma 2020-10-02 12:34 ` Christoph Hellwig @ 2020-10-02 20:21 ` Stefano Stabellini -1 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2020-10-02 20:21 UTC (permalink / raw) To: Christoph Hellwig Cc: xen-devel, iommu, Stefano Stabellini, Konrad Rzeszutek Wilk On Fri, 2 Oct 2020, Christoph Hellwig wrote: > Hi Stefano, > > I've looked over xen-swiotlb in linux-next, that is with your recent > changes to take dma offsets into account. One thing that puzzles me > is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as > the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact > that the argument is a dma_addr_t and both other callers translate > from a physical to the dma address. Was this an oversight? Hi Christoph, It was not an oversight, it was done on purpose, although maybe I could have been wrong. There was a brief discussion on this topic here: https://marc.info/?l=linux-kernel&m=159011972107683&w=2 https://marc.info/?l=linux-kernel&m=159018047129198&w=2 I'll repeat and summarize here for convenience. swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual address (xen_io_tlb_start), which gets converted to phys and stored in io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl. Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The second parameter, dma_addr_t tbl_dma_addr, is used to calculate the right slot in the swiotlb buffer to use, comparing it against io_tlb_start. Thus, I think it makes sense for xen_swiotlb_map_page to call swiotlb_tbl_map_single passing an address meant to be compared with io_tlb_start, which is __pa(xen_io_tlb_start), so virt_to_phys(xen_io_tlb_start) seems to be what we want. However, you are right that it is strange that tbl_dma_addr is a dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter to swiotlb_tbl_map_single should be a phys address instead? Or it could be swiotlb_init_with_tbl to be wrong and it should take a dma address to initialize the swiotlb buffer. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma @ 2020-10-02 20:21 ` Stefano Stabellini 0 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2020-10-02 20:21 UTC (permalink / raw) To: Christoph Hellwig Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, xen-devel, iommu On Fri, 2 Oct 2020, Christoph Hellwig wrote: > Hi Stefano, > > I've looked over xen-swiotlb in linux-next, that is with your recent > changes to take dma offsets into account. One thing that puzzles me > is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as > the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact > that the argument is a dma_addr_t and both other callers translate > from a physical to the dma address. Was this an oversight? Hi Christoph, It was not an oversight, it was done on purpose, although maybe I could have been wrong. There was a brief discussion on this topic here: https://marc.info/?l=linux-kernel&m=159011972107683&w=2 https://marc.info/?l=linux-kernel&m=159018047129198&w=2 I'll repeat and summarize here for convenience. swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual address (xen_io_tlb_start), which gets converted to phys and stored in io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl. Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The second parameter, dma_addr_t tbl_dma_addr, is used to calculate the right slot in the swiotlb buffer to use, comparing it against io_tlb_start. Thus, I think it makes sense for xen_swiotlb_map_page to call swiotlb_tbl_map_single passing an address meant to be compared with io_tlb_start, which is __pa(xen_io_tlb_start), so virt_to_phys(xen_io_tlb_start) seems to be what we want. However, you are right that it is strange that tbl_dma_addr is a dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter to swiotlb_tbl_map_single should be a phys address instead? Or it could be swiotlb_init_with_tbl to be wrong and it should take a dma address to initialize the swiotlb buffer. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma 2020-10-02 20:21 ` Stefano Stabellini @ 2020-10-06 8:26 ` Christoph Hellwig -1 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2020-10-06 8:26 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, iommu, Christoph Hellwig, Konrad Rzeszutek Wilk On Fri, Oct 02, 2020 at 01:21:25PM -0700, Stefano Stabellini wrote: > On Fri, 2 Oct 2020, Christoph Hellwig wrote: > > Hi Stefano, > > > > I've looked over xen-swiotlb in linux-next, that is with your recent > > changes to take dma offsets into account. One thing that puzzles me > > is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as > > the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact > > that the argument is a dma_addr_t and both other callers translate > > from a physical to the dma address. Was this an oversight? > > Hi Christoph, > > It was not an oversight, it was done on purpose, although maybe I could > have been wrong. There was a brief discussion on this topic here: > > https://marc.info/?l=linux-kernel&m=159011972107683&w=2 > https://marc.info/?l=linux-kernel&m=159018047129198&w=2 > > I'll repeat and summarize here for convenience. > > swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual > address (xen_io_tlb_start), which gets converted to phys and stored in > io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl. Yes. > Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The > second parameter, dma_addr_t tbl_dma_addr, is used to calculate the > right slot in the swiotlb buffer to use, comparing it against > io_tlb_start. It is not compared against io_tlb_start. It is just used to pick a slot that fits the dma_get_seg_boundary limitation in a somewhat awkward way. > Thus, I think it makes sense for xen_swiotlb_map_page to call > swiotlb_tbl_map_single passing an address meant to be compared with > io_tlb_start, which is __pa(xen_io_tlb_start), so > virt_to_phys(xen_io_tlb_start) seems to be what we want. No, it doesn't. tlb_addr is used to ensure the picked slots satisfies the segment boundary, and for that you need a dma_addr_t. The index variable in swiotlb_tbl_map_single is derived from io_tlb_index, not io_tlb_start. > However, you are right that it is strange that tbl_dma_addr is a > dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter > to swiotlb_tbl_map_single should be a phys address instead? > Or it could be swiotlb_init_with_tbl to be wrong and it should take a > dma address to initialize the swiotlb buffer. No, it must be a dma_addr_t so that the dma_get_seg_boundary check works. I think we need something like this (against linux-next): --- From 07b39a62b235ed2d4b2215700d99968998fbf6c0 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Tue, 6 Oct 2020 10:22:19 +0200 Subject: swiotlb: remove the tlb_addr argument to swiotlb_tbl_map_single The tlb_addr always must be the dma view of io_tlb_start so that the segment boundary checks work. Remove the argument and do the right thing inside swiotlb_tbl_map_single. This fixes the swiotlb-xen case that failed to take DMA offset into account. The issue probably did not show up very much in practice as the typical dma offsets are large enough to not affect the segment boundaries for most devices. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/iommu/intel/iommu.c | 5 ++--- drivers/xen/swiotlb-xen.c | 3 +-- include/linux/swiotlb.h | 10 +++------- kernel/dma/swiotlb.c | 16 ++++++---------- 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 5ee0b7921b0b37..d473811fcfacd5 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -3815,9 +3815,8 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size, * page aligned, we don't need to use a bounce page. */ if (!IS_ALIGNED(paddr | size, VTD_PAGE_SIZE)) { - tlb_addr = swiotlb_tbl_map_single(dev, - phys_to_dma_unencrypted(dev, io_tlb_start), - paddr, size, aligned_size, dir, attrs); + tlb_addr = swiotlb_tbl_map_single(dev, paddr, size, + aligned_size, dir, attrs); if (tlb_addr == DMA_MAPPING_ERROR) { goto swiotlb_error; } else { diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 030a225624b060..953186f6d7d222 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -395,8 +395,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, */ trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force); - map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), - phys, size, size, dir, attrs); + map = swiotlb_tbl_map_single(dev, phys, size, size, dir, attrs); if (map == (phys_addr_t)DMA_MAPPING_ERROR) return DMA_MAPPING_ERROR; diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 513913ff748626..3bb72266a75a1d 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -45,13 +45,9 @@ enum dma_sync_target { SYNC_FOR_DEVICE = 1, }; -extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, - dma_addr_t tbl_dma_addr, - phys_addr_t phys, - size_t mapping_size, - size_t alloc_size, - enum dma_data_direction dir, - unsigned long attrs); +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys, + size_t mapping_size, size_t alloc_size, + enum dma_data_direction dir, unsigned long attrs); extern void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr, diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 995c1b4cb427ee..8d0b7c3971e81e 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -441,14 +441,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, } } -phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, - dma_addr_t tbl_dma_addr, - phys_addr_t orig_addr, - size_t mapping_size, - size_t alloc_size, - enum dma_data_direction dir, - unsigned long attrs) +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, + size_t mapping_size, size_t alloc_size, + enum dma_data_direction dir, unsigned long attrs) { + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); unsigned long flags; phys_addr_t tlb_addr; unsigned int nslots, stride, index, wrap; @@ -667,9 +664,8 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t paddr, size_t size, trace_swiotlb_bounced(dev, phys_to_dma(dev, paddr), size, swiotlb_force); - swiotlb_addr = swiotlb_tbl_map_single(dev, - phys_to_dma_unencrypted(dev, io_tlb_start), - paddr, size, size, dir, attrs); + swiotlb_addr = swiotlb_tbl_map_single(dev, paddr, size, size, dir, + attrs); if (swiotlb_addr == (phys_addr_t)DMA_MAPPING_ERROR) return DMA_MAPPING_ERROR; -- 2.28.0 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma @ 2020-10-06 8:26 ` Christoph Hellwig 0 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2020-10-06 8:26 UTC (permalink / raw) To: Stefano Stabellini Cc: Christoph Hellwig, Konrad Rzeszutek Wilk, xen-devel, iommu On Fri, Oct 02, 2020 at 01:21:25PM -0700, Stefano Stabellini wrote: > On Fri, 2 Oct 2020, Christoph Hellwig wrote: > > Hi Stefano, > > > > I've looked over xen-swiotlb in linux-next, that is with your recent > > changes to take dma offsets into account. One thing that puzzles me > > is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as > > the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact > > that the argument is a dma_addr_t and both other callers translate > > from a physical to the dma address. Was this an oversight? > > Hi Christoph, > > It was not an oversight, it was done on purpose, although maybe I could > have been wrong. There was a brief discussion on this topic here: > > https://marc.info/?l=linux-kernel&m=159011972107683&w=2 > https://marc.info/?l=linux-kernel&m=159018047129198&w=2 > > I'll repeat and summarize here for convenience. > > swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual > address (xen_io_tlb_start), which gets converted to phys and stored in > io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl. Yes. > Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The > second parameter, dma_addr_t tbl_dma_addr, is used to calculate the > right slot in the swiotlb buffer to use, comparing it against > io_tlb_start. It is not compared against io_tlb_start. It is just used to pick a slot that fits the dma_get_seg_boundary limitation in a somewhat awkward way. > Thus, I think it makes sense for xen_swiotlb_map_page to call > swiotlb_tbl_map_single passing an address meant to be compared with > io_tlb_start, which is __pa(xen_io_tlb_start), so > virt_to_phys(xen_io_tlb_start) seems to be what we want. No, it doesn't. tlb_addr is used to ensure the picked slots satisfies the segment boundary, and for that you need a dma_addr_t. The index variable in swiotlb_tbl_map_single is derived from io_tlb_index, not io_tlb_start. > However, you are right that it is strange that tbl_dma_addr is a > dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter > to swiotlb_tbl_map_single should be a phys address instead? > Or it could be swiotlb_init_with_tbl to be wrong and it should take a > dma address to initialize the swiotlb buffer. No, it must be a dma_addr_t so that the dma_get_seg_boundary check works. I think we need something like this (against linux-next): --- From 07b39a62b235ed2d4b2215700d99968998fbf6c0 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Tue, 6 Oct 2020 10:22:19 +0200 Subject: swiotlb: remove the tlb_addr argument to swiotlb_tbl_map_single The tlb_addr always must be the dma view of io_tlb_start so that the segment boundary checks work. Remove the argument and do the right thing inside swiotlb_tbl_map_single. This fixes the swiotlb-xen case that failed to take DMA offset into account. The issue probably did not show up very much in practice as the typical dma offsets are large enough to not affect the segment boundaries for most devices. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/iommu/intel/iommu.c | 5 ++--- drivers/xen/swiotlb-xen.c | 3 +-- include/linux/swiotlb.h | 10 +++------- kernel/dma/swiotlb.c | 16 ++++++---------- 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 5ee0b7921b0b37..d473811fcfacd5 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -3815,9 +3815,8 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size, * page aligned, we don't need to use a bounce page. */ if (!IS_ALIGNED(paddr | size, VTD_PAGE_SIZE)) { - tlb_addr = swiotlb_tbl_map_single(dev, - phys_to_dma_unencrypted(dev, io_tlb_start), - paddr, size, aligned_size, dir, attrs); + tlb_addr = swiotlb_tbl_map_single(dev, paddr, size, + aligned_size, dir, attrs); if (tlb_addr == DMA_MAPPING_ERROR) { goto swiotlb_error; } else { diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 030a225624b060..953186f6d7d222 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -395,8 +395,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, */ trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force); - map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), - phys, size, size, dir, attrs); + map = swiotlb_tbl_map_single(dev, phys, size, size, dir, attrs); if (map == (phys_addr_t)DMA_MAPPING_ERROR) return DMA_MAPPING_ERROR; diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index 513913ff748626..3bb72266a75a1d 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -45,13 +45,9 @@ enum dma_sync_target { SYNC_FOR_DEVICE = 1, }; -extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, - dma_addr_t tbl_dma_addr, - phys_addr_t phys, - size_t mapping_size, - size_t alloc_size, - enum dma_data_direction dir, - unsigned long attrs); +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys, + size_t mapping_size, size_t alloc_size, + enum dma_data_direction dir, unsigned long attrs); extern void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr, diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 995c1b4cb427ee..8d0b7c3971e81e 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -441,14 +441,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, } } -phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, - dma_addr_t tbl_dma_addr, - phys_addr_t orig_addr, - size_t mapping_size, - size_t alloc_size, - enum dma_data_direction dir, - unsigned long attrs) +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, + size_t mapping_size, size_t alloc_size, + enum dma_data_direction dir, unsigned long attrs) { + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); unsigned long flags; phys_addr_t tlb_addr; unsigned int nslots, stride, index, wrap; @@ -667,9 +664,8 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t paddr, size_t size, trace_swiotlb_bounced(dev, phys_to_dma(dev, paddr), size, swiotlb_force); - swiotlb_addr = swiotlb_tbl_map_single(dev, - phys_to_dma_unencrypted(dev, io_tlb_start), - paddr, size, size, dir, attrs); + swiotlb_addr = swiotlb_tbl_map_single(dev, paddr, size, size, dir, + attrs); if (swiotlb_addr == (phys_addr_t)DMA_MAPPING_ERROR) return DMA_MAPPING_ERROR; -- 2.28.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma 2020-10-06 8:26 ` Christoph Hellwig @ 2020-10-06 20:46 ` Stefano Stabellini -1 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2020-10-06 20:46 UTC (permalink / raw) To: Christoph Hellwig Cc: xen-devel, iommu, Stefano Stabellini, Konrad Rzeszutek Wilk On Tue, 6 Oct 2020, Christoph Hellwig wrote: > On Fri, Oct 02, 2020 at 01:21:25PM -0700, Stefano Stabellini wrote: > > On Fri, 2 Oct 2020, Christoph Hellwig wrote: > > > Hi Stefano, > > > > > > I've looked over xen-swiotlb in linux-next, that is with your recent > > > changes to take dma offsets into account. One thing that puzzles me > > > is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as > > > the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact > > > that the argument is a dma_addr_t and both other callers translate > > > from a physical to the dma address. Was this an oversight? > > > > Hi Christoph, > > > > It was not an oversight, it was done on purpose, although maybe I could > > have been wrong. There was a brief discussion on this topic here: > > > > https://marc.info/?l=linux-kernel&m=159011972107683&w=2 > > https://marc.info/?l=linux-kernel&m=159018047129198&w=2 > > > > I'll repeat and summarize here for convenience. > > > > swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual > > address (xen_io_tlb_start), which gets converted to phys and stored in > > io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl. > > Yes. > > > Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The > > second parameter, dma_addr_t tbl_dma_addr, is used to calculate the > > right slot in the swiotlb buffer to use, comparing it against > > io_tlb_start. > > It is not compared against io_tlb_start. It is just used to pick > a slot that fits the dma_get_seg_boundary limitation in a somewhat > awkward way. > > > Thus, I think it makes sense for xen_swiotlb_map_page to call > > swiotlb_tbl_map_single passing an address meant to be compared with > > io_tlb_start, which is __pa(xen_io_tlb_start), so > > virt_to_phys(xen_io_tlb_start) seems to be what we want. > > No, it doesn't. tlb_addr is used to ensure the picked slots satisfies > the segment boundary, and for that you need a dma_addr_t. > > The index variable in swiotlb_tbl_map_single is derived from > io_tlb_index, not io_tlb_start. > > > However, you are right that it is strange that tbl_dma_addr is a > > dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter > > to swiotlb_tbl_map_single should be a phys address instead? > > Or it could be swiotlb_init_with_tbl to be wrong and it should take a > > dma address to initialize the swiotlb buffer. > > No, it must be a dma_addr_t so that the dma_get_seg_boundary check works. > > I think we need something like this (against linux-next): > > --- > >From 07b39a62b235ed2d4b2215700d99968998fbf6c0 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Tue, 6 Oct 2020 10:22:19 +0200 > Subject: swiotlb: remove the tlb_addr argument to swiotlb_tbl_map_single > > The tlb_addr always must be the dma view of io_tlb_start so that the > segment boundary checks work. Remove the argument and do the right > thing inside swiotlb_tbl_map_single. This fixes the swiotlb-xen case > that failed to take DMA offset into account. The issue probably did > not show up very much in practice as the typical dma offsets are > large enough to not affect the segment boundaries for most devices. OK, this makes a lot of sense, and I like the patch because it makes the swiotlb interface clearer. Just one comment below. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/iommu/intel/iommu.c | 5 ++--- > drivers/xen/swiotlb-xen.c | 3 +-- > include/linux/swiotlb.h | 10 +++------- > kernel/dma/swiotlb.c | 16 ++++++---------- > 4 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 5ee0b7921b0b37..d473811fcfacd5 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -3815,9 +3815,8 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size, > * page aligned, we don't need to use a bounce page. > */ > if (!IS_ALIGNED(paddr | size, VTD_PAGE_SIZE)) { > - tlb_addr = swiotlb_tbl_map_single(dev, > - phys_to_dma_unencrypted(dev, io_tlb_start), > - paddr, size, aligned_size, dir, attrs); > + tlb_addr = swiotlb_tbl_map_single(dev, paddr, size, > + aligned_size, dir, attrs); > if (tlb_addr == DMA_MAPPING_ERROR) { > goto swiotlb_error; > } else { > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 030a225624b060..953186f6d7d222 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -395,8 +395,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > */ > trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force); > > - map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), > - phys, size, size, dir, attrs); > + map = swiotlb_tbl_map_single(dev, phys, size, size, dir, attrs); > if (map == (phys_addr_t)DMA_MAPPING_ERROR) > return DMA_MAPPING_ERROR; > > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > index 513913ff748626..3bb72266a75a1d 100644 > --- a/include/linux/swiotlb.h > +++ b/include/linux/swiotlb.h > @@ -45,13 +45,9 @@ enum dma_sync_target { > SYNC_FOR_DEVICE = 1, > }; > > -extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > - dma_addr_t tbl_dma_addr, > - phys_addr_t phys, > - size_t mapping_size, > - size_t alloc_size, > - enum dma_data_direction dir, > - unsigned long attrs); > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys, > + size_t mapping_size, size_t alloc_size, > + enum dma_data_direction dir, unsigned long attrs); > > extern void swiotlb_tbl_unmap_single(struct device *hwdev, > phys_addr_t tlb_addr, > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 995c1b4cb427ee..8d0b7c3971e81e 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -441,14 +441,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, > } > } > > -phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > - dma_addr_t tbl_dma_addr, > - phys_addr_t orig_addr, > - size_t mapping_size, > - size_t alloc_size, > - enum dma_data_direction dir, > - unsigned long attrs) > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, > + size_t mapping_size, size_t alloc_size, > + enum dma_data_direction dir, unsigned long attrs) > { > + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); This is supposed to be hwdev, not dev > unsigned long flags; > phys_addr_t tlb_addr; > unsigned int nslots, stride, index, wrap; > @@ -667,9 +664,8 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t paddr, size_t size, > trace_swiotlb_bounced(dev, phys_to_dma(dev, paddr), size, > swiotlb_force); > > - swiotlb_addr = swiotlb_tbl_map_single(dev, > - phys_to_dma_unencrypted(dev, io_tlb_start), > - paddr, size, size, dir, attrs); > + swiotlb_addr = swiotlb_tbl_map_single(dev, paddr, size, size, dir, > + attrs); > if (swiotlb_addr == (phys_addr_t)DMA_MAPPING_ERROR) > return DMA_MAPPING_ERROR; > > -- > 2.28.0 > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma @ 2020-10-06 20:46 ` Stefano Stabellini 0 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2020-10-06 20:46 UTC (permalink / raw) To: Christoph Hellwig Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, xen-devel, iommu On Tue, 6 Oct 2020, Christoph Hellwig wrote: > On Fri, Oct 02, 2020 at 01:21:25PM -0700, Stefano Stabellini wrote: > > On Fri, 2 Oct 2020, Christoph Hellwig wrote: > > > Hi Stefano, > > > > > > I've looked over xen-swiotlb in linux-next, that is with your recent > > > changes to take dma offsets into account. One thing that puzzles me > > > is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as > > > the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact > > > that the argument is a dma_addr_t and both other callers translate > > > from a physical to the dma address. Was this an oversight? > > > > Hi Christoph, > > > > It was not an oversight, it was done on purpose, although maybe I could > > have been wrong. There was a brief discussion on this topic here: > > > > https://marc.info/?l=linux-kernel&m=159011972107683&w=2 > > https://marc.info/?l=linux-kernel&m=159018047129198&w=2 > > > > I'll repeat and summarize here for convenience. > > > > swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual > > address (xen_io_tlb_start), which gets converted to phys and stored in > > io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl. > > Yes. > > > Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The > > second parameter, dma_addr_t tbl_dma_addr, is used to calculate the > > right slot in the swiotlb buffer to use, comparing it against > > io_tlb_start. > > It is not compared against io_tlb_start. It is just used to pick > a slot that fits the dma_get_seg_boundary limitation in a somewhat > awkward way. > > > Thus, I think it makes sense for xen_swiotlb_map_page to call > > swiotlb_tbl_map_single passing an address meant to be compared with > > io_tlb_start, which is __pa(xen_io_tlb_start), so > > virt_to_phys(xen_io_tlb_start) seems to be what we want. > > No, it doesn't. tlb_addr is used to ensure the picked slots satisfies > the segment boundary, and for that you need a dma_addr_t. > > The index variable in swiotlb_tbl_map_single is derived from > io_tlb_index, not io_tlb_start. > > > However, you are right that it is strange that tbl_dma_addr is a > > dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter > > to swiotlb_tbl_map_single should be a phys address instead? > > Or it could be swiotlb_init_with_tbl to be wrong and it should take a > > dma address to initialize the swiotlb buffer. > > No, it must be a dma_addr_t so that the dma_get_seg_boundary check works. > > I think we need something like this (against linux-next): > > --- > >From 07b39a62b235ed2d4b2215700d99968998fbf6c0 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Tue, 6 Oct 2020 10:22:19 +0200 > Subject: swiotlb: remove the tlb_addr argument to swiotlb_tbl_map_single > > The tlb_addr always must be the dma view of io_tlb_start so that the > segment boundary checks work. Remove the argument and do the right > thing inside swiotlb_tbl_map_single. This fixes the swiotlb-xen case > that failed to take DMA offset into account. The issue probably did > not show up very much in practice as the typical dma offsets are > large enough to not affect the segment boundaries for most devices. OK, this makes a lot of sense, and I like the patch because it makes the swiotlb interface clearer. Just one comment below. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/iommu/intel/iommu.c | 5 ++--- > drivers/xen/swiotlb-xen.c | 3 +-- > include/linux/swiotlb.h | 10 +++------- > kernel/dma/swiotlb.c | 16 ++++++---------- > 4 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 5ee0b7921b0b37..d473811fcfacd5 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -3815,9 +3815,8 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size, > * page aligned, we don't need to use a bounce page. > */ > if (!IS_ALIGNED(paddr | size, VTD_PAGE_SIZE)) { > - tlb_addr = swiotlb_tbl_map_single(dev, > - phys_to_dma_unencrypted(dev, io_tlb_start), > - paddr, size, aligned_size, dir, attrs); > + tlb_addr = swiotlb_tbl_map_single(dev, paddr, size, > + aligned_size, dir, attrs); > if (tlb_addr == DMA_MAPPING_ERROR) { > goto swiotlb_error; > } else { > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 030a225624b060..953186f6d7d222 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -395,8 +395,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > */ > trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force); > > - map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), > - phys, size, size, dir, attrs); > + map = swiotlb_tbl_map_single(dev, phys, size, size, dir, attrs); > if (map == (phys_addr_t)DMA_MAPPING_ERROR) > return DMA_MAPPING_ERROR; > > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h > index 513913ff748626..3bb72266a75a1d 100644 > --- a/include/linux/swiotlb.h > +++ b/include/linux/swiotlb.h > @@ -45,13 +45,9 @@ enum dma_sync_target { > SYNC_FOR_DEVICE = 1, > }; > > -extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > - dma_addr_t tbl_dma_addr, > - phys_addr_t phys, > - size_t mapping_size, > - size_t alloc_size, > - enum dma_data_direction dir, > - unsigned long attrs); > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys, > + size_t mapping_size, size_t alloc_size, > + enum dma_data_direction dir, unsigned long attrs); > > extern void swiotlb_tbl_unmap_single(struct device *hwdev, > phys_addr_t tlb_addr, > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 995c1b4cb427ee..8d0b7c3971e81e 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -441,14 +441,11 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, > } > } > > -phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > - dma_addr_t tbl_dma_addr, > - phys_addr_t orig_addr, > - size_t mapping_size, > - size_t alloc_size, > - enum dma_data_direction dir, > - unsigned long attrs) > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, > + size_t mapping_size, size_t alloc_size, > + enum dma_data_direction dir, unsigned long attrs) > { > + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); This is supposed to be hwdev, not dev > unsigned long flags; > phys_addr_t tlb_addr; > unsigned int nslots, stride, index, wrap; > @@ -667,9 +664,8 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t paddr, size_t size, > trace_swiotlb_bounced(dev, phys_to_dma(dev, paddr), size, > swiotlb_force); > > - swiotlb_addr = swiotlb_tbl_map_single(dev, > - phys_to_dma_unencrypted(dev, io_tlb_start), > - paddr, size, size, dir, attrs); > + swiotlb_addr = swiotlb_tbl_map_single(dev, paddr, size, size, dir, > + attrs); > if (swiotlb_addr == (phys_addr_t)DMA_MAPPING_ERROR) > return DMA_MAPPING_ERROR; > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma 2020-10-06 20:46 ` Stefano Stabellini @ 2020-10-07 6:00 ` Christoph Hellwig -1 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2020-10-07 6:00 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, iommu, Christoph Hellwig, Konrad Rzeszutek Wilk On Tue, Oct 06, 2020 at 01:46:12PM -0700, Stefano Stabellini wrote: > OK, this makes a lot of sense, and I like the patch because it makes the > swiotlb interface clearer. > > Just one comment below. > > > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, > > + size_t mapping_size, size_t alloc_size, > > + enum dma_data_direction dir, unsigned long attrs) > > { > > + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); > > This is supposed to be hwdev, not dev Yeah, te compiler would be rather unhappy oterwise. I'll resend it after the dma-mapping and Xen trees are merged by Linus to avoid a merge conflict. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma @ 2020-10-07 6:00 ` Christoph Hellwig 0 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2020-10-07 6:00 UTC (permalink / raw) To: Stefano Stabellini Cc: Christoph Hellwig, Konrad Rzeszutek Wilk, xen-devel, iommu On Tue, Oct 06, 2020 at 01:46:12PM -0700, Stefano Stabellini wrote: > OK, this makes a lot of sense, and I like the patch because it makes the > swiotlb interface clearer. > > Just one comment below. > > > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, > > + size_t mapping_size, size_t alloc_size, > > + enum dma_data_direction dir, unsigned long attrs) > > { > > + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); > > This is supposed to be hwdev, not dev Yeah, te compiler would be rather unhappy oterwise. I'll resend it after the dma-mapping and Xen trees are merged by Linus to avoid a merge conflict. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma 2020-10-07 6:00 ` Christoph Hellwig @ 2020-10-07 17:15 ` Stefano Stabellini -1 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2020-10-07 17:15 UTC (permalink / raw) To: Christoph Hellwig Cc: xen-devel, iommu, Stefano Stabellini, Konrad Rzeszutek Wilk On Wed, 7 Oct 2020, Christoph Hellwig wrote: > On Tue, Oct 06, 2020 at 01:46:12PM -0700, Stefano Stabellini wrote: > > OK, this makes a lot of sense, and I like the patch because it makes the > > swiotlb interface clearer. > > > > Just one comment below. > > > > > > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, > > > + size_t mapping_size, size_t alloc_size, > > > + enum dma_data_direction dir, unsigned long attrs) > > > { > > > + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); > > > > This is supposed to be hwdev, not dev > > Yeah, te compiler would be rather unhappy oterwise. > > I'll resend it after the dma-mapping and Xen trees are merged by Linus > to avoid a merge conflict. Sounds good, thanks. Please add Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: xen-swiotlb vs phys_to_dma @ 2020-10-07 17:15 ` Stefano Stabellini 0 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2020-10-07 17:15 UTC (permalink / raw) To: Christoph Hellwig Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, xen-devel, iommu On Wed, 7 Oct 2020, Christoph Hellwig wrote: > On Tue, Oct 06, 2020 at 01:46:12PM -0700, Stefano Stabellini wrote: > > OK, this makes a lot of sense, and I like the patch because it makes the > > swiotlb interface clearer. > > > > Just one comment below. > > > > > > +phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr, > > > + size_t mapping_size, size_t alloc_size, > > > + enum dma_data_direction dir, unsigned long attrs) > > > { > > > + dma_addr_t tbl_dma_addr = phys_to_dma_unencrypted(dev, io_tlb_start); > > > > This is supposed to be hwdev, not dev > > Yeah, te compiler would be rather unhappy oterwise. > > I'll resend it after the dma-mapping and Xen trees are merged by Linus > to avoid a merge conflict. Sounds good, thanks. Please add Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-07 17:48 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-02 12:34 xen-swiotlb vs phys_to_dma Christoph Hellwig 2020-10-02 12:34 ` Christoph Hellwig 2020-10-02 20:21 ` Stefano Stabellini 2020-10-02 20:21 ` Stefano Stabellini 2020-10-06 8:26 ` Christoph Hellwig 2020-10-06 8:26 ` Christoph Hellwig 2020-10-06 20:46 ` Stefano Stabellini 2020-10-06 20:46 ` Stefano Stabellini 2020-10-07 6:00 ` Christoph Hellwig 2020-10-07 6:00 ` Christoph Hellwig 2020-10-07 17:15 ` Stefano Stabellini 2020-10-07 17:15 ` Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.