NACK/Cmnt: [SRU][PATCH 11/13] swiotlb: remove the tbl_dma_addr argument to swiotlb_tbl_map_single

[Khalid Elmously]

On 2021-10-12 10:11:27 , Stefan Bader wrote:
> On 07.10.21 20:06, Khalid Elmously wrote:
> > From: Christoph Hellwig <hch at lst.de>
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1943902
> > 
> > The tbl_dma_addr argument is used to check the DMA boundary for the
> > allocations, and thus needs to be a dma_addr_t.  swiotlb-xen instead
> > passed a physical address, which could lead to incorrect results for
> > strange offsets.  Fix this by removing the parameter entirely and hard
> > code the DMA address for io_tlb_start instead.
> > 
> > Fixes: 91ffe4ad534a ("swiotlb-xen: introduce phys_to_dma/dma_to_phys translations")
> > Signed-off-by: Christoph Hellwig <hch at lst.de>
> > Reviewed-by: Stefano Stabellini <sstabellini at kernel.org>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk at oracle.com>
> > (backported from commit fc0021aa340af65a0a37d77be39e22aa886a6132)
> > [ kmously: context changes, variables had been renamed in swiotlb-xen.c,
> >   and drivers/iommu/intel/iommu.c moved to drivers/iommu/intel-iommu.c ]
> > Signed-off-by: Khalid Elmously <khalid.elmously at canonical.com>
> > ---
> >   drivers/iommu/intel-iommu.c |  1 -
> >   drivers/xen/swiotlb-xen.c   |  3 +--
> >   include/linux/swiotlb.h     | 10 +++-------
> >   kernel/dma/swiotlb.c        |  8 +++-----
> >   4 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index ebb874fe6dbb4..2a1409e380485 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -3872,7 +3872,6 @@ bounce_map_single(struct device *dev, phys_addr_t paddr, size_t size,
> >   	 */
> >   	if (!IS_ALIGNED(paddr | size, VTD_PAGE_SIZE)) {
> >   		tlb_addr = swiotlb_tbl_map_single(dev,
> > -				__phys_to_dma(dev, io_tlb_start),
> >   				paddr, size, aligned_size, dir, attrs);
> >   		if (tlb_addr == DMA_MAPPING_ERROR) {
> >   			goto swiotlb_error;
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 06346422f7432..32b92b733da1f 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -392,8 +392,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, start_dma_addr, 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 f7aadd297aa98..c7b08840a4ac1 100644
> > --- a/include/linux/swiotlb.h
> > +++ b/include/linux/swiotlb.h
> > @@ -46,13 +46,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 5474376542b5a..e8a705b6896fe 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -559,9 +559,8 @@ static int find_slots(struct device *dev, phys_addr_t orig_addr,
> >   	return index;
> >   }
> > -phys_addr_t swiotlb_tbl_map_single(struct device *dev, dma_addr_t dma_addr,
> > -				phys_addr_t orig_addr, size_t mapping_size,
> > -				size_t alloc_size,
> > +phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> > +				size_t mapping_size, size_t alloc_size,
> >   				enum dma_data_direction dir,
> >   				unsigned long attrs)
> >   {
> 
> 
> I think you are missing a piece of the patch here. The dma_addr argument
> gets dropped but not set.
> 
> -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(hwdev, io_tlb_start);
>         ^ I would expect a backport of this line in the patch...

Thanks. That is one of the backporting context changes actually.
That line is not needed. tbl_dma_addr is no longer used in swiotlb_tbl_map_single() at all since "swiotlb: refactor swiotlb_tbl_map_single". That entire functionality has been moved to find_slots() since that commit. The reason this appears as a difference here from the upstream commit is because I added this commit after the refactor instead of before it as in mainline (I didn't include it in the v1 and wasn't part of the fix)

In any case to simplify the code review I will send a v4 with the patches in-order.



> 
> > @@ -706,8 +705,7 @@ bool swiotlb_map(struct device *dev, phys_addr_t *phys, dma_addr_t *dma_addr,
> >   	}
> >   	/* Oh well, have to allocate and map a bounce buffer. */
> > -	*phys = swiotlb_tbl_map_single(dev, __phys_to_dma(dev, io_tlb_start),
> > -			*phys, size, size, dir, attrs);
> > +	*phys = swiotlb_tbl_map_single(dev, *phys, size, size, dir, attrs);
> >   	if (*phys == (phys_addr_t)DMA_MAPPING_ERROR)
> >   		return false;
> > 
> 
>