| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.DEB.2.21.1908281525450.8175@sstabellini-ThinkPad-T480s>
Date: Thu, 29 Aug 2019 15:47:48 -0700 (PDT)
From: Stefano Stabellini <sstabellini@...nel.org>
To: Christoph Hellwig <hch@....de>
cc: Stefano Stabellini <sstabellini@...nel.org>,
Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
x86@...nel.org, linux-arm-kernel@...ts.infradead.org,
xen-devel@...ts.xenproject.org, iommu@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/11] swiotlb-xen: simplify cache maintainance
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> Now that we know we always have the dma-noncoherent.h helpers available
> if we are on an architecture with support for non-coherent devices,
> we can just call them directly, and remove the calls to the dma-direct
> routines, including the fact that we call the dma_direct_map_page
> routines but ignore the value returned from it. Instead we now have
> Xen wrappers for the arch_sync_dma_for_{device,cpu} helpers that call
> the special Xen versions of those routines for foreign pages.
>
> Note that the new helpers get the physical address passed in addition
> to the dma address to avoid another translation for the local cache
> maintainance. The pfn_valid checks remain on the dma address as in
> the old code, even if that looks a little funny.
>
> Signed-off-by: Christoph Hellwig <hch@....de>
>
> ---
> arch/arm/xen/mm.c | 64 ++++++----------------
> arch/x86/include/asm/xen/page-coherent.h | 11 ----
> drivers/xen/swiotlb-xen.c | 20 +++----
> include/xen/arm/page-coherent.h | 69 ++----------------------
> 4 files changed, 31 insertions(+), 133 deletions(-)
WOW nice! Now I really can see why this series was worth doing :-)
Reviewed-by: Stefano Stabellini <sstabellini@...nel.org>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index b7d53415532b..7096652f5a1e 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -61,63 +61,33 @@ static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
> } while (size);
> }
>
> -static void __xen_dma_page_dev_to_cpu(struct device *hwdev, dma_addr_t handle,
> - size_t size, enum dma_data_direction dir)
> +/*
> + * Dom0 is mapped 1:1, and while the Linux page can span across multiple Xen
> + * pages, it is not possible for it to contain a mix of local and foreign Xen
> + * pages. Calling pfn_valid on a foreign mfn will always return false, so if
> + * pfn_valid returns true the pages is local and we can use the native
> + * dma-direct functions, otherwise we call the Xen specific version.
> + */
> +void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
> + phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> {
> - if (dir != DMA_TO_DEVICE)
> + if (pfn_valid(PFN_DOWN(handle)))
> + arch_sync_dma_for_cpu(dev, paddr, size, dir);
> + else if (dir != DMA_TO_DEVICE)
> dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
> }
>
> -static void __xen_dma_page_cpu_to_dev(struct device *hwdev, dma_addr_t handle,
> - size_t size, enum dma_data_direction dir)
> +void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
> + phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> {
> - if (dir == DMA_FROM_DEVICE)
> + if (pfn_valid(PFN_DOWN(handle)))
> + arch_sync_dma_for_device(dev, paddr, size, dir);
> + else if (dir == DMA_FROM_DEVICE)
> dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
> else
> dma_cache_maint(handle, size, GNTTAB_CACHE_CLEAN);
> }
>
> -void __xen_dma_map_page(struct device *hwdev, struct page *page,
> - dma_addr_t dev_addr, unsigned long offset, size_t size,
> - enum dma_data_direction dir, unsigned long attrs)
> -{
> - if (dev_is_dma_coherent(hwdev))
> - return;
> - if (attrs & DMA_ATTR_SKIP_CPU_SYNC)
> - return;
> -
> - __xen_dma_page_cpu_to_dev(hwdev, dev_addr, size, dir);
> -}
> -
> -void __xen_dma_unmap_page(struct device *hwdev, dma_addr_t handle,
> - size_t size, enum dma_data_direction dir,
> - unsigned long attrs)
> -
> -{
> - if (dev_is_dma_coherent(hwdev))
> - return;
> - if (attrs & DMA_ATTR_SKIP_CPU_SYNC)
> - return;
> -
> - __xen_dma_page_dev_to_cpu(hwdev, handle, size, dir);
> -}
> -
> -void __xen_dma_sync_single_for_cpu(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction dir)
> -{
> - if (dev_is_dma_coherent(hwdev))
> - return;
> - __xen_dma_page_dev_to_cpu(hwdev, handle, size, dir);
> -}
> -
> -void __xen_dma_sync_single_for_device(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction dir)
> -{
> - if (dev_is_dma_coherent(hwdev))
> - return;
> - __xen_dma_page_cpu_to_dev(hwdev, handle, size, dir);
> -}
> -
> bool xen_arch_need_swiotlb(struct device *dev,
> phys_addr_t phys,
> dma_addr_t dev_addr)
> diff --git a/arch/x86/include/asm/xen/page-coherent.h b/arch/x86/include/asm/xen/page-coherent.h
> index 8ee33c5edded..c9c8398a31ff 100644
> --- a/arch/x86/include/asm/xen/page-coherent.h
> +++ b/arch/x86/include/asm/xen/page-coherent.h
> @@ -2,17 +2,6 @@
> #ifndef _ASM_X86_XEN_PAGE_COHERENT_H
> #define _ASM_X86_XEN_PAGE_COHERENT_H
>
> -#include <asm/page.h>
> -#include <linux/dma-mapping.h>
> -
> -static inline void xen_dma_map_page(struct device *hwdev, struct page *page,
> - dma_addr_t dev_addr, unsigned long offset, size_t size,
> - enum dma_data_direction dir, unsigned long attrs) { }
> -
> -static inline void xen_dma_unmap_page(struct device *hwdev, dma_addr_t handle,
> - size_t size, enum dma_data_direction dir,
> - unsigned long attrs) { }
> -
> static inline void xen_dma_sync_single_for_cpu(struct device *hwdev,
> dma_addr_t handle, size_t size, enum dma_data_direction dir) { }
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index f9dd4cb6e4b3..a642e284f1e2 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -28,6 +28,7 @@
>
> #include <linux/memblock.h>
> #include <linux/dma-direct.h>
> +#include <linux/dma-noncoherent.h>
> #include <linux/export.h>
> #include <xen/swiotlb-xen.h>
> #include <xen/page.h>
> @@ -390,6 +391,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
> if (map == (phys_addr_t)DMA_MAPPING_ERROR)
> return DMA_MAPPING_ERROR;
>
> + phys = map;
> dev_addr = xen_phys_to_bus(map);
>
> /*
> @@ -401,14 +403,9 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
> return DMA_MAPPING_ERROR;
> }
>
> - page = pfn_to_page(map >> PAGE_SHIFT);
> - offset = map & ~PAGE_MASK;
> done:
> - /*
> - * we are not interested in the dma_addr returned by xen_dma_map_page,
> - * only in the potential cache flushes executed by the function.
> - */
> - xen_dma_map_page(dev, page, dev_addr, offset, size, dir, attrs);
> + if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> + xen_dma_sync_for_device(dev, dev_addr, phys, size, dir);
> return dev_addr;
> }
>
> @@ -428,7 +425,8 @@ static void xen_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
>
> BUG_ON(dir == DMA_NONE);
>
> - xen_dma_unmap_page(hwdev, dev_addr, size, dir, attrs);
> + if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> + xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
>
> /* NOTE: We use dev_addr here, not paddr! */
> if (is_xen_swiotlb_buffer(dev_addr))
> @@ -448,7 +446,8 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
> {
> phys_addr_t paddr = xen_bus_to_phys(dma_addr);
>
> - xen_dma_sync_single_for_cpu(dev, dma_addr, size, dir);
> + if (!dev_is_dma_coherent(dev))
> + xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
>
> if (is_xen_swiotlb_buffer(dma_addr))
> swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
> @@ -463,7 +462,8 @@ xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
> if (is_xen_swiotlb_buffer(dma_addr))
> swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
>
> - xen_dma_sync_single_for_device(dev, dma_addr, size, dir);
> + if (!dev_is_dma_coherent(dev))
> + xen_dma_sync_for_device(dev, dma_addr, paddr, size, dir);
> }
>
> /*
> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
> index 07c104dbc21f..635492d41ebe 100644
> --- a/include/xen/arm/page-coherent.h
> +++ b/include/xen/arm/page-coherent.h
> @@ -2,70 +2,9 @@
> #ifndef _XEN_ARM_PAGE_COHERENT_H
> #define _XEN_ARM_PAGE_COHERENT_H
>
> -#include <linux/dma-mapping.h>
> -#include <asm/page.h>
> -
> -void __xen_dma_map_page(struct device *hwdev, struct page *page,
> - dma_addr_t dev_addr, unsigned long offset, size_t size,
> - enum dma_data_direction dir, unsigned long attrs);
> -void __xen_dma_unmap_page(struct device *hwdev, dma_addr_t handle,
> - size_t size, enum dma_data_direction dir,
> - unsigned long attrs);
> -void __xen_dma_sync_single_for_cpu(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction dir);
> -void __xen_dma_sync_single_for_device(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction dir);
> -
> -static inline void xen_dma_sync_single_for_cpu(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction dir)
> -{
> - unsigned long pfn = PFN_DOWN(handle);
> -
> - if (pfn_valid(pfn))
> - dma_direct_sync_single_for_cpu(hwdev, handle, size, dir);
> - else
> - __xen_dma_sync_single_for_cpu(hwdev, handle, size, dir);
> -}
> -
> -static inline void xen_dma_sync_single_for_device(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction dir)
> -{
> - unsigned long pfn = PFN_DOWN(handle);
> - if (pfn_valid(pfn))
> - dma_direct_sync_single_for_device(hwdev, handle, size, dir);
> - else
> - __xen_dma_sync_single_for_device(hwdev, handle, size, dir);
> -}
> -
> -static inline void xen_dma_map_page(struct device *hwdev, struct page *page,
> - dma_addr_t dev_addr, unsigned long offset, size_t size,
> - enum dma_data_direction dir, unsigned long attrs)
> -{
> - unsigned long pfn = PFN_DOWN(dev_addr);
> -
> - /*
> - * Dom0 is mapped 1:1, and while the Linux page can span across multiple
> - * Xen pages, it is not possible for it to contain a mix of local and
> - * foreign Xen pages. Calling pfn_valid on a foreign mfn will always
> - * return false, so if pfn_valid returns true the pages is local and we
> - * can use the native dma-direct functions, otherwise we call the Xen
> - * specific version.
> - */
> - if (pfn_valid(pfn))
> - dma_direct_map_page(hwdev, page, offset, size, dir, attrs);
> - else
> - __xen_dma_map_page(hwdev, page, dev_addr, offset, size, dir, attrs);
> -}
> -
> -static inline void xen_dma_unmap_page(struct device *hwdev, dma_addr_t handle,
> - size_t size, enum dma_data_direction dir, unsigned long attrs)
> -{
> - unsigned long pfn = PFN_DOWN(handle);
> -
> - if (pfn_valid(pfn))
> - dma_direct_unmap_page(hwdev, handle, size, dir, attrs);
> - else
> - __xen_dma_unmap_page(hwdev, handle, size, dir, attrs);
> -}
> +void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
> + phys_addr_t paddr, size_t size, enum dma_data_direction dir);
> +void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
> + phys_addr_t paddr, size_t size, enum dma_data_direction dir);
>
> #endif /* _XEN_ARM_PAGE_COHERENT_H */
> --
> 2.20.1
>
Powered by blists - more mailing lists