Re: xen-swiotlb vs phys_to_dma
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: xen-swiotlb vs phys_to_dma
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
Re: xen-swiotlb vs phys_to_dma
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 > 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 > --- > 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), > -
Re: xen-swiotlb vs phys_to_dma
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 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 --- 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 +
Re: xen-swiotlb vs phys_to_dma
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
xen-swiotlb vs phys_to_dma
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