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

[Stefan Bader]

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...

> @@ -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;
>   
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20211012/5e41470f/attachment-0001.sig>